Chromium Code Reviews
Help | Chromium Project | Sign in
(2)

Issue 2697123004: Convert RemoveDataMask from enum to pointers and split it between content and embedder

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 5 days ago by msramek
Modified:
6 days, 11 hours ago
CC:
chromium-reviews, msramek+watch_chromium.org, tburkard+watch_chromium.org, johnme+watch_chromium.org, bnc+watch_chromium.org, markusheintz_, extensions-reviews_chromium.org, cbentzel+watch_chromium.org, net-reviews_chromium.org, michaelpg+watch-md-settings_chromium.org, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, harkness+watch_chromium.org, dbeam+watch-options_chromium.org, Peter Beverloo (OOO - Mar 6th), gavinp+prer_chromium.org, oshima+watch_chromium.org, michaelpg+watch-options_chromium.org, eroman, davemoore+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, mmenke (Out Feb 4 to March 5)
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Convert RemoveDataMask from enum to pointers and split it between content and embedder As the next step of the migration of browsing_data codebase to content, we need to split RemoveDataMask into platform datatypes (defined in content/) and Chrome-specific datatypes (defined in chrome/). The datatypes from BrowsingDataRemover::RemoveDataMask are now represented as global static objects of the BrowsingDataType type; BrowsingDataRemover and related classes use their addresses (const BrowsingDataType*) to distinguish them. To pass a mask consisting of multiple datatypes, we use std::set<const BrowsingDataType*>. See the design doc for more details why this approach was chosen: https://docs.google.com/document/d/1I6m4QwbTNhG6wdtazamhTnArJN-UMUGqpvwH6InBEaM/ Details (a.k.a. how to review this CL): 1. Platform datatypes are defined in the content namespace in the file content/public/browsing_data_types.[h|cc]. 2. Chrome datatypes are defined in the default namespace in the file c/b/browsing_data/chrome_browsing_data_types.[h|cc]; this file also imports the platform datatypes to the default namespace, since embedder callsites should not have to know where a datatype originates. 3. The rest of the CL consists mostly of equivalent replacements; this is largely possible thanks to C++11 initializers allowing syntax such as std::set<X> x = { x1, x2, x3 }; to replace what was previously enum bitmask "x1 | x2 | x3"; bitmasks bitwise operations are more complicated to replace; there, we use set operations from the <algorithm> library. 4. The filterability of a datatype is now defined as an attribute of the BrowsingDataType object. 5. Other nonequivalent changes suggested in the above doc, such as the introduction of a "storage" concept in place of "site data" are not a part of this CL. BUG=668114

Patch Set 1 #

Patch Set 2 : Fixes #

Patch Set 3 : More compilation error fixes. #

Total comments: 14

Patch Set 4 : More fixes... #

Patch Set 5 : Addressed comments except formatting. #

Patch Set 6 : Formatting. #

Patch Set 7 : Fix browsertest typo #

Patch Set 8 : Rebased over https://codereview.chromium.org/2664253006/, added external protocol data #

Patch Set 9 : Change sets to functions, fix typo. #

Patch Set 10 : Android compilation #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1150 lines, -921 lines) Patch
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/android/preferences/pref_service_bridge.cc View 1 2 3 4 5 6 7 8 9 6 chunks +24 lines, -13 lines 0 comments Download
M chrome/browser/android/signin/signin_manager_android.cc View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/browsing_data/browsing_data_remover.h View 1 2 3 4 5 6 7 6 chunks +19 lines, -94 lines 0 comments Download
M chrome/browser/browsing_data/browsing_data_remover_browsertest.cc View 1 2 3 4 5 6 7 8 11 chunks +17 lines, -16 lines 0 comments Download
M chrome/browser/browsing_data/browsing_data_remover_delegate.h View 2 chunks +4 lines, -1 line 0 comments Download
M chrome/browser/browsing_data/browsing_data_remover_impl.h View 1 2 3 4 5 7 chunks +16 lines, -14 lines 0 comments Download
M chrome/browser/browsing_data/browsing_data_remover_impl.cc View 1 2 3 4 5 19 chunks +56 lines, -34 lines 0 comments Download
M chrome/browser/browsing_data/browsing_data_remover_impl_unittest.cc View 1 2 3 4 5 6 7 34 chunks +135 lines, -262 lines 0 comments Download
M chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc View 1 2 3 4 5 6 7 18 chunks +41 lines, -37 lines 0 comments Download
M chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc View 1 2 3 4 5 6 7 8 50 chunks +81 lines, -83 lines 0 comments Download
A chrome/browser/browsing_data/chrome_browsing_data_types.h View 1 2 3 4 5 6 7 8 1 chunk +75 lines, -0 lines 0 comments Download
A chrome/browser/browsing_data/chrome_browsing_data_types.cc View 1 2 3 4 5 6 7 8 1 chunk +115 lines, -0 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 8 4 chunks +17 lines, -16 lines 0 comments Download
M chrome/browser/chrome_content_browser_client_unittest.cc View 1 2 3 4 5 6 7 8 10 chunks +85 lines, -60 lines 0 comments Download
M chrome/browser/chromeos/profiles/profile_helper.cc View 1 2 3 4 5 6 7 8 2 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/extensions/api/browsing_data/browsing_data_api.h View 19 chunks +25 lines, -17 lines 0 comments Download
M chrome/browser/extensions/api/browsing_data/browsing_data_api.cc View 1 2 3 4 5 8 chunks +82 lines, -56 lines 0 comments Download
M chrome/browser/extensions/api/browsing_data/browsing_data_test.cc View 1 2 3 4 5 6 7 8 13 chunks +190 lines, -156 lines 0 comments Download
M chrome/browser/net/errorpage_browsertest.cc View 1 2 3 4 5 6 7 3 chunks +3 lines, -4 lines 0 comments Download
M chrome/browser/net/predictor_browsertest.cc View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/net/sdch_browsertest.cc View 1 2 3 4 5 6 7 8 3 chunks +7 lines, -4 lines 0 comments Download
M chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc View 1 2 3 4 5 6 7 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/prerender/prerender_browsertest.cc View 1 2 3 4 5 6 7 4 chunks +6 lines, -3 lines 0 comments Download
M chrome/browser/profile_resetter/profile_resetter.cc View 1 2 3 4 5 6 7 8 3 chunks +6 lines, -3 lines 0 comments Download
M chrome/browser/profiles/profiles_state.cc View 1 2 3 4 5 6 7 8 3 chunks +5 lines, -2 lines 0 comments Download
M chrome/browser/push_messaging/push_messaging_browsertest.cc View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc View 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/browser_commands.cc View 1 2 3 4 5 3 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/ui/webui/net_internals/net_internals_ui.cc View 1 2 3 4 5 6 7 3 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/options/clear_browser_data_handler.cc View 1 2 3 4 5 6 7 8 3 chunks +17 lines, -14 lines 0 comments Download
M chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc View 1 2 3 4 5 6 7 8 3 chunks +18 lines, -14 lines 0 comments Download
M content/browser/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
A content/browser/browsing_data/browsing_data_types.cc View 1 2 3 4 5 1 chunk +36 lines, -0 lines 0 comments Download
M content/public/browser/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
A content/public/browser/browsing_data_types.h View 1 chunk +41 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 55 (42 generated)
msramek
Hi Bernhard and Mike, Here's the result of our discussion about RemoveDataMask from the doc. ...
1 week, 5 days ago (2017-02-16 22:20:19 UTC) #9
Bernhard Bauer
Nice! https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing_data/browsing_data_remover_impl.cc File chrome/browser/browsing_data/browsing_data_remover_impl.cc (right): https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing_data/browsing_data_remover_impl.cc#newcode123 chrome/browser/browsing_data/browsing_data_remover_impl.cc:123: if (data_type && !data_type->filterable) Why the null check? ...
1 week, 5 days ago (2017-02-17 11:10:44 UTC) #18
msramek
https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing_data/browsing_data_remover_impl.cc File chrome/browser/browsing_data/browsing_data_remover_impl.cc (right): https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing_data/browsing_data_remover_impl.cc#newcode123 chrome/browser/browsing_data/browsing_data_remover_impl.cc:123: if (data_type && !data_type->filterable) On 2017/02/17 11:10:43, Bernhard Bauer ...
1 week, 4 days ago (2017-02-17 18:00:20 UTC) #25
Bernhard Bauer
https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing_data/chrome_browsing_data_types.cc File chrome/browser/browsing_data/chrome_browsing_data_types.cc (right): https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing_data/chrome_browsing_data_types.cc#newcode98 chrome/browser/browsing_data/chrome_browsing_data_types.cc:98: const std::set<const content::BrowsingDataType*>& kBrowsingDataTypeSetSiteData = On 2017/02/17 18:00:20, msramek ...
1 week, 2 days ago (2017-02-20 14:19:16 UTC) #32
msramek
I fixed a few more red tests, rebased (merge conflict), and addressed your comment :) ...
1 week, 2 days ago (2017-02-20 17:11:17 UTC) #43
Bernhard Bauer
LGTM!
1 week, 2 days ago (2017-02-20 17:15:18 UTC) #44
msramek
Wow, that was much faster than I expected with this CL :) Thanks, Bernhard! Also ...
1 week, 2 days ago (2017-02-20 17:22:33 UTC) #45
msramek
+jam@ PTAL at content/public interface +dbeam@ PTAL at options/ change (it's an equivalent replacement, but ...
1 week, 2 days ago (2017-02-20 17:29:17 UTC) #47
msramek
+dbeam@ this time really :)
1 week, 2 days ago (2017-02-20 17:30:11 UTC) #49
jam
I just read the doc section about extending the enum. Extending an enum declared in ...
1 week, 1 day ago (2017-02-21 16:15:49 UTC) #52
msramek
On 2017/02/21 16:15:49, jam (OOO) wrote: > I just read the doc section about extending ...
1 week ago (2017-02-21 19:35:32 UTC) #53
jam
On 2017/02/21 19:35:32, msramek wrote: > On 2017/02/21 16:15:49, jam (OOO) wrote: > > I ...
1 week ago (2017-02-21 20:50:12 UTC) #54
Dan Beam
6 days, 11 hours ago (2017-02-23 06:34:45 UTC) #55
lgtm but using "mask" in variable names that are now sets is weird, IMO
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd