Use an experiment value to set the instant_extended.enabled pref default.

Use an experiment value to set the instant_extended.enabled pref default.

BUG=171863
TEST=unit_tests.exe
TBR=sky

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

[robertshield, 2013-01-31 03:54:03]

Hi MAD, 

  Please could you take a look and see if this is similar to what you were
thinking. I'm also not sure how to test the finch experiment settings, I'll come
bug you tomorrow for help with that.

Thanks!
Robert

[MAD, 2013-01-31 15:44:57]

Très cool... Some comments...

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/browser_ins...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.cc:84: } else if
(instant_extended_default_setting == search::INSTANT_USE_EXISTING) {
No need for {} here... All one liners...

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/search/sear...
File chrome/browser/ui/search/search.cc (right):

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/search/sear...
chrome/browser/ui/search/search.cc:53: InstantExtendedDefault
InstantExtendedDefaultFromInt64(int64 default_value) {
I usually prefer having functions not available outside this source file (e.g.,
not declared in any header file), to be part of unnamed namespace.

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/search/sear...
chrome/browser/ui/search/search.cc:68:
base::FieldTrialList::Find(kInstantExtendedFieldTrialName);
There's also a method on FieldTrialList to get the group name directly. It
returns an empty string if the trial isn't active, so provide what you're
looking for here in a single call. I guess the ones who wrote/reviewed the code
you copied from below didn't know about it... :-)

I wonder if there could be a way to extract this common code somehow...

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/search/sear...
chrome/browser/ui/search/search.cc:182: bool GetFieldTrialInfo(const
std::string& group_name,
Now that we return success, group_number pointer could be optional (e.g., accept
NULL) since none of the calls in this file actually use it.

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/search/sear...
File chrome/browser/ui/search/search.h (right):

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/search/sear...
chrome/browser/ui/search/search.h:59: // Exposed for testing only.
Document return value?

[robertshield, 2013-01-31 18:56:51]

Thanks, PTAL!

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/browser_ins...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.cc:84: } else if
(instant_extended_default_setting == search::INSTANT_USE_EXISTING) {
On 2013/01/31 15:44:57, MAD wrote:
> No need for {} here... All one liners...

Done.

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/search/sear...
File chrome/browser/ui/search/search.cc (right):

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/search/sear...
chrome/browser/ui/search/search.cc:53: InstantExtendedDefault
InstantExtendedDefaultFromInt64(int64 default_value) {
On 2013/01/31 15:44:57, MAD wrote:
> I usually prefer having functions not available outside this source file
(e.g.,
> not declared in any header file), to be part of unnamed namespace.

Ordinarily I would have, but since most of this function uses stuff in the
chrome::search:: namespace it got really verbose. Ask me again and I'll move it
if you want :)

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/search/sear...
chrome/browser/ui/search/search.cc:68:
base::FieldTrialList::Find(kInstantExtendedFieldTrialName);
On 2013/01/31 15:44:57, MAD wrote:
> There's also a method on FieldTrialList to get the group name directly. It
> returns an empty string if the trial isn't active, so provide what you're
> looking for here in a single call. I guess the ones who wrote/reviewed the
code
> you copied from below didn't know about it... :-)
> 
> I wonder if there could be a way to extract this common code somehow...

Used the method you suggest. With the refactoring now done, this code isn't long
enough to be worth extracting imo.

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/search/sear...
chrome/browser/ui/search/search.cc:182: bool GetFieldTrialInfo(const
std::string& group_name,
On 2013/01/31 15:44:57, MAD wrote:
> Now that we return success, group_number pointer could be optional (e.g.,
accept
> NULL) since none of the calls in this file actually use it.

Done.

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/search/sear...
File chrome/browser/ui/search/search.h (right):

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/search/sear...
chrome/browser/ui/search/search.h:59: // Exposed for testing only.
On 2013/01/31 15:44:57, MAD wrote:
> Document return value?

Done.

[MAD, 2013-01-31 20:14:53]

LGTM with two nit requests and a unneeded/unexplained change removal request..

Thanks!

BYE
MAD

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/search/sear...
File chrome/browser/ui/search/search.cc (right):

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/search/sear...
chrome/browser/ui/search/search.cc:53: InstantExtendedDefault
InstantExtendedDefaultFromInt64(int64 default_value) {
On 2013/01/31 18:56:52, robertshield wrote:
> On 2013/01/31 15:44:57, MAD wrote:
> > I usually prefer having functions not available outside this source file
> (e.g.,
> > not declared in any header file), to be part of unnamed namespace.
> 
> Ordinarily I would have, but since most of this function uses stuff in the
> chrome::search:: namespace it got really verbose. Ask me again and I'll move
it
> if you want :)

OK... Forget it...

https://codereview.chromium.org/12094073/diff/1/chrome/browser/ui/search/sear...
chrome/browser/ui/search/search.cc:68:
base::FieldTrialList::Find(kInstantExtendedFieldTrialName);
On 2013/01/31 18:56:52, robertshield wrote:
> On 2013/01/31 15:44:57, MAD wrote:
> > There's also a method on FieldTrialList to get the group name directly. It
> > returns an empty string if the trial isn't active, so provide what you're
> > looking for here in a single call. I guess the ones who wrote/reviewed the
> code
> > you copied from below didn't know about it... :-)
> > 
> > I wonder if there could be a way to extract this common code somehow...
> 
> Used the method you suggest. With the refactoring now done, this code isn't
long
> enough to be worth extracting imo.

Agreed...

https://codereview.chromium.org/12094073/diff/5001/base/process_util_win.cc
File base/process_util_win.cc (right):

https://codereview.chromium.org/12094073/diff/5001/base/process_util_win.cc#n...
base/process_util_win.cc:249: //NOTREACHED();
Really?

https://codereview.chromium.org/12094073/diff/5001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search.cc (right):

https://codereview.chromium.org/12094073/diff/5001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search.cc:183: return false;
Nit: Multi-line conditions calls for {}

https://codereview.chromium.org/12094073/diff/5001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search.h (right):

https://codereview.chromium.org/12094073/diff/5001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search.h:62: // false otherwise.
nit: false otherwise is not needed and sometimes frowned upon.

[robertshield, 2013-01-31 20:24:28]

Thanks!

https://codereview.chromium.org/12094073/diff/5001/base/process_util_win.cc
File base/process_util_win.cc (right):

https://codereview.chromium.org/12094073/diff/5001/base/process_util_win.cc#n...
base/process_util_win.cc:249: //NOTREACHED();
On 2013/01/31 20:14:53, MAD wrote:
> Really?

Heh, oops, local patch until Brett reads
https://codereview.chromium.org/12087104.

Removed

https://codereview.chromium.org/12094073/diff/5001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search.cc (right):

https://codereview.chromium.org/12094073/diff/5001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search.cc:183: return false;
On 2013/01/31 20:14:53, MAD wrote:
> Nit: Multi-line conditions calls for {}

Done.

https://codereview.chromium.org/12094073/diff/5001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search.h (right):

https://codereview.chromium.org/12094073/diff/5001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search.h:62: // false otherwise.
On 2013/01/31 20:14:53, MAD wrote:
> nit: false otherwise is not needed and sometimes frowned upon.

People shouldn't frown, makes for wrinkles. Removed and switched if -> iff to
maintain correctness.

[robertshield, 2013-01-31 20:25:05]

+Scott for owners

[sky, 2013-01-31 21:28:09]

sky->dhollowa

[dhollowa, 2013-01-31 21:47:18]

lgtm

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

https://codereview.chromium.org/12094073/diff/5002/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:81: bool instant_extended_value
= true;
How about using a switch here with no "default:" case?  Then you'll get compiler
support if the IED enum ever changes.

[robertshield, 2013-01-31 22:25:50]

Thanks!

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

https://codereview.chromium.org/12094073/diff/5002/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:81: bool instant_extended_value
= true;
On 2013/01/31 21:47:19, dhollowa wrote:
> How about using a switch here with no "default:" case?  Then you'll get
compiler
> support if the IED enum ever changes.

Done.

[commit-bot: I haz the power, 2013-01-31 22:40:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertshield@chromium.org/12094073/8002

[commit-bot: I haz the power, 2013-01-31 22:40:15]

Presubmit check for 12094073-8002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[robertshield, 2013-01-31 22:42:14]

TBRing sky for change in browser_instant_controller.cc which isn't under
ui/search

On 2013/01/31 22:40:15, I haz the power (commit-bot) wrote:
> Presubmit check for 12094073-8002 failed and returned exit status 1.
> 
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     chrome/browser/ui
> 
> Was the presubmit check useful? Please send feedback & hate mail to
> maruel@chromium.org!

[commit-bot: I haz the power, 2013-01-31 23:02:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertshield@chromium.org/12094073/8002

[commit-bot: I haz the power, 2013-01-31 23:57:39]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-02-01 01:53:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertshield@chromium.org/12094073/8002

[commit-bot: I haz the power, 2013-02-01 05:24:51]

Change committed as 180084