Supervised user SafeSites: Add a field trial to turn on/off

Supervised user SafeSites: Add a field trial to turn on/off.

Also combine the enable/disable flags for blacklist and online check into a single multi-value flag.

BUG=417722
Committed: https://crrev.com/e2082a0e325cd4d19b9608cc699ee81a347537bf
Cr-Commit-Position: refs/heads/master@{#324208}

[Marc Treib, 2015-03-31 09:29:57]

treib@chromium.org changed reviewers:
+ bauerb@chromium.org

[Marc Treib, 2015-03-31 09:29:58]

PTAL!
This is mostly so we can turn this off once the dogfood/trusted tester
population outgrows the prototype.

[Bernhard Bauer, 2015-04-07 11:13:50]

https://codereview.chromium.org/698253002/diff/20001/chrome/browser/supervise...
File
chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc
(right):

https://codereview.chromium.org/698253002/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc:33:
// take precedence over enable args. Default to false if no arg is present.
It's a bit strange that specifying a command line flag for the static blacklist
has an effect on the online check. Is that really what we want? Would it maybe
make sense to use a single command line flag that can take either of the
combinations as a value? (We could directly use it to override the field trial,
for code simplicity).

https://codereview.chromium.org/698253002/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc:35:
*safesites = is_safesites && !no_safesites;
Return in this case?

https://codereview.chromium.org/698253002/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc:38:
if (!is_blacklist && !no_blacklist && !is_safesites && !no_safesites) {
This condition is already implied, no?

https://codereview.chromium.org/698253002/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc:60:
GetState(&blacklist, &safesites);
It seems a bit awkward to have one of the out parameters of this method always
ignored... We could store the flags in global variables and only set them once
(via a third flag that states whether we have calculated the flag values).

[Marc Treib, 2015-04-07 12:56:19]

https://codereview.chromium.org/698253002/diff/20001/chrome/browser/supervise...
File
chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc
(right):

https://codereview.chromium.org/698253002/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc:33:
// take precedence over enable args. Default to false if no arg is present.
On 2015/04/07 11:13:49, Bernhard Bauer wrote:
> It's a bit strange that specifying a command line flag for the static
blacklist
> has an effect on the online check. Is that really what we want? Would it maybe
> make sense to use a single command line flag that can take either of the
> combinations as a value? (We could directly use it to override the field
trial,
> for code simplicity).

Yeah, that makes sense. I've replaced the various flags with a single
multi-value flag.

https://codereview.chromium.org/698253002/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc:35:
*safesites = is_safesites && !no_safesites;
On 2015/04/07 11:13:49, Bernhard Bauer wrote:
> Return in this case?

Done.

https://codereview.chromium.org/698253002/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc:38:
if (!is_blacklist && !no_blacklist && !is_safesites && !no_safesites) {
On 2015/04/07 11:13:49, Bernhard Bauer wrote:
> This condition is already implied, no?

Indeed. Fixed, thanks!

https://codereview.chromium.org/698253002/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc:60:
GetState(&blacklist, &safesites);
On 2015/04/07 11:13:49, Bernhard Bauer wrote:
> It seems a bit awkward to have one of the out parameters of this method always
> ignored... We could store the flags in global variables and only set them once
> (via a third flag that states whether we have calculated the flag values).

Hm. Introducing global variables feels even more awkward to me. It's also
dangerous, since the field trial state can change at runtime.
I've created an enum of the possible states instead, so I can just return it
normally.

[Bernhard Bauer, 2015-04-07 13:34:03]

LGTM

https://codereview.chromium.org/698253002/diff/20001/chrome/browser/supervise...
File
chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc
(right):

https://codereview.chromium.org/698253002/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc:60:
GetState(&blacklist, &safesites);
On 2015/04/07 12:56:19, Marc Treib wrote:
> On 2015/04/07 11:13:49, Bernhard Bauer wrote:
> > It seems a bit awkward to have one of the out parameters of this method
always
> > ignored... We could store the flags in global variables and only set them
once
> > (via a third flag that states whether we have calculated the flag values).
> 
> Hm. Introducing global variables feels even more awkward to me. It's also
> dangerous, since the field trial state can change at runtime.

Really? I thought field trials are fixed, which is why we require a restart for
new values to take place? (Hence the unsuitability of field trials for immediate
kill switches)

> I've created an enum of the possible states instead, so I can just return it
> normally.

[Marc Treib, 2015-04-07 13:45:00]

https://codereview.chromium.org/698253002/diff/20001/chrome/browser/supervise...
File
chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc
(right):

https://codereview.chromium.org/698253002/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc:60:
GetState(&blacklist, &safesites);
On 2015/04/07 13:34:03, Bernhard Bauer wrote:
> On 2015/04/07 12:56:19, Marc Treib wrote:
> > On 2015/04/07 11:13:49, Bernhard Bauer wrote:
> > > It seems a bit awkward to have one of the out parameters of this method
> always
> > > ignored... We could store the flags in global variables and only set them
> once
> > > (via a third flag that states whether we have calculated the flag values).
> > 
> > Hm. Introducing global variables feels even more awkward to me. It's also
> > dangerous, since the field trial state can change at runtime.
> 
> Really? I thought field trials are fixed, which is why we require a restart
for
> new values to take place? (Hence the unsuitability of field trials for
immediate
> kill switches)

Hm, at least the initial state is fetched asynchronously. But once fetched,
maybe it's only applied on the next restart; I haven't checked. Either way,
doesn't really matter now :)

> > I've created an enum of the possible states instead, so I can just return it
> > normally.
>

[Marc Treib, 2015-04-07 13:45:50]

treib@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Marc Treib, 2015-04-07 13:45:50]

+asvitkine for histograms.xml OWNERS approval.

[Alexei Svitkine (slow), 2015-04-07 18:43:14]

lgtm

[Bernhard Bauer, 2015-04-07 21:27:37]

https://codereview.chromium.org/698253002/diff/20001/chrome/browser/supervise...
File
chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc
(right):

https://codereview.chromium.org/698253002/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc:60:
GetState(&blacklist, &safesites);
On 2015/04/07 13:45:00, Marc Treib wrote:
> On 2015/04/07 13:34:03, Bernhard Bauer wrote:
> > On 2015/04/07 12:56:19, Marc Treib wrote:
> > > On 2015/04/07 11:13:49, Bernhard Bauer wrote:
> > > > It seems a bit awkward to have one of the out parameters of this method
> > always
> > > > ignored... We could store the flags in global variables and only set
them
> > once
> > > > (via a third flag that states whether we have calculated the flag
values).
> > > 
> > > Hm. Introducing global variables feels even more awkward to me. It's also
> > > dangerous, since the field trial state can change at runtime.
> > 
> > Really? I thought field trials are fixed, which is why we require a restart
> for
> > new values to take place? (Hence the unsuitability of field trials for
> immediate
> > kill switches)
> 
> Hm, at least the initial state is fetched asynchronously. But once fetched,
> maybe it's only applied on the next restart; I haven't checked. Either way,
> doesn't really matter now :)
> 
> > > I've created an enum of the possible states instead, so I can just return
it
> > > normally.
> > 
> 

Yeah, I think the state is always fetched asynchronously (it has to, because the
alternative would be to block), but FieldTrialList will always return the same
value during a single browser run. That's why the first Chrome run doesn't get
Finch experiments.

But yes, don't let that stop you from moving forward with this CL :)

[Marc Treib, 2015-04-08 08:39:08]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-04-08 08:39:09]

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

[commit-bot: I haz the power, 2015-04-08 08:39:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/698253002/60001

[commit-bot: I haz the power, 2015-04-08 10:15:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-08 10:16:33]

Patchset 4 (id:??) landed as
https://crrev.com/e2082a0e325cd4d19b9608cc699ee81a347537bf
Cr-Commit-Position: refs/heads/master@{#324208}