Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(49)

Issue 2827523003: Move BrowsingDataRemover to content/ (Closed)

Created:
3 years, 8 months ago by msramek
Modified:
3 years, 8 months ago
CC:
bnc+watch_chromium.org, cbentzel+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, davemoore+watch_chromium.org, dbeam+watch-options_chromium.org, dbeam+watch-settings_chromium.org, eroman, extensions-reviews_chromium.org, gavinp+prer_chromium.org, harkness+watch_chromium.org, jam, johnme+watch_chromium.org, markusheintz_, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-options_chromium.org, mmenke, msramek+watch_chromium.org, net-reviews_chromium.org, oshima+watch_chromium.org, Peter Beverloo, rginda+watch_chromium.org, stevenjb+watch-md-settings_chromium.org, tburkard+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Move BrowsingDataRemover to content/ Changes in this CL: 1. The public interfaces BrowsingDataRemover and BrowsingDataRemoverDelegate move to content/public/browser. BrowsingDataRemoverImpl moves to content/browser/browsing_data. 2. Previously, BrowsingDataRemoverImpl was a KeyedService created by BrowsingDataRemoverFactory. After this CL, it is owned by BrowserContext. Conversely, ChromeBrowsingRemoverDelegate (which stays in chrome/) becomes a KeyedService. The former BrowsingDataRemoverFactory is thus adapted into ChromeBrowsingDataRemoverDelegateFactory. BrowsingDataRemoverDelegate is now owned by the embedder (rather than BrowsingDataRemover) and supplied by embedders through BrowserContext::GetBrowsingDataRemoverDelegate(). Profile (in chrome/) supplies ChromeBrowsingDataRemoverDelegate. The implementation for other embedders is left as TODO. 3. BrowsingDataRemoverImplTest moves to content/browser/browsing/data. browsing_data_remover_test_util that is used by this test as well as various tests in chrome/ is moved to content/public/test. 4. BrowsingDataRemover can only delete download history if this is feature is not disabled by administrator. Previously, BrowsingDataRemover queried PrefService directly. We now pass this information through BrowsingDataRemoverDelegate. The default value is true, ChromeBrowsingDataRemoverDelegate queries PrefService. 5. The removal of ContentBrowserClient interfaces ClearCookies(), ClearCache(), ClearSiteData() and their replacement by BrowsingDataRemover is left as a TODO. 6. All other changes are adding / removing includes, updating BUILD files, replacing BrowsingDataRemoverFactory with BrowserContext::GetBrowsingDataRemover, formatting (mostly triggered by prepending content:: to class names), and the removal of forgotten "#ifdef (EXTENSIONS)" in BrowsingDataRemoverImplTest. TBR=dbeam@chromium.org BUG=668114 Review-Url: https://codereview.chromium.org/2827523003 Cr-Commit-Position: refs/heads/master@{#467241} Committed: https://chromium.googlesource.com/chromium/src/+/e169ccbe5d1f406337c2b5e758f58b7966c51b6c

Patch Set 1 #

Total comments: 7

Patch Set 2 : Addressed comments, fixed Win compilation #

Total comments: 4

Patch Set 3 : Addressed comments. #

Total comments: 22

Patch Set 4 : Addressed some comments. #

Total comments: 6

Patch Set 5 : Addressed comments. #

Patch Set 6 : Rebase. #

Patch Set 7 : Rebase over codereview.chromium.org/2815913005 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+852 lines, -3958 lines) Patch
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 2 chunks +2 lines, -5 lines 0 comments Download
M chrome/browser/android/browsing_data/browsing_data_bridge.cc View 4 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/android/signin/signin_manager_android.cc View 4 chunks +5 lines, -5 lines 0 comments Download
D chrome/browser/browsing_data/browsing_data_remover.h View 1 chunk +0 lines, -203 lines 0 comments Download
M chrome/browser/browsing_data/browsing_data_remover_browsertest.cc View 1 2 3 4 5 8 chunks +20 lines, -19 lines 0 comments Download
D chrome/browser/browsing_data/browsing_data_remover_delegate.h View 1 chunk +0 lines, -52 lines 0 comments Download
D chrome/browser/browsing_data/browsing_data_remover_factory.h View 1 chunk +0 lines, -41 lines 0 comments Download
D chrome/browser/browsing_data/browsing_data_remover_factory.cc View 1 chunk +0 lines, -101 lines 0 comments Download
D chrome/browser/browsing_data/browsing_data_remover_impl.h View 1 chunk +0 lines, -243 lines 0 comments Download
D chrome/browser/browsing_data/browsing_data_remover_impl.cc View 1 2 3 4 5 1 chunk +0 lines, -681 lines 0 comments Download
D chrome/browser/browsing_data/browsing_data_remover_impl_unittest.cc View 1 2 3 4 5 6 1 chunk +0 lines, -1715 lines 0 comments Download
D chrome/browser/browsing_data/browsing_data_remover_test_util.h View 1 chunk +0 lines, -67 lines 0 comments Download
D chrome/browser/browsing_data/browsing_data_remover_test_util.cc View 1 chunk +0 lines, -64 lines 0 comments Download
M chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h View 1 2 3 4 5 chunks +50 lines, -37 lines 0 comments Download
M chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc View 1 2 3 4 5 12 chunks +36 lines, -24 lines 0 comments Download
A chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_factory.h View 1 chunk +43 lines, -0 lines 0 comments Download
A + chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_factory.cc View 4 chunks +17 lines, -17 lines 0 comments Download
M chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc View 1 2 3 4 5 6 36 chunks +87 lines, -63 lines 0 comments Download
M chrome/browser/browsing_data/mock_browsing_data_remover_delegate.h View 1 2 3 4 1 chunk +6 lines, -4 lines 0 comments Download
M chrome/browser/browsing_data/mock_browsing_data_remover_delegate.cc View 1 2 3 4 1 chunk +6 lines, -2 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 8 chunks +15 lines, -17 lines 0 comments Download
M chrome/browser/chrome_content_browser_client_unittest.cc View 1 2 3 8 chunks +26 lines, -30 lines 0 comments Download
M chrome/browser/chromeos/profiles/profile_helper.h View 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/chromeos/profiles/profile_helper.cc View 4 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/extensions/api/browsing_data/browsing_data_api.h View 3 chunks +7 lines, -4 lines 0 comments Download
M chrome/browser/extensions/api/browsing_data/browsing_data_api.cc View 1 2 3 4 5 11 chunks +30 lines, -30 lines 0 comments Download
M chrome/browser/extensions/api/browsing_data/browsing_data_test.cc View 16 chunks +62 lines, -59 lines 0 comments Download
M chrome/browser/net/errorpage_browsertest.cc View 1 2 3 4 5 4 chunks +10 lines, -10 lines 0 comments Download
M chrome/browser/net/predictor_browsertest.cc View 1 2 3 4 5 3 chunks +4 lines, -5 lines 0 comments Download
M chrome/browser/net/sdch_browsertest.cc View 1 2 3 4 5 4 chunks +14 lines, -15 lines 0 comments Download
M chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc View 1 2 3 4 5 5 chunks +11 lines, -9 lines 0 comments Download
M chrome/browser/prerender/prerender_browsertest.cc View 1 2 3 4 5 5 chunks +11 lines, -10 lines 0 comments Download
M chrome/browser/profile_resetter/profile_resetter.h View 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/profile_resetter/profile_resetter.cc View 4 chunks +5 lines, -5 lines 0 comments Download
M chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc View 1 2 3 4 5 4 chunks +6 lines, -6 lines 0 comments Download
M chrome/browser/profiles/profile_impl.h View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/profiles/profile_impl.cc View 1 2 3 4 5 2 chunks +7 lines, -0 lines 0 comments Download
M chrome/browser/profiles/profile_manager_browsertest.cc View 3 chunks +2 lines, -3 lines 0 comments Download
M chrome/browser/profiles/profiles_state.cc View 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/push_messaging/push_messaging_browsertest.cc View 3 chunks +6 lines, -7 lines 0 comments Download
M chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc View 3 chunks +8 lines, -7 lines 0 comments Download
M chrome/browser/ui/browser_commands.cc View 1 2 3 4 5 3 chunks +6 lines, -6 lines 0 comments Download
M chrome/browser/ui/webui/net_internals/net_internals_ui.cc View 1 2 3 4 5 3 chunks +5 lines, -7 lines 0 comments Download
M chrome/browser/ui/webui/options/clear_browser_data_browsertest.cc View 2 chunks +4 lines, -5 lines 0 comments Download
M chrome/browser/ui/webui/options/clear_browser_data_handler.h View 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/webui/options/clear_browser_data_handler.cc View 4 chunks +14 lines, -15 lines 0 comments Download
M chrome/browser/ui/webui/profile_helper_browsertest.cc View 3 chunks +4 lines, -5 lines 0 comments Download
M chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc View 5 chunks +21 lines, -20 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 2 chunks +0 lines, -5 lines 0 comments Download
M content/browser/BUILD.gn View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/browser_context.cc View 1 2 3 4 5 6 4 chunks +22 lines, -0 lines 0 comments Download
A + content/browser/browsing_data/browsing_data_remover_impl.h View 1 2 3 8 chunks +36 lines, -42 lines 0 comments Download
A + content/browser/browsing_data/browsing_data_remover_impl.cc View 1 2 3 4 5 19 chunks +52 lines, -86 lines 0 comments Download
A + content/browser/browsing_data/browsing_data_remover_impl_unittest.cc View 1 2 3 4 5 6 43 chunks +86 lines, -144 lines 0 comments Download
M content/public/browser/BUILD.gn View 1 chunk +2 lines, -0 lines 0 comments Download
M content/public/browser/browser_context.h View 1 2 3 4 5 6 4 chunks +16 lines, -5 lines 0 comments Download
A + content/public/browser/browsing_data_remover.h View 1 2 3 8 chunks +22 lines, -16 lines 0 comments Download
A + content/public/browser/browsing_data_remover_delegate.h View 1 2 3 4 3 chunks +13 lines, -8 lines 0 comments Download
M content/public/browser/content_browser_client.h View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
A + content/public/test/browsing_data_remover_test_util.h View 3 chunks +9 lines, -5 lines 0 comments Download
A + content/public/test/browsing_data_remover_test_util.cc View 3 chunks +7 lines, -3 lines 0 comments Download
M content/public/test/cache_test_util.h View 5 chunks +7 lines, -7 lines 0 comments Download
M content/test/BUILD.gn View 1 2 3 4 5 6 2 chunks +3 lines, -0 lines 0 comments Download

Messages

Total messages: 95 (77 generated)
msramek
Hi Mike and Bernhard! *It's happening!* All the refactoring CLs I previously sent you led ...
3 years, 8 months ago (2017-04-19 08:20:52 UTC) #36
Bernhard Bauer
https://codereview.chromium.org/2827523003/diff/30066/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (left): https://codereview.chromium.org/2827523003/diff/30066/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc#oldcode202 chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:202: BrowsingDataRemoverFactory::GetInstance(); Add ChromeBrowsingDataRemoverDelegateFactory::GetInstance() instead? https://codereview.chromium.org/2827523003/diff/30066/content/browser/browsing_data/browsing_data_remover_impl.h File content/browser/browsing_data/browsing_data_remover_impl.h (right): https://codereview.chromium.org/2827523003/diff/30066/content/browser/browsing_data/browsing_data_remover_impl.h#newcode37 ...
3 years, 8 months ago (2017-04-19 09:42:41 UTC) #37
Mike West
I took a quick pass, and I think the mechanical bits look good, and I'm ...
3 years, 8 months ago (2017-04-19 10:14:53 UTC) #40
msramek
https://codereview.chromium.org/2827523003/diff/30066/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h (right): https://codereview.chromium.org/2827523003/diff/30066/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h#newcode49 chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h:49: , On 2017/04/19 10:14:53, Mike West wrote: > Nit: ...
3 years, 8 months ago (2017-04-19 11:31:02 UTC) #43
Bernhard Bauer
Thanks! LGTM with nits: https://codereview.chromium.org/2827523003/diff/130001/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (right): https://codereview.chromium.org/2827523003/diff/130001/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc#newcode214 chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:214: ChromeBrowsingDataRemoverDelegateFactory::GetInstance(); Nit: Move this further ...
3 years, 8 months ago (2017-04-19 12:39:38 UTC) #46
msramek
Thank you! https://codereview.chromium.org/2827523003/diff/130001/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (right): https://codereview.chromium.org/2827523003/diff/130001/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc#newcode214 chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:214: ChromeBrowsingDataRemoverDelegateFactory::GetInstance(); On 2017/04/19 12:39:38, Bernhard Bauer wrote: ...
3 years, 8 months ago (2017-04-19 13:39:18 UTC) #49
msramek
...and adding +John, please have a look as a content/ owner. Primarily, at: - New ...
3 years, 8 months ago (2017-04-19 13:45:31 UTC) #51
jam
just looked at content/public for now. i suspect the rest is mechanical moves, and it'll ...
3 years, 8 months ago (2017-04-20 00:39:48 UTC) #54
msramek
Thanks, John! I addressed some of your comments, for the rest I have clarification questions ...
3 years, 8 months ago (2017-04-21 15:22:12 UTC) #55
jam
lgtm https://codereview.chromium.org/2827523003/diff/150001/content/public/browser/browsing_data_remover.h File content/public/browser/browsing_data_remover.h (right): https://codereview.chromium.org/2827523003/diff/150001/content/public/browser/browsing_data_remover.h#newcode122 content/public/browser/browsing_data_remover.h:122: class Observer { On 2017/04/21 15:22:12, msramek wrote: ...
3 years, 8 months ago (2017-04-21 22:58:31 UTC) #56
msramek
Thanks, John! Please take one more look, whether you're OK with moving the downloads history ...
3 years, 8 months ago (2017-04-25 01:29:10 UTC) #60
jam
still lgtm
3 years, 8 months ago (2017-04-26 00:20:16 UTC) #74
msramek
Thanks, John! I'm adding +Dan to TBR for the webui/ files (ClearBrowserDataHandler and its test), ...
3 years, 8 months ago (2017-04-26 01:10:56 UTC) #78
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2827523003/250001
3 years, 8 months ago (2017-04-26 01:12:18 UTC) #81
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/420426)
3 years, 8 months ago (2017-04-26 01:22:36 UTC) #83
msramek
...and another rebase. codereview.chromium.org/2815913005 added scoped pointers for UserData, which caused a merge conflict in ...
3 years, 8 months ago (2017-04-26 05:13:24 UTC) #89
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2827523003/290001
3 years, 8 months ago (2017-04-26 05:14:30 UTC) #92
commit-bot: I haz the power
3 years, 8 months ago (2017-04-26 05:22:26 UTC) #95
Message was sent while issue was closed.
Committed patchset #7 (id:290001) as
https://chromium.googlesource.com/chromium/src/+/e169ccbe5d1f406337c2b5e758f5...

Powered by Google App Engine
This is Rietveld 408576698