Move BrowsingDataRemover to content/

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

[msramek, 2017-04-18 12:03:40]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 12:03:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 12:22:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-18 12:22:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msramek, 2017-04-18 14:03:48]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 14:04:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 14:15:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-18 14:15:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msramek, 2017-04-18 18:13:49]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 18:14:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 18:33:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-18 18:33:40]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msramek, 2017-04-18 19:51:15]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 19:51:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 20:17:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-18 20:17:42]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[msramek, 2017-04-18 20:40:06]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 20:40:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2017-04-18 20:52:44]

Patchset #1 (id:1) has been deleted

[msramek, 2017-04-18 20:52:56]

Patchset #1 (id:20001) has been deleted

[msramek, 2017-04-18 20:53:06]

Patchset #1 (id:40001) has been deleted

[msramek, 2017-04-18 20:53:20]

Patchset #1 (id:60001) has been deleted

[msramek, 2017-04-18 20:59:15]

Description was changed from

==========
Move BrowsingDataRemover to content/

WIP; Please do not review yet.

BUG=668114
==========

to

==========
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 ContentBrowserClient.
The default value is true, ChromeContentBrowserClient 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.

BUG=668114
==========

[msramek, 2017-04-18 21:01:26]

Description was changed from

==========
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 ContentBrowserClient.
The default value is true, ChromeContentBrowserClient 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.

BUG=668114
==========

to

==========
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 ContentBrowserClient.
The default value is true, ChromeContentBrowserClient 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.

BUG=668114
==========

[commit-bot: I haz the power, 2017-04-18 21:12:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-18 21:12:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msramek, 2017-04-19 07:47:08]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[msramek, 2017-04-19 07:47:20]

Patchset #1 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-04-19 07:47:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 08:13:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 08:13:17]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[msramek, 2017-04-19 08:19:11]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[msramek, 2017-04-19 08:19:26]

Patchset #1 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-04-19 08:19:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2017-04-19 08:20:51]

msramek@chromium.org changed reviewers:
+ bauerb@chromium.org, mkwst@chromium.org

[msramek, 2017-04-19 08:20:52]

Hi Mike and Bernhard!

*It's happening!* All the refactoring CLs I previously sent you led to this
point. Please have a look!

This is quite a large CL, but there are no longer any dependencies that could be
extracted to a separate one. In fact, I left two TODOs - see points #2 and #5 in
the CL description. I expect to be asked to resolve #2 immediately, but I wanted
to at least make the first patchset smaller. However, I believe that #5 is
sufficiently logically independent to be addressed in a follow-up.

Thanks,
Martin

[Bernhard Bauer, 2017-04-19 09:42:41]

https://codereview.chromium.org/2827523003/diff/30066/chrome/browser/profiles...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (left):

https://codereview.chromium.org/2827523003/diff/30066/chrome/browser/profiles...
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/browsin...
File content/browser/browsing_data/browsing_data_remover_impl.h (right):

https://codereview.chromium.org/2827523003/diff/30066/content/browser/browsin...
content/browser/browsing_data/browsing_data_remover_impl.h:37: public
base::RefCountedThreadSafe<BrowsingDataRemoverImpl> {
Why the refcounting now?

[commit-bot: I haz the power, 2017-04-19 09:47:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 09:47:43]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Mike West, 2017-04-19 10:14:53]

I took a quick pass, and I think the mechanical bits look good, and I'm happy
with the general shape of delegation. The refcounting jumped out at me as
strange, though: why did you need to add it?

https://codereview.chromium.org/2827523003/diff/30066/chrome/browser/browsing...
File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h
(right):

https://codereview.chromium.org/2827523003/diff/30066/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h:49: ,
Nit: Weird whitespace. Is this what `git cl format` came up with?

https://codereview.chromium.org/2827523003/diff/30066/content/browser/browsin...
File content/browser/browsing_data/browsing_data_remover_impl.h (right):

https://codereview.chromium.org/2827523003/diff/30066/content/browser/browsin...
content/browser/browsing_data/browsing_data_remover_impl.h:37: public
base::RefCountedThreadSafe<BrowsingDataRemoverImpl> {
Why is this refcounted? Why does it need to be threadsafe?

[msramek, 2017-04-19 11:30:38]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 11:30:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2017-04-19 11:31:02]

https://codereview.chromium.org/2827523003/diff/30066/chrome/browser/browsing...
File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h
(right):

https://codereview.chromium.org/2827523003/diff/30066/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h:49: ,
On 2017/04/19 10:14:53, Mike West wrote:
> Nit: Weird whitespace. Is this what `git cl format` came up with?

Yes, this whole CL has been auto-formatted. Two lines above, we need a comma at
the end of the line, here we need it at the beginning. There's no pretty way to
write this, so I don't blame it :)

https://codereview.chromium.org/2827523003/diff/30066/chrome/browser/profiles...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (left):

https://codereview.chromium.org/2827523003/diff/30066/chrome/browser/profiles...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:202:
BrowsingDataRemoverFactory::GetInstance();
On 2017/04/19 09:42:41, Bernhard Bauer wrote:
> Add ChromeBrowsingDataRemoverDelegateFactory::GetInstance() instead?

Done. Thanks for noticing!

https://codereview.chromium.org/2827523003/diff/30066/content/browser/browsin...
File content/browser/browsing_data/browsing_data_remover_impl.h (right):

https://codereview.chromium.org/2827523003/diff/30066/content/browser/browsin...
content/browser/browsing_data/browsing_data_remover_impl.h:37: public
base::RefCountedThreadSafe<BrowsingDataRemoverImpl> {
On 2017/04/19 10:14:53, Mike West wrote:
> Why is this refcounted? Why does it need to be threadsafe?

Sorry, I added this in an early version of this CL and forgot about it.

This was required by UserDataAdapter, which I previously used to wrap
BrowsingDataRemoverImpl before putting it into BrowserContext. That's
unnecessary, since BrowsingDataRemoverImpl is already
base::SupportsUserData::Data.

Simplified, removed. Thanks for the sanity check!

[commit-bot: I haz the power, 2017-04-19 12:37:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 12:37:19]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-04-19 12:39:38]

Thanks! LGTM with nits:

https://codereview.chromium.org/2827523003/diff/130001/chrome/browser/profile...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(right):

https://codereview.chromium.org/2827523003/diff/130001/chrome/browser/profile...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:214:
ChromeBrowsingDataRemoverDelegateFactory::GetInstance();
Nit: Move this further up, before CloudPrintProxy or so?

https://codereview.chromium.org/2827523003/diff/130001/content/browser/browse...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/2827523003/diff/130001/content/browser/browse...
content/browser/browser_context.cc:237: BrowsingDataRemoverImpl* remover = new
BrowsingDataRemoverImpl(context);
Nit: put this into a std::unique_ptr<> and use base::MakeUnique? Maybe it's a
bit silly if it gets released two lines later, but at this point I kind of
dislike seeing raw pointers and `new` allocations ;-)

[msramek, 2017-04-19 13:36:46]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 13:37:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2017-04-19 13:39:18]

Thank you!

https://codereview.chromium.org/2827523003/diff/130001/chrome/browser/profile...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(right):

https://codereview.chromium.org/2827523003/diff/130001/chrome/browser/profile...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:214:
ChromeBrowsingDataRemoverDelegateFactory::GetInstance();
On 2017/04/19 12:39:38, Bernhard Bauer wrote:
> Nit: Move this further up, before CloudPrintProxy or so?

Yeah... binary search insert into an array that is not sorted is an undefined
behavior :) I sorted it a bit.

https://codereview.chromium.org/2827523003/diff/130001/content/browser/browse...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/2827523003/diff/130001/content/browser/browse...
content/browser/browser_context.cc:237: BrowsingDataRemoverImpl* remover = new
BrowsingDataRemoverImpl(context);
On 2017/04/19 12:39:38, Bernhard Bauer wrote:
> Nit: put this into a std::unique_ptr<> and use base::MakeUnique? Maybe it's a
> bit silly if it gets released two lines later, but at this point I kind of
> dislike seeing raw pointers and `new` allocations ;-)

Done.

A small downside is that we can no longer use a private constructor with a
friend class BrowserContext to express that you should not construct the class
manually.

[msramek, 2017-04-19 13:45:30]

msramek@chromium.org changed reviewers:
+ jam@chromium.org

[msramek, 2017-04-19 13:45:31]

...and adding +John, please have a look as a content/ owner.

Primarily, at:
- New classes in content/public
- The way BrowsingDataRemover is hooked into BrowserContext and
ChromeBrowsingDataRemoverDelegate injected from Profile
- Whether you want me to address the TODO in BrowserContext right now or in a
followup CL

And as a high-level owner, please also stamp the mechanical changes :)

Thanks!
Martin

[commit-bot: I haz the power, 2017-04-19 15:05:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 15:05:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2017-04-20 00:39:48]

just looked at content/public for now. i suspect the rest is mechanical moves,
and it'll change slightly depending on the outcome of these comments

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
File content/public/browser/browsing_data_remover.h (right):

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/browsing_data_remover.h:15: #include
"content/public/browser/browsing_data_remover_delegate.h"
nit: just forward declare

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/browsing_data_remover.h:56: class CONTENT_EXPORT
BrowsingDataRemover {
nit: not needed on interfaces, so remove along with the include

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/browsing_data_remover.h:122: class Observer {
for this class and the next one, can we just use callbacks instead of adding two
single-method interfaces nested in this interface?

we used to have single-method interfaces before bind came along, but now it's
much more verbose (and one has to worry about lifetime)

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/browsing_data_remover.h:151: virtual
BrowsingDataRemoverDelegate* GetEmbedderDelegate() const = 0;
it's a bit odd that the embedder sets the delegate, and then needs to ask for it
again? i.e. why does it have to reach back to content to know about its class?

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
File content/public/browser/browsing_data_remover_delegate.h (right):

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/browsing_data_remover_delegate.h:25: class CONTENT_EXPORT
BrowsingDataRemoverDelegate {
ditto re export

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/browsing_data_remover_delegate.h:41: virtual
EmbedderOriginTypeMatcher GetOriginTypeMatcher() const = 0;
why is this passed through a delegate, can't it just be an argument for
constructing the BrowsingDataRemover object?

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/browsing_data_remover_delegate.h:50: const base::Closure&
callback) = 0;
if the first method is removed, can this one also be passed as a callback to
BrowsingDataRemover and then we can get rid of this delegate interface
altogether?

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/content_browser_client.h:565: virtual bool
MayDeleteHistory(BrowserContext* browser_context);
it looks like DATA_TYPE_DOWNLOADS is only set by the embedder (i.e. in response
to settings UI or extensions code). if so, instead of chrome telling content to
delete downloads, and content having to check back with chrome, why not have
chrome do this check before telling content? then you don't need this method

[msramek, 2017-04-21 15:22:12]

Thanks, John! I addressed some of your comments, for the rest I have
clarification questions :)

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
File content/public/browser/browsing_data_remover.h (right):

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/browsing_data_remover.h:15: #include
"content/public/browser/browsing_data_remover_delegate.h"
On 2017/04/20 00:39:48, jam wrote:
> nit: just forward declare

Done. Also added a few more forward declarations that are now necessary.

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/browsing_data_remover.h:56: class CONTENT_EXPORT
BrowsingDataRemover {
On 2017/04/20 00:39:48, jam wrote:
> nit: not needed on interfaces, so remove along with the include

Done.

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/browsing_data_remover.h:122: class Observer {
On 2017/04/20 00:39:48, jam wrote:
> for this class and the next one, can we just use callbacks instead of adding
two
> single-method interfaces nested in this interface?
> 
> we used to have single-method interfaces before bind came along, but now it's
> much more verbose (and one has to worry about lifetime)

The second class (CompletionInhibitor) can be removed. I have already done the
change you suggest in a separate CL, then rebased this one on top of it, and it
apparently accidentally sneaked the class back in while addressing merge
conflicts. Removed.

Regarding the first class (Observer), I was doing some refactoring in the past
where I considered replacing it with a callback, but it seemed to me that
keeping it as a class was better. Here's the discussion:
https://docs.google.com/document/d/1oIbmZJlq5m5FvHcN7trCpp0h4mtydA0UfZasxEIe3...

I'm happy to hear your thoughts on this. However, even if we did end up
converting it to a callback, I would still prefer having a follow-up CL, since
it also requires updating numerous callsites.

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/browsing_data_remover.h:151: virtual
BrowsingDataRemoverDelegate* GetEmbedderDelegate() const = 0;
On 2017/04/20 00:39:48, jam wrote:
> it's a bit odd that the embedder sets the delegate, and then needs to ask for
it
> again? i.e. why does it have to reach back to content to know about its class?

We don't, and removing it really made the code a bit cleaner :)

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
File content/public/browser/browsing_data_remover_delegate.h (right):

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/browsing_data_remover_delegate.h:25: class CONTENT_EXPORT
BrowsingDataRemoverDelegate {
On 2017/04/20 00:39:48, jam wrote:
> ditto re export

Done.

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/browsing_data_remover_delegate.h:41: virtual
EmbedderOriginTypeMatcher GetOriginTypeMatcher() const = 0;
On 2017/04/20 00:39:48, jam wrote:
> why is this passed through a delegate, can't it just be an argument for
> constructing the BrowsingDataRemover object?

There should be one such method/callback per delegate (or rather per delegate
class, which could be achieved with templates, but rather not). When delegate
changes using SetEmbedderDelegate(), it needs to provide a new
EmbedderOriginTypeMatcher.

However, in practice delegates don't need to change during the lifetime of
BrowsingDataRemover, so we can simplify this. Remove SetEmbedderDelegate() and
pass both methods as callbacks in the constructor.

That means replacing

virtual BrowsingDataRemoverDelegate*
BrowserContext::GetBrowsingDataRemoverDelegate() = 0;

with

virtual EmbedderOriginTypeMatcher BrowserContext::GetOriginTypeMatcher() = 0;
virtual base::Callback<...> BrowserContext::GetRemoveEmbedderDataCallback() = 0;

?

Which seems to me that we're trading one less class in content/public with a
messier BrowserContext interface. I just want to confirm if you're sure about
that.

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/browsing_data_remover_delegate.h:50: const base::Closure&
callback) = 0;
On 2017/04/20 00:39:48, jam wrote:
> if the first method is removed, can this one also be passed as a callback to
> BrowsingDataRemover and then we can get rid of this delegate interface
> altogether?

Yes. Please see the above answer.

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/content_browser_client.h:565: virtual bool
MayDeleteHistory(BrowserContext* browser_context);
On 2017/04/20 00:39:48, jam wrote:
> it looks like DATA_TYPE_DOWNLOADS is only set by the embedder (i.e. in
response
> to settings UI or extensions code). if so, instead of chrome telling content
to
> delete downloads, and content having to check back with chrome, why not have
> chrome do this check before telling content? then you don't need this method

Isn't that dangerous though? If someone adds a new DATA_TYPE_DOWNLOADS deletion
callsite inside content/, this check would be skipped. And it's an intended
result of this refactoring that people may start using BrowsingDataRemover for
deleting datatypes in content.

If DATA_TYPE_DOWNLOADS is to stay in content/, I think we need the current
format. Alternatively, we can move it into the embedder until someone needs it
in content/ for the first time.

[jam, 2017-04-21 22:58:31]

lgtm

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
File content/public/browser/browsing_data_remover.h (right):

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/browsing_data_remover.h:122: class Observer {
On 2017/04/21 15:22:12, msramek wrote:
> On 2017/04/20 00:39:48, jam wrote:
> > for this class and the next one, can we just use callbacks instead of adding
> two
> > single-method interfaces nested in this interface?
> > 
> > we used to have single-method interfaces before bind came along, but now
it's
> > much more verbose (and one has to worry about lifetime)
> 
> The second class (CompletionInhibitor) can be removed. I have already done the
> change you suggest in a separate CL, then rebased this one on top of it, and
it
> apparently accidentally sneaked the class back in while addressing merge
> conflicts. Removed.
> 
> Regarding the first class (Observer), I was doing some refactoring in the past
> where I considered replacing it with a callback, but it seemed to me that
> keeping it as a class was better. Here's the discussion:
>
https://docs.google.com/document/d/1oIbmZJlq5m5FvHcN7trCpp0h4mtydA0UfZasxEIe3...
> 
> I'm happy to hear your thoughts on this. However, even if we did end up
> converting it to a callback, I would still prefer having a follow-up CL, since
> it also requires updating numerous callsites.

Thanks, I read the discussion above. The major point seems to be adding a weak
pointer factory to the callsites. I went through the existing callsites, and of
the 10 or so, almost all either had a weakptrfactory already or were refcounted.
So I think it'll be simpler to use a callback. However I agree with you to not
bloat this further, so doing it in a followup is fine.

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
File content/public/browser/browsing_data_remover_delegate.h (right):

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/browsing_data_remover_delegate.h:41: virtual
EmbedderOriginTypeMatcher GetOriginTypeMatcher() const = 0;
On 2017/04/21 15:22:12, msramek wrote:
> On 2017/04/20 00:39:48, jam wrote:
> > why is this passed through a delegate, can't it just be an argument for
> > constructing the BrowsingDataRemover object?
> 
> There should be one such method/callback per delegate (or rather per delegate
> class, which could be achieved with templates, but rather not). When delegate
> changes using SetEmbedderDelegate(), it needs to provide a new
> EmbedderOriginTypeMatcher.
> 
> However, in practice delegates don't need to change during the lifetime of
> BrowsingDataRemover, so we can simplify this. Remove SetEmbedderDelegate() and
> pass both methods as callbacks in the constructor.
> 
> That means replacing
> 
> virtual BrowsingDataRemoverDelegate*
> BrowserContext::GetBrowsingDataRemoverDelegate() = 0;
> 
> with
> 
> virtual EmbedderOriginTypeMatcher BrowserContext::GetOriginTypeMatcher() = 0;
> virtual base::Callback<...> BrowserContext::GetRemoveEmbedderDataCallback() =
0;
> 
> ?
> 
> Which seems to me that we're trading one less class in content/public with a
> messier BrowserContext interface. I just want to confirm if you're sure about
> that.

ah, I was not thinking of the case when content creates the data remover itself.
i.e. if chrome is always creating it, it could have passed this in as an
argument. so nvm then.

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/content_browser_client.h:565: virtual bool
MayDeleteHistory(BrowserContext* browser_context);
On 2017/04/21 15:22:12, msramek wrote:
> On 2017/04/20 00:39:48, jam wrote:
> > it looks like DATA_TYPE_DOWNLOADS is only set by the embedder (i.e. in
> response
> > to settings UI or extensions code). if so, instead of chrome telling content
> to
> > delete downloads, and content having to check back with chrome, why not have
> > chrome do this check before telling content? then you don't need this method
> 
> Isn't that dangerous though? If someone adds a new DATA_TYPE_DOWNLOADS
deletion
> callsite inside content/, this check would be skipped. And it's an intended
> result of this refactoring that people may start using BrowsingDataRemover for
> deleting datatypes in content.
> 
> If DATA_TYPE_DOWNLOADS is to stay in content/, I think we need the current
> format. Alternatively, we can move it into the embedder until someone needs it
> in content/ for the first time.

Can you perhaps add a dcheck that DATA_TYPE_DOWNLOADS is only specified by the
embedder? then only if we need to call this in content do we add this new api

https://codereview.chromium.org/2827523003/diff/170001/content/browser/browsi...
File content/browser/browsing_data/browsing_data_remover_impl.cc (right):

https://codereview.chromium.org/2827523003/diff/170001/content/browser/browsi...
content/browser/browsing_data/browsing_data_remover_impl.cc:433:
content::StoragePartition* storage_partition;
no content::

https://codereview.chromium.org/2827523003/diff/170001/content/browser/browsi...
File content/browser/browsing_data/browsing_data_remover_impl_unittest.cc
(right):

https://codereview.chromium.org/2827523003/diff/170001/content/browser/browsi...
content/browser/browsing_data/browsing_data_remover_impl_unittest.cc:515:
content::BrowserContext::GetBrowsingDataRemover(
ditto

https://codereview.chromium.org/2827523003/diff/170001/content/public/browser...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/2827523003/diff/170001/content/public/browser...
content/public/browser/browser_context.h:91: static
content::BrowsingDataRemover* GetBrowsingDataRemover(
nit: no content::

[msramek, 2017-04-25 01:20:30]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 01:20:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2017-04-25 01:21:50]

Patchset #5 (id:190001) has been deleted

[msramek, 2017-04-25 01:29:10]

Thanks, John! Please take one more look, whether you're OK with moving the
downloads history deletion test to the delegate interface.

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
File content/public/browser/browsing_data_remover.h (right):

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/browsing_data_remover.h:122: class Observer {
On 2017/04/21 22:58:30, jam wrote:
> On 2017/04/21 15:22:12, msramek wrote:
> > On 2017/04/20 00:39:48, jam wrote:
> > > for this class and the next one, can we just use callbacks instead of
adding
> > two
> > > single-method interfaces nested in this interface?
> > > 
> > > we used to have single-method interfaces before bind came along, but now
> it's
> > > much more verbose (and one has to worry about lifetime)
> > 
> > The second class (CompletionInhibitor) can be removed. I have already done
the
> > change you suggest in a separate CL, then rebased this one on top of it, and
> it
> > apparently accidentally sneaked the class back in while addressing merge
> > conflicts. Removed.
> > 
> > Regarding the first class (Observer), I was doing some refactoring in the
past
> > where I considered replacing it with a callback, but it seemed to me that
> > keeping it as a class was better. Here's the discussion:
> >
>
https://docs.google.com/document/d/1oIbmZJlq5m5FvHcN7trCpp0h4mtydA0UfZasxEIe3...
> > 
> > I'm happy to hear your thoughts on this. However, even if we did end up
> > converting it to a callback, I would still prefer having a follow-up CL,
since
> > it also requires updating numerous callsites.
> 
> Thanks, I read the discussion above. The major point seems to be adding a weak
> pointer factory to the callsites. I went through the existing callsites, and
of
> the 10 or so, almost all either had a weakptrfactory already or were
refcounted.
> So I think it'll be simpler to use a callback. However I agree with you to not
> bloat this further, so doing it in a followup is fine.

Alright! I'll send you a followup to convert them to callbacks soon.

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
File content/public/browser/browsing_data_remover_delegate.h (right):

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/browsing_data_remover_delegate.h:41: virtual
EmbedderOriginTypeMatcher GetOriginTypeMatcher() const = 0;
On 2017/04/21 22:58:30, jam wrote:
> On 2017/04/21 15:22:12, msramek wrote:
> > On 2017/04/20 00:39:48, jam wrote:
> > > why is this passed through a delegate, can't it just be an argument for
> > > constructing the BrowsingDataRemover object?
> > 
> > There should be one such method/callback per delegate (or rather per
delegate
> > class, which could be achieved with templates, but rather not). When
delegate
> > changes using SetEmbedderDelegate(), it needs to provide a new
> > EmbedderOriginTypeMatcher.
> > 
> > However, in practice delegates don't need to change during the lifetime of
> > BrowsingDataRemover, so we can simplify this. Remove SetEmbedderDelegate()
and
> > pass both methods as callbacks in the constructor.
> > 
> > That means replacing
> > 
> > virtual BrowsingDataRemoverDelegate*
> > BrowserContext::GetBrowsingDataRemoverDelegate() = 0;
> > 
> > with
> > 
> > virtual EmbedderOriginTypeMatcher BrowserContext::GetOriginTypeMatcher() =
0;
> > virtual base::Callback<...> BrowserContext::GetRemoveEmbedderDataCallback()
=
> 0;
> > 
> > ?
> > 
> > Which seems to me that we're trading one less class in content/public with a
> > messier BrowserContext interface. I just want to confirm if you're sure
about
> > that.
> 
> ah, I was not thinking of the case when content creates the data remover
itself.
> i.e. if chrome is always creating it, it could have passed this in as an
> argument. so nvm then.

Acknowledged. I'm keeping the current structure then.

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2827523003/diff/150001/content/public/browser...
content/public/browser/content_browser_client.h:565: virtual bool
MayDeleteHistory(BrowserContext* browser_context);
On 2017/04/21 22:58:30, jam wrote:
> On 2017/04/21 15:22:12, msramek wrote:
> > On 2017/04/20 00:39:48, jam wrote:
> > > it looks like DATA_TYPE_DOWNLOADS is only set by the embedder (i.e. in
> > response
> > > to settings UI or extensions code). if so, instead of chrome telling
content
> > to
> > > delete downloads, and content having to check back with chrome, why not
have
> > > chrome do this check before telling content? then you don't need this
method
> > 
> > Isn't that dangerous though? If someone adds a new DATA_TYPE_DOWNLOADS
> deletion
> > callsite inside content/, this check would be skipped. And it's an intended
> > result of this refactoring that people may start using BrowsingDataRemover
for
> > deleting datatypes in content.
> > 
> > If DATA_TYPE_DOWNLOADS is to stay in content/, I think we need the current
> > format. Alternatively, we can move it into the embedder until someone needs
it
> > in content/ for the first time.
> 
> Can you perhaps add a dcheck that DATA_TYPE_DOWNLOADS is only specified by the
> embedder? then only if we need to call this in content do we add this new api

Unfortunately, no. Inside BrowsingDataRemoverImpl::RemoveImpl, we don't know who
the caller is.

But now that we decided to keep BrowsingDataRemoverDelegate after all, can we
just put the method there? I have done that in my latest patchset. The name
MayRemoveDownloadHistory() is still somewhat Chrome-specific at this point, but
can be easily expanded to something more generic like MayRemove(int remove_mask)
if needed.

https://codereview.chromium.org/2827523003/diff/170001/content/browser/browsi...
File content/browser/browsing_data/browsing_data_remover_impl.cc (right):

https://codereview.chromium.org/2827523003/diff/170001/content/browser/browsi...
content/browser/browsing_data/browsing_data_remover_impl.cc:433:
content::StoragePartition* storage_partition;
On 2017/04/21 22:58:31, jam wrote:
> no content::

Done.

https://codereview.chromium.org/2827523003/diff/170001/content/browser/browsi...
File content/browser/browsing_data/browsing_data_remover_impl_unittest.cc
(right):

https://codereview.chromium.org/2827523003/diff/170001/content/browser/browsi...
content/browser/browsing_data/browsing_data_remover_impl_unittest.cc:515:
content::BrowserContext::GetBrowsingDataRemover(
On 2017/04/21 22:58:31, jam wrote:
> ditto

Done.

https://codereview.chromium.org/2827523003/diff/170001/content/public/browser...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/2827523003/diff/170001/content/public/browser...
content/public/browser/browser_context.h:91: static
content::BrowsingDataRemover* GetBrowsingDataRemover(
On 2017/04/21 22:58:31, jam wrote:
> nit: no content::

Done. Here and in the old code below.

[msramek, 2017-04-25 01:29:31]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 01:29:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 01:34:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 01:34:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msramek, 2017-04-25 01:58:00]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 01:58:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 02:11:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 02:11:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msramek, 2017-04-25 04:05:59]

Patchset #6 (id:230001) has been deleted

[msramek, 2017-04-25 04:06:38]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 04:07:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 05:44:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 05:44:45]

Dry run: This issue passed the CQ dry run.

[jam, 2017-04-26 00:20:16]

still lgtm

[msramek, 2017-04-26 01:03:50]

Description was changed from

==========
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 ContentBrowserClient.
The default value is true, ChromeContentBrowserClient 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.

BUG=668114
==========

to

==========
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 ContentBrowserClient.
The default value is true, ChromeContentBrowserClient 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
==========

[msramek, 2017-04-26 01:03:50]

msramek@chromium.org changed reviewers:
+ dbeam@chromium.org

[msramek, 2017-04-26 01:04:55]

Description was changed from

==========
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 ContentBrowserClient.
The default value is true, ChromeContentBrowserClient 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
==========

to

==========
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
==========

[msramek, 2017-04-26 01:10:56]

Thanks, John!

I'm adding +Dan to TBR for the webui/ files (ClearBrowserDataHandler and its
test), as all the changes there are just equivalent replacements (added
content:: namespaces, changed includes for moved files, and changed the factory
method call for BrowsingDataRemover to the new one).

I updated the description with the changes from the review and will proceed to
land this.

[msramek, 2017-04-26 01:11:42]

The CQ bit was checked by msramek@chromium.org

[msramek, 2017-04-26 01:11:43]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2827523003/#ps250001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-04-26 01:12:18]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 01:22:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 01:22:36]

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_presub...)

[msramek, 2017-04-26 01:48:03]

Patchset #7 (id:270001) has been deleted

[msramek, 2017-04-26 01:49:08]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 01:49:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 03:13:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 03:13:53]

Dry run: This issue passed the CQ dry run.

[msramek, 2017-04-26 05:13:24]

...and another rebase.

codereview.chromium.org/2815913005 added scoped pointers for UserData, which
caused a merge conflict in browsing_data_remover_impl.cc (where a
DownloadManager pointer changed to a scoped ptr), and required changing
"remover.release()" to "std::move(remover)" in BrowserContext. Fixed now, trying
to land again...

[msramek, 2017-04-26 05:14:07]

The CQ bit was checked by msramek@chromium.org

[msramek, 2017-04-26 05:14:08]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2827523003/#ps290001
(title: "Rebase over codereview.chromium.org/2815913005")

[commit-bot: I haz the power, 2017-04-26 05:14:30]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 05:21:48]

CQ is committing da patch.

Bot data: {"patchset_id": 290001, "attempt_start_ts": 1493183647769200,
"parent_rev": "ac3c4618a593af5860af38169befa3890e248b34", "commit_rev":
"e169ccbe5d1f406337c2b5e758f58b7966c51b6c"}

[commit-bot: I haz the power, 2017-04-26 05:22:24]

Description was changed from

==========
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
==========

to

==========
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/+/e169ccbe5d1f406337c2b5e758f5...
==========

[commit-bot: I haz the power, 2017-04-26 05:22:26]

Committed patchset #7 (id:290001) as
https://chromium.googlesource.com/chromium/src/+/e169ccbe5d1f406337c2b5e758f5...