[subresource_filter] s/ShouldSuppressActivation/OnPageActivationComputed

[subresource_filter] s/ShouldSuppressActivation/OnPageActivationComputed

This patch slightly refactors the SubresourceFilterClient::ShouldSuppressActivation
to be the very last check in activation computation, as well renaming it to
OnPageActivationComputed (which takes an extra |activated| bool).

This will be used as an accurate //chrome level signal for when activation is disabled
for a given site.

BUG=689487
Review-Url: https://codereview.chromium.org/2874663002
Cr-Commit-Position: refs/heads/master@{#470885}
Committed: https://chromium.googlesource.com/chromium/src/+/61d9b5150be41b4e7e5390a5c1e242f90709539a

[Charlie Harrison, 2017-05-10 01:51:15]

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

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

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-10 02:17:02]

Description was changed from

==========
[subresource_filter] s/ShouldSuppressActivation/OnPageActivationComputed

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

to

==========
[subresource_filter] s/ShouldSuppressActivation/OnPageActivationComputed

This patch slightly refactors the
SubresourceFilterClient::ShouldSuppressActivation
to be the very last check in activation computation, as well renaming it to
OnPageActivationComputed (which takes an extra |activated| bool).

This will be used as an accurate //chrome level signal for when activation is
disabled
for a given site.

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

[Charlie Harrison, 2017-05-10 02:17:49]

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

[Charlie Harrison, 2017-05-10 02:17:50]

engedy, shivanisha: PTAL?

[commit-bot: I haz the power, 2017-05-10 02:53:35]

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

[commit-bot: I haz the power, 2017-05-10 02:53:36]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Charlie Harrison, 2017-05-10 03:35:08]

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

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

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

[commit-bot: I haz the power, 2017-05-10 05:41:11]

Dry run: This issue passed the CQ dry run.

[engedy, 2017-05-10 09:44:26]

LGTM % nits. Thanks!

https://codereview.chromium.org/2874663002/diff/20001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2874663002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:190:
// The client wants the best attempt at activation computation, so make sure
nit: Check for the whitelisted status last, so that the client gets an accurate
indication of whether there would be activation otherwise.

https://codereview.chromium.org/2874663002/diff/20001/components/subresource_...
File components/subresource_filter/content/browser/subresource_filter_client.h
(right):

https://codereview.chromium.org/2874663002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/subresource_filter_client.h:31: //
Call when the activation is otherwise completely computedby the subresource
nit: Called .. activation decision .. computed by

https://codereview.chromium.org/2874663002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/subresource_filter_client.h:32: //
filter. All that is left is embedder veto. Returns whether the activation
nit: At this point, the embedder still has a chance to return false to
suppress/veto the activation.

https://codereview.chromium.org/2874663002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/subresource_filter_client.h:33: //
should be whitelisted.
What's the lifetime of the veto? This navigation? Forever? Let's mention that.

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

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

[commit-bot: I haz the power, 2017-05-10 12:19:32]

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-10 12:19:34]

Thanks! Shivani PTAL?

https://codereview.chromium.org/2874663002/diff/20001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2874663002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:190:
// The client wants the best attempt at activation computation, so make sure
On 2017/05/10 09:44:25, engedy wrote:
> nit: Check for the whitelisted status last, so that the client gets an
accurate
> indication of whether there would be activation otherwise.

Done.

https://codereview.chromium.org/2874663002/diff/20001/components/subresource_...
File components/subresource_filter/content/browser/subresource_filter_client.h
(right):

https://codereview.chromium.org/2874663002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/subresource_filter_client.h:31: //
Call when the activation is otherwise completely computedby the subresource
On 2017/05/10 09:44:25, engedy wrote:
> nit: Called .. activation decision .. computed by

Done.

https://codereview.chromium.org/2874663002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/subresource_filter_client.h:32: //
filter. All that is left is embedder veto. Returns whether the activation
On 2017/05/10 09:44:25, engedy wrote:
> nit: At this point, the embedder still has a chance to return false to
> suppress/veto the activation.

Done.

https://codereview.chromium.org/2874663002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/subresource_filter_client.h:33: //
should be whitelisted.
On 2017/05/10 09:44:25, engedy wrote:
> What's the lifetime of the veto? This navigation? Forever? Let's mention that.

Added "should be whitelisted for this navigation"

[engedy, 2017-05-10 13:09:20]

LGTM, thanks!

[commit-bot: I haz the power, 2017-05-10 13:16:59]

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

[commit-bot: I haz the power, 2017-05-10 13:16:59]

Dry run: This issue passed the CQ dry run.

[shivanisha, 2017-05-10 17:34:26]

lgtm % nit

https://codereview.chromium.org/2874663002/diff/40001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
(right):

https://codereview.chromium.org/2874663002/diff/40001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:196:
whitelisted_hosts_.end();
nit (for sake of conciseness):
 
return activated && whitelisted_hosts_.count(handle->GetURL().host()) != 0;

[engedy, 2017-05-10 17:40:20]

https://codereview.chromium.org/2874663002/diff/40001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
(right):

https://codereview.chromium.org/2874663002/diff/40001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:196:
whitelisted_hosts_.end();
On 2017/05/10 17:34:25, shivanisha wrote:
> nit (for sake of conciseness):
>  
> return activated && whitelisted_hosts_.count(handle->GetURL().host()) != 0;

Actually, I just discovered there is base::ContainsValue(...) which does exactly
this (and also terminates after the first match).

[Charlie Harrison, 2017-05-10 17:52:13]

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

[commit-bot: I haz the power, 2017-05-10 17:53: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-10 18:16:42]

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

[commit-bot: I haz the power, 2017-05-10 18:17:20]

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-10 18:17:21]

Thanks

https://codereview.chromium.org/2874663002/diff/40001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
(right):

https://codereview.chromium.org/2874663002/diff/40001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:196:
whitelisted_hosts_.end();
On 2017/05/10 17:40:20, engedy wrote:
> On 2017/05/10 17:34:25, shivanisha wrote:
> > nit (for sake of conciseness):
> >  
> > return activated && whitelisted_hosts_.count(handle->GetURL().host()) != 0;
> 
> Actually, I just discovered there is base::ContainsValue(...) which does
exactly
> this (and also terminates after the first match).

Changed to use base::ContainsValue, but it's a set so count() will also
terminate after the first match.

[engedy, 2017-05-10 18:18:27]

On 2017/05/10 18:17:21, Charlie Harrison wrote:
> Thanks
> 
>
https://codereview.chromium.org/2874663002/diff/40001/components/subresource_...
> File
>
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2874663002/diff/40001/components/subresource_...
>
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:196:
> whitelisted_hosts_.end();
> On 2017/05/10 17:40:20, engedy wrote:
> > On 2017/05/10 17:34:25, shivanisha wrote:
> > > nit (for sake of conciseness):
> > >  
> > > return activated && whitelisted_hosts_.count(handle->GetURL().host()) !=
0;
> > 
> > Actually, I just discovered there is base::ContainsValue(...) which does
> exactly
> > this (and also terminates after the first match).
> 
> Changed to use base::ContainsValue, but it's a set so count() will also
> terminate after the first match.

In that case we should use set::count, because base::ContainsValue will be O(n).

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

On 2017/05/10 18:18:27, engedy wrote:
> On 2017/05/10 18:17:21, Charlie Harrison wrote:
> > Thanks
> > 
> >
>
https://codereview.chromium.org/2874663002/diff/40001/components/subresource_...
> > File
> >
>
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2874663002/diff/40001/components/subresource_...
> >
>
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:196:
> > whitelisted_hosts_.end();
> > On 2017/05/10 17:40:20, engedy wrote:
> > > On 2017/05/10 17:34:25, shivanisha wrote:
> > > > nit (for sake of conciseness):
> > > >  
> > > > return activated && whitelisted_hosts_.count(handle->GetURL().host()) !=
> 0;
> > > 
> > > Actually, I just discovered there is base::ContainsValue(...) which does
> > exactly
> > > this (and also terminates after the first match).
> > 
> > Changed to use base::ContainsValue, but it's a set so count() will also
> > terminate after the first match.
> 
> In that case we should use set::count, because base::ContainsValue will be
O(n).

doh of course. I thought ContainsValue was just a wrapper around ".find() !=
.end()". Moved back to count()

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

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

[commit-bot: I haz the power, 2017-05-10 18:30:11]

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-10 18:43:19]

https://codereview.chromium.org/2874663002/diff/100001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
(right):

https://codereview.chromium.org/2874663002/diff/100001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:196:
whitelisted_hosts_.end();
Did you mean to move back to count()?

[Charlie Harrison, 2017-05-10 18:55:19]

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

[Charlie Harrison, 2017-05-10 18:55:21]

https://codereview.chromium.org/2874663002/diff/100001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
(right):

https://codereview.chromium.org/2874663002/diff/100001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:196:
whitelisted_hosts_.end();
On 2017/05/10 18:43:18, shivanisha wrote:
> Did you mean to move back to count()?

Oh, I just forgot to update the test only implementation of the Mock client.
I'll update this one to use count() too.

[Charlie Harrison, 2017-05-10 18:57:32]

https://codereview.chromium.org/2874663002/diff/100001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
(right):

https://codereview.chromium.org/2874663002/diff/100001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:196:
whitelisted_hosts_.end();
On 2017/05/10 18:55:21, Charlie Harrison wrote:
> On 2017/05/10 18:43:18, shivanisha wrote:
> > Did you mean to move back to count()?
> 
> Oh, I just forgot to update the test only implementation of the Mock client.
> I'll update this one to use count() too.

Done. OK landing now :)

[commit-bot: I haz the power, 2017-05-10 18:57:37]

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

The CQ bit was unchecked by csharrison@chromium.org

[Charlie Harrison, 2017-05-10 19:37:46]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-05-10 19:37:46]

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

[commit-bot: I haz the power, 2017-05-10 19:38:25]

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

[commit-bot: I haz the power, 2017-05-10 19:38:26]

This CL has an open dependency (Issue 2871013002 Patch 40001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[Charlie Harrison, 2017-05-10 23:55:05]

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

[commit-bot: I haz the power, 2017-05-10 23:56: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-05-11 00:41:16]

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

[commit-bot: I haz the power, 2017-05-11 00:41:17]

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

[Charlie Harrison, 2017-05-11 04:14:17]

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

[commit-bot: I haz the power, 2017-05-11 04:15:27]

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-11 04:29:42]

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

[commit-bot: I haz the power, 2017-05-11 04:29:55]

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-11 04:31:46]

I had to make some minor tweaks to the code to get this working but it should be
fairly straightforward.

One last pass from one of you would be nice. Feel free to CQ too.

[commit-bot: I haz the power, 2017-05-11 06:08:09]

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

[commit-bot: I haz the power, 2017-05-11 06:08:10]

Dry run: This issue passed the CQ dry run.

[engedy, 2017-05-11 09:13:14]

Still LGTM.

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

https://codereview.chromium.org/2874663002/diff/180001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:200:
// Check for whitelisted status last, so that the client gets an accurate
follow up nit: Any reason to not do this block of logic at the very end of
ComputeActivationForMainFrameNavigation?

https://codereview.chromium.org/2874663002/diff/180001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
(right):

https://codereview.chromium.org/2874663002/diff/180001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:802:
// Whitelisting is only applied when the page will otherwise activate.
I started deprioritizing ActivationDecision::WHITELISTED in the multiple
simultaneous configs CL, and this concludes that change. I like it!

[engedy, 2017-05-11 09:13:21]

The CQ bit was checked by engedy@chromium.org

[engedy, 2017-05-11 09:13:21]

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

[commit-bot: I haz the power, 2017-05-11 09:13:52]

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 09:19:52]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1494494001332820,
"parent_rev": "715c833d5ba12cedbf0250778ce9c7db4a517490", "commit_rev":
"61d9b5150be41b4e7e5390a5c1e242f90709539a"}

[commit-bot: I haz the power, 2017-05-11 09:20:06]

Description was changed from

==========
[subresource_filter] s/ShouldSuppressActivation/OnPageActivationComputed

This patch slightly refactors the
SubresourceFilterClient::ShouldSuppressActivation
to be the very last check in activation computation, as well renaming it to
OnPageActivationComputed (which takes an extra |activated| bool).

This will be used as an accurate //chrome level signal for when activation is
disabled
for a given site.

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

to

==========
[subresource_filter] s/ShouldSuppressActivation/OnPageActivationComputed

This patch slightly refactors the
SubresourceFilterClient::ShouldSuppressActivation
to be the very last check in activation computation, as well renaming it to
OnPageActivationComputed (which takes an extra |activated| bool).

This will be used as an accurate //chrome level signal for when activation is
disabled
for a given site.

BUG=689487

Review-Url: https://codereview.chromium.org/2874663002
Cr-Commit-Position: refs/heads/master@{#470885}
Committed:
https://chromium.googlesource.com/chromium/src/+/61d9b5150be41b4e7e5390a5c1e2...
==========

[commit-bot: I haz the power, 2017-05-11 09:20:07]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/61d9b5150be41b4e7e5390a5c1e2...

[Charlie Harrison, 2017-05-11 12:07:16]

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

https://codereview.chromium.org/2874663002/diff/180001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:200:
// Check for whitelisted status last, so that the client gets an accurate
On 2017/05/11 09:13:14, engedy wrote:
> follow up nit: Any reason to not do this block of logic at the very end of
> ComputeActivationForMainFrameNavigation?

I chose to do this here so that we can continue to use early return semantics in
ComputeActivation*. Otherwise the method gets messier and harder to reason
about.

I don't feel very strongly about it though.

https://codereview.chromium.org/2874663002/diff/180001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
(right):

https://codereview.chromium.org/2874663002/diff/180001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:802:
// Whitelisting is only applied when the page will otherwise activate.
On 2017/05/11 09:13:14, engedy wrote:
> I started deprioritizing ActivationDecision::WHITELISTED in the multiple
> simultaneous configs CL, and this concludes that change. I like it!

woohoo!