Split BrowsingDataRemover into an abstract interface and implementation.

Split BrowsingDataRemover into an abstract interface and implementation.

As a part of the migration of BrowsingDataRemover to content/, we split
it to a pure abstract interface BrowsingDataRemover that declares
- Subclasses and enums
- Removal methods.
- Observer methods.
- Inspection methods for testing.
and its implementation BrowsingDataRemoverImpl.

In the future, BrowsingDataRemover will live in content/public/ to be
used by embedders and BrowsingDataRemoverImpl in content/browsing_data/
as its only implementation. The unittest and browsertest, whose
content-related parts will be later extracted to content/browsing_data/,
have been updated to make a static_cast<> to BrowsingDataRemoverImpl
(as this is a valid assumption).

Note that the SubTask subclass has been duplicated in
BrowsingDataRemoverImpl and ChromeBrowsingDataRemoverDelegate. This is
because the fact that the deletion is organized into subtasks is an
implementation detail that does not belong to the abstract interface
of BrowsingDataRemover; and the fact that both classes do it the same
way is a "coincidence". TODO: Investigate how to avoid this duplication.

BUG=668114
Review-Url: https://codereview.chromium.org/2613833004
Cr-Commit-Position: refs/heads/master@{#442566}
Committed: https://chromium.googlesource.com/chromium/src/+/78976d92399cc5d737657fb15269fd4d987208e7

[msramek, 2017-01-05 23:57:40]

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

[commit-bot: I haz the power, 2017-01-05 23:58:13]

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

[msramek, 2017-01-06 00:20:19]

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

[msramek, 2017-01-06 00:20:20]

Hi Bernhard!

Can you please have a look? This is another step in the BrowsingDataRemover
migration to content.

Btw, I'm still thinking about how to reconcile SubTasks and CompletionInhibitor
(as we discussed in an earlier CL), as the few approaches I tried weren't really
elegant / didn't cut many lines of code. The situation is becoming a bit more
complicated after this CL, so I'll try to focus on that again.

Thanks,
Martin

[commit-bot: I haz the power, 2017-01-06 00:22:06]

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

[commit-bot: I haz the power, 2017-01-06 00:22:07]

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-01-10 10:32:39]

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

[commit-bot: I haz the power, 2017-01-10 10:32:51]

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-01-10 10:34:27]

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

[commit-bot: I haz the power, 2017-01-10 10:34:28]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Bernhard Bauer, 2017-01-10 10:39:47]

LGTM

Yeah, it's a bit unfortunate to have to duplicate SubTask... My hope is that
it's simple enough it doesn't have a big impact.

https://codereview.chromium.org/2613833004/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/2613833004/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:1182:
BrowsingDataRemoverImpl* remover = static_cast<BrowsingDataRemoverImpl*>(
Don't we already have this in |remover_|?

[msramek, 2017-01-10 10:59:47]

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

[commit-bot: I haz the power, 2017-01-10 10:59:56]

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-01-10 12:06:18]

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

[commit-bot: I haz the power, 2017-01-10 12:06:18]

Dry run: This issue passed the CQ dry run.

[msramek, 2017-01-10 12:08:34]

Thanks!

I had to fix the Android compilation and rebase in the meantime. I addressed
your comment in the last patchset and will proceed to land this now.

https://codereview.chromium.org/2613833004/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/2613833004/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:1182:
BrowsingDataRemoverImpl* remover = static_cast<BrowsingDataRemoverImpl*>(
On 2017/01/10 10:39:47, Bernhard Bauer wrote:
> Don't we already have this in |remover_|?

Indeed. This is probably a remnant from the times when BrowsingDataRemover
wasn't a KeyedService and had to be instantiated each time. Replaced here and
below.

[msramek, 2017-01-10 12:08:43]

The CQ bit was checked by msramek@chromium.org

[msramek, 2017-01-10 12:08:44]

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/2613833004/#ps60001
(title: "Removed unnecessary instantiations.")

[commit-bot: I haz the power, 2017-01-10 12:08:56]

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-01-10 13:11:45]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484050123985030,
"parent_rev": "149f53e13220adcbdf02bc8126840f0677a49050", "commit_rev":
"78976d92399cc5d737657fb15269fd4d987208e7"}

[commit-bot: I haz the power, 2017-01-10 13:12:19]

Description was changed from

==========
Split BrowsingDataRemover into an abstract interface and implementation.

As a part of the migration of BrowsingDataRemover to content/, we split
it to a pure abstract interface BrowsingDataRemover that declares
- Subclasses and enums
- Removal methods.
- Observer methods.
- Inspection methods for testing.
and its implementation BrowsingDataRemoverImpl.

In the future, BrowsingDataRemover will live in content/public/ to be
used by embedders and BrowsingDataRemoverImpl in content/browsing_data/
as its only implementation. The unittest and browsertest, whose
content-related parts will be later extracted to content/browsing_data/,
have been updated to make a static_cast<> to BrowsingDataRemoverImpl
(as this is a valid assumption).

Note that the SubTask subclass has been duplicated in
BrowsingDataRemoverImpl and ChromeBrowsingDataRemoverDelegate. This is
because the fact that the deletion is organized into subtasks is an
implementation detail that does not belong to the abstract interface
of BrowsingDataRemover; and the fact that both classes do it the same
way is a "coincidence". TODO: Investigate how to avoid this duplication.

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

to

==========
Split BrowsingDataRemover into an abstract interface and implementation.

As a part of the migration of BrowsingDataRemover to content/, we split
it to a pure abstract interface BrowsingDataRemover that declares
- Subclasses and enums
- Removal methods.
- Observer methods.
- Inspection methods for testing.
and its implementation BrowsingDataRemoverImpl.

In the future, BrowsingDataRemover will live in content/public/ to be
used by embedders and BrowsingDataRemoverImpl in content/browsing_data/
as its only implementation. The unittest and browsertest, whose
content-related parts will be later extracted to content/browsing_data/,
have been updated to make a static_cast<> to BrowsingDataRemoverImpl
(as this is a valid assumption).

Note that the SubTask subclass has been duplicated in
BrowsingDataRemoverImpl and ChromeBrowsingDataRemoverDelegate. This is
because the fact that the deletion is organized into subtasks is an
implementation detail that does not belong to the abstract interface
of BrowsingDataRemover; and the fact that both classes do it the same
way is a "coincidence". TODO: Investigate how to avoid this duplication.

BUG=668114

Review-Url: https://codereview.chromium.org/2613833004
Cr-Commit-Position: refs/heads/master@{#442566}
Committed:
https://chromium.googlesource.com/chromium/src/+/78976d92399cc5d737657fb15269...
==========

[commit-bot: I haz the power, 2017-01-10 13:12:20]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/78976d92399cc5d737657fb15269...