Add flag to enable/disable affiliaton based matching.

Add flag to enable/disable affiliaton based matching.

BUG=437865
Committed: https://crrev.com/a4b285eb8f69af5c9409ffe152abbfebdca276e5
Cr-Commit-Position: refs/heads/master@{#314545}

[engedy, 2015-02-02 19:56:42]

engedy@chromium.org changed reviewers:
+ asvitkine@chromium.org, gcasto@chromium.org

[engedy, 2015-02-02 20:02:15]

@Garrett, please take a look. If you prefer, I can hold off on landing this
unless we have an actual implementation toggled by this (currently, the flag
will do nothing).

@Alexei, could you please review affiliation_utils.cc, and one particular
question:

If we specify a "forcing_flag" in the study config, is accessing variation
params enough to get the experiment group forced for UMA purposes, or do I need
to query the current field trial group explicitly?

[Garrett Casto, 2015-02-03 05:32:41]

Cross timezone LGTM

https://codereview.chromium.org/898553002/diff/1/components/password_manager/...
File components/password_manager/core/browser/affiliation_utils.cc (right):

https://codereview.chromium.org/898553002/diff/1/components/password_manager/...
components/password_manager/core/browser/affiliation_utils.cc:294: "enabled") ==
"true";
Alexei can talk more about how appropriate it is to use this function instead of
base::FieldTrial::FindFullName(), but I think that if you want to use forcing
flags, this call needs to go above the return values from the command line flags
(go/finch-and-flags).

[Alexei Svitkine (slow), 2015-02-03 17:27:41]

https://codereview.chromium.org/898553002/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/898553002/diff/1/chrome/browser/about_flags.c...
chrome/browser/about_flags.cc:1054: "enable-affiliation-based-matching",
I think you may need to add an entry to histograms.xml for this flag.

https://codereview.chromium.org/898553002/diff/1/components/password_manager/...
File components/password_manager/core/browser/affiliation_utils.cc (right):

https://codereview.chromium.org/898553002/diff/1/components/password_manager/...
components/password_manager/core/browser/affiliation_utils.cc:294: "enabled") ==
"true";
On 2015/02/03 05:32:41, Garrett Casto wrote:
> Alexei can talk more about how appropriate it is to use this function instead
of
> base::FieldTrial::FindFullName(), but I think that if you want to use forcing
> flags, this call needs to go above the return values from the command line
flags
> (go/finch-and-flags).

Indeed, it's needed to activate the trial.

Using params like this provides more flexibility. However, can you confirm that
this is only used from the browser process? If it's used from subprocesses, then
variation params will not work.

[engedy, 2015-02-03 18:21:07]

Patchset #2 (id:20001) has been deleted

[engedy, 2015-02-03 18:21:11]

Patchset #2 (id:40001) has been deleted

[engedy, 2015-02-03 18:24:27]

@Alexei, please take another look.

https://codereview.chromium.org/898553002/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/898553002/diff/1/chrome/browser/about_flags.c...
chrome/browser/about_flags.cc:1054: "enable-affiliation-based-matching",
On 2015/02/03 17:27:41, asvitkine (slow this week) wrote:
> I think you may need to add an entry to histograms.xml for this flag.

Ah, thanks, done.

https://codereview.chromium.org/898553002/diff/1/components/password_manager/...
File components/password_manager/core/browser/affiliation_utils.cc (right):

https://codereview.chromium.org/898553002/diff/1/components/password_manager/...
components/password_manager/core/browser/affiliation_utils.cc:294: "enabled") ==
"true";
On 2015/02/03 17:27:41, asvitkine (slow this week) wrote:
> On 2015/02/03 05:32:41, Garrett Casto wrote:
> > Alexei can talk more about how appropriate it is to use this function
instead
> of
> > base::FieldTrial::FindFullName(), but I think that if you want to use
forcing
> > flags, this call needs to go above the return values from the command line
> flags
> > (go/finch-and-flags).
> 
> Indeed, it's needed to activate the trial.
> 
> Using params like this provides more flexibility. However, can you confirm
that
> this is only used from the browser process? If it's used from subprocesses,
then
> variation params will not work.

Thanks. Then I might as well just use field trials, because this will be used
solely as staged roll-out mechanism and kill switch.

I will not be accessing the experiment from subprocesses.

[Alexei Svitkine (slow), 2015-02-03 19:05:22]

lgtm

[engedy, 2015-02-04 09:13:10]

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

[engedy, 2015-02-04 09:13:10]

@Vaclav, as discussed, could you please take a look at the strings?

[vabr (Chromium), 2015-02-04 09:31:23]

LGTM with off-line comments :).

Cheers,
Vaclav

[engedy, 2015-02-04 09:41:17]

New patchsets have been uploaded after l-g-t-m from vabr@chromium.org

[engedy, 2015-02-04 09:46:01]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2015-02-04 09:46:40]

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

[commit-bot: I haz the power, 2015-02-04 09:49:35]

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

[commit-bot: I haz the power, 2015-02-04 09:49:36]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[engedy, 2015-02-04 10:26:19]

Patchset #6 (id:140001) has been deleted

[engedy, 2015-02-04 10:56:37]

Patchset #6 (id:160001) has been deleted

[engedy, 2015-02-04 11:00:00]

Vaclav, I have updated the strings as per our offline discussion. Please take a
final look.

[vabr (Chromium), 2015-02-04 11:04:06]

English text in the CL LGTM!
Cheers,
Vaclav

https://codereview.chromium.org/898553002/diff/180001/components/password_man...
File components/password_manager/core/browser/affiliation_utils.cc (right):

https://codereview.chromium.org/898553002/diff/180001/components/password_man...
components/password_manager/core/browser/affiliation_utils.cc:292: // Note: It
is important to always query the field trial state, to ensure that
This is much clearer now!

[engedy, 2015-02-04 11:30:12]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2015-02-04 11:30:50]

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

[commit-bot: I haz the power, 2015-02-04 12:30:53]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2015-02-04 12:31:55]

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