Consolidate Origin- and RegistrableDomain- FilterBuilder into one class

Consolidate Origin- and RegistrableDomain- FilterBuilder into one class

...called BrowsingDataFilterBuilderImpl.

The new class will eventually live in content/ (together with
BrowsingDataRemoverImpl) but will also be used by chrome/, therefore
we also create an abstract interface BrowsingDataFilterBuilder.

We also consolidate the tests into one file. As the new class is theoretically
able to handle origins and registrable domains at the same time, we add new
tests for these cases (although this situation does not occur in production).

The previous state with two different classes increased the complexity in
ChromeContentBrowserClientTest, which we now simplified.

We also introduced a Copy() method to the new class which allowed us to
duplicate the filter builder in BrowsingDataRemoverImplTest and
ChromeBrowsingDataRemoverDelegateTest where needed, which means we no longer
rely on passing the class via reference, thus we don't need to use the private
BrowsingDataRemoverImpl::RemoveInternal method, and therefore we could remove
friend declarations from BrowsingDataRemoverImpl. As a side effect, calling
the public methods now means that checks for filterable datatypes are
performed, and since HISTORY and PASSWORDS are not yet marked as filterable,
we had to disable the corresponding tests.

TBR=bauerb@chromium.org
BUG=668114
Review-Url: https://codereview.chromium.org/2647683002
Cr-Commit-Position: refs/heads/master@{#445706}
Committed: https://chromium.googlesource.com/chromium/src/+/c7ee9d5e15a8d98d5f5fe2afcff74bec2f3ae7d6

[msramek, 2017-01-19 15:59:12]

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

[commit-bot: I haz the power, 2017-01-19 15:59:31]

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-19 16:13:57]

Patchset #1 (id:1) has been deleted

[msramek, 2017-01-19 16:14:14]

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

[commit-bot: I haz the power, 2017-01-19 16:14:31]

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-19 16:15:51]

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

[msramek, 2017-01-19 16:15:52]

Hi Mike,

Please have a look!

This is based on https://codereview.chromium.org/2641853002/ which I sent to
Bernhard, but the dependency is only on the fact that the test files were split
in that CL; the rest of the changes is valid regardless.

Thanks,
Martin

[commit-bot: I haz the power, 2017-01-19 16:46:40]

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

[commit-bot: I haz the power, 2017-01-19 16:46:41]

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

[msramek, 2017-01-19 16:51:50]

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

[commit-bot: I haz the power, 2017-01-19 16:52: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-19 17:37:37]

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

[commit-bot: I haz the power, 2017-01-19 17:37:38]

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-01-19 17:45:10]

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

[commit-bot: I haz the power, 2017-01-19 17:45: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-01-19 18:31:41]

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

[commit-bot: I haz the power, 2017-01-19 18:31:42]

Dry run: Try jobs failed on following builders:
  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-01-19 19:00:51]

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

[commit-bot: I haz the power, 2017-01-19 19:01: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-01-19 21:07:16]

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

[commit-bot: I haz the power, 2017-01-19 21:07:17]

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

[msramek, 2017-01-19 21:52:45]

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

[commit-bot: I haz the power, 2017-01-19 21:53:06]

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-19 22:54:14]

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

[commit-bot: I haz the power, 2017-01-19 22:54:15]

Dry run: This issue passed the CQ dry run.

[Mike West, 2017-01-24 08:37:27]

LGTM % some questions about the tests (sorry; I added comments a few days ago
and then never sent them. :( )

https://codereview.chromium.org/2647683002/diff/100001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_filter_builder_impl_unittest.cc
(right):

https://codereview.chromium.org/2647683002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_filter_builder_impl_unittest.cc:519:
};
Did you mean to run these test cases?

https://codereview.chromium.org/2647683002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_filter_builder_impl_unittest.cc:539:
};
These too?

https://codereview.chromium.org/2647683002/diff/100001/chrome/browser/browsin...
File
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
(right):

https://codereview.chromium.org/2647683002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1351:
// Disabled, since history is not yet marked as a filterable datatype.
Please add a TODO pointing to the bug.

https://codereview.chromium.org/2647683002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1381:
// Disabled, since passwords are not yet marked as a filterable datatype.
Ditto.

https://codereview.chromium.org/2647683002/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client_unittest.cc (right):

https://codereview.chromium.org/2647683002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client_unittest.cc:414: bool
filter_is_important)
Nit: What does "important" mean? How about "should_compare_filter"?
"filter_is_not_relevant_to_this_test"?

[msramek, 2017-01-24 10:57:44]

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

[commit-bot: I haz the power, 2017-01-24 10:58:02]

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-24 10:58:19]

Patchset #6 (id:120001) has been deleted

[msramek, 2017-01-24 11:00:23]

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

[commit-bot: I haz the power, 2017-01-24 11:00:35]

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-24 11:50:29]

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

[commit-bot: I haz the power, 2017-01-24 11:50:31]

Dry run: This issue passed the CQ dry run.

[msramek, 2017-01-24 11:52:31]

Addressed your comments and rebased. Thanks, Mike, especially for catching those
bogus tests :)

https://codereview.chromium.org/2647683002/diff/100001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_filter_builder_impl_unittest.cc
(right):

https://codereview.chromium.org/2647683002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_filter_builder_impl_unittest.cc:519:
};
On 2017/01/24 08:37:27, Mike West (sloooooow) wrote:
> Did you mean to run these test cases?

Oops! Thanks for catching this! Done.

https://codereview.chromium.org/2647683002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_filter_builder_impl_unittest.cc:539:
};
On 2017/01/24 08:37:27, Mike West (sloooooow) wrote:
> These too?

Yes :) Done.

https://codereview.chromium.org/2647683002/diff/100001/chrome/browser/browsin...
File
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
(right):

https://codereview.chromium.org/2647683002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1351:
// Disabled, since history is not yet marked as a filterable datatype.
On 2017/01/24 08:37:27, Mike West (sloooooow) wrote:
> Please add a TODO pointing to the bug.

Done.

https://codereview.chromium.org/2647683002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1381:
// Disabled, since passwords are not yet marked as a filterable datatype.
On 2017/01/24 08:37:27, Mike West (sloooooow) wrote:
> Ditto.

Done.

https://codereview.chromium.org/2647683002/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client_unittest.cc (right):

https://codereview.chromium.org/2647683002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client_unittest.cc:414: bool
filter_is_important)
On 2017/01/24 08:37:27, Mike West (sloooooow) wrote:
> Nit: What does "important" mean? How about "should_compare_filter"?
> "filter_is_not_relevant_to_this_test"?

Yeah, good point. Let's call it "should_compare_filter".

[msramek, 2017-01-24 11:53:11]

Description was changed from

==========
Consolidate Origin- and RegistrableDomain- FilterBuilder into one class

...called BrowsingDataFilterBuilderImpl.

The new class will eventually live in content/ (together with
BrowsingDataRemoverImpl) but will also be used by chrome/, therefore
we also create an abstract interface BrowsingDataFilterBuilder.

We also consolidate the tests into one file. As the new class is theoretically
able to handle origins and registrable domains at the same time, we add new
tests for these cases (although this situation does not occur in production).

The previous state with two different classes increased the complexity in
ChromeContentBrowserClientTest, which we now simplified.

We also introduced a Copy() method to the new class which allowed us to
duplicate the filter builder in BrowsingDataRemoverImplTest and
ChromeBrowsingDataRemoverDelegateTest where needed, which means we no longer
rely on passing the class via reference, thus we don't need to use the private
BrowsingDataRemoverImpl::RemoveInternal method, and therefore we could remove
friend declarations from BrowsingDataRemoverImpl. As a side effect, calling
the public methods now means that checks for filterable datatypes are
performed, and since HISTORY and PASSWORDS are not yet marked as filterable,
we had to disable the corresponding tests.

BASE=2641853002
BUG=668114
==========

to

==========
Consolidate Origin- and RegistrableDomain- FilterBuilder into one class

...called BrowsingDataFilterBuilderImpl.

The new class will eventually live in content/ (together with
BrowsingDataRemoverImpl) but will also be used by chrome/, therefore
we also create an abstract interface BrowsingDataFilterBuilder.

We also consolidate the tests into one file. As the new class is theoretically
able to handle origins and registrable domains at the same time, we add new
tests for these cases (although this situation does not occur in production).

The previous state with two different classes increased the complexity in
ChromeContentBrowserClientTest, which we now simplified.

We also introduced a Copy() method to the new class which allowed us to
duplicate the filter builder in BrowsingDataRemoverImplTest and
ChromeBrowsingDataRemoverDelegateTest where needed, which means we no longer
rely on passing the class via reference, thus we don't need to use the private
BrowsingDataRemoverImpl::RemoveInternal method, and therefore we could remove
friend declarations from BrowsingDataRemoverImpl. As a side effect, calling
the public methods now means that checks for filterable datatypes are
performed, and since HISTORY and PASSWORDS are not yet marked as filterable,
we had to disable the corresponding tests.

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

[msramek, 2017-01-24 11:55:32]

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

[msramek, 2017-01-24 11:55:33]

Adding +Bernhard to TBR for pref_service_bridge.cc, since the change there is
mechanical; and proceeding to land.

[msramek, 2017-01-24 11:56:16]

The CQ bit was checked by msramek@chromium.org

[msramek, 2017-01-24 11:56:16]

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

[commit-bot: I haz the power, 2017-01-24 11:56:24]

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

[Bernhard Bauer, 2017-01-24 11:59:21]

lgtm

[commit-bot: I haz the power, 2017-01-24 12:01:09]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485258976024300,
"parent_rev": "acc84fbd540feec6c0fa6b2165b0da7eeb8d0456", "commit_rev":
"c7ee9d5e15a8d98d5f5fe2afcff74bec2f3ae7d6"}

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

Description was changed from

==========
Consolidate Origin- and RegistrableDomain- FilterBuilder into one class

...called BrowsingDataFilterBuilderImpl.

The new class will eventually live in content/ (together with
BrowsingDataRemoverImpl) but will also be used by chrome/, therefore
we also create an abstract interface BrowsingDataFilterBuilder.

We also consolidate the tests into one file. As the new class is theoretically
able to handle origins and registrable domains at the same time, we add new
tests for these cases (although this situation does not occur in production).

The previous state with two different classes increased the complexity in
ChromeContentBrowserClientTest, which we now simplified.

We also introduced a Copy() method to the new class which allowed us to
duplicate the filter builder in BrowsingDataRemoverImplTest and
ChromeBrowsingDataRemoverDelegateTest where needed, which means we no longer
rely on passing the class via reference, thus we don't need to use the private
BrowsingDataRemoverImpl::RemoveInternal method, and therefore we could remove
friend declarations from BrowsingDataRemoverImpl. As a side effect, calling
the public methods now means that checks for filterable datatypes are
performed, and since HISTORY and PASSWORDS are not yet marked as filterable,
we had to disable the corresponding tests.

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

to

==========
Consolidate Origin- and RegistrableDomain- FilterBuilder into one class

...called BrowsingDataFilterBuilderImpl.

The new class will eventually live in content/ (together with
BrowsingDataRemoverImpl) but will also be used by chrome/, therefore
we also create an abstract interface BrowsingDataFilterBuilder.

We also consolidate the tests into one file. As the new class is theoretically
able to handle origins and registrable domains at the same time, we add new
tests for these cases (although this situation does not occur in production).

The previous state with two different classes increased the complexity in
ChromeContentBrowserClientTest, which we now simplified.

We also introduced a Copy() method to the new class which allowed us to
duplicate the filter builder in BrowsingDataRemoverImplTest and
ChromeBrowsingDataRemoverDelegateTest where needed, which means we no longer
rely on passing the class via reference, thus we don't need to use the private
BrowsingDataRemoverImpl::RemoveInternal method, and therefore we could remove
friend declarations from BrowsingDataRemoverImpl. As a side effect, calling
the public methods now means that checks for filterable datatypes are
performed, and since HISTORY and PASSWORDS are not yet marked as filterable,
we had to disable the corresponding tests.

TBR=bauerb@chromium.org
BUG=668114

Review-Url: https://codereview.chromium.org/2647683002
Cr-Commit-Position: refs/heads/master@{#445706}
Committed:
https://chromium.googlesource.com/chromium/src/+/c7ee9d5e15a8d98d5f5fe2afcff7...
==========

[commit-bot: I haz the power, 2017-01-24 12:01:42]

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