Remove the content settings dependencies from BrowsingDataFilterBuilder et al.

Remove the content settings dependencies from BrowsingDataFilterBuilder et al.

As a part of BrowsingDataRemover's migration to //content, we will also
need to move the auxiliary classes such as BrowsingDataFilterBuilder and
its subclasses.

This is currently not possible, as BrowsingDataFilterBuilder has
a dependency on content settings, which are a //chrome feature. As splitting
the BrowsingDataFilterBuilder and its subclasses into //content and //chrome
versions would be an overkill, we choose a simpler approach, where
ChromeBrowsingDataRemoverDelegate contains an adapter to convert
origin-scoped content settings patterns to a GURL, which is then used with
a regular GURL filter.

Note that a domain-scoped content settings pattern cannot be converted
to a GURL, which means we lose the ability to delete those patterns.
However, this functionality was added only for completeness; none of the
website settings or content settings supported by
ChromeBrowsingDataRemoverDelegate uses domain-scoped patterns. This is
verified using a DCHECK.

We remove the associated tests for content settings pattern filters.
The current functionality of website settings and content settings deletion
is still covered by BrowsingDataRemoverTest and by GURL filter tests of
BrowsingDataFilterBuilder subclasses.

Finally, remove one instance of the content settings pattern filter from
WebsitePreferenceBridge. This can be replaced by simply setting a NULL
value in HostContentSettingsMap.

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

[msramek, 2017-01-05 13:01:18]

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

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

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-05 13:24:22]

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

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

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-05 16:04:30]

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

[commit-bot: I haz the power, 2017-01-05 16:04:50]

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-05 16:52:52]

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

[commit-bot: I haz the power, 2017-01-05 16:52:52]

Dry run: This issue passed the CQ dry run.

[msramek, 2017-01-05 17:06:24]

Description was changed from

==========
Remove the content settings dependencies from BrowsingDataFilterBuilder et al.

As a part of BrowsingDataRemover's migration to //content, we will also
need to move the auxiliary classes such as BrowsingDataFilterBuilder and
its subclasses.

This is currently not possible, as BrowsingDataFilterBuilder has
a dependency on content settings, which are a //chrome feature. As splitting
the BrowsingDataFilterBuilder and its subclasses into //content and //chrome
versions would be an overkill, we choose a simpler approach, where
ChromeBrowsingDataRemoverDelegate contains an adapter to convert
origin-scoped content settings patterns to a GURL, which is then used with
a regular GURL filter.

Note that a domain-scoped content settings pattern cannot be converted
to a GURL, which means we lose the ability to delete those patterns.
However, this functionality was added only for completeness; none of the
website settings or content settings supported by
ChromeBrowsingDataRemoverDelegate uses domain-scoped patterns. This is
verified using a DCHECK.

We remove the associated tests for content settings pattern filters.
The current functionality of website settings and content settings deletion
is still covered by BrowsingDataRemoverTest and by GURL filter tests of
BrowsingDataFilterBuilder subclasses.

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

to

==========
Remove the content settings dependencies from BrowsingDataFilterBuilder et al.

As a part of BrowsingDataRemover's migration to //content, we will also
need to move the auxiliary classes such as BrowsingDataFilterBuilder and
its subclasses.

This is currently not possible, as BrowsingDataFilterBuilder has
a dependency on content settings, which are a //chrome feature. As splitting
the BrowsingDataFilterBuilder and its subclasses into //content and //chrome
versions would be an overkill, we choose a simpler approach, where
ChromeBrowsingDataRemoverDelegate contains an adapter to convert
origin-scoped content settings patterns to a GURL, which is then used with
a regular GURL filter.

Note that a domain-scoped content settings pattern cannot be converted
to a GURL, which means we lose the ability to delete those patterns.
However, this functionality was added only for completeness; none of the
website settings or content settings supported by
ChromeBrowsingDataRemoverDelegate uses domain-scoped patterns. This is
verified using a DCHECK.

We remove the associated tests for content settings pattern filters.
The current functionality of website settings and content settings deletion
is still covered by BrowsingDataRemoverTest and by GURL filter tests of
BrowsingDataFilterBuilder subclasses.

Finally, remove one instance of the content settings pattern filter from
WebsitePreferenceBridge. This can be replaced by simply setting a NULL
value in HostContentSettingsMap.

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

[msramek, 2017-01-05 17:07:21]

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

[msramek, 2017-01-05 17:07:22]

Hi Mike!

Happy new year! Please have a look at this when you're back from OOO :)

Thanks,
Martin

[Mike West, 2017-01-09 08:53:48]

LGTM assuming the answer to my question is something like "It turns into a
no-op, but doesn't cause any real problems."

https://codereview.chromium.org/2612903005/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc
(right):

https://codereview.chromium.org/2612903005/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:144:
DCHECK(url.is_valid());
What happens if this DCHECK fails in Release? Will passing an invalid URL to
`predicate.Run()` explode somewhere in an ugly way?

[msramek, 2017-01-09 09:39:42]

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

[msramek, 2017-01-09 09:39:43]

Thanks Mike!

+Maria, can you have a look at the change in website_preference_bridge.cc? I
could just copy the change in ChromeBrowsingDataRemoverDelegate, but in this
case, it should be sufficient to call SetWebsiteSettingDefaultScope(nullptr), as
there cannot be more than one exception per origin.

https://codereview.chromium.org/2612903005/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc
(right):

https://codereview.chromium.org/2612903005/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:144:
DCHECK(url.is_valid());
On 2017/01/09 08:53:48, Mike West (sloooooow) wrote:
> What happens if this DCHECK fails in Release? Will passing an invalid URL to
> `predicate.Run()` explode somewhere in an ugly way?

An invalid URL will be passed to the filter, where we will try to match it with
listed origins (OriginFilterBuilder) or domains
(RegistrableDomainFilterBuilder). It will not be equal to any entry, which means
it will never be matched by whitelist and always be by blacklist. But it will
not explode.

I have already reached to raymes@ to discuss this, as the way we use
ContentSettingPattern in places where Origin would suffice seems cumbersome (and
I'm actually surprised this is the first time it comes up). This DCHECK should
eventually be handled somewhere in WebsiteSettingsRegistry.

[msramek, 2017-01-09 09:39:54]

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

[commit-bot: I haz the power, 2017-01-09 09:40:07]

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-09 10:30:47]

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

[commit-bot: I haz the power, 2017-01-09 10:30:48]

Dry run: This issue passed the CQ dry run.

[Maria, 2017-01-10 07:05:07]

lgtm

[msramek, 2017-01-10 09:25:10]

Thanks!

[msramek, 2017-01-10 09:25:17]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2017-01-10 09:25: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-01-10 09:29:07]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484040317987020,
"parent_rev": "ea1b4f5cfba38034eadfced04ce0906ef193f9a5", "commit_rev":
"dc0c3729d77398a6a36eb717f51b5a56bcad38dc"}

[commit-bot: I haz the power, 2017-01-10 09:29:39]

Description was changed from

==========
Remove the content settings dependencies from BrowsingDataFilterBuilder et al.

As a part of BrowsingDataRemover's migration to //content, we will also
need to move the auxiliary classes such as BrowsingDataFilterBuilder and
its subclasses.

This is currently not possible, as BrowsingDataFilterBuilder has
a dependency on content settings, which are a //chrome feature. As splitting
the BrowsingDataFilterBuilder and its subclasses into //content and //chrome
versions would be an overkill, we choose a simpler approach, where
ChromeBrowsingDataRemoverDelegate contains an adapter to convert
origin-scoped content settings patterns to a GURL, which is then used with
a regular GURL filter.

Note that a domain-scoped content settings pattern cannot be converted
to a GURL, which means we lose the ability to delete those patterns.
However, this functionality was added only for completeness; none of the
website settings or content settings supported by
ChromeBrowsingDataRemoverDelegate uses domain-scoped patterns. This is
verified using a DCHECK.

We remove the associated tests for content settings pattern filters.
The current functionality of website settings and content settings deletion
is still covered by BrowsingDataRemoverTest and by GURL filter tests of
BrowsingDataFilterBuilder subclasses.

Finally, remove one instance of the content settings pattern filter from
WebsitePreferenceBridge. This can be replaced by simply setting a NULL
value in HostContentSettingsMap.

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

to

==========
Remove the content settings dependencies from BrowsingDataFilterBuilder et al.

As a part of BrowsingDataRemover's migration to //content, we will also
need to move the auxiliary classes such as BrowsingDataFilterBuilder and
its subclasses.

This is currently not possible, as BrowsingDataFilterBuilder has
a dependency on content settings, which are a //chrome feature. As splitting
the BrowsingDataFilterBuilder and its subclasses into //content and //chrome
versions would be an overkill, we choose a simpler approach, where
ChromeBrowsingDataRemoverDelegate contains an adapter to convert
origin-scoped content settings patterns to a GURL, which is then used with
a regular GURL filter.

Note that a domain-scoped content settings pattern cannot be converted
to a GURL, which means we lose the ability to delete those patterns.
However, this functionality was added only for completeness; none of the
website settings or content settings supported by
ChromeBrowsingDataRemoverDelegate uses domain-scoped patterns. This is
verified using a DCHECK.

We remove the associated tests for content settings pattern filters.
The current functionality of website settings and content settings deletion
is still covered by BrowsingDataRemoverTest and by GURL filter tests of
BrowsingDataFilterBuilder subclasses.

Finally, remove one instance of the content settings pattern filter from
WebsitePreferenceBridge. This can be replaced by simply setting a NULL
value in HostContentSettingsMap.

BUG=668114

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

[commit-bot: I haz the power, 2017-01-10 09:29:40]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/dc0c3729d77398a6a36eb717f51b...