[Herb] Set up field trial for UI prototypes

[Herb] Set up field trial for UI prototypes

* Add a field trial for bucketing users into different prototypes.
  The trial stores the herb flavor in a variations param.

* Consolidate Herb string checking into FeatureUtilities, which
  checks whether VariationsAssociatedParams are active and
  whether they're overridden by about:flags.

* Adds a "disabled" group to about:flags to explicitly disable the
  experiment, users on about:flags now default to whatever they're
  bucketed into.

* Stop toasting to the user that they need the restart Chrome for
  cached experiment flags to take effect.  Users won't understand
  why this is relevant.

BUG=582539
Committed: https://crrev.com/b584cb20c80e31ab5ac85a9269e92efaa2cfbcf7
Cr-Commit-Position: refs/heads/master@{#377454}

[gone, 2016-02-22 21:29:14]

dfalcantara@chromium.org changed reviewers:
+ tedchoc@chromium.org, twellington@chromium.org

[gone, 2016-02-22 21:29:15]

+Theresa: You've played with field trials before, right?  Does the usage in
feature_utilities.cc look right to you?

+Ted: A bunch of stuff moved into FeatureUtilities because it seemed to make
more sense.  Also had to explicitly add a disabled state for the control group.

[gone, 2016-02-22 23:19:42]

Swapped over to variations params.

[gone, 2016-02-22 23:20:57]

Description was changed from

==========
[Herb] Set up field trial for UI prototypes

* Add a field trial for bucketing users into different prototypes.

* Consolidate Herb string checking into FeatureUtilities, which calls
  native to see what field trials are active and whether they're
  overridden by about:flags.

* Adds a "disabled" group to about:flags to explicitly disable the
  experiment, users on about:flags now default to whatever they're
  bucketed into.

* Stop toasting to the user that they need the restart Chrome for
  cached experiment flags to take effect.  Users won't understand
  why this is relevant.

BUG=582539
==========

to

==========
[Herb] Set up field trial for UI prototypes

* Add a field trial for bucketing users into different prototypes.
  The trial stores the herb flavor in a variations param.

* Consolidate Herb string checking into FeatureUtilities, which
  checks whether VariationsAssociatedParams are active and 
  whether they're overridden by about:flags.

* Adds a "disabled" group to about:flags to explicitly disable the
  experiment, users on about:flags now default to whatever they're
  bucketed into.

* Stop toasting to the user that they need the restart Chrome for
  cached experiment flags to take effect.  Users won't understand
  why this is relevant.

BUG=582539
==========

[Ted C, 2016-02-22 23:31:21]

lgtm w/ pending the distinct command lines question

https://codereview.chromium.org/1716313002/diff/20001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1716313002/diff/20001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:533: switches::kTabManagementExperimentType,
"disabled"},
I have a sinking suspicion that you need five distinct command lines so you can
use forcing_flag option in your server side configs.

Hopefully someone on the finch side can confirm that for us.

[gone, 2016-02-22 23:32:39]

dfalcantara@chromium.org changed reviewers:
+ isherman@chromium.org

[gone, 2016-02-22 23:32:39]

Ilya: Is there documentation on how variations params and about:flags are
supposed to interact?  In this case, we've got about:flags managing a single
command line parameter that has four different values.  I don't know how to link
these with the VariationsAssociatedData, though.  That seems to check command
line flags, but I'm not sure how to make it check the same one that about:flags
would be controlling.  Ideas?

[gone, 2016-02-23 00:19:36]

Corresponding json file:
http://cr/115283081

[gone, 2016-02-23 23:57:47]

Please check that things are still on the up and up.  I've updated the
corresponding json file, too: cr/115283081

https://chromiumcodereview.appspot.com/1716313002/diff/20001/chrome/browser/a...
File chrome/browser/about_flags.cc (right):

https://chromiumcodereview.appspot.com/1716313002/diff/20001/chrome/browser/a...
chrome/browser/about_flags.cc:533: switches::kTabManagementExperimentType,
"disabled"},
On 2016/02/22 23:31:21, Ted C wrote:
> I have a sinking suspicion that you need five distinct command lines so you
can
> use forcing_flag option in your server side configs.
> 
> Hopefully someone on the finch side can confirm that for us.

Talked with Dan (and Alexei over chat), who confirmed that this is necessary.

[Ted C, 2016-02-24 00:03:13]

https://chromiumcodereview.appspot.com/1716313002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java
(right):

https://chromiumcodereview.appspot.com/1716313002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java:195:
if (isDocumentMode(context)) return null;
instead of returning null, should we return DISABLED to make it easier for
clients to not have to check null and disabled.

[gone, 2016-02-24 00:18:33]

https://chromiumcodereview.appspot.com/1716313002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java
(right):

https://chromiumcodereview.appspot.com/1716313002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java:195:
if (isDocumentMode(context)) return null;
On 2016/02/24 00:03:13, Ted C wrote:
> instead of returning null, should we return DISABLED to make it easier for
> clients to not have to check null and disabled.

We'll need to keep checking disabled since null could have been saved out on
previous runs, but I've updated this code path to stop returning null. 
Hopefully won't matter too much in the future once a single prototype is
selected.

[Ilya Sherman, 2016-02-24 01:42:35]

https://chromiumcodereview.appspot.com/1716313002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java
(right):

https://chromiumcodereview.appspot.com/1716313002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java:225:
HERB_EXPERIMENT_NAME, HERB_EXPERIMENT_FLAVOR_PARAM);
It seems like you're actually wanting to get the active group, rather than a
param associated with the group.  So, I suspect you're calling
not-quite-the-right function here -- or else the naming you've chosen is quite
confusing.

[gone, 2016-02-24 02:16:15]

https://chromiumcodereview.appspot.com/1716313002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java
(right):

https://chromiumcodereview.appspot.com/1716313002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java:225:
HERB_EXPERIMENT_NAME, HERB_EXPERIMENT_FLAVOR_PARAM);
On 2016/02/24 01:42:35, Ilya Sherman wrote:
> It seems like you're actually wanting to get the active group, rather than a
> param associated with the group.  So, I suspect you're calling
> not-quite-the-right function here -- or else the naming you've chosen is quite
> confusing.

Yeah, I'd originally been using the FieldTrialList class instead of the
VariationsAssociatedData.  I'll swap back over.

[Ilya Sherman, 2016-02-24 02:33:23]

Field trials LGTM, thanks.

[Alexei Svitkine (slow), 2016-02-24 17:08:02]

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

[Alexei Svitkine (slow), 2016-02-24 17:08:03]

https://chromiumcodereview.appspot.com/1716313002/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java
(right):

https://chromiumcodereview.appspot.com/1716313002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java:230:
|| TextUtils.equals(ChromeSwitches.HERB_FLAVOR_CONTROL, newFlavor)
It's not a good idea to use equals() when comparing group names from
findFullName(). Instead, use startsWith(). Otherwise, you don't have the option
of renaming your groups (e.g. when having a separate dogfood config or when
making changes to your percentage to avoid bias from old users).

(Even better would be to use the Feature API, which also avoids these problems.)

[gone, 2016-02-24 17:47:51]

https://chromiumcodereview.appspot.com/1716313002/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java
(right):

https://chromiumcodereview.appspot.com/1716313002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java:230:
|| TextUtils.equals(ChromeSwitches.HERB_FLAVOR_CONTROL, newFlavor)
On 2016/02/24 17:08:03, Alexei Svitkine (slow) wrote:
> It's not a good idea to use equals() when comparing group names from
> findFullName(). Instead, use startsWith(). Otherwise, you don't have the
option
> of renaming your groups (e.g. when having a separate dogfood config or when
> making changes to your percentage to avoid bias from old users).
> 
> (Even better would be to use the Feature API, which also avoids these
problems.)

The Feature API is a boolean only thing, isn't it (enabled/disabled)?  I'm not
sure if the Feature API works here, unless we define 4 different Features that
don't overlap.

[Alexei Svitkine (slow), 2016-02-24 23:10:35]

lgtm % comment

https://chromiumcodereview.appspot.com/1716313002/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java
(right):

https://chromiumcodereview.appspot.com/1716313002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java:230:
|| TextUtils.equals(ChromeSwitches.HERB_FLAVOR_CONTROL, newFlavor)
On 2016/02/24 17:47:51, dfalcantara wrote:
> On 2016/02/24 17:08:03, Alexei Svitkine (slow) wrote:
> > It's not a good idea to use equals() when comparing group names from
> > findFullName(). Instead, use startsWith(). Otherwise, you don't have the
> option
> > of renaming your groups (e.g. when having a separate dogfood config or when
> > making changes to your percentage to avoid bias from old users).
> > 
> > (Even better would be to use the Feature API, which also avoids these
> problems.)
> 
> The Feature API is a boolean only thing, isn't it (enabled/disabled)?  I'm not
> sure if the Feature API works here, unless we define 4 different Features that
> don't overlap.

That's fair. So just change the .equals() checks to be .startsWith().

[gone, 2016-02-24 23:57:04]

TextUtils didn't have a startsWith, but TIL the regular Java String does.

[gone, 2016-02-25 00:00:06]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2016-02-25 00:00:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, isherman@chromium.org, asvitkine@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1716313002/#ps160001 (title:
"startsWith")

[commit-bot: I haz the power, 2016-02-25 00:03:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1716313002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1716313002/160001

[commit-bot: I haz the power, 2016-02-25 01:23:35]

Description was changed from

==========
[Herb] Set up field trial for UI prototypes

* Add a field trial for bucketing users into different prototypes.
  The trial stores the herb flavor in a variations param.

* Consolidate Herb string checking into FeatureUtilities, which
  checks whether VariationsAssociatedParams are active and 
  whether they're overridden by about:flags.

* Adds a "disabled" group to about:flags to explicitly disable the
  experiment, users on about:flags now default to whatever they're
  bucketed into.

* Stop toasting to the user that they need the restart Chrome for
  cached experiment flags to take effect.  Users won't understand
  why this is relevant.

BUG=582539
==========

to

==========
[Herb] Set up field trial for UI prototypes

* Add a field trial for bucketing users into different prototypes.
  The trial stores the herb flavor in a variations param.

* Consolidate Herb string checking into FeatureUtilities, which
  checks whether VariationsAssociatedParams are active and 
  whether they're overridden by about:flags.

* Adds a "disabled" group to about:flags to explicitly disable the
  experiment, users on about:flags now default to whatever they're
  bucketed into.

* Stop toasting to the user that they need the restart Chrome for
  cached experiment flags to take effect.  Users won't understand
  why this is relevant.

BUG=582539
==========

[commit-bot: I haz the power, 2016-02-25 01:23:37]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-02-25 01:24:36]

Description was changed from

==========
[Herb] Set up field trial for UI prototypes

* Add a field trial for bucketing users into different prototypes.
  The trial stores the herb flavor in a variations param.

* Consolidate Herb string checking into FeatureUtilities, which
  checks whether VariationsAssociatedParams are active and 
  whether they're overridden by about:flags.

* Adds a "disabled" group to about:flags to explicitly disable the
  experiment, users on about:flags now default to whatever they're
  bucketed into.

* Stop toasting to the user that they need the restart Chrome for
  cached experiment flags to take effect.  Users won't understand
  why this is relevant.

BUG=582539
==========

to

==========
[Herb] Set up field trial for UI prototypes

* Add a field trial for bucketing users into different prototypes.
  The trial stores the herb flavor in a variations param.

* Consolidate Herb string checking into FeatureUtilities, which
  checks whether VariationsAssociatedParams are active and
  whether they're overridden by about:flags.

* Adds a "disabled" group to about:flags to explicitly disable the
  experiment, users on about:flags now default to whatever they're
  bucketed into.

* Stop toasting to the user that they need the restart Chrome for
  cached experiment flags to take effect.  Users won't understand
  why this is relevant.

BUG=582539

Committed: https://crrev.com/b584cb20c80e31ab5ac85a9269e92efaa2cfbcf7
Cr-Commit-Position: refs/heads/master@{#377454}
==========

[commit-bot: I haz the power, 2016-02-25 01:24:38]

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