Introduce subresource_filter::ConfigurationList and make querying it cheap.

Introduce subresource_filter::ConfigurationList and make querying it cheap.

For now, the list will still have exactly one Configuration, the one that is
parsed from the field trial configuration.

Therefore the benefits of this change for now is that tests, which change the
configuration after their WebContents is created, will no longer need to inject
the new configuration manually into the classes under test, because those
classes can now directly work with the authoritative global state.

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

[engedy, 2017-04-21 16:27:49]

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

[commit-bot: I haz the power, 2017-04-21 16:28:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2017-04-21 16:30:26]

Description was changed from

==========
Introduce subresource_filter::ConfigurationList and cache it.

The benefits of this change are:

 -- Tests that change the configuration after the creation of their WebContents
 will no longer need to inject the new configuration manually into the
 ContentSubresourceFilterDriverFactory.

 -- It will be possible to override the subresource filtering configuration
 (including running multiple simultaneous configurations) in tests in a cleaner
 way, without having to tamper with field trial and/or variation parameter
 state, which is problematic in browser tests.

For now, the list will still always have exactly one Configuration that is
parsed from the field trial configuration.

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

to

==========
Introduce subresource_filter::ConfigurationList and make querying it cheap.

For now, the list will still always have exactly one Configuration that is
parsed from the field trial configuration.

Therefore the benefits of this change for now is that tests that change the
configuration after the creation of their WebContents will no longer need to
inject the new configuration manually into the classes under test, as those
classes always query the authoritative global state (which is no cheap).

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

[engedy, 2017-04-21 16:30:26]

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

[engedy, 2017-04-21 16:31:24]

Description was changed from

==========
Introduce subresource_filter::ConfigurationList and make querying it cheap.

For now, the list will still always have exactly one Configuration that is
parsed from the field trial configuration.

Therefore the benefits of this change for now is that tests that change the
configuration after the creation of their WebContents will no longer need to
inject the new configuration manually into the classes under test, as those
classes always query the authoritative global state (which is no cheap).

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

to

==========
Introduce subresource_filter::ConfigurationList and make querying it cheap.

For now, the list will still always have exactly one Configuration that is
parsed from the field trial configuration.

Therefore the benefits of this change for now is that tests that change the
configuration after the creation of their WebContents will no longer need to
inject the new configuration manually into the classes under test, as those
classes always query the authoritative global state (which is no cheap).

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

[engedy, 2017-04-21 16:41:58]

Description was changed from

==========
Introduce subresource_filter::ConfigurationList and make querying it cheap.

For now, the list will still always have exactly one Configuration that is
parsed from the field trial configuration.

Therefore the benefits of this change for now is that tests that change the
configuration after the creation of their WebContents will no longer need to
inject the new configuration manually into the classes under test, as those
classes always query the authoritative global state (which is no cheap).

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

to

==========
Introduce subresource_filter::ConfigurationList and make querying it cheap.

For now, the list will still have exactly one Configuration, the one that is
parsed from the field trial configuration.

Therefore the benefits of this change for now is that tests, which change the
configuration after their WebContents is created, will no longer need to inject
the new configuration manually into the classes under test, because those
classes can now directly work with the authoritative global state.

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

[engedy, 2017-04-21 16:42:35]

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

[commit-bot: I haz the power, 2017-04-21 16:42:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2017-04-21 16:42:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2017-04-21 16:43:16]

Charlie, please take a look.

[Charlie Harrison, 2017-04-21 17:37:27]

LGTM % comments.

https://codereview.chromium.org/2831373002/diff/40001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.h
(right):

https://codereview.chromium.org/2831373002/diff/40001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:75: //
Retrieves all currently enabled subresource filtering configurations. Cheap
Hm, can we elide "Cheap to call"? To me, cheap to call means we can do it in a
loop or something. Since we're using a lock I would instead say "pre-parsed" or
something along those longs.

https://codereview.chromium.org/2831373002/diff/40001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:78:
scoped_refptr<ConfigurationList> GetActiveConfigurations();
Could this just be a static member on ConfigurationList e.g.
ConfigurationList::GetActive()?

https://codereview.chromium.org/2831373002/diff/40001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:84:
void ClearCachedActiveConfigurations();
ditto, can append ForTesting instead of using the namespace.

[commit-bot: I haz the power, 2017-04-21 17:40:41]

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

[commit-bot: I haz the power, 2017-04-21 17:40:41]

Dry run: This issue passed the CQ dry run.

[engedy, 2017-04-24 12:26:25]

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

[commit-bot: I haz the power, 2017-04-24 12:26:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2017-04-24 14:35:35]

Please take another look.

https://codereview.chromium.org/2831373002/diff/40001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.h
(right):

https://codereview.chromium.org/2831373002/diff/40001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:75: //
Retrieves all currently enabled subresource filtering configurations. Cheap
On 2017/04/21 17:37:27, Charlie Harrison wrote:
> Hm, can we elide "Cheap to call"? To me, cheap to call means we can do it in a
> loop or something. Since we're using a lock I would instead say "pre-parsed"
or
> something along those longs.

Done.

https://codereview.chromium.org/2831373002/diff/40001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:78:
scoped_refptr<ConfigurationList> GetActiveConfigurations();
On 2017/04/21 17:37:27, Charlie Harrison wrote:
> Could this just be a static member on ConfigurationList e.g.
> ConfigurationList::GetActive()?

Do you feel strongly about this? I don't much like classes that mix singleton
and non-singleton traits in such a way.

https://codereview.chromium.org/2831373002/diff/40001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:84:
void ClearCachedActiveConfigurations();
On 2017/04/21 17:37:26, Charlie Harrison wrote:
> ditto, can append ForTesting instead of using the namespace.

Ditto. :)

[engedy, 2017-04-24 14:36:43]

engedy@chromium.org changed reviewers:
+ nparker@chromium.org, sorin@chromium.org

[engedy, 2017-04-24 14:38:04]

Dear Owners, please review mechanical changes:

 @Sorin:  c/b/component_updater,
 @Nathan: c/b/safe_browsing.

Thank you!

[Charlie Harrison, 2017-04-24 14:40:59]

still LGTM

https://codereview.chromium.org/2831373002/diff/40001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.h
(right):

https://codereview.chromium.org/2831373002/diff/40001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:78:
scoped_refptr<ConfigurationList> GetActiveConfigurations();
On 2017/04/24 14:35:35, engedy wrote:
> On 2017/04/21 17:37:27, Charlie Harrison wrote:
> > Could this just be a static member on ConfigurationList e.g.
> > ConfigurationList::GetActive()?
> 
> Do you feel strongly about this? I don't much like classes that mix singleton
> and non-singleton traits in such a way.

Nope, consider me pushed back!

[engedy, 2017-04-24 14:43:26]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2017-04-24 14:43:54]

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 14:50:58]

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

[commit-bot: I haz the power, 2017-04-24 14:51:00]

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

[Sorin Jianu, 2017-04-24 21:58:59]

lgtm

Thank you! component updater lgtm

https://codereview.chromium.org/2831373002/diff/80001/chrome/browser/componen...
File chrome/browser/component_updater/subresource_filter_component_installer.cc
(right):

https://codereview.chromium.org/2831373002/diff/80001/chrome/browser/componen...
chrome/browser/component_updater/subresource_filter_component_installer.cc:115:
std::string ruleset_flavor = subresource_filter::GetActiveConfigurations()
can this be declared const?

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

https://codereview.chromium.org/2831373002/diff/80001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:118:
auto configurations = GetActiveConfigurations();
const?

Also, please review the similar call sites below.

https://codereview.chromium.org/2831373002/diff/80001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.h
(right):

https://codereview.chromium.org/2831373002/diff/80001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:62:
ConfigurationList(Configuration config);
explicit?

https://codereview.chromium.org/2831373002/diff/80001/components/subresource_...
File
components/subresource_filter/core/browser/subresource_filter_features_unittest.cc
(right):

https://codereview.chromium.org/2831373002/diff/80001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:45:
auto active_configurations = GetActiveConfigurations();
can this be const, here and below?

[Nathan Parker, 2017-04-24 22:36:58]

safe_browsing/* LGTM

[Charlie Harrison, 2017-04-25 01:06:26]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2017-04-25 01:07:08]

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-25 01:29:46]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493082386693690,
"parent_rev": "03f5058e764e8b2b01d571f29426075a83f6049b", "commit_rev":
"f1269b6b08358849f9fed290de619122e34135b1"}

[commit-bot: I haz the power, 2017-04-25 01:29:57]

Description was changed from

==========
Introduce subresource_filter::ConfigurationList and make querying it cheap.

For now, the list will still have exactly one Configuration, the one that is
parsed from the field trial configuration.

Therefore the benefits of this change for now is that tests, which change the
configuration after their WebContents is created, will no longer need to inject
the new configuration manually into the classes under test, because those
classes can now directly work with the authoritative global state.

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

to

==========
Introduce subresource_filter::ConfigurationList and make querying it cheap.

For now, the list will still have exactly one Configuration, the one that is
parsed from the field trial configuration.

Therefore the benefits of this change for now is that tests, which change the
configuration after their WebContents is created, will no longer need to inject
the new configuration manually into the classes under test, because those
classes can now directly work with the authoritative global state.

BUG=708181

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

[commit-bot: I haz the power, 2017-04-25 01:29:58]

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

[engedy, 2017-04-25 08:34:24]

Thanks, Sorin, and apologies for the many nits.

Unfortunately we were a bit trigger-happy with sending this to the CQ, but I
have addressed your comments in a follow-up CL:
https://codereview.chromium.org/2837163004/.

https://codereview.chromium.org/2831373002/diff/80001/chrome/browser/componen...
File chrome/browser/component_updater/subresource_filter_component_installer.cc
(right):

https://codereview.chromium.org/2831373002/diff/80001/chrome/browser/componen...
chrome/browser/component_updater/subresource_filter_component_installer.cc:115:
std::string ruleset_flavor = subresource_filter::GetActiveConfigurations()
On 2017/04/24 21:58:59, Sorin Jianu wrote:
> can this be declared const?

Yep, done.

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

https://codereview.chromium.org/2831373002/diff/80001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:118:
auto configurations = GetActiveConfigurations();
On 2017/04/24 21:58:59, Sorin Jianu wrote:
> const?
> 
> Also, please review the similar call sites below.

Done.

https://codereview.chromium.org/2831373002/diff/80001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.h
(right):

https://codereview.chromium.org/2831373002/diff/80001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:62:
ConfigurationList(Configuration config);
On 2017/04/24 21:58:59, Sorin Jianu wrote:
> explicit?

Done.

https://codereview.chromium.org/2831373002/diff/80001/components/subresource_...
File
components/subresource_filter/core/browser/subresource_filter_features_unittest.cc
(right):

https://codereview.chromium.org/2831373002/diff/80001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:45:
auto active_configurations = GetActiveConfigurations();
On 2017/04/24 21:58:59, Sorin Jianu wrote:
> can this be const, here and below?

Yep, done.

[Charlie Harrison, 2017-04-25 11:56:56]

On 2017/04/25 08:34:24, engedy wrote:
> Thanks, Sorin, and apologies for the many nits.
> 
> Unfortunately we were a bit trigger-happy with sending this to the CQ, but I
> have addressed your comments in a follow-up CL:
> https://codereview.chromium.org/2837163004/.
> 
>
https://codereview.chromium.org/2831373002/diff/80001/chrome/browser/componen...
> File
chrome/browser/component_updater/subresource_filter_component_installer.cc
> (right):
> 
>
https://codereview.chromium.org/2831373002/diff/80001/chrome/browser/componen...
>
chrome/browser/component_updater/subresource_filter_component_installer.cc:115:
> std::string ruleset_flavor = subresource_filter::GetActiveConfigurations()
> On 2017/04/24 21:58:59, Sorin Jianu wrote:
> > can this be declared const?
> 
> Yep, done.
> 
>
https://codereview.chromium.org/2831373002/diff/80001/components/subresource_...
> File
>
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
> (right):
> 
>
https://codereview.chromium.org/2831373002/diff/80001/components/subresource_...
>
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:118:
> auto configurations = GetActiveConfigurations();
> On 2017/04/24 21:58:59, Sorin Jianu wrote:
> > const?
> > 
> > Also, please review the similar call sites below.
> 
> Done.
> 
>
https://codereview.chromium.org/2831373002/diff/80001/components/subresource_...
> File components/subresource_filter/core/browser/subresource_filter_features.h
> (right):
> 
>
https://codereview.chromium.org/2831373002/diff/80001/components/subresource_...
> components/subresource_filter/core/browser/subresource_filter_features.h:62:
> ConfigurationList(Configuration config);
> On 2017/04/24 21:58:59, Sorin Jianu wrote:
> > explicit?
> 
> Done.
> 
>
https://codereview.chromium.org/2831373002/diff/80001/components/subresource_...
> File
>
components/subresource_filter/core/browser/subresource_filter_features_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2831373002/diff/80001/components/subresource_...
>
components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:45:
> auto active_configurations = GetActiveConfigurations();
> On 2017/04/24 21:58:59, Sorin Jianu wrote:
> > can this be const, here and below?
> 
> Yep, done.

Sorry! That was my fault. For some reason I thought those had been addressed.