[subresource_filter] Add domains to content settings whitelist

[subresource_filter] Add domains to content settings whitelist

BUG=689487
Review-Url: https://codereview.chromium.org/2772393002
Cr-Commit-Position: refs/heads/master@{#460748}
Committed: https://chromium.googlesource.com/chromium/src/+/459465b1d6d569a612b16526a30af878a8df1b10

[Charlie Harrison, 2017-03-27 18:32:22]

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

[commit-bot: I haz the power, 2017-03-27 18:33:06]

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

[Charlie Harrison, 2017-03-27 18:33:57]

csharrison@chromium.org changed reviewers:
+ melandory@chromium.org

[Charlie Harrison, 2017-03-27 18:33:58]

Tanja: here is the second piece of the content settings hook up. This removes
the old mechanism for whitelisting for all but the whitelist_on_reload
experiment in favor of asking the client for content settings information.

[Charlie Harrison, 2017-03-27 18:40:19]

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

[commit-bot: I haz the power, 2017-03-27 18:40:38]

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-03-27 19:36:16]

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

[commit-bot: I haz the power, 2017-03-27 19:36:17]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-03-28 20:07:38]

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

[commit-bot: I haz the power, 2017-03-28 20:08:05]

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

[Charlie Harrison, 2017-03-28 20:20:38]

Note: this means a site that is whitelisted cannot yet be removed from a
whitelist, until the content settings UI work is completed. I think this is
fine, WDYT?

[commit-bot: I haz the power, 2017-03-28 21:01:28]

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

[commit-bot: I haz the power, 2017-03-28 21:01:29]

Dry run: This issue passed the CQ dry run.

[melandory, 2017-03-29 14:55:18]

On 2017/03/28 20:20:38, Charlie Harrison wrote:
> Note: this means a site that is whitelisted cannot yet be removed from a
> whitelist, until the content settings UI work is completed. I think this is
> fine, WDYT?
We should't do this. Since we will save the data without giving the ability to
get rid of it.
It's not the best behavior from the privacy perspective (problem is that it will
be active for soc eng sites). What I suggest: for the content setting related UI
I've added the feature kSafeBrowsingSubresourceFilterExperimentalUI. So maybe we
should distinguish between the behavior based on this feature. And once new UI
is enabled we can nuke old behavior.

[Charlie Harrison, 2017-03-29 14:58:08]

On 2017/03/29 14:55:18, melandory wrote:
> On 2017/03/28 20:20:38, Charlie Harrison wrote:
> > Note: this means a site that is whitelisted cannot yet be removed from a
> > whitelist, until the content settings UI work is completed. I think this is
> > fine, WDYT?
> We should't do this. Since we will save the data without giving the ability to
> get rid of it.
> It's not the best behavior from the privacy perspective (problem is that it
will
> be active for soc eng sites). What I suggest: for the content setting related
UI
> I've added the feature kSafeBrowsingSubresourceFilterExperimentalUI. So maybe
we
> should distinguish between the behavior based on this feature. And once new UI
> is enabled we can nuke old behavior.

Sounds good to me, I'll amend the current PS with this behavior.

[Charlie Harrison, 2017-03-29 17:32:17]

Tanja: PTAL, I've guarded adding the content setting by the experimental UI
feature flag.

[Charlie Harrison, 2017-03-29 17:32:35]

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

[commit-bot: I haz the power, 2017-03-29 17:33: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-03-29 18:36:06]

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

[commit-bot: I haz the power, 2017-03-29 18:36:07]

Dry run: This issue passed the CQ dry run.

[melandory, 2017-03-30 12:21:53]

lgtm

https://codereview.chromium.org/2772393002/diff/60001/chrome/browser/subresou...
File chrome/browser/subresource_filter/subresource_filter_browsertest.cc
(right):

https://codereview.chromium.org/2772393002/diff/60001/chrome/browser/subresou...
chrome/browser/subresource_filter/subresource_filter_browsertest.cc:245:
kSafeBrowsingSubresourceFilterExperimentalUI.name));
nit: We could test both behaviors, but  it's totally optional

[Charlie Harrison, 2017-03-30 12:28:54]

Thanks for the review!

https://codereview.chromium.org/2772393002/diff/60001/chrome/browser/subresou...
File chrome/browser/subresource_filter/subresource_filter_browsertest.cc
(right):

https://codereview.chromium.org/2772393002/diff/60001/chrome/browser/subresou...
chrome/browser/subresource_filter/subresource_filter_browsertest.cc:245:
kSafeBrowsingSubresourceFilterExperimentalUI.name));
On 2017/03/30 12:21:53, melandory wrote:
> nit: We could test both behaviors, but  it's totally optional

Relatively soon we'll be switching over wholesale and using the new UI
everywhere, so I'm okay keeping it as-is for the moment. 

In a followup I may do a refactor of these tests anyways which will pull the
content settings ones into their own harness.

[Charlie Harrison, 2017-03-30 12:29:29]

Description was changed from

==========
[subresource_filter] Add domains to content settings whitelist

BUG=
==========

to

==========
[subresource_filter] Add domains to content settings whitelist

BUG=689487
==========

[Charlie Harrison, 2017-03-30 12:29:35]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2017-03-30 12:29:59]

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

[Charlie Harrison, 2017-03-30 12:34:54]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-03-30 12:34:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
melandory@chromium.org
Link to the patchset: https://codereview.chromium.org/2772393002/#ps100001
(title: "fix join string rebase")

[commit-bot: I haz the power, 2017-03-30 12:35:16]

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-03-30 14:08:33]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490877294887060,
"parent_rev": "00f6a541cedaa119f5219d63f3e8a1de748f1ea6", "commit_rev":
"459465b1d6d569a612b16526a30af878a8df1b10"}

[commit-bot: I haz the power, 2017-03-30 14:09:22]

Description was changed from

==========
[subresource_filter] Add domains to content settings whitelist

BUG=689487
==========

to

==========
[subresource_filter] Add domains to content settings whitelist

BUG=689487

Review-Url: https://codereview.chromium.org/2772393002
Cr-Commit-Position: refs/heads/master@{#460748}
Committed:
https://chromium.googlesource.com/chromium/src/+/459465b1d6d569a612b16526a30a...
==========

[commit-bot: I haz the power, 2017-03-30 14:09:26]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/459465b1d6d569a612b16526a30a...