Enable Google Now on trybots

Enable Google Now on trybots (and all users on Trunk).

BUG=164227
R=asvitkine@chromium.org, kalman@chromium.org, robliao@chromium.org, skare@chromium.org

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

[Alexei Svitkine (slow), 2013-11-01 15:16:37]

https://codereview.chromium.org/48103003/diff/1/chrome/browser/extensions/com...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/48103003/diff/1/chrome/browser/extensions/com...
chrome/browser/extensions/component_loader.cc:517:
base::FieldTrialList::FindFullName(kFieldTrialName);
It seems this logic is also duplicated in geolocation_options_handler.cc.

Can you refactor this to a single function that both places call, so that they
don't diverge?

https://codereview.chromium.org/48103003/diff/1/chrome/browser/extensions/com...
chrome/browser/extensions/component_loader.cc:520: field_trial_result.empty() ||
Not convinced this is a good solution - for example field trial information is
not available on first run.

Another possibility is to have a client-configured field trial that will be
overridden by the server config, e.g. using
FieldTrialList::FactoryGetFieldTrial().

If you do this, how I imagine it will work:

trial = FieldTrialList::FactoryGetFieldTrial("GoogleNow", ...);
if (stable || beta)
  trial->AppendGroup("Enable", 0);
else
  trial->AppendGroup("Enable", 100);
std::string field_trial_result = trail->group_name();

How it will work is that in absence of the config, it will enable the feature
except on beta and stable platforms, and disable otherwise.

However, if there is a server-side config, the percentages specified by the
client code will be overridden by the server config. So it's still possible to
enable it on beta or stable server-side.

You can also make it so that the 100% enable is only applied when current
channel is UNKNOWN - which I think is what our bots run. That way, you'll get
coverage on the bots but not for real users (on real channels) until you enable
the server config.

[vadimt, 2013-11-01 17:48:19]

https://codereview.chromium.org/48103003/diff/1/chrome/browser/extensions/com...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/48103003/diff/1/chrome/browser/extensions/com...
chrome/browser/extensions/component_loader.cc:517:
base::FieldTrialList::FindFullName(kFieldTrialName);
On 2013/11/01 15:16:37, Alexei Svitkine wrote:
> It seems this logic is also duplicated in geolocation_options_handler.cc.
> 
> Can you refactor this to a single function that both places call, so that they
> don't diverge?

In geolocation, the code is pretty dead, and that flag is never shown. We are
experimenting now with NOT providing geolocation from the client, if this
succeeds, we'll delete that code. If succeeds, we'll revive that code, and do
the unification.

https://codereview.chromium.org/48103003/diff/1/chrome/browser/extensions/com...
chrome/browser/extensions/component_loader.cc:520: field_trial_result.empty() ||
Done. I chose the second approach, since this is the minimum that we need.
GN will be enabled (unless disabled via flags):
1. On trybots (I've verified that they run on "unknown" channel.
2. For Chromium users, which is good in terms of making them run this code too.

For Canary and up it's still disabled by default.

[Alexei Svitkine (slow), 2013-11-01 17:56:00]

LGTM % comment

https://codereview.chromium.org/48103003/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/48103003/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/component_loader.cc:537: bool
enabled_via_field_trial_or_on_unknown_channel =
Nit: move this right below |is_on_unknown_channel|, so it follows the
declaration of the two fields its using. Also, add a comment to explain this
logic (e.g. to get coverage on the bots).

[skare_, 2013-11-01 17:59:18]

lgtm

[vadimt, 2013-11-01 18:03:50]

https://codereview.chromium.org/48103003/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/48103003/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/component_loader.cc:537: bool
enabled_via_field_trial_or_on_unknown_channel =
On 2013/11/01 17:56:01, Alexei Svitkine wrote:
> Nit: move this right below |is_on_unknown_channel|, so it follows the
> declaration of the two fields its using. Also, add a comment to explain this
> logic (e.g. to get coverage on the bots).

Done.

[vadimt, 2013-11-01 18:04:26]

kalman@, please provide OWNER's approval

[not at google - send to devlin, 2013-11-01 18:15:22]

I am not happy about 34 lines of code to decide whether to turn a feature on.

https://codereview.chromium.org/48103003/diff/180001/chrome/browser/extension...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/48103003/diff/180001/chrome/browser/extension...
chrome/browser/extensions/component_loader.cc:542: enabled_via_flag) {
This is getting increasingly hard to follow. How about this:

bool enabled_via_trunk_build = chrome::VersionInfo::GetChannel() ==    
chrome::VersionInfo::CHANNEL_UNKNOWN;

bool enabled_via_flag = ...;

bool disabled_via_flag = ...;

if ((enabled_via_field_trial && !disabled_via_flag) ||
    (enabled_via_trunk_build && !disabled_via_flag) ||
    enabled_via_flag)) {
  Add(...);
}

[robliao, 2013-11-01 18:48:19]

lgtm

[vadimt, 2013-11-01 19:57:21]

https://codereview.chromium.org/48103003/diff/180001/chrome/browser/extension...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/48103003/diff/180001/chrome/browser/extension...
chrome/browser/extensions/component_loader.cc:542: enabled_via_flag) {
Done.

[not at google - send to devlin, 2013-11-01 19:58:18]

lgtm

[commit-bot: I haz the power, 2013-11-01 20:00:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vadimt@chromium.org/48103003/230001

[vadimt, 2013-11-02 00:23:38]

Committed patchset #6 manually as r232524 (presubmit successful).