Reduce BrowsingDataRemover's dependencies on Chrome

Reduce BrowsingDataRemover's dependencies on Chrome

1. Remove TimePeriod-related methods from BDR and its tests. Explanation:

TimePeriod represents the deletion options provided in the Chrome UI, and
is used in communication between Chrome UI and BDR, and thus there is no
reason for content/ to know about it. In fact, if BDR was moved to content/
while still containing TimePeriod-related methods, we would have to introduce
a new build target in components/ to make TimePeriod usable in all three of
chrome/, ios/, and content/.

2. Remove the TimeRange class from BDR entirely. Explanation:

TimeRange is a class inside BDR which would move to content/ together with it.
The UI needs a way to convert TimePeriod to TimeRange. Such a conversion cannot
live in content/ (which doesn't know about TimePeriod) nor components/ (which
wouldn't know about TimeRange). It could be detached from BDR to a separate
file in chrome/.

However, the update cost of detaching it to a separate file is the same as
breaking it into a pair of base::Time arguments. Here are reasons why the latter
is better:
-> It reduces the complexity of keeping two confusingly similar TimePeriod and
TimeRange classes
-> Consistency - basically all data storage backends, and also the BDRDelegate,
have an interface with two base::Time arguments rather than base::TimeRange
-> It can be argued that this definition does not logically belong to BDR;
if anyone, it should be base/time/ who defines a class representing
a base::Time interval class

3. Replace Profile with BrowserContext inside BDR (as that's the content/
equivalent).

4. Split the REMOVE_DOWNLOADS section to two parts. The first part (download
history deletion) stays in content/, the second part (download path reset) moves
to the embedder.

5. Add a comment explaining that the |may_delete_history| variable must be honored
in content/ (while deleting download history), but must be provided by the embedder
(from policy or supervised users) through ContentBrowserClient. This is not possible
to fix while BDR is still in chrome/, as chrome/ cannot depend on
ChromeContentBrowserClient.

TBR=jochen@chromium.org
BUG=668114
Committed: https://crrev.com/1c8e19d3f6a71793b80525aa995723ece0574756
Cr-Commit-Position: refs/heads/master@{#441440}

[msramek, 2016-12-14 20:36:25]

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

[commit-bot: I haz the power, 2016-12-14 20:37:21]

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, 2016-12-14 20:53:15]

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

[commit-bot: I haz the power, 2016-12-14 20:53:16]

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

[msramek, 2016-12-14 22:54:06]

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

[commit-bot: I haz the power, 2016-12-14 22:54:55]

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, 2016-12-15 00:06:21]

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

[commit-bot: I haz the power, 2016-12-15 00:06:22]

Dry run: This issue passed the CQ dry run.

[msramek, 2016-12-15 12:39:38]

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

[commit-bot: I haz the power, 2016-12-15 12:39:52]

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

[msramek, 2016-12-15 12:41:10]

Patchset #1 (id:1) has been deleted

[msramek, 2016-12-15 12:43:07]

Patchset #1 (id:20001) has been deleted

[msramek, 2016-12-15 12:48:48]

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

[msramek, 2016-12-15 12:48:49]

Hi Bernhard!

Please have a look at this next CL in the process of content-ifying BDR.

This CL evolved sort of organically, and it might make sense to break it down to
smaller parts; let me know if you prefer to do so. But basically, #1 is the
large change, #2 is a not strictly necessary but I think a reasonable followup,
and #3, #4, #5 are just minor changes that thematically belong but could be
split.

Thanks!
Martin

[commit-bot: I haz the power, 2016-12-15 13:40:30]

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

[commit-bot: I haz the power, 2016-12-15 13:40:31]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-12-16 17:03:05]

LGTM

https://codereview.chromium.org/2578723002/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/2578723002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:399: base::Time
HourAgo() {
Small nit: AnHourAgo()?

[msramek, 2017-01-03 11:54:27]

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

[commit-bot: I haz the power, 2017-01-03 11:54:37]

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-03 11:59:04]

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

[commit-bot: I haz the power, 2017-01-03 11:59:09]

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-03 12:06:03]

Description was changed from

==========
Reduce BrowsingDataRemover's dependencies on Chrome

1. Remove TimePeriod-related methods from BDR and its tests. Explanation:

TimePeriod represents the deletion options provided in the Chrome UI, and
is used in communication between Chrome UI and BDR, and thus there is no
reason for content/ to know about it. In fact, if BDR was moved to content/
while still containing TimePeriod-related methods, we would have to introduce
a new build target in components/ to make TimePeriod usable in all three of
chrome/, ios/, and content/.

2. Remove the TimeRange class from BDR entirely. Explanation:

TimeRange is a class inside BDR which would move to content/ together with it.
The UI needs a way to convert TimePeriod to TimeRange. Such a conversion cannot
live in content/ (which doesn't know about TimePeriod) nor components/ (which
wouldn't know about TimeRange). It could be detached from BDR to a separate
file in chrome/.

However, the update cost of detaching it to a separate file is the same as
breaking it into a pair of base::Time arguments. Here are reasons why the latter
is better:
-> It reduces the complexity of keeping two confusingly similar TimePeriod and
TimeRange classes
-> Consistency - basically all data storage backends, and also the BDRDelegate,
have an interface with two base::Time arguments rather than base::TimeRange
-> It can be argued that this definition does not logically belong to BDR;
if anyone, it should be base/time/ who defines a class representing
a base::Time interval class

3. Replace Profile with BrowserContext inside BDR (as that's the content/
equivalent).

4. Split the REMOVE_DOWNLOADS section to two parts. The first part (download
history deletion) stays in content/, the second part (download path reset) moves
to the embedder.

5. Add a comment explaining that the |may_delete_history| variable must be
honored
in content/ (while deleting download history), but must be provided by the
embedder
(from policy or supervised users) through ContentBrowserClient. This is not
possible
to fix while BDR is still in chrome/, as chrome/ cannot depend on
ChromeContentBrowserClient.

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

to

==========
Reduce BrowsingDataRemover's dependencies on Chrome

1. Remove TimePeriod-related methods from BDR and its tests. Explanation:

TimePeriod represents the deletion options provided in the Chrome UI, and
is used in communication between Chrome UI and BDR, and thus there is no
reason for content/ to know about it. In fact, if BDR was moved to content/
while still containing TimePeriod-related methods, we would have to introduce
a new build target in components/ to make TimePeriod usable in all three of
chrome/, ios/, and content/.

2. Remove the TimeRange class from BDR entirely. Explanation:

TimeRange is a class inside BDR which would move to content/ together with it.
The UI needs a way to convert TimePeriod to TimeRange. Such a conversion cannot
live in content/ (which doesn't know about TimePeriod) nor components/ (which
wouldn't know about TimeRange). It could be detached from BDR to a separate
file in chrome/.

However, the update cost of detaching it to a separate file is the same as
breaking it into a pair of base::Time arguments. Here are reasons why the latter
is better:
-> It reduces the complexity of keeping two confusingly similar TimePeriod and
TimeRange classes
-> Consistency - basically all data storage backends, and also the BDRDelegate,
have an interface with two base::Time arguments rather than base::TimeRange
-> It can be argued that this definition does not logically belong to BDR;
if anyone, it should be base/time/ who defines a class representing
a base::Time interval class

3. Replace Profile with BrowserContext inside BDR (as that's the content/
equivalent).

4. Split the REMOVE_DOWNLOADS section to two parts. The first part (download
history deletion) stays in content/, the second part (download path reset) moves
to the embedder.

5. Add a comment explaining that the |may_delete_history| variable must be
honored
in content/ (while deleting download history), but must be provided by the
embedder
(from policy or supervised users) through ContentBrowserClient. This is not
possible
to fix while BDR is still in chrome/, as chrome/ cannot depend on
ChromeContentBrowserClient.

TBR=jochen@chromium.org
BUG=668114
==========

[msramek, 2017-01-03 12:06:52]

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

[msramek, 2017-01-03 12:06:53]

Thanks Bernhard!

+Dan PTAL at the changes in webui/ - it's just a replacement of TimeRange with
the new interface, but it's a few lines, so I decided not to TBR.

+Jochen as TBR for all other chrome/browser/ files that are not browsing_data/
or webui/, those are really just one-line replacements

And happy new year!

https://codereview.chromium.org/2578723002/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/2578723002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:399: base::Time
HourAgo() {
On 2016/12/16 17:03:05, Bernhard (just came back) wrote:
> Small nit: AnHourAgo()?

Done.

[commit-bot: I haz the power, 2017-01-03 12:11:40]

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

[commit-bot: I haz the power, 2017-01-03 12:11:41]

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

[jochen (gone - plz use gerrit), 2017-01-03 12:13:03]

lgtm

[msramek, 2017-01-03 12:37:48]

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

[commit-bot: I haz the power, 2017-01-03 12:38:04]

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-03 13:25:05]

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

[commit-bot: I haz the power, 2017-01-03 13:25:06]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-01-04 18:56:53]

lgtm

[msramek, 2017-01-04 19:09:49]

Thanks both!

[msramek, 2017-01-04 19:09:58]

The CQ bit was checked by msramek@chromium.org

[msramek, 2017-01-04 19:09:59]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2578723002/#ps100001
(title: "A new callsite appeared through rebase - fixed the compilation error.")

[commit-bot: I haz the power, 2017-01-04 19:10:27]

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-04 20:04:56]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483556998995500,
"parent_rev": "83a3c697fd939affc02eb326c38ad3517073dce1", "commit_rev":
"e34e8cd26cf4c9bd47bf69d3c2d6a98e80e8e29d"}

[commit-bot: I haz the power, 2017-01-04 20:05:24]

Description was changed from

==========
Reduce BrowsingDataRemover's dependencies on Chrome

1. Remove TimePeriod-related methods from BDR and its tests. Explanation:

TimePeriod represents the deletion options provided in the Chrome UI, and
is used in communication between Chrome UI and BDR, and thus there is no
reason for content/ to know about it. In fact, if BDR was moved to content/
while still containing TimePeriod-related methods, we would have to introduce
a new build target in components/ to make TimePeriod usable in all three of
chrome/, ios/, and content/.

2. Remove the TimeRange class from BDR entirely. Explanation:

TimeRange is a class inside BDR which would move to content/ together with it.
The UI needs a way to convert TimePeriod to TimeRange. Such a conversion cannot
live in content/ (which doesn't know about TimePeriod) nor components/ (which
wouldn't know about TimeRange). It could be detached from BDR to a separate
file in chrome/.

However, the update cost of detaching it to a separate file is the same as
breaking it into a pair of base::Time arguments. Here are reasons why the latter
is better:
-> It reduces the complexity of keeping two confusingly similar TimePeriod and
TimeRange classes
-> Consistency - basically all data storage backends, and also the BDRDelegate,
have an interface with two base::Time arguments rather than base::TimeRange
-> It can be argued that this definition does not logically belong to BDR;
if anyone, it should be base/time/ who defines a class representing
a base::Time interval class

3. Replace Profile with BrowserContext inside BDR (as that's the content/
equivalent).

4. Split the REMOVE_DOWNLOADS section to two parts. The first part (download
history deletion) stays in content/, the second part (download path reset) moves
to the embedder.

5. Add a comment explaining that the |may_delete_history| variable must be
honored
in content/ (while deleting download history), but must be provided by the
embedder
(from policy or supervised users) through ContentBrowserClient. This is not
possible
to fix while BDR is still in chrome/, as chrome/ cannot depend on
ChromeContentBrowserClient.

TBR=jochen@chromium.org
BUG=668114
==========

to

==========
Reduce BrowsingDataRemover's dependencies on Chrome

1. Remove TimePeriod-related methods from BDR and its tests. Explanation:

TimePeriod represents the deletion options provided in the Chrome UI, and
is used in communication between Chrome UI and BDR, and thus there is no
reason for content/ to know about it. In fact, if BDR was moved to content/
while still containing TimePeriod-related methods, we would have to introduce
a new build target in components/ to make TimePeriod usable in all three of
chrome/, ios/, and content/.

2. Remove the TimeRange class from BDR entirely. Explanation:

TimeRange is a class inside BDR which would move to content/ together with it.
The UI needs a way to convert TimePeriod to TimeRange. Such a conversion cannot
live in content/ (which doesn't know about TimePeriod) nor components/ (which
wouldn't know about TimeRange). It could be detached from BDR to a separate
file in chrome/.

However, the update cost of detaching it to a separate file is the same as
breaking it into a pair of base::Time arguments. Here are reasons why the latter
is better:
-> It reduces the complexity of keeping two confusingly similar TimePeriod and
TimeRange classes
-> Consistency - basically all data storage backends, and also the BDRDelegate,
have an interface with two base::Time arguments rather than base::TimeRange
-> It can be argued that this definition does not logically belong to BDR;
if anyone, it should be base/time/ who defines a class representing
a base::Time interval class

3. Replace Profile with BrowserContext inside BDR (as that's the content/
equivalent).

4. Split the REMOVE_DOWNLOADS section to two parts. The first part (download
history deletion) stays in content/, the second part (download path reset) moves
to the embedder.

5. Add a comment explaining that the |may_delete_history| variable must be
honored
in content/ (while deleting download history), but must be provided by the
embedder
(from policy or supervised users) through ContentBrowserClient. This is not
possible
to fix while BDR is still in chrome/, as chrome/ cannot depend on
ChromeContentBrowserClient.

TBR=jochen@chromium.org
BUG=668114

Review-Url: https://codereview.chromium.org/2578723002
==========

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

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2017-01-04 20:09:37]

Description was changed from

==========
Reduce BrowsingDataRemover's dependencies on Chrome

1. Remove TimePeriod-related methods from BDR and its tests. Explanation:

TimePeriod represents the deletion options provided in the Chrome UI, and
is used in communication between Chrome UI and BDR, and thus there is no
reason for content/ to know about it. In fact, if BDR was moved to content/
while still containing TimePeriod-related methods, we would have to introduce
a new build target in components/ to make TimePeriod usable in all three of
chrome/, ios/, and content/.

2. Remove the TimeRange class from BDR entirely. Explanation:

TimeRange is a class inside BDR which would move to content/ together with it.
The UI needs a way to convert TimePeriod to TimeRange. Such a conversion cannot
live in content/ (which doesn't know about TimePeriod) nor components/ (which
wouldn't know about TimeRange). It could be detached from BDR to a separate
file in chrome/.

However, the update cost of detaching it to a separate file is the same as
breaking it into a pair of base::Time arguments. Here are reasons why the latter
is better:
-> It reduces the complexity of keeping two confusingly similar TimePeriod and
TimeRange classes
-> Consistency - basically all data storage backends, and also the BDRDelegate,
have an interface with two base::Time arguments rather than base::TimeRange
-> It can be argued that this definition does not logically belong to BDR;
if anyone, it should be base/time/ who defines a class representing
a base::Time interval class

3. Replace Profile with BrowserContext inside BDR (as that's the content/
equivalent).

4. Split the REMOVE_DOWNLOADS section to two parts. The first part (download
history deletion) stays in content/, the second part (download path reset) moves
to the embedder.

5. Add a comment explaining that the |may_delete_history| variable must be
honored
in content/ (while deleting download history), but must be provided by the
embedder
(from policy or supervised users) through ContentBrowserClient. This is not
possible
to fix while BDR is still in chrome/, as chrome/ cannot depend on
ChromeContentBrowserClient.

TBR=jochen@chromium.org
BUG=668114

Review-Url: https://codereview.chromium.org/2578723002
==========

to

==========
Reduce BrowsingDataRemover's dependencies on Chrome

1. Remove TimePeriod-related methods from BDR and its tests. Explanation:

TimePeriod represents the deletion options provided in the Chrome UI, and
is used in communication between Chrome UI and BDR, and thus there is no
reason for content/ to know about it. In fact, if BDR was moved to content/
while still containing TimePeriod-related methods, we would have to introduce
a new build target in components/ to make TimePeriod usable in all three of
chrome/, ios/, and content/.

2. Remove the TimeRange class from BDR entirely. Explanation:

TimeRange is a class inside BDR which would move to content/ together with it.
The UI needs a way to convert TimePeriod to TimeRange. Such a conversion cannot
live in content/ (which doesn't know about TimePeriod) nor components/ (which
wouldn't know about TimeRange). It could be detached from BDR to a separate
file in chrome/.

However, the update cost of detaching it to a separate file is the same as
breaking it into a pair of base::Time arguments. Here are reasons why the latter
is better:
-> It reduces the complexity of keeping two confusingly similar TimePeriod and
TimeRange classes
-> Consistency - basically all data storage backends, and also the BDRDelegate,
have an interface with two base::Time arguments rather than base::TimeRange
-> It can be argued that this definition does not logically belong to BDR;
if anyone, it should be base/time/ who defines a class representing
a base::Time interval class

3. Replace Profile with BrowserContext inside BDR (as that's the content/
equivalent).

4. Split the REMOVE_DOWNLOADS section to two parts. The first part (download
history deletion) stays in content/, the second part (download path reset) moves
to the embedder.

5. Add a comment explaining that the |may_delete_history| variable must be
honored
in content/ (while deleting download history), but must be provided by the
embedder
(from policy or supervised users) through ContentBrowserClient. This is not
possible
to fix while BDR is still in chrome/, as chrome/ cannot depend on
ChromeContentBrowserClient.

TBR=jochen@chromium.org
BUG=668114

Committed: https://crrev.com/1c8e19d3f6a71793b80525aa995723ece0574756
Cr-Commit-Position: refs/heads/master@{#441440}
==========

[commit-bot: I haz the power, 2017-01-04 20:09:39]

Patchset 4 (id:??) landed as
https://crrev.com/1c8e19d3f6a71793b80525aa995723ece0574756
Cr-Commit-Position: refs/heads/master@{#441440}