[subresource_filter] Refactor activation suppression

[subresource_filter] Refactor activation suppression

Right now, we query for activation suppression in two places
1. Computing ActivationDecision in the driver_factory
2. Right before sending the Activation IPC in the throttle_manager

Since (1) fully controls notifying page activation to the
throttle_manager, (2) is completely unnecessary. This CL removes that
functionality from the throttle manager delegate.

BUG=689487
Review-Url: https://codereview.chromium.org/2871013002
Cr-Commit-Position: refs/heads/master@{#470765}
Committed: https://chromium.googlesource.com/chromium/src/+/19811b689efb15501224d8c8151abe0bbc8392a6

[Charlie Harrison, 2017-05-09 21:26:56]

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

[Charlie Harrison, 2017-05-09 21:28:08]

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

[Charlie Harrison, 2017-05-09 21:28:09]

shivanisha, engedy: PTAL? I feel this change will make landing
https://codereview.chromium.org/2859783002 easier.

[commit-bot: I haz the power, 2017-05-09 21:28:11]

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 21:40:40]

Hm thinking about it I think we need
s/OnWillTriggerActivation/OnActivationComputed/ or something similar so the code
will know when a site is deactivated. Let me fix the CL.

[Charlie Harrison, 2017-05-09 22:00:01]

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

[Charlie Harrison, 2017-05-09 22:00:16]

Description was changed from

==========
[subresource_filter] Refactor activation suppression

Right now, we query for activation suppression in two places
1. Computing ActivationDecision in the driver_factory
2. Right before sending the Activation IPC in the throttle_manager

Since (1) fully controls notifying page activation to the
throttle_manager, (2) is completely unnecessary. This CL removes that
functionality from the throttle manager delegate.

Additionally, this CL refactors suppression calculation by ensuring it
is the very last thing the driver_factory does when computing the
ActivationDecision. This ensures that the ChromeClient can be sure that
the navigations it is deciding suppression for would have been activated
were it not for //chrome specific policy.

This last piece will be useful when we need data on whether a site is
marked for activation without taking into account content settings.

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

to

==========
[subresource_filter] Refactor activation suppression

Right now, we query for activation suppression in two places
1. Computing ActivationDecision in the driver_factory
2. Right before sending the Activation IPC in the throttle_manager

Since (1) fully controls notifying page activation to the
throttle_manager, (2) is completely unnecessary. This CL removes that
functionality from the throttle manager delegate.

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

[commit-bot: I haz the power, 2017-05-09 22:00:19]

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 22:01:04]

The current CL just removes the activation suppression call.

I'll do the OnPageActivationComputed behavior in a follow up.

[engedy, 2017-05-09 22:13:35]

LGTM, thanks!

https://codereview.chromium.org/2871013002/diff/20001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc
(left):

https://codereview.chromium.org/2871013002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:79:
it->second->NotifyPageActivationWithRuleset(EnsureRulesetHandle(),
Unrelated question: after https://codereview.chromium.org/2844063002/, this
might be called with activation_state == DISABLED. This is not going to break
things, right? (I haven't looked into it at all.)

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

https://codereview.chromium.org/2871013002/diff/20001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc
(left):

https://codereview.chromium.org/2871013002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:79:
it->second->NotifyPageActivationWithRuleset(EnsureRulesetHandle(),
On 2017/05/09 22:13:35, engedy wrote:
> Unrelated question: after https://codereview.chromium.org/2844063002/, this
> might be called with activation_state == DISABLED. This is not going to break
> things, right? (I haven't looked into it at all.)

Ah, it isn't great. We don't want to call EnsureRulesetHandle() in that case,
just pass null if there is no activation. Other than that it should work.

[Charlie Harrison, 2017-05-09 22:33:58]

https://codereview.chromium.org/2871013002/diff/20001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc
(left):

https://codereview.chromium.org/2871013002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:79:
it->second->NotifyPageActivationWithRuleset(EnsureRulesetHandle(),
On 2017/05/09 22:27:17, Charlie Harrison wrote:
> On 2017/05/09 22:13:35, engedy wrote:
> > Unrelated question: after https://codereview.chromium.org/2844063002/, this
> > might be called with activation_state == DISABLED. This is not going to
break
> > things, right? (I haven't looked into it at all.)
> 
> Ah, it isn't great. We don't want to call EnsureRulesetHandle() in that case,
> just pass null if there is no activation. Other than that it should work.

Will it actually be called like this actually? I see
DCHECK_NE(activation_options_.activation_level, ActivationLevel::DISABLED);
right before calling this method.

[engedy, 2017-05-09 22:55:48]

https://codereview.chromium.org/2871013002/diff/20001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc
(left):

https://codereview.chromium.org/2871013002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:79:
it->second->NotifyPageActivationWithRuleset(EnsureRulesetHandle(),
On 2017/05/09 22:33:58, Charlie Harrison wrote:
> On 2017/05/09 22:27:17, Charlie Harrison wrote:
> > On 2017/05/09 22:13:35, engedy wrote:
> > > Unrelated question: after https://codereview.chromium.org/2844063002/,
this
> > > might be called with activation_state == DISABLED. This is not going to
> break
> > > things, right? (I haven't looked into it at all.)
> > 
> > Ah, it isn't great. We don't want to call EnsureRulesetHandle() in that
case,
> > just pass null if there is no activation. Other than that it should work.
> 
> Will it actually be called like this actually? I see
> DCHECK_NE(activation_options_.activation_level, ActivationLevel::DISABLED);
> right before calling this method.

Ahh, that's right. Crisis averted!

[commit-bot: I haz the power, 2017-05-10 00:06:27]

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

[commit-bot: I haz the power, 2017-05-10 00:06:29]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has
not started yet; builder either lacks capacity or does not exist (misspelled?))
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[Charlie Harrison, 2017-05-10 00:10:26]

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

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

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 02:14:53]

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

[commit-bot: I haz the power, 2017-05-10 02:14:54]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

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

On 2017/05/09 at 22:55:48, engedy wrote:
>
https://codereview.chromium.org/2871013002/diff/20001/components/subresource_...
> File
components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc
(left):
> 
>
https://codereview.chromium.org/2871013002/diff/20001/components/subresource_...
>
components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:79:
it->second->NotifyPageActivationWithRuleset(EnsureRulesetHandle(),
> On 2017/05/09 22:33:58, Charlie Harrison wrote:
> > On 2017/05/09 22:27:17, Charlie Harrison wrote:
> > > On 2017/05/09 22:13:35, engedy wrote:
> > > > Unrelated question: after https://codereview.chromium.org/2844063002/,
this
> > > > might be called with activation_state == DISABLED. This is not going to
> > break
> > > > things, right? (I haven't looked into it at all.)
> > > 
> > > Ah, it isn't great. We don't want to call EnsureRulesetHandle() in that
case,
> > > just pass null if there is no activation. Other than that it should work.
> > 
> > Will it actually be called like this actually? I see
> > DCHECK_NE(activation_options_.activation_level, ActivationLevel::DISABLED);
> > right before calling this method.
> 
> Ahh, that's right. Crisis averted!

lgtm

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

The CQ bit was checked by csharrison@chromium.org

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

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 17:35:03]

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

[commit-bot: I haz the power, 2017-05-10 17:35:04]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

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

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

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

The CQ bit was checked by csharrison@chromium.org

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

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/2871013002/#ps40001
(title: "rebase on #470635")

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

The CQ bit was unchecked by csharrison@chromium.org

[Charlie Harrison, 2017-05-10 18:56:52]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2017-05-10 18:58:59]

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 23:14:49]

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

[commit-bot: I haz the power, 2017-05-10 23:14:50]

Try jobs failed on following builders:
  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 23:27:34]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2017-05-10 23:29:20]

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:47:41]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494458854485560,
"parent_rev": "9880581d62ccb38559e5d8503c802e78c414ef22", "commit_rev":
"19811b689efb15501224d8c8151abe0bbc8392a6"}

[commit-bot: I haz the power, 2017-05-11 00:47:53]

Description was changed from

==========
[subresource_filter] Refactor activation suppression

Right now, we query for activation suppression in two places
1. Computing ActivationDecision in the driver_factory
2. Right before sending the Activation IPC in the throttle_manager

Since (1) fully controls notifying page activation to the
throttle_manager, (2) is completely unnecessary. This CL removes that
functionality from the throttle manager delegate.

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

to

==========
[subresource_filter] Refactor activation suppression

Right now, we query for activation suppression in two places
1. Computing ActivationDecision in the driver_factory
2. Right before sending the Activation IPC in the throttle_manager

Since (1) fully controls notifying page activation to the
throttle_manager, (2) is completely unnecessary. This CL removes that
functionality from the throttle manager delegate.

BUG=689487

Review-Url: https://codereview.chromium.org/2871013002
Cr-Commit-Position: refs/heads/master@{#470765}
Committed:
https://chromium.googlesource.com/chromium/src/+/19811b689efb15501224d8c8151a...
==========

[commit-bot: I haz the power, 2017-05-11 00:47:54]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/19811b689efb15501224d8c8151a...