Make ChromeSubresourceFilterClient a WebContentsUserData

Make ChromeSubresourceFilterClient a WebContentsUserData

This also modifies the ownership of the subresource filter client, which now must
outlive the throttle manager / driver factory.

BUG=717590
Review-Url: https://codereview.chromium.org/2850373002
Cr-Commit-Position: refs/heads/master@{#469331}
Committed: https://chromium.googlesource.com/chromium/src/+/f8f2850cf1004785650d0ab597c4b517aa8204a2

[Charlie Harrison, 2017-05-02 05:02:35]

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

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

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-02 05:24:57]

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

[Charlie Harrison, 2017-05-02 05:24:57]

engedy, bmcquade: PTAL?

This is the first step to landing something like [1].

My proposed next steps are:
1. This CL, which exposes the client to //chrome code

2. Make SB throttles attach to every main frame navigation, moving *creation*
logic into *should query SB* logic. This allows the throttle to process sites if
we have activation_scope all_sites. This step deletes navigation_throttle_util.

3. Move activation decision logic to the SB throttle, nuking a bunch of code in
the driver factory. We can store a mapping of navigation handles to SB throttles
to implement a "GetActivationDecisionForNavigation" method on the client, used
by metrics code.

[1]: https://codereview.chromium.org/2841933003/

[Charlie Harrison, 2017-05-02 05:28:28]

On 2017/05/02 05:24:57, Charlie (ooo-ish until may 2) wrote:
> engedy, bmcquade: PTAL?
> 
> This is the first step to landing something like [1].
> 
> My proposed next steps are:
> 1. This CL, which exposes the client to //chrome code
> 
> 2. Make SB throttles attach to every main frame navigation, moving *creation*
> logic into *should query SB* logic. This allows the throttle to process sites
if
> we have activation_scope all_sites. This step deletes
navigation_throttle_util.
> 
> 3. Move activation decision logic to the SB throttle, nuking a bunch of code
in
> the driver factory. We can store a mapping of navigation handles to SB
throttles
> to implement a "GetActivationDecisionForNavigation" method on the client, used
> by metrics code.
> 
> [1]: https://codereview.chromium.org/2841933003/

Oh, at (2) we can also make the client a factory for all the throttles.

[commit-bot: I haz the power, 2017-05-02 06:20:49]

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

[commit-bot: I haz the power, 2017-05-02 06:20:49]

Dry run: This issue passed the CQ dry run.

[engedy, 2017-05-02 12:24:20]

This CL and long-term plan LGTM.

https://codereview.chromium.org/2850373002/diff/1/chrome/browser/subresource_...
File chrome/browser/subresource_filter/subresource_filter_unittest.cc (right):

https://codereview.chromium.org/2850373002/diff/1/chrome/browser/subresource_...
chrome/browser/subresource_filter/subresource_filter_unittest.cc:27: #include
"components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h"
nit: Is this still needed?

https://codereview.chromium.org/2850373002/diff/1/components/subresource_filt...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h
(right):

https://codereview.chromium.org/2850373002/diff/1/components/subresource_filt...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:45:
: public base::SupportsUserData::Data,
nit: If you are changing this, could you also spruce this class up so that it
derives from content::WebContentsUserData and uses
DEFINE_WEB_CONTENTS_USER_DATA_KEY?

[Charlie Harrison, 2017-05-02 16:01:01]

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

[commit-bot: I haz the power, 2017-05-02 16:01:24]

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-02 16:01:57]

Thanks Balazs.
bmcquade: PTAL?

https://codereview.chromium.org/2850373002/diff/1/chrome/browser/subresource_...
File chrome/browser/subresource_filter/subresource_filter_unittest.cc (right):

https://codereview.chromium.org/2850373002/diff/1/chrome/browser/subresource_...
chrome/browser/subresource_filter/subresource_filter_unittest.cc:27: #include
"components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h"
On 2017/05/02 12:24:20, engedy wrote:
> nit: Is this still needed?

Nope, Done.

https://codereview.chromium.org/2850373002/diff/1/components/subresource_filt...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h
(right):

https://codereview.chromium.org/2850373002/diff/1/components/subresource_filt...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:45:
: public base::SupportsUserData::Data,
On 2017/05/02 12:24:20, engedy wrote:
> nit: If you are changing this, could you also spruce this class up so that it
> derives from content::WebContentsUserData and uses
> DEFINE_WEB_CONTENTS_USER_DATA_KEY?

Done.

[commit-bot: I haz the power, 2017-05-02 16:07:51]

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

[commit-bot: I haz the power, 2017-05-02 16:07:52]

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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  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_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  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_chromeos_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_...)

[Charlie Harrison, 2017-05-02 16:13:51]

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

[commit-bot: I haz the power, 2017-05-02 16:14: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-02 16:46:59]

Description was changed from

==========
Make ChromeSubresourceFilterClient a WebContentsUserData

This also modifies the ownership of the subresource filter client, which now
must
outlive the throttle manager / driver factory.
==========

to

==========
Make ChromeSubresourceFilterClient a WebContentsUserData

This also modifies the ownership of the subresource filter client, which now
must
outlive the throttle manager / driver factory.

BUG=717590
==========

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

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

[commit-bot: I haz the power, 2017-05-02 17:02:12]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-05-02 20:51:59]

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

[Charlie Harrison, 2017-05-02 20:52:00]

avi: PTAL at tab_helpers?

[Avi (use Gerrit), 2017-05-03 00:05:45]

lgtm

[Bryan McQuade, 2017-05-04 13:45:52]

lgtm

[Charlie Harrison, 2017-05-04 13:48:04]

Thanks!

[Charlie Harrison, 2017-05-04 13:48:12]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-05-04 13:48:13]

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

[commit-bot: I haz the power, 2017-05-04 13:48:23]

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-04 13:51:38]

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

[commit-bot: I haz the power, 2017-05-04 13:51:39]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2017-05-04 14:04:18]

The CQ bit was checked by csharrison@chromium.org

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

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

[commit-bot: I haz the power, 2017-05-04 14:04:33]

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-04 14:52:51]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493906658698970,
"parent_rev": "d54208b027fb61e6123a978e4e6145250fc18964", "commit_rev":
"f8f2850cf1004785650d0ab597c4b517aa8204a2"}

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

Description was changed from

==========
Make ChromeSubresourceFilterClient a WebContentsUserData

This also modifies the ownership of the subresource filter client, which now
must
outlive the throttle manager / driver factory.

BUG=717590
==========

to

==========
Make ChromeSubresourceFilterClient a WebContentsUserData

This also modifies the ownership of the subresource filter client, which now
must
outlive the throttle manager / driver factory.

BUG=717590

Review-Url: https://codereview.chromium.org/2850373002
Cr-Commit-Position: refs/heads/master@{#469331}
Committed:
https://chromium.googlesource.com/chromium/src/+/f8f2850cf1004785650d0ab597c4...
==========

[commit-bot: I haz the power, 2017-05-04 14:54:00]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f8f2850cf1004785650d0ab597c4...

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

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f8f2850cf1004785650d0ab597c4...