Control zero suggest with a pref, not a switch.

Control zero suggest with a pref, not a switch.

This makes it easier to try out the feature: don't need a new command line flag
each time you start up Chrome. It also makes it possible to test it on ChromeOS
devices.

TESTED=Changing setting on chrome://instant works as expected.
BUG=


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

[samarth, 2012-09-11 14:50:31]

Hey Sreeram,

Mind taking a first pass over this before I send it off to OWNERS?

Thanks,
Samarth

[sreeram, 2012-09-11 14:58:20]

lgtm

[samarth, 2012-09-11 15:07:27]

Thanks for the review Sreeram!

jhawkins: you can find some more context about this feature in
http://codereview.chromium.org/10877021/ and
https://chromiumcodereview.appspot.com/10832256/. Feel free to ping me if you'd
like to see it in action and I can show it to you / give you instructions.

Thanks,
Samarth

[samarth, 2012-09-12 18:01:20]

Sorry, sent this to jhawkins@google.com by accident -- see previous comment for
details about this change.

Thanks,
Samarth

[James Hawkins, 2012-09-13 21:32:52]

Have you gotten this approved by UI leads or what have you?  This goes against
general practice (using switches).

I don't understand the need for a pref when the flag can be switched in
about:flags.

[David Black, 2012-09-14 01:02:35]

On 2012/09/13 21:32:52, James Hawkins wrote:
> Have you gotten this approved by UI leads or what have you?  This goes against
> general practice (using switches).
> 
> I don't understand the need for a pref when the flag can be switched in
> about:flags.

I believe the main reason for doing it this way is because not all chromebooks
have access to about:flags.  (Corp managed ones often don't, for example.)

[James Hawkins, 2012-09-14 01:07:27]

On 2012/09/14 01:02:35, David Black wrote:
> On 2012/09/13 21:32:52, James Hawkins wrote:
> > Have you gotten this approved by UI leads or what have you?  This goes
against
> > general practice (using switches).
> > 
> > I don't understand the need for a pref when the flag can be switched in
> > about:flags.
> 
> I believe the main reason for doing it this way is because not all chromebooks
> have access to about:flags.  (Corp managed ones often don't, for example.)

OK.  What about approval from leads?

[samarth, 2012-09-14 01:09:20]

(Sorry, I thought I already sent this..)

Hi James,


On 2012/09/14 01:07:27, James Hawkins wrote:
> On 2012/09/14 01:02:35, David Black wrote:
> > On 2012/09/13 21:32:52, James Hawkins wrote:
> > > Have you gotten this approved by UI leads or what have you?  This goes
> against
> > > general practice (using switches).

Marc and Linus and aware of the feature and of the plans to implement it this
way.  Peter approved the original change that added the feature behind a switch.

> > > 
> > > I don't understand the need for a pref when the flag can be switched in
> > > about:flags.
> > 

The switch isn't boolean: you need to set it to point to a server that will set
the zero-suggestions.  As far as I know, you can't set these flags from
about:flags.

Thanks,
Samarth

[James Hawkins, 2012-09-14 15:36:13]

On 2012/09/14 01:09:20, samarth wrote:
> (Sorry, I thought I already sent this..)
> 
> Hi James,
> 
> 
> On 2012/09/14 01:07:27, James Hawkins wrote:
> > On 2012/09/14 01:02:35, David Black wrote:
> > > On 2012/09/13 21:32:52, James Hawkins wrote:
> > > > Have you gotten this approved by UI leads or what have you?  This goes
> > against
> > > > general practice (using switches).
> 
> Marc and Linus and aware of the feature and of the plans to implement it this
> way.  Peter approved the original change that added the feature behind a
switch.
> 

They've approved that we're adding a new pref in the settings page?

> > > > 
> > > > I don't understand the need for a pref when the flag can be switched in
> > > > about:flags.
> > > 
> 
> The switch isn't boolean: you need to set it to point to a server that will
set
> the zero-suggestions.  As far as I know, you can't set these flags from
> about:flags.
> 
> Thanks,
> Samarth

[sreeram, 2012-09-14 15:42:34]

On 2012/09/14 15:36:13, James Hawkins wrote:
> They've approved that we're adding a new pref in the settings page?

The pref is exposed in chrome://instant, which is technically a WebUI page, but
is not actually accessible from any menu or chrome://settings. It's akin to one
of the many hidden pages such as chrome://predictors or chrome://dns.

Just to summarize: We can't use a commandline flag because most ChromeOS devices
can't set it. We can't use an about:flag because some ChromeOS devices can't
access about:flags, and also, about:flags doesn't allow you to specify an
arbitrary string value (only booleans or a predefined list).

[James Hawkins, 2012-09-14 15:45:05]

LGTM with nit.

http://codereview.chromium.org/10933023/diff/1/chrome/browser/autocomplete/ze...
File chrome/browser/autocomplete/zero_suggest_provider.h (right):

http://codereview.chromium.org/10933023/diff/1/chrome/browser/autocomplete/ze...
chrome/browser/autocomplete/zero_suggest_provider.h:53: // Create and return an
instance of this provider if the feature is enabled.
nit: Creates and returns

[samarth, 2012-09-14 18:25:33]

Thanks for the review! Waiting for trybots to run successfully and then I'll
commit.

Thanks,
Samarth

http://codereview.chromium.org/10933023/diff/1/chrome/browser/autocomplete/ze...
File chrome/browser/autocomplete/zero_suggest_provider.h (right):

http://codereview.chromium.org/10933023/diff/1/chrome/browser/autocomplete/ze...
chrome/browser/autocomplete/zero_suggest_provider.h:53: // Create and return an
instance of this provider if the feature is enabled.
On 2012/09/14 15:45:05, James Hawkins wrote:
> nit: Creates and returns

Done.

[commit-bot: I haz the power, 2012-09-14 23:02:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/10933023/10012

[commit-bot: I haz the power, 2012-09-15 05:08:00]

Change committed as 156986