Introduce subresource_filter::Configuration.

Introduce subresource_filter::Configuration.

Instead of exposing the effective parameters, which define how the subresource
filter should behave, piecemeal, introduce a single struct that encapsulates
all parameters of the feature.

This will allow making the current configuration (with activation on Safe
Browsing phishing sites) the hard-coded default, as well as adding support for
multiple active configurations at the same time.

BUG=609747
Review-Url: https://codereview.chromium.org/2798983002
Cr-Commit-Position: refs/heads/master@{#463190}
Committed: https://chromium.googlesource.com/chromium/src/+/4e926ec962c82ee6f61f1208b4bf18411d605a59

[engedy, 2017-04-05 16:33:56]

Description was changed from

==========
Introduce subresource_filter::Configuration.

Instead of exposing parameters (specified by the field trials) for the
subresource filter feature one by one, introduce a struct that encapulates all
parameters that define how the feature should operate. This is in preparation
to adding support for multiple simultaneously active configurations.

BUG=609747
==========

to

==========
Introduce subresource_filter::Configuration.

Instead of exposing parameters for the subresource filter feature one by one,
introduce a struct that encapulates all parameters that define how the feature
should operate. This is in preparation to adding support for multiple
simultaneously active configurations.

BUG=609747
==========

[engedy, 2017-04-05 16:33:56]

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

[engedy, 2017-04-05 16:34:04]

Description was changed from

==========
Introduce subresource_filter::Configuration.

Instead of exposing parameters for the subresource filter feature one by one,
introduce a struct that encapulates all parameters that define how the feature
should operate. This is in preparation to adding support for multiple
simultaneously active configurations.

BUG=609747
==========

to

==========
Introduce subresource_filter::Configuration.

Instead of exposing parameters for the subresource filter feature one by one,
introduce a struct that encapsulates all parameters that define how the feature
should operate. This is in preparation to adding support for multiple
simultaneously active configurations.

BUG=609747
==========

[engedy, 2017-04-05 16:48:26]

Description was changed from

==========
Introduce subresource_filter::Configuration.

Instead of exposing parameters for the subresource filter feature one by one,
introduce a struct that encapsulates all parameters that define how the feature
should operate. This is in preparation to adding support for multiple
simultaneously active configurations.

BUG=609747
==========

to

==========
Introduce subresource_filter::Configuration.

Instead of exposing parameters for the subresource filter feature one by one,
introduce a struct that encapsulates all parameters that define how the feature
should operate.

This will allow making the current configuration (with activation on Safe
Browsing phishing sites) the hard-coded default, as well as adding support for
multiple active configurations at the same time.

BUG=609747
==========

[engedy, 2017-04-05 16:49:24]

Description was changed from

==========
Introduce subresource_filter::Configuration.

Instead of exposing parameters for the subresource filter feature one by one,
introduce a struct that encapsulates all parameters that define how the feature
should operate.

This will allow making the current configuration (with activation on Safe
Browsing phishing sites) the hard-coded default, as well as adding support for
multiple active configurations at the same time.

BUG=609747
==========

to

==========
Introduce subresource_filter::Configuration.

Instead of exposing the effective parameters that define how the subresource
filter should behave one by one, introduce a single struct that encapsulates
all parameters of the feature.

This will allow making the current configuration (with activation on Safe
Browsing phishing sites) the hard-coded default, as well as adding support for
multiple active configurations at the same time.

BUG=609747
==========

[engedy, 2017-04-05 16:50:13]

Description was changed from

==========
Introduce subresource_filter::Configuration.

Instead of exposing the effective parameters that define how the subresource
filter should behave one by one, introduce a single struct that encapsulates
all parameters of the feature.

This will allow making the current configuration (with activation on Safe
Browsing phishing sites) the hard-coded default, as well as adding support for
multiple active configurations at the same time.

BUG=609747
==========

to

==========
Introduce subresource_filter::Configuration.

Instead of exposing the effective parameters, which define how the subresource
filter should behave, piecemeal, introduce a single struct that encapsulates
all parameters of the feature.

This will allow making the current configuration (with activation on Safe
Browsing phishing sites) the hard-coded default, as well as adding support for
multiple active configurations at the same time.

BUG=609747
==========

[engedy, 2017-04-05 16:53:24]

Patchset #1 (id:1) has been deleted

[engedy, 2017-04-05 17:11:10]

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

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

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-05 17:12:27]

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

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

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-05 17:13:34]

Charlie, please take a look.

There is a bit of a hack involved (temporarily), let me know if you'd prefer
changing that to base::Optional<> or if you see a better solution.

[engedy, 2017-04-05 17:13:35]

Charlie, please take a look.

There is a bit of a hack involved (temporarily), let me know if you'd prefer
changing that to base::Optional<> or if you see a better solution.

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

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

[commit-bot: I haz the power, 2017-04-05 18:08:15]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-04-05 20:43:04]

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

https://codereview.chromium.org/2798983002/diff/60001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:267:
configuration_ = GetActiveConfiguration();
Eek this scares me, especially if code accidentally gets added in the meantime
that uses configuration_ before DidStartNavigation.

What about keeping this as a unique_ptr and exposing a
set_configuration_for_testing()?  It means more code to cleanup, unfortunately.

I would also be okay with using base::Optional, so that we get DCHECks around
picking configuration_.value().

https://codereview.chromium.org/2798983002/diff/60001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.cc
(right):

https://codereview.chromium.org/2798983002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.cc:20:
base::StringPiece GetVariationParamOrEmpty(
A little odd to have this return a non-owning StringPiece. Can you document that
the underlying buffer is owned by the map?

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

https://codereview.chromium.org/2798983002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:26: //
The activation scope, that is, the subset of page loads where subresource
s/scope, that is/ scope. That is/

[engedy, 2017-04-07 08:26:23]

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

[commit-bot: I haz the power, 2017-04-07 08:26:44]

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-07 08:38:10]

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

[commit-bot: I haz the power, 2017-04-07 08:38:16]

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-07 08:40:07]

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

[engedy, 2017-04-07 08:40:12]

Patchset #4 (id:80001) has been deleted

[engedy, 2017-04-07 08:40:19]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-04-07 08:40: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-07 08:40:58]

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

https://codereview.chromium.org/2798983002/diff/60001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:267:
configuration_ = GetActiveConfiguration();
On 2017/04/05 20:43:04, Charlie Harrison wrote:
> Eek this scares me, especially if code accidentally gets added in the meantime
> that uses configuration_ before DidStartNavigation.
> 
> What about keeping this as a unique_ptr and exposing a
> set_configuration_for_testing()?  It means more code to cleanup,
unfortunately.
> 
> I would also be okay with using base::Optional, so that we get DCHECks around
> picking configuration_.value().

Much relieved. I was worried that it might not scare you and if I pick one of
these more verbose alternatives, I might get accused of over-engineering things.
:-)

How about this?

https://codereview.chromium.org/2798983002/diff/60001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.cc
(right):

https://codereview.chromium.org/2798983002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.cc:20:
base::StringPiece GetVariationParamOrEmpty(
On 2017/04/05 20:43:04, Charlie Harrison wrote:
> A little odd to have this return a non-owning StringPiece. Can you document
that
> the underlying buffer is owned by the map?

Indeed. I reworked this slightly, what do you think?

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

https://codereview.chromium.org/2798983002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:26: //
The activation scope, that is, the subset of page loads where subresource
On 2017/04/05 20:43:04, Charlie Harrison wrote:
> s/scope, that is/ scope. That is/

Done.

[commit-bot: I haz the power, 2017-04-07 09:09:34]

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

[commit-bot: I haz the power, 2017-04-07 09:09:35]

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

[pkalinnikov, 2017-04-07 09:22:05]

pkalinnikov@chromium.org changed reviewers:
+ pkalinnikov@chromium.org

[pkalinnikov, 2017-04-07 09:22:06]

Thanks for this clean up! I've put my 2 cents.

https://codereview.chromium.org/2798983002/diff/120001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2798983002/diff/120001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:42:
// TODO(pkalinnikov): Cache |rate| and other variation params in
nit: You can now remove this TODO because this CL essentially fulfills it.
nit: I suppose, now you can pass |rate| in by value.

[engedy, 2017-04-07 09:24:15]

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

[commit-bot: I haz the power, 2017-04-07 09:24:32]

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-07 09:25:40]

Patchset #5 (id:140001) has been deleted

[engedy, 2017-04-07 09:28:09]

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

[commit-bot: I haz the power, 2017-04-07 09:28:14]

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-07 09:28:29]

https://codereview.chromium.org/2798983002/diff/120001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2798983002/diff/120001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:42:
// TODO(pkalinnikov): Cache |rate| and other variation params in
On 2017/04/07 09:22:06, pkalinnikov wrote:
> nit: You can now remove this TODO because this CL essentially fulfills it.
> nit: I suppose, now you can pass |rate| in by value.

Done.

[engedy, 2017-04-07 09:29:54]

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

[engedy, 2017-04-07 09:31:33]

Dear Owners, could you please take a quick look at mechanical changes in:

@Nathan: c/b/safe_browsing/safe_browsing_service_browsertest.cc
@Sorin: c/b/component_updater/subresource_filter_component_installer.cc

[commit-bot: I haz the power, 2017-04-07 10:04:41]

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

[commit-bot: I haz the power, 2017-04-07 10:04:42]

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

[Charlie Harrison, 2017-04-07 12:22:18]

LGTM

[engedy, 2017-04-07 13:44:58]

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

[commit-bot: I haz the power, 2017-04-07 13:45:09]

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-07 13:46:03]

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

[commit-bot: I haz the power, 2017-04-07 13:46:04]

Dry run: Try jobs failed on following builders:
  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...)

[Sorin Jianu, 2017-04-07 16:07:55]

lgtm

Thank you!
component updater paths lgtm

[engedy, 2017-04-07 16:48:10]

The CQ bit was checked by engedy@chromium.org

[engedy, 2017-04-07 16:48:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2798983002/#ps200001
(title: "Rebase.")

[engedy, 2017-04-07 16:48:11]

The CQ bit was unchecked by engedy@chromium.org

[engedy, 2017-04-07 16:48:18]

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

[commit-bot: I haz the power, 2017-04-07 16:49:12]

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-07 17:36:26]

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

[commit-bot: I haz the power, 2017-04-07 17:36:27]

Dry run: This issue passed the CQ dry run.

[Nathan Parker, 2017-04-07 23:08:16]

lgtm!

[engedy, 2017-04-10 07:28:54]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2017-04-10 07:29:05]

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-10 08:15:12]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1491809334964960,
"parent_rev": "4b0c49ec412c02103e092593041c99cb69f17191", "commit_rev":
"4e926ec962c82ee6f61f1208b4bf18411d605a59"}

[commit-bot: I haz the power, 2017-04-10 08:15:56]

Description was changed from

==========
Introduce subresource_filter::Configuration.

Instead of exposing the effective parameters, which define how the subresource
filter should behave, piecemeal, introduce a single struct that encapsulates
all parameters of the feature.

This will allow making the current configuration (with activation on Safe
Browsing phishing sites) the hard-coded default, as well as adding support for
multiple active configurations at the same time.

BUG=609747
==========

to

==========
Introduce subresource_filter::Configuration.

Instead of exposing the effective parameters, which define how the subresource
filter should behave, piecemeal, introduce a single struct that encapsulates
all parameters of the feature.

This will allow making the current configuration (with activation on Safe
Browsing phishing sites) the hard-coded default, as well as adding support for
multiple active configurations at the same time.

BUG=609747

Review-Url: https://codereview.chromium.org/2798983002
Cr-Commit-Position: refs/heads/master@{#463190}
Committed:
https://chromium.googlesource.com/chromium/src/+/4e926ec962c82ee6f61f1208b4bf...
==========

[commit-bot: I haz the power, 2017-04-10 08:15:57]

Committed patchset #7 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/4e926ec962c82ee6f61f1208b4bf...