Restrict Instant field trial to UMA opt-in users.

Restrict Instant field trial to UMA opt-in users.

Note that if the Instant experiment has already been enabled, and *then* the
user opts out of UMA, this won't disable the experiment immediately (since the
global field trial state doesn't change). Instead, it will be disabled at
browser restart. I think this is acceptable since, if the user is fiddling with
preferences after Instant has been enabled, they can already click the Instant
checkbox to turn it off.

In addition:
+ Fix a typo in the "Omnibox.QueryTime.*" histogram.
+ Update the comment in the .h file so it flows better.

BUG=91820
TEST=Disable UMA reporting (as is default in a Chromium build), and verify that the Instant experiment doesn't get enabled.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96057

[sreeram, 2011-08-05 20:30:49]

Please review.

@sky seems to be offline, and this is release blocking (urgent request from
@chrome-privacy), so it'd be great if any of you can jump into this as soon as
possible. Thanks!

[brettw, 2011-08-05 23:23:43]

Please list who you want to review what. For now, I'm going to assume I don't
need to look at this.

[sreeram, 2011-08-06 05:50:50]

-brettw

[sky, 2011-08-08 16:10:53]

Which call ends up failing if the user didn't opt into UMA?
This LGTM, but wait for jar to chime in.

http://codereview.chromium.org/7583012/diff/1/chrome/browser/instant/instant_...
File chrome/browser/instant/instant_field_trial.cc (right):

http://codereview.chromium.org/7583012/diff/1/chrome/browser/instant/instant_...
chrome/browser/instant/instant_field_trial.cc:14: int kControlGroupId1 = 0;
Add descriptions. Using kFoo implies these are constant, when they aren't
really.

http://codereview.chromium.org/7583012/diff/1/chrome/browser/instant/instant_...
chrome/browser/instant/instant_field_trial.cc:55: if (group == kControlGroupId1)
{
no {}

[sreeram, 2011-08-08 16:30:10]

On 2011/08/08 16:10:53, sky wrote:
> Which call ends up failing if the user didn't opt into UMA?

The call to UseOneTimeRandomization(). I've added a comment now to explain that.

http://codereview.chromium.org/7583012/diff/1/chrome/browser/instant/instant_...
File chrome/browser/instant/instant_field_trial.cc (right):

http://codereview.chromium.org/7583012/diff/1/chrome/browser/instant/instant_...
chrome/browser/instant/instant_field_trial.cc:14: int kControlGroupId1 = 0;
On 2011/08/08 16:10:54, sky wrote:
> Add descriptions. Using kFoo implies these are constant, when they aren't
> really.

Done.

http://codereview.chromium.org/7583012/diff/1/chrome/browser/instant/instant_...
chrome/browser/instant/instant_field_trial.cc:55: if (group == kControlGroupId1)
{
On 2011/08/08 16:10:54, sky wrote:
> no {}

Done.

[jar (doing other things), 2011-08-08 22:28:29]

I'm fine with your "not disabling the experiment immediately."  When a use opts
out of UMA, we stop sending data, so there is no privacy loss IMO.  In most
experiments (not requiring persistence of the coin toss), users are in/out
independent of whether they opt into UMA.

http://codereview.chromium.org/7583012/diff/3001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_field_trial.cc (right):

http://codereview.chromium.org/7583012/diff/3001/chrome/browser/instant/insta...
chrome/browser/instant/instant_field_trial.cc:16: int kControlGroupId1 = 0;
As you noted, these are not const, and hence should not have a k prefix.

In addition, they appear to be global (to the file/namespace), so they should
have a g_ prefix.

Alternatively, you might want to make them static member variables, or if there
is only one instance of InstantFieldTrial, make them member variables.

http://codereview.chromium.org/7583012/diff/3001/chrome/browser/instant/insta...
chrome/browser/instant/instant_field_trial.cc:35: kExperimentGroupId1 =
trial->AppendGroup("InstantExperiment1", 500);  // 5%
Notice that you quite reasonably only use probabilities as low as 5%.  As a
result, I really don't think you need more than 1000 distinct
one-time-randomization values (which would facilitate 0.1% granularity in
experiments).  As I said, I'm generally in favor of less user tagging when
possible, to avoid any possible privacy concerns.

[sreeram, 2011-08-08 22:42:23]

http://codereview.chromium.org/7583012/diff/3001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_field_trial.cc (right):

http://codereview.chromium.org/7583012/diff/3001/chrome/browser/instant/insta...
chrome/browser/instant/instant_field_trial.cc:16: int kControlGroupId1 = 0;
On 2011/08/08 22:28:29, jar wrote:
> As you noted, these are not const, and hence should not have a k prefix.
> 
> In addition, they appear to be global (to the file/namespace), so they should
> have a g_ prefix.
> 
> Alternatively, you might want to make them static member variables, or if
there
> is only one instance of InstantFieldTrial, make them member variables.

Done. These are unnecessary implementation details that I don't want to expose
in the interface, so I'd rather not have them as instance or static members. So,
I've changed the names to be g_*.

http://codereview.chromium.org/7583012/diff/3001/chrome/browser/instant/insta...
chrome/browser/instant/instant_field_trial.cc:35: kExperimentGroupId1 =
trial->AppendGroup("InstantExperiment1", 500);  // 5%
On 2011/08/08 22:28:29, jar wrote:
> Notice that you quite reasonably only use probabilities as low as 5%.  As a
> result, I really don't think you need more than 1000 distinct
> one-time-randomization values (which would facilitate 0.1% granularity in
> experiments).  As I said, I'm generally in favor of less user tagging when
> possible, to avoid any possible privacy concerns.

Done. I've reduced the granularity to 0.1%, but note that even if we had the
divisor as a million or billion, it wouldn't affect privacy. The random number
is never stored in preferences, unlike the previous code (which was not
restricted to UMA users only).

[jar (doing other things), 2011-08-09 16:34:27]

LGTM

http://codereview.chromium.org/7583012/diff/3001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_field_trial.cc (right):

http://codereview.chromium.org/7583012/diff/3001/chrome/browser/instant/insta...
chrome/browser/instant/instant_field_trial.cc:35: kExperimentGroupId1 =
trial->AppendGroup("InstantExperiment1", 500);  // 5%
On 2011/08/08 22:42:23, sreeram wrote:
> On 2011/08/08 22:28:29, jar wrote:
> > Notice that you quite reasonably only use probabilities as low as 5%.  As a
> > result, I really don't think you need more than 1000 distinct
> > one-time-randomization values (which would facilitate 0.1% granularity in
> > experiments).  As I said, I'm generally in favor of less user tagging when
> > possible, to avoid any possible privacy concerns.
> 
> Done. I've reduced the granularity to 0.1%, but note that even if we had the
> divisor as a million or billion, it wouldn't affect privacy. The random number
> is never stored in preferences, unlike the previous code (which was not
> restricted to UMA users only).

ah... you are correct.  My mistake.  Thanks!

[commit-bot: I haz the power, 2011-08-09 20:40:11]

Change committed as 96057