Add new instant extended specific pref.

Add new instant extended specific pref.

Since we don't want to trip the regular instant pref via sync, and we want to set instant on by default when the instant extended flag is set, and we want it to sync, so we created a new pref, specifically for instant extended.


BUG=None


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

[MAD, 2012-12-07 19:01:23]

Are you OK with this? Feel free to redirect to other reviewers...

Thanks!

BYE
MAD

[dhollowa, 2012-12-07 19:50:37]

Ya, LGTM.  Thanks.  Adding estade@ for js/options review.

https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/browser_options_handler.cc:399:
Profile::FromWebUI(web_ui()))) {
nit: indent

[MAD, 2012-12-07 19:56:50]

Thanks... But not sure about your nit...

I was going to add estade@ for the owners review anyway... :-)

I also need owner's review from browser/ui to... Peter, can you do it? Or should
I ask sky?

https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/browser_options_handler.cc:399:
Profile::FromWebUI(web_ui()))) {
On 2012/12/07 19:50:38, dhollowa wrote:
> nit: indent

What's wrong? Isn't this supposed to be double indent? It's part of the previous
line...

[dhollowa, 2012-12-07 20:05:12]

https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/browser_options_handler.cc:399:
Profile::FromWebUI(web_ui()))) {
On 2012/12/07 19:56:51, MAD wrote:
> On 2012/12/07 19:50:38, dhollowa wrote:
> > nit: indent
> 
> What's wrong? Isn't this supposed to be double indent? It's part of the
previous
> line...

Function arguments should indent 4-spaces from start of function name.

[MAD, 2012-12-07 20:26:14]

On 2012/12/07 20:05:12, dhollowa wrote:
>
https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/webui/op...
> File chrome/browser/ui/webui/options/browser_options_handler.cc (right):
> 
>
https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/webui/op...
> chrome/browser/ui/webui/options/browser_options_handler.cc:399:
> Profile::FromWebUI(web_ui()))) {
> On 2012/12/07 19:56:51, MAD wrote:
> > On 2012/12/07 19:50:38, dhollowa wrote:
> > > nit: indent
> > 
> > What's wrong? Isn't this supposed to be double indent? It's part of the
> previous
> > line...
> 
> Function arguments should indent 4-spaces from start of function name.

I didn't know that, and can't find it in the coding style... The example they
give indents from beginning of line, including return value type at the
beginning of the line, so I usually align 4 spaces indent from the beginning of
the if...

[Peter Kasting, 2012-12-07 21:50:08]

https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/browser_options_handler.cc:399:
Profile::FromWebUI(web_ui()))) {
On 2012/12/07 20:05:13, dhollowa wrote:
> On 2012/12/07 19:56:51, MAD wrote:
> > On 2012/12/07 19:50:38, dhollowa wrote:
> > > nit: indent
> > 
> > What's wrong? Isn't this supposed to be double indent? It's part of the
> previous
> > line...
> 
> Function arguments should indent 4-spaces from start of function name.

Negative, ghostrider; mad's current indenting is correct.

[dhollowa, 2012-12-07 21:56:12]

https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/browser_options_handler.cc:399:
Profile::FromWebUI(web_ui()))) {
On 2012/12/07 21:50:08, Peter Kasting wrote:
> On 2012/12/07 20:05:13, dhollowa wrote:
> > On 2012/12/07 19:56:51, MAD wrote:
> > > On 2012/12/07 19:50:38, dhollowa wrote:
> > > > nit: indent
> > > 
> > > What's wrong? Isn't this supposed to be double indent? It's part of the
> > previous
> > > line...
> > 
> > Function arguments should indent 4-spaces from start of function name.
> 
> Negative, ghostrider; mad's current indenting is correct.

Really?  Wow, sorry. 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls
seems pretty clear to me.  The discussion on wrapping conditionals is only for
sequential clauses, a && b for example.  Can you point me to the section that
applies here?

[Peter Kasting, 2012-12-07 22:01:50]

On 2012/12/07 21:56:12, dhollowa wrote:
>
https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/webui/op...
> File chrome/browser/ui/webui/options/browser_options_handler.cc (right):
> 
>
https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/webui/op...
> chrome/browser/ui/webui/options/browser_options_handler.cc:399:
> Profile::FromWebUI(web_ui()))) {
> On 2012/12/07 21:50:08, Peter Kasting wrote:
> > On 2012/12/07 20:05:13, dhollowa wrote:
> > > On 2012/12/07 19:56:51, MAD wrote:
> > > > On 2012/12/07 19:50:38, dhollowa wrote:
> > > > > nit: indent
> > > > 
> > > > What's wrong? Isn't this supposed to be double indent? It's part of the
> > > previous
> > > > line...
> > > 
> > > Function arguments should indent 4-spaces from start of function name.
> > 
> > Negative, ghostrider; mad's current indenting is correct.
> 
> Really?  Wow, sorry. 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls
> seems pretty clear to me.  The discussion on wrapping conditionals is only for
> sequential clauses, a && b for example.  Can you point me to the section that
> applies here?

It's not an explicit section.  The wording of the indent in the style guide is
always non-qualified: "4-space indent" as opposed to "4-space indent from the
start of the function name".  And the examples reinforce what would be the
obvious interpretation, namely that this is "from the beginning of the line",
and not from something on the previous line: see for example the 4-space indent
in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_...
or in the third example in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
.  I was also advised by another readability reviewer some time ago to indent 4
spaces from the beginning of the line.  Finally, while Chrome code is not 100%
consistent, the "from beginning of line" interpretatino seems predominant to me.

[MAD, 2012-12-07 22:02:25]

On 2012/12/07 21:56:12, dhollowa wrote:
>
https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/webui/op...
> File chrome/browser/ui/webui/options/browser_options_handler.cc (right):
> 
>
https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/webui/op...
> chrome/browser/ui/webui/options/browser_options_handler.cc:399:
> Profile::FromWebUI(web_ui()))) {
> On 2012/12/07 21:50:08, Peter Kasting wrote:
> > On 2012/12/07 20:05:13, dhollowa wrote:
> > > On 2012/12/07 19:56:51, MAD wrote:
> > > > On 2012/12/07 19:50:38, dhollowa wrote:
> > > > > nit: indent
> > > > 
> > > > What's wrong? Isn't this supposed to be double indent? It's part of the
> > > previous
> > > > line...
> > > 
> > > Function arguments should indent 4-spaces from start of function name.
> > 
> > Negative, ghostrider; mad's current indenting is correct.
> 
> Really?  Wow, sorry. 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls
> seems pretty clear to me.  The discussion on wrapping conditionals is only for
> sequential clauses, a && b for example.  Can you point me to the section that
> applies here?
I think the section you showed applied, but it's not the beginning of the
function name that is indented 4 spaces, it's the actual beginning of the line,
it's just that the function name starts at the beginning of the line, thus the
confusion... no?

[Evan Stade, 2012-12-07 22:06:23]

https://codereview.chromium.org/11475028/diff/4002/chrome/browser/resources/o...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/11475028/diff/4002/chrome/browser/resources/o...
chrome/browser/resources/options/browser_options.html:149:
pref="instant.enabled" dialog-pref>
I think you could do i18n-values="pref:someVariableName"

and make sure that loadTimeData has someVariableName set to either
instant.enabled or instant_extended.enabled

[MAD, 2012-12-07 22:42:41]

Thanks evan, I didn't know about this, I'll try tomorrow...

And Gideon suggested I added sky@ in case Peter doesn't have a chance to look at
this over the weekend...

We would like to have this patch in for the dev channel cut during the night of
Sunday to Monday...

Thanks all!

BYE
MAD

[sky, 2012-12-07 23:31:31]

The non webui stuff LGTM (with the following change).

https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:30: if
(chrome::search::IsInstantExtendedAPIEnabled(profile))
Style guide says 'Don't use else after return', so either:
if (foo)
  return;
return;

or return XX ? YY : ZZ;

[Peter Kasting, 2012-12-07 23:34:15]

-me as reviewer due to sky's review.

[MAD, 2012-12-08 15:53:10]

Cool, thanks all!

Comments addressed!.Only missing webui OWNERS lgtm...

BYE
MAD

https://codereview.chromium.org/11475028/diff/4002/chrome/browser/resources/o...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/11475028/diff/4002/chrome/browser/resources/o...
chrome/browser/resources/options/browser_options.html:149:
pref="instant.enabled" dialog-pref>
On 2012/12/07 22:06:24, Evan Stade wrote:
> I think you could do i18n-values="pref:someVariableName"
> 
> and make sure that loadTimeData has someVariableName set to either
> instant.enabled or instant_extended.enabled

Done.

https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11475028/diff/4002/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:30: if
(chrome::search::IsInstantExtendedAPIEnabled(profile))
On 2012/12/07 23:31:31, sky wrote:
> Style guide says 'Don't use else after return', so either:
> if (foo)
>   return;
> return;
> 
> or return XX ? YY : ZZ;

Done.

[Evan Stade, 2012-12-08 23:45:21]

lgtm if you re-add the <span>

https://codereview.chromium.org/11475028/diff/7010/chrome/browser/resources/o...
File chrome/browser/resources/options/browser_options.html (left):

https://codereview.chromium.org/11475028/diff/7010/chrome/browser/resources/o...
chrome/browser/resources/options/browser_options.html:150: <span>
why did you delete this

https://codereview.chromium.org/11475028/diff/7010/chrome/browser/resources/o...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/11475028/diff/7010/chrome/browser/resources/o...
chrome/browser/resources/options/browser_options.js:162:
'instant_extended.enabled') {
4 more indent

https://codereview.chromium.org/11475028/diff/7010/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/11475028/diff/7010/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/browser_options_handler.cc:399:
Profile::FromWebUI(web_ui()))) {
4 more indent

[MAD, 2012-12-09 00:27:10]

OK, thanks! Fixed the span removal, was a mishap... Answered the nits too...
Coding style says it's OK as is...

Will CQ now...

https://codereview.chromium.org/11475028/diff/7010/chrome/browser/resources/o...
File chrome/browser/resources/options/browser_options.html (left):

https://codereview.chromium.org/11475028/diff/7010/chrome/browser/resources/o...
chrome/browser/resources/options/browser_options.html:150: <span>
On 2012/12/08 23:45:22, Evan Stade wrote:
> why did you delete this

Oupsss... Sorry didn't notice... It's a mistake...

https://codereview.chromium.org/11475028/diff/7010/chrome/browser/resources/o...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/11475028/diff/7010/chrome/browser/resources/o...
chrome/browser/resources/options/browser_options.js:162:
'instant_extended.enabled') {
On 2012/12/08 23:45:22, Evan Stade wrote:
> 4 more indent

Why? As done at line 960, the coding style says: "When the function call is
itself indented, you're free to start the 4-space indent relative to the
beginning of the original statement or relative to the beginning of the current
function call."

So I thought 4 spaces from the beginning of the "if (" line.

https://codereview.chromium.org/11475028/diff/7010/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/11475028/diff/7010/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/browser_options_handler.cc:399:
Profile::FromWebUI(web_ui()))) {
On 2012/12/08 23:45:22, Evan Stade wrote:
> 4 more indent

Again, coding style says only 4 as is done here. There was an argument between
Peter and David about and we concluded on keeping it like this.

[commit-bot: I haz the power, 2012-12-09 00:27:42]

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

[commit-bot: I haz the power, 2012-12-09 09:14:24]

Change committed as 172004