Add SubresourceFilterProfileContext for all profile scoped context

Add SubresourceFilterProfileContext for all profile scoped context

This gives us an object to hang profile-scoped members, which will be
important if we ever need a broader scope for making policy decisions
than a WebContents-scope.

This CL changes no functionality.

BUG=689992
Review-Url: https://codereview.chromium.org/2829313002
Cr-Commit-Position: refs/heads/master@{#466655}
Committed: https://chromium.googlesource.com/chromium/src/+/97dde4e681d09822423e59c6a9dd7c5ece27283b

[Charlie Harrison, 2017-04-21 20:28:34]

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

[commit-bot: I haz the power, 2017-04-21 20:28: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-04-21 20:29:52]

Description was changed from

==========
Add SubresourceFilterProfileContext for all profile scoped context

BUG=689992
==========

to

==========
Add SubresourceFilterProfileContext for all profile scoped context

This gives us an object to hang profile-scoped members, which will be
important if we ever need a broader scope for making policy decisions
than a WebContents-scope.

This CL changes no functionality.

BUG=689992
==========

[Charlie Harrison, 2017-04-21 20:32:01]

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

[Charlie Harrison, 2017-04-21 20:32:02]

engedy: WDYT? I thought about doing this before but now I feel we might need it
with the new UI (will share a design soon). This will allow us to answer
questions like "is site X activated in any WebContents hosted by the Profile".

[commit-bot: I haz the power, 2017-04-21 20:33:07]

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

[commit-bot: I haz the power, 2017-04-21 20:33:07]

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_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  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_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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-04-21 20:41:11]

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

[commit-bot: I haz the power, 2017-04-21 20:41:33]

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-04-21 22:11:35]

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

[commit-bot: I haz the power, 2017-04-21 22:11:35]

Dry run: This issue passed the CQ dry run.

[engedy, 2017-04-24 10:25:42]

LGTM % nits, thanks!

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

https://codereview.chromium.org/2829313002/diff/20001/chrome/browser/subresou...
chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:26:
SubresourceFilterProfileContextFactory::EnsureForProfile(
nit: Could you explain to me why we are not relying on overriding
ServiceIsCreatedWithBrowserContext() instead here?

https://codereview.chromium.org/2829313002/diff/20001/chrome/browser/subresou...
File
chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc
(right):

https://codereview.chromium.org/2829313002/diff/20001/chrome/browser/subresou...
chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc:10:
#include "chrome/browser/subresource_filter/chrome_subresource_filter_client.h"
nit: Is this still needed?

https://codereview.chromium.org/2829313002/diff/20001/chrome/browser/subresou...
File chrome/browser/subresource_filter/subresource_filter_profile_context.cc
(right):

https://codereview.chromium.org/2829313002/diff/20001/chrome/browser/subresou...
chrome/browser/subresource_filter/subresource_filter_profile_context.cc:8:
#include "chrome/browser/profiles/profile.h"
nit: I think this is not actually used.

[Charlie Harrison, 2017-04-24 13:41:15]

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

[commit-bot: I haz the power, 2017-04-24 13:41:39]

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-04-24 13:41:58]

Responded to most of the comments, LMK what you think.

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

https://codereview.chromium.org/2829313002/diff/20001/chrome/browser/subresou...
chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:26:
SubresourceFilterProfileContextFactory::EnsureForProfile(
On 2017/04/24 10:25:41, engedy wrote:
> nit: Could you explain to me why we are not relying on overriding
> ServiceIsCreatedWithBrowserContext() instead here?

The major reasons are
1. It is more cumbersome to unit test by relying on
ServiceIsCreatedWithBrowserContext, which requires more than just a
TestingProfile afaict.

2. Keeping this class lazily instantiated means we could omit it if we're gating
this feature off. Not currently done today though.

https://codereview.chromium.org/2829313002/diff/20001/chrome/browser/subresou...
File
chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc
(right):

https://codereview.chromium.org/2829313002/diff/20001/chrome/browser/subresou...
chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc:10:
#include "chrome/browser/subresource_filter/chrome_subresource_filter_client.h"
On 2017/04/24 10:25:41, engedy wrote:
> nit: Is this still needed?

Yes, for histogram buckets.

https://codereview.chromium.org/2829313002/diff/20001/chrome/browser/subresou...
File chrome/browser/subresource_filter/subresource_filter_profile_context.cc
(right):

https://codereview.chromium.org/2829313002/diff/20001/chrome/browser/subresou...
chrome/browser/subresource_filter/subresource_filter_profile_context.cc:8:
#include "chrome/browser/profiles/profile.h"
On 2017/04/24 10:25:42, engedy wrote:
> nit: I think this is not actually used.

Removed.

[engedy, 2017-04-24 13:44:01]

Still LGTM, thanks!

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

https://codereview.chromium.org/2829313002/diff/20001/chrome/browser/subresou...
chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:26:
SubresourceFilterProfileContextFactory::EnsureForProfile(
On 2017/04/24 13:41:58, Charlie Harrison wrote:
> On 2017/04/24 10:25:41, engedy wrote:
> > nit: Could you explain to me why we are not relying on overriding
> > ServiceIsCreatedWithBrowserContext() instead here?
> 
> The major reasons are
> 1. It is more cumbersome to unit test by relying on
> ServiceIsCreatedWithBrowserContext, which requires more than just a
> TestingProfile afaict.
> 
> 2. Keeping this class lazily instantiated means we could omit it if we're
gating
> this feature off. Not currently done today though.

Acknowledged.

https://codereview.chromium.org/2829313002/diff/20001/chrome/browser/subresou...
File
chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc
(right):

https://codereview.chromium.org/2829313002/diff/20001/chrome/browser/subresou...
chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc:10:
#include "chrome/browser/subresource_filter/chrome_subresource_filter_client.h"
On 2017/04/24 13:41:58, Charlie Harrison wrote:
> On 2017/04/24 10:25:41, engedy wrote:
> > nit: Is this still needed?
> 
> Yes, for histogram buckets.

Acknowledged.

[Charlie Harrison, 2017-04-24 13:49:55]

The CQ bit was unchecked by csharrison@chromium.org

[Charlie Harrison, 2017-04-24 13:49:56]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2017-04-24 13:50:25]

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-04-24 16:35:53]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493041796036950,
"parent_rev": "3b0deba895e763195d439c7fcd4d5f947509904d", "commit_rev":
"97dde4e681d09822423e59c6a9dd7c5ece27283b"}

[commit-bot: I haz the power, 2017-04-24 16:36:05]

Description was changed from

==========
Add SubresourceFilterProfileContext for all profile scoped context

This gives us an object to hang profile-scoped members, which will be
important if we ever need a broader scope for making policy decisions
than a WebContents-scope.

This CL changes no functionality.

BUG=689992
==========

to

==========
Add SubresourceFilterProfileContext for all profile scoped context

This gives us an object to hang profile-scoped members, which will be
important if we ever need a broader scope for making policy decisions
than a WebContents-scope.

This CL changes no functionality.

BUG=689992

Review-Url: https://codereview.chromium.org/2829313002
Cr-Commit-Position: refs/heads/master@{#466655}
Committed:
https://chromium.googlesource.com/chromium/src/+/97dde4e681d09822423e59c6a9dd...
==========

[commit-bot: I haz the power, 2017-04-24 16:36:05]

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