[subresource_filter] Move activation computation to the SB throttle

[subresource_filter] Move activation computation to the SB throttle

This CL moves computation of the ActivationDecision / ActivationOptions
from the driver factory to the SafeBrowsing throttle. Page activation is
then forwarded to the driver factory, which forwards it to the observer
manager.

This is in preparation to remove the driver factory entirely.

BUG=708181
Review-Url: https://codereview.chromium.org/2894023002
Cr-Commit-Position: refs/heads/master@{#474644}
Committed: https://chromium.googlesource.com/chromium/src/+/27543b37fdfe82a4a1df1f0435969875b182585b

[Charlie Harrison, 2017-05-18 21:31:53]

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

[commit-bot: I haz the power, 2017-05-18 21:33: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-18 21:40:48]

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

[commit-bot: I haz the power, 2017-05-18 21:40:49]

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...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  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-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Harrison, 2017-05-22 17:45:58]

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

[commit-bot: I haz the power, 2017-05-22 17:46:37]

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-22 18:14:52]

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

[commit-bot: I haz the power, 2017-05-22 18:14:53]

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

[Charlie Harrison, 2017-05-23 17:55:02]

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

[commit-bot: I haz the power, 2017-05-23 17:55:52]

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-23 18:02:15]

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

[Charlie Harrison, 2017-05-23 18:02:16]

Shivani would you PTAL?

[shivanisha, 2017-05-24 19:34:07]

https://codereview.chromium.org/2894023002/diff/60001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h
(right):

https://codereview.chromium.org/2894023002/diff/60001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:48:
void OnPageActivationComputed(
Need comment for this API. Is this intended to be a SubresourceFilterObservers
and override the function with the same name there?

https://codereview.chromium.org/2894023002/diff/60001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:55:
ActivationDecision GetActivationDecisionForLastCommittedPageLoad() const {
A TODO is probably needed here or are they getting removed in a subsequent CL? I
actually see a TODO in code search for this.

https://codereview.chromium.org/2894023002/diff/60001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:60:
// document. Do not rely on this API, it is only temporary.
 A TODO here for when this will be removed?

https://codereview.chromium.org/2894023002/diff/60001/components/subresource_...
File
components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h
(right):

https://codereview.chromium.org/2894023002/diff/60001/components/subresource_...
components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h:72:
// activation |conditions| of a given configuration, except for |priority|.
|priority| ?

[Charlie Harrison, 2017-05-24 20:49:51]

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

[Charlie Harrison, 2017-05-24 20:50:12]

https://codereview.chromium.org/2894023002/diff/60001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h
(right):

https://codereview.chromium.org/2894023002/diff/60001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:48:
void OnPageActivationComputed(
On 2017/05/24 19:34:06, shivanisha wrote:
> Need comment for this API. Is this intended to be a SubresourceFilterObservers
> and override the function with the same name there?

Heh no this method actually notifies the observers. This patch was written
before the SubresourceFilterObserver stuff was so let me rename this method.

https://codereview.chromium.org/2894023002/diff/60001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:55:
ActivationDecision GetActivationDecisionForLastCommittedPageLoad() const {
On 2017/05/24 19:34:06, shivanisha wrote:
> A TODO is probably needed here or are they getting removed in a subsequent CL?
I
> actually see a TODO in code search for this.

Yeah this CL needs to be rebased, there is currently a TODO.

https://codereview.chromium.org/2894023002/diff/60001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:60:
// document. Do not rely on this API, it is only temporary.
On 2017/05/24 19:34:06, shivanisha wrote:
>  A TODO here for when this will be removed?

Done.

https://codereview.chromium.org/2894023002/diff/60001/components/subresource_...
File
components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h
(right):

https://codereview.chromium.org/2894023002/diff/60001/components/subresource_...
components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h:72:
// activation |conditions| of a given configuration, except for |priority|.
On 2017/05/24 19:34:06, shivanisha wrote:
> |priority| ?

It is a member of |conditions|

[commit-bot: I haz the power, 2017-05-24 20:50: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-05-24 22:21:51]

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

[commit-bot: I haz the power, 2017-05-24 22:21:52]

Dry run: This issue passed the CQ dry run.

[shivanisha, 2017-05-25 12:52:46]

On 2017/05/24 at 20:50:12, csharrison wrote:
>
https://codereview.chromium.org/2894023002/diff/60001/components/subresource_...
> File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h
(right):
> 
>
https://codereview.chromium.org/2894023002/diff/60001/components/subresource_...
>
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:48:
void OnPageActivationComputed(
> On 2017/05/24 19:34:06, shivanisha wrote:
> > Need comment for this API. Is this intended to be a
SubresourceFilterObservers
> > and override the function with the same name there?
> 
> Heh no this method actually notifies the observers. This patch was written
before the SubresourceFilterObserver stuff was so let me rename this method.
> 
>
https://codereview.chromium.org/2894023002/diff/60001/components/subresource_...
>
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:55:
ActivationDecision GetActivationDecisionForLastCommittedPageLoad() const {
> On 2017/05/24 19:34:06, shivanisha wrote:
> > A TODO is probably needed here or are they getting removed in a subsequent
CL? I
> > actually see a TODO in code search for this.
> 
> Yeah this CL needs to be rebased, there is currently a TODO.
> 
>
https://codereview.chromium.org/2894023002/diff/60001/components/subresource_...
>
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:60:
// document. Do not rely on this API, it is only temporary.
> On 2017/05/24 19:34:06, shivanisha wrote:
> >  A TODO here for when this will be removed?
> 
> Done.
> 
>
https://codereview.chromium.org/2894023002/diff/60001/components/subresource_...
> File
components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h
(right):
> 
>
https://codereview.chromium.org/2894023002/diff/60001/components/subresource_...
>
components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h:72:
// activation |conditions| of a given configuration, except for |priority|.
> On 2017/05/24 19:34:06, shivanisha wrote:
> > |priority| ?
> 
> It is a member of |conditions|

lgtm

[Charlie Harrison, 2017-05-25 13:30:40]

Description was changed from

==========
[subresource_filter] Move activation computation to the SB throttle

BUG=708181
==========

to

==========
[subresource_filter] Move activation computation to the SB throttle

This CL moves computation of the ActivationDecision / ActivationOptions
from the driver factory to the SafeBrowsing throttle. Page activation is
then forwarded to the driver factory, which forwards it to the observer
manager.

This is in preparation to remove the driver factory entirely.

BUG=708181
==========

[Charlie Harrison, 2017-05-25 14:20:27]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2017-05-25 14:20:39]

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-25 14:25:15]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495722027328440,
"parent_rev": "fb9a235342811e5c63be2ae03956c126b745fc0a", "commit_rev":
"27543b37fdfe82a4a1df1f0435969875b182585b"}

[commit-bot: I haz the power, 2017-05-25 14:25:30]

Description was changed from

==========
[subresource_filter] Move activation computation to the SB throttle

This CL moves computation of the ActivationDecision / ActivationOptions
from the driver factory to the SafeBrowsing throttle. Page activation is
then forwarded to the driver factory, which forwards it to the observer
manager.

This is in preparation to remove the driver factory entirely.

BUG=708181
==========

to

==========
[subresource_filter] Move activation computation to the SB throttle

This CL moves computation of the ActivationDecision / ActivationOptions
from the driver factory to the SafeBrowsing throttle. Page activation is
then forwarded to the driver factory, which forwards it to the observer
manager.

This is in preparation to remove the driver factory entirely.

BUG=708181

Review-Url: https://codereview.chromium.org/2894023002
Cr-Commit-Position: refs/heads/master@{#474644}
Committed:
https://chromium.googlesource.com/chromium/src/+/27543b37fdfe82a4a1df1f043596...
==========

[commit-bot: I haz the power, 2017-05-25 14:25:31]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/27543b37fdfe82a4a1df1f043596...