Enable auto-reload by default.

Enable auto-reload by default.

Stop honoring the Finch enable/disable groups, but still honor explicit flips
of the flags in chrome://flags.

BUG=383169
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288081

[Elly Fong-Jones, 2014-08-05 19:32:20]

mmenke: PTAL?

[mmenke, 2014-08-05 19:33:50]

On 2014/08/05 19:32:20, Elly Jones wrote:
> mmenke: PTAL?

We're past feature freeze.  Do we have the signoffs to do this in M38?

[mmenke, 2014-08-05 19:36:59]

https://codereview.chromium.org/442913002/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/442913002/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1126: "AutoReloadExperiment");
If we're getting rid of the field trial, should get rid of this line.  If we're
just changing the value when there's no field trial, should instead change the
last line to "return group != "Disabled";"

Same goes for the other function.

[Elly Fong-Jones, 2014-08-05 19:41:50]

On 2014/08/05 19:33:50, mmenke wrote:
> On 2014/08/05 19:32:20, Elly Jones wrote:
> > mmenke: PTAL?
> 
> We're past feature freeze.  Do we have the signoffs to do this in M38?

I think so and I think they are on the linked bug. Are there other signoffs I
need?

[Elly Fong-Jones, 2014-08-05 19:41:56]

https://codereview.chromium.org/442913002/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/442913002/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1126: "AutoReloadExperiment");
On 2014/08/05 19:36:59, mmenke wrote:
> If we're getting rid of the field trial, should get rid of this line.  If
we're
> just changing the value when there's no field trial, should instead change the
> last line to "return group != "Disabled";"
> 
> Same goes for the other function.

No, we need to leave the field trial there. The field trial will still be useful
for gathering stats (in particular, it tracks what proportions of users have
force-enabled/disabled autoreload using chrome://flags or group policy). For
that to happen, we have to call FindFullName() at least once on each trial. I
can add a comment about why it's unused if you'd like.

[mmenke, 2014-08-05 20:34:00]

https://codereview.chromium.org/442913002/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/442913002/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1126: "AutoReloadExperiment");
On 2014/08/05 19:41:56, Elly Jones wrote:
> On 2014/08/05 19:36:59, mmenke wrote:
> > If we're getting rid of the field trial, should get rid of this line.  If
> we're
> > just changing the value when there's no field trial, should instead change
the
> > last line to "return group != "Disabled";"
> > 
> > Same goes for the other function.
> 
> No, we need to leave the field trial there. The field trial will still be
useful
> for gathering stats (in particular, it tracks what proportions of users have
> force-enabled/disabled autoreload using chrome://flags or group policy). For
> that to happen, we have to call FindFullName() at least once on each trial. I
> can add a comment about why it's unused if you'd like.

I'm not following.  How does this record whether people have it enabled or
disabled by policy or about:flags?  about:flags adds a switch, and there doesn't
seem to be any way for UMA to know the switch is related to the field trial.

[chromium-reviews, 2014-08-06 19:57:55]

UMA knows on the server side which flags are related to the field trial,
and tells Chrome about this.


On Tue, Aug 5, 2014 at 4:33 PM, <mmenke@chromium.org> wrote:

>
> https://codereview.chromium.org/442913002/diff/1/chrome/
> browser/chrome_content_browser_client.cc
> File chrome/browser/chrome_content_browser_client.cc (right):
>
> https://codereview.chromium.org/442913002/diff/1/chrome/
> browser/chrome_content_browser_client.cc#newcode1126
> chrome/browser/chrome_content_browser_client.cc:1126:
> "AutoReloadExperiment");
> On 2014/08/05 19:41:56, Elly Jones wrote:
>
>> On 2014/08/05 19:36:59, mmenke wrote:
>> > If we're getting rid of the field trial, should get rid of this
>>
> line.  If
>
>> we're
>> > just changing the value when there's no field trial, should instead
>>
> change the
>
>> > last line to "return group != "Disabled";"
>> >
>> > Same goes for the other function.
>>
>
>  No, we need to leave the field trial there. The field trial will still
>>
> be useful
>
>> for gathering stats (in particular, it tracks what proportions of
>>
> users have
>
>> force-enabled/disabled autoreload using chrome://flags or group
>>
> policy). For
>
>> that to happen, we have to call FindFullName() at least once on each
>>
> trial. I
>
>> can add a comment about why it's unused if you'd like.
>>
>
> I'm not following.  How does this record whether people have it enabled
> or disabled by policy or about:flags?  about:flags adds a switch, and
> there doesn't seem to be any way for UMA to know the switch is related
> to the field trial.
>
> https://codereview.chromium.org/442913002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmenke, 2014-08-06 20:01:40]

On 2014/08/06 19:57:55, chromium-reviews wrote:
> UMA knows on the server side which flags are related to the field trial,
> and tells Chrome about this.

OK, LGTM then, but should add a comment about that (And eventually, should get
rid of the groups entirely - don't think that keeping track of how many people
disable the feature is something we want to keep, long term).

[Elly Fong-Jones, 2014-08-07 13:53:37]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2014-08-07 13:54:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ellyjones@chromium.org/442913002/20001

[commit-bot: I haz the power, 2014-08-07 16:44:31]

Change committed as 288081