[subresource_filter] Make website setting existence imply site activation

[subresource_filter] Make website setting existence imply site activation

This patch tries its best to make the relationship 1:1 between site activation
status and the WebsiteSetting metadata being present for that origin.

The setting is added when a site is activated and the UI is shown (to prevent cases
where no resources are filtered or we are activated but suppressing UI).

The setting is removed when a site is found to not activate for a particular top level
navigation (% content setting whitelists), or when a url matching the setting's origin
is deleted from history. Since we don't want to clear this state if the user makes
some explicit action (like whitelisting), we make sure the whitelisting check is the
very last one in activation computation and don't take it into account for setting
removal.

In this way the setting is a best approximation for a cached activation state (minus
content setting policy) for a given origin. Currently it is only used for the smart UI, 
but in follow up patches we will make other settings UI surfaces dependent on this
 state.

Note: The setting is now used on both Desktop and Android, and is now NOT_LOSSY.

BUG=689487
Review-Url: https://codereview.chromium.org/2859783002
Cr-Commit-Position: refs/heads/master@{#470945}
Committed: https://chromium.googlesource.com/chromium/src/+/31a9f6426ec06155f4b95e5e125d76e0371d0c74

[Charlie Harrison, 2017-05-03 05:15:49]

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

[commit-bot: I haz the power, 2017-05-03 05:15:59]

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-05-03 05:53:57]

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

[commit-bot: I haz the power, 2017-05-03 05:53:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2017-05-03 17:21:35]

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

[commit-bot: I haz the power, 2017-05-03 17:22:11]

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-05-03 18:14:51]

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

[commit-bot: I haz the power, 2017-05-03 18:14:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2017-05-09 04:35:26]

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

[commit-bot: I haz the power, 2017-05-09 04:37:13]

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-05-09 04:44:52]

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

[commit-bot: I haz the power, 2017-05-09 04:44:53]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  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-...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Charlie Harrison, 2017-05-09 12:45:24]

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

[commit-bot: I haz the power, 2017-05-09 12:45:57]

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-05-09 12:56:15]

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

[commit-bot: I haz the power, 2017-05-09 12:56:38]

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-05-09 13:01:45]

Description was changed from

==========
Clear the metadata when activation is disabled

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

to

==========
Clear the metadata when activation is disabled

This patch tries its best to make the relationship 1:1 between site activation
and
the WebsiteSetting metadata being present for that origin.

The setting is added when a site is activated and the UI is shown (to prevent
cases
where no resources are filtered or we are activated suppressing UI)l

The setting is removed when a site is found to be not activated anymore, or when
a url matching the setting's origin is deleted from history.

In this way the setting is a best approximation for a cached activation state
for a
given origin. Currently it is only used for the smart UI, but in follow up
patches we
will make other settings UI surfaces dependent on this state.

Note: The setting is now used on both Desktop and Android, and is now NOT_LOSSY.

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

[Charlie Harrison, 2017-05-09 13:03:35]

Description was changed from

==========
Clear the metadata when activation is disabled

This patch tries its best to make the relationship 1:1 between site activation
and
the WebsiteSetting metadata being present for that origin.

The setting is added when a site is activated and the UI is shown (to prevent
cases
where no resources are filtered or we are activated suppressing UI)l

The setting is removed when a site is found to be not activated anymore, or when
a url matching the setting's origin is deleted from history.

In this way the setting is a best approximation for a cached activation state
for a
given origin. Currently it is only used for the smart UI, but in follow up
patches we
will make other settings UI surfaces dependent on this state.

Note: The setting is now used on both Desktop and Android, and is now NOT_LOSSY.

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

to

==========
Clear the metadata when activation is disabled

This patch tries its best to make the relationship 1:1 between site activation
and
the WebsiteSetting metadata being present for that origin.

The setting is added when a site is activated and the UI is shown (to prevent
cases
where no resources are filtered or we are activated but suppressing UI).

The setting is removed when a site is found to be not activated anymore, or when
a url matching the setting's origin is deleted from history.

In this way the setting is a best approximation for a cached activation state
for a
given origin. Currently it is only used for the smart UI, but in follow up
patches we
will make other settings UI surfaces dependent on this state.

Note: The setting is now used on both Desktop and Android, and is now NOT_LOSSY.

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

[Charlie Harrison, 2017-05-09 13:05:47]

Description was changed from

==========
Clear the metadata when activation is disabled

This patch tries its best to make the relationship 1:1 between site activation
and
the WebsiteSetting metadata being present for that origin.

The setting is added when a site is activated and the UI is shown (to prevent
cases
where no resources are filtered or we are activated but suppressing UI).

The setting is removed when a site is found to be not activated anymore, or when
a url matching the setting's origin is deleted from history.

In this way the setting is a best approximation for a cached activation state
for a
given origin. Currently it is only used for the smart UI, but in follow up
patches we
will make other settings UI surfaces dependent on this state.

Note: The setting is now used on both Desktop and Android, and is now NOT_LOSSY.

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

to

==========
[subresource_filter] Make website setting imply cached activation

This patch tries its best to make the relationship 1:1 between site activation
and
the WebsiteSetting metadata being present for that origin.

The setting is added when a site is activated and the UI is shown (to prevent
cases
where no resources are filtered or we are activated but suppressing UI).

The setting is removed when a site is found to be not activated anymore, or when
a url matching the setting's origin is deleted from history.

In this way the setting is a best approximation for a cached activation state
for a
given origin. Currently it is only used for the smart UI, but in follow up
patches we
will make other settings UI surfaces dependent on this state.

Note: The setting is now used on both Desktop and Android, and is now NOT_LOSSY.

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

[Charlie Harrison, 2017-05-09 13:06:28]

Description was changed from

==========
[subresource_filter] Make website setting imply cached activation

This patch tries its best to make the relationship 1:1 between site activation
and
the WebsiteSetting metadata being present for that origin.

The setting is added when a site is activated and the UI is shown (to prevent
cases
where no resources are filtered or we are activated but suppressing UI).

The setting is removed when a site is found to be not activated anymore, or when
a url matching the setting's origin is deleted from history.

In this way the setting is a best approximation for a cached activation state
for a
given origin. Currently it is only used for the smart UI, but in follow up
patches we
will make other settings UI surfaces dependent on this state.

Note: The setting is now used on both Desktop and Android, and is now NOT_LOSSY.

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

to

==========
[subresource_filter] Make website setting existence imply cached activation

This patch tries its best to make the relationship 1:1 between site activation
and
the WebsiteSetting metadata being present for that origin.

The setting is added when a site is activated and the UI is shown (to prevent
cases
where no resources are filtered or we are activated but suppressing UI).

The setting is removed when a site is found to be not activated anymore, or when
a url matching the setting's origin is deleted from history.

In this way the setting is a best approximation for a cached activation state
for a
given origin. Currently it is only used for the smart UI, but in follow up
patches we
will make other settings UI surfaces dependent on this state.

Note: The setting is now used on both Desktop and Android, and is now NOT_LOSSY.

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

[Charlie Harrison, 2017-05-09 13:09:45]

csharrison@chromium.org changed reviewers:
+ msramek@chromium.org, shivanisha@chromium.org

[Charlie Harrison, 2017-05-09 13:09:46]

msramek: PTAL at //chrome and content_settings parts. This is basically an
extension of the previous CL, with an added criteria for clearing the setting
(when activation is disabled for an origin).

shivanisha: PTAL at everything. subresource_filter parts are mainly mechanical
to plumb the activation state up to //chrome.

[commit-bot: I haz the power, 2017-05-09 14:13:55]

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

[commit-bot: I haz the power, 2017-05-09 14:13:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2017-05-09 15:16:54]

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

[commit-bot: I haz the power, 2017-05-09 15:17:23]

Dry run: CQ is trying da patch.

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

[shivanisha, 2017-05-09 15:43:36]

Nice. First pass comments.

https://codereview.chromium.org/2859783002/diff/80001/chrome/browser/subresou...
File chrome/browser/subresource_filter/chrome_subresource_filter_client.cc
(right):

https://codereview.chromium.org/2859783002/diff/80001/chrome/browser/subresou...
chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:148: }
Can this callback also be used for "setting" the site metadata to maintain the
invariant that metadata implies activated ? The kInfobarLastShownTimeKey can
then be updated in OnDidShowUI() ?

https://codereview.chromium.org/2859783002/diff/80001/components/content_sett...
File components/content_settings/core/browser/website_settings_registry.cc
(right):

https://codereview.chromium.org/2859783002/diff/80001/components/content_sett...
components/content_settings/core/browser/website_settings_registry.cc:175:
DESKTOP | PLATFORM_ANDROID, WebsiteSettingsInfo::INHERIT_IN_INCOGNITO);
Remove the "Currently only used on Android." comment from
CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER_DATA definition point in
content_settings_types.h

https://codereview.chromium.org/2859783002/diff/80001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc
(right):

https://codereview.chromium.org/2859783002/diff/80001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:72:
weak_ptr_factory_(this) {}
DCHECK(client_ != nullptr) here or an if (client_) when we invoke
OnPageActivationComputed later or both?

https://codereview.chromium.org/2859783002/diff/80001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h
(right):

https://codereview.chromium.org/2859783002/diff/80001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:69:
ContentSubresourceFilterThrottleManager(
It will be good to have a comment for the constructor as well describing the
lifetime guarantees that the consumer must follow for all of the raw pointers
passed into the constructor.

[Charlie Harrison, 2017-05-09 16:16:46]

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

[commit-bot: I haz the power, 2017-05-09 16:17:18]

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-05-09 16:19:23]

https://codereview.chromium.org/2859783002/diff/80001/chrome/browser/subresou...
File chrome/browser/subresource_filter/chrome_subresource_filter_client.cc
(right):

https://codereview.chromium.org/2859783002/diff/80001/chrome/browser/subresou...
chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:148: }
On 2017/05/09 15:43:35, shivanisha wrote:
> Can this callback also be used for "setting" the site metadata to maintain the
> invariant that metadata implies activated ? The kInfobarLastShownTimeKey can
> then be updated in OnDidShowUI() ? 
I am a little bit nervous about that because we will use this to gate settings
UI surfaces. Additionally, we really don't want this to be set if we have
should_suppress_notifications = true. 

Doing this would require going through the driver_factory (rather than accessing
the client directly in the manager), especially once
https://codereview.chromium.org/2844063002/ lands.

Let me add a TODO to consider adopting that behavior once we've removed the
driver factory. WDYT?

https://codereview.chromium.org/2859783002/diff/80001/components/content_sett...
File components/content_settings/core/browser/website_settings_registry.cc
(right):

https://codereview.chromium.org/2859783002/diff/80001/components/content_sett...
components/content_settings/core/browser/website_settings_registry.cc:175:
DESKTOP | PLATFORM_ANDROID, WebsiteSettingsInfo::INHERIT_IN_INCOGNITO);
On 2017/05/09 15:43:35, shivanisha wrote:
> Remove the "Currently only used on Android." comment from
> CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER_DATA definition point in
> content_settings_types.h

Good catch, done.

https://codereview.chromium.org/2859783002/diff/80001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc
(right):

https://codereview.chromium.org/2859783002/diff/80001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:72:
weak_ptr_factory_(this) {}
On 2017/05/09 15:43:35, shivanisha wrote:
> DCHECK(client_ != nullptr) here or an if (client_) when we invoke
> OnPageActivationComputed later or both?

Done. Client owns us (essentially), so should never be null.

https://codereview.chromium.org/2859783002/diff/80001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h
(right):

https://codereview.chromium.org/2859783002/diff/80001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:69:
ContentSubresourceFilterThrottleManager(
On 2017/05/09 15:43:35, shivanisha wrote:
> It will be good to have a comment for the constructor as well describing the
> lifetime guarantees that the consumer must follow for all of the raw pointers
> passed into the constructor.

Done.

[shivanisha, 2017-05-09 16:34:28]

lgtm % nit

https://codereview.chromium.org/2859783002/diff/120001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2859783002/diff/120001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:1:
// // Raw pointers passed in here must all outlive the class.
Remove the 1st line comment?

[Charlie Harrison, 2017-05-09 16:50:13]

https://codereview.chromium.org/2859783002/diff/120001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2859783002/diff/120001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:1:
// // Raw pointers passed in here must all outlive the class.
On 2017/05/09 16:34:27, shivanisha wrote:
> Remove the 1st line comment?

LOL done. Sorry about that.

[Charlie Harrison, 2017-05-09 18:58:44]

This CL is slightly wrong, we want to be plumbing the actual activation
decision, not the eventual activation state that the page ends up in.

This is important because we do not want to clear the metadata if a site is
whitelisted in ContentSettings.

Let me re-jigger the logic here and re-request a review from you Shivani. I
think the //chrome parts should still be ok.

[Charlie Harrison, 2017-05-09 20:23:46]

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

[commit-bot: I haz the power, 2017-05-09 20:24:44]

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-05-09 20:26:55]

Description was changed from

==========
[subresource_filter] Make website setting existence imply cached activation

This patch tries its best to make the relationship 1:1 between site activation
and
the WebsiteSetting metadata being present for that origin.

The setting is added when a site is activated and the UI is shown (to prevent
cases
where no resources are filtered or we are activated but suppressing UI).

The setting is removed when a site is found to be not activated anymore, or when
a url matching the setting's origin is deleted from history.

In this way the setting is a best approximation for a cached activation state
for a
given origin. Currently it is only used for the smart UI, but in follow up
patches we
will make other settings UI surfaces dependent on this state.

Note: The setting is now used on both Desktop and Android, and is now NOT_LOSSY.

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

to

==========
[subresource_filter] Make website setting existence imply SB blacklist

This patch tries its best to make the relationship 1:1 between site blacklist
status in Safe Browsing and the WebsiteSetting metadata being present for that
origin.

The setting is added when a site is activated and the UI is shown (to prevent
cases
where no resources are filtered or we are activated but suppressing UI).

The setting is removed when a site is found to match the SAFE threat type, or
when
a url matching the setting's origin is deleted from history.

In this way the setting is a best approximation for a cached blacklist state for
a
given origin. Currently it is only used for the smart UI, but in follow up
patches we
will make other settings UI surfaces dependent on this state.

Note: The setting is now used on both Desktop and Android, and is now NOT_LOSSY.

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

[commit-bot: I haz the power, 2017-05-09 21:48:44]

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

[commit-bot: I haz the power, 2017-05-09 21:48:45]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  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-...)

[Charlie Harrison, 2017-05-10 04:02:56]

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

[commit-bot: I haz the power, 2017-05-10 04:03: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-05-10 05:47:02]

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

[commit-bot: I haz the power, 2017-05-10 05:47:03]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-05-10 12:22:59]

Description was changed from

==========
[subresource_filter] Make website setting existence imply SB blacklist

This patch tries its best to make the relationship 1:1 between site blacklist
status in Safe Browsing and the WebsiteSetting metadata being present for that
origin.

The setting is added when a site is activated and the UI is shown (to prevent
cases
where no resources are filtered or we are activated but suppressing UI).

The setting is removed when a site is found to match the SAFE threat type, or
when
a url matching the setting's origin is deleted from history.

In this way the setting is a best approximation for a cached blacklist state for
a
given origin. Currently it is only used for the smart UI, but in follow up
patches we
will make other settings UI surfaces dependent on this state.

Note: The setting is now used on both Desktop and Android, and is now NOT_LOSSY.

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

to

==========
[subresource_filter] Make website setting existence site activation

This patch tries its best to make the relationship 1:1 between site activation
status and the WebsiteSetting metadata being present for that origin.

The setting is added when a site is activated and the UI is shown (to prevent
cases
where no resources are filtered or we are activated but suppressing UI).

The setting is removed when a site is found to not activate for a particular top
level
navigation (% content setting whitelists), or when a url matching the setting's
origin
is deleted from history. Since we don't want to clear this state if the user
makes
some explicit action (like whitelisting), we make sure the whitelisting check is
the
very last one in activation computation.

In this way the setting is a best approximation for a cached activation state
for a
given origin. Currently it is only used for the smart UI, but in follow up
patches we
will make other settings UI surfaces dependent on this state.

Note: The setting is now used on both Desktop and Android, and is now NOT_LOSSY.

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

[Charlie Harrison, 2017-05-10 12:23:19]

Description was changed from

==========
[subresource_filter] Make website setting existence site activation

This patch tries its best to make the relationship 1:1 between site activation
status and the WebsiteSetting metadata being present for that origin.

The setting is added when a site is activated and the UI is shown (to prevent
cases
where no resources are filtered or we are activated but suppressing UI).

The setting is removed when a site is found to not activate for a particular top
level
navigation (% content setting whitelists), or when a url matching the setting's
origin
is deleted from history. Since we don't want to clear this state if the user
makes
some explicit action (like whitelisting), we make sure the whitelisting check is
the
very last one in activation computation.

In this way the setting is a best approximation for a cached activation state
for a
given origin. Currently it is only used for the smart UI, but in follow up
patches we
will make other settings UI surfaces dependent on this state.

Note: The setting is now used on both Desktop and Android, and is now NOT_LOSSY.

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

to

==========
[subresource_filter] Make website setting existence imply site activation

This patch tries its best to make the relationship 1:1 between site activation
status and the WebsiteSetting metadata being present for that origin.

The setting is added when a site is activated and the UI is shown (to prevent
cases
where no resources are filtered or we are activated but suppressing UI).

The setting is removed when a site is found to not activate for a particular top
level
navigation (% content setting whitelists), or when a url matching the setting's
origin
is deleted from history. Since we don't want to clear this state if the user
makes
some explicit action (like whitelisting), we make sure the whitelisting check is
the
very last one in activation computation.

In this way the setting is a best approximation for a cached activation state
for a
given origin. Currently it is only used for the smart UI, but in follow up
patches we
will make other settings UI surfaces dependent on this state.

Note: The setting is now used on both Desktop and Android, and is now NOT_LOSSY.

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

[Charlie Harrison, 2017-05-10 12:26:44]

Description was changed from

==========
[subresource_filter] Make website setting existence imply site activation

This patch tries its best to make the relationship 1:1 between site activation
status and the WebsiteSetting metadata being present for that origin.

The setting is added when a site is activated and the UI is shown (to prevent
cases
where no resources are filtered or we are activated but suppressing UI).

The setting is removed when a site is found to not activate for a particular top
level
navigation (% content setting whitelists), or when a url matching the setting's
origin
is deleted from history. Since we don't want to clear this state if the user
makes
some explicit action (like whitelisting), we make sure the whitelisting check is
the
very last one in activation computation.

In this way the setting is a best approximation for a cached activation state
for a
given origin. Currently it is only used for the smart UI, but in follow up
patches we
will make other settings UI surfaces dependent on this state.

Note: The setting is now used on both Desktop and Android, and is now NOT_LOSSY.

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

to

==========
[subresource_filter] Make website setting existence imply site activation

This patch tries its best to make the relationship 1:1 between site activation
status and the WebsiteSetting metadata being present for that origin.

The setting is added when a site is activated and the UI is shown (to prevent
cases
where no resources are filtered or we are activated but suppressing UI).

The setting is removed when a site is found to not activate for a particular top
level
navigation (% content setting whitelists), or when a url matching the setting's
origin
is deleted from history. Since we don't want to clear this state if the user
makes
some explicit action (like whitelisting), we make sure the whitelisting check is
the
very last one in activation computation and don't take it into account for
setting
removal.

In this way the setting is a best approximation for a cached activation state
(minus
content setting policy) for a given origin. Currently it is only used for the
smart UI, 
but in follow up patches we will make other settings UI surfaces dependent on
this
 state.

Note: The setting is now used on both Desktop and Android, and is now NOT_LOSSY.

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

[Charlie Harrison, 2017-05-10 12:30:00]

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

[Charlie Harrison, 2017-05-10 12:30:01]

OK I think this CL is now ready for review. shivanisha@ would you take another
look? Sorry for the run-around here.

+engedy for shadow review as well, would you also take special thought for if
this will work OK with multiple simultaneous configurations? I'm pretty sure
everything will be OK.

[engedy, 2017-05-10 13:19:10]

The only gotcha I can think of is that users who are running a dry-run
performance testing experiment on all sites will seemingly never clear website
settings if they are ever set. Otherwise LGTM.

[Charlie Harrison, 2017-05-10 18:39:09]

On 2017/05/10 13:19:10, engedy wrote:
> The only gotcha I can think of is that users who are running a dry-run
> performance testing experiment on all sites will seemingly never clear website
> settings if they are ever set. Otherwise LGTM.

Hm what if we also plumb the |should_suppress_notifications| up to the client
and clear the setting if that is true as well?

[Charlie Harrison, 2017-05-10 18:39:42]

On 2017/05/10 18:39:09, Charlie Harrison wrote:
> On 2017/05/10 13:19:10, engedy wrote:
> > The only gotcha I can think of is that users who are running a dry-run
> > performance testing experiment on all sites will seemingly never clear
website
> > settings if they are ever set. Otherwise LGTM.
> 
> Hm what if we also plumb the |should_suppress_notifications| up to the client
> and clear the setting if that is true as well?

Need to make sure that DRYRUNs where we don't want to show any UI mark that bit
as well.

[Charlie Harrison, 2017-05-10 18:44:27]

On 2017/05/10 18:39:42, Charlie Harrison wrote:
> On 2017/05/10 18:39:09, Charlie Harrison wrote:
> > On 2017/05/10 13:19:10, engedy wrote:
> > > The only gotcha I can think of is that users who are running a dry-run
> > > performance testing experiment on all sites will seemingly never clear
> website
> > > settings if they are ever set. Otherwise LGTM.
> > 
> > Hm what if we also plumb the |should_suppress_notifications| up to the
client
> > and clear the setting if that is true as well?
> 
> Need to make sure that DRYRUNs where we don't want to show any UI mark that
bit
> as well.

Sorry for all the spam. Now I am thinking we could just plumb ActivationDecision
== ACTIVATED && ActivationLevel == ENABLED. Clearing this setting if the site is
in DRYRUN mode sounds reasonable to me.

[engedy, 2017-05-10 18:45:34]

> Sorry for all the spam. Now I am thinking we could just plumb
ActivationDecision
> == ACTIVATED && ActivationLevel == ENABLED. Clearing this setting if the site
is
> in DRYRUN mode sounds reasonable to me.

That SGTM.

[engedy, 2017-05-10 18:45:35]

> Sorry for all the spam. Now I am thinking we could just plumb
ActivationDecision
> == ACTIVATED && ActivationLevel == ENABLED. Clearing this setting if the site
is
> in DRYRUN mode sounds reasonable to me.

That SGTM.

[Charlie Harrison, 2017-05-10 19:14:19]

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

[Charlie Harrison, 2017-05-10 19:15:36]

OK, I've updated the PS accordingly and added a test for this case.

[commit-bot: I haz the power, 2017-05-10 19:16:24]

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-05-10 20:25:51]

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

[commit-bot: I haz the power, 2017-05-10 20:25:52]

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-05-10 20:27:10]

LGTM

[Charlie Harrison, 2017-05-11 12:57:58]

Thanks!

[Charlie Harrison, 2017-05-11 12:58:22]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-05-11 12:58:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
shivanisha@chromium.org, engedy@chromium.org, msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/2859783002/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-11 12:58:44]

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-05-11 14:22:46]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1494507502714440,
"parent_rev": "a226fc9126a5ef1ed75fe435eb7a44d010e470fb", "commit_rev":
"31a9f6426ec06155f4b95e5e125d76e0371d0c74"}

[commit-bot: I haz the power, 2017-05-11 14:22:57]

Description was changed from

==========
[subresource_filter] Make website setting existence imply site activation

This patch tries its best to make the relationship 1:1 between site activation
status and the WebsiteSetting metadata being present for that origin.

The setting is added when a site is activated and the UI is shown (to prevent
cases
where no resources are filtered or we are activated but suppressing UI).

The setting is removed when a site is found to not activate for a particular top
level
navigation (% content setting whitelists), or when a url matching the setting's
origin
is deleted from history. Since we don't want to clear this state if the user
makes
some explicit action (like whitelisting), we make sure the whitelisting check is
the
very last one in activation computation and don't take it into account for
setting
removal.

In this way the setting is a best approximation for a cached activation state
(minus
content setting policy) for a given origin. Currently it is only used for the
smart UI, 
but in follow up patches we will make other settings UI surfaces dependent on
this
 state.

Note: The setting is now used on both Desktop and Android, and is now NOT_LOSSY.

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

to

==========
[subresource_filter] Make website setting existence imply site activation

This patch tries its best to make the relationship 1:1 between site activation
status and the WebsiteSetting metadata being present for that origin.

The setting is added when a site is activated and the UI is shown (to prevent
cases
where no resources are filtered or we are activated but suppressing UI).

The setting is removed when a site is found to not activate for a particular top
level
navigation (% content setting whitelists), or when a url matching the setting's
origin
is deleted from history. Since we don't want to clear this state if the user
makes
some explicit action (like whitelisting), we make sure the whitelisting check is
the
very last one in activation computation and don't take it into account for
setting
removal.

In this way the setting is a best approximation for a cached activation state
(minus
content setting policy) for a given origin. Currently it is only used for the
smart UI, 
but in follow up patches we will make other settings UI surfaces dependent on
this
 state.

Note: The setting is now used on both Desktop and Android, and is now NOT_LOSSY.

BUG=689487

Review-Url: https://codereview.chromium.org/2859783002
Cr-Commit-Position: refs/heads/master@{#470945}
Committed:
https://chromium.googlesource.com/chromium/src/+/31a9f6426ec06155f4b95e5e125d...
==========

[commit-bot: I haz the power, 2017-05-11 14:23:02]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/31a9f6426ec06155f4b95e5e125d...