[AiS] Command-line flag and field trial changes.

[AiS] Command-line flag and field trial changes.

- Finch field trial parameter
- Command-line flag to force the feature off
- OmniboxFieldTrial::EnableAnswersInSuggest(), which considers command-line flags and field trial settings.

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

[Mark P, 2014-06-19 23:50:21]

Do you want to revise your about:flags to be a three-valued flags (enable,
default, disable) to use expose your new flag?

Otherwise, this changelist looks generally fine.

Note that I'm not an approver for some of the directories you're touching.

--mark

https://codereview.chromium.org/348433009/diff/1/chrome/browser/omnibox/omnib...
File chrome/browser/omnibox/omnibox_field_trial.cc (right):

https://codereview.chromium.org/348433009/diff/1/chrome/browser/omnibox/omnib...
chrome/browser/omnibox/omnibox_field_trial.cc:455: const char
OmniboxFieldTrial::kAnswersInSuggestRule[] = "AnswersInSuggest";
I admit the above list in the .h / .cc has no sense of order.  (I think its
partially alphabetic, partially people append when they add.)  Regardless,
please keep the order of the .h and the .cc consistent.

https://codereview.chromium.org/348433009/diff/1/chrome/browser/omnibox/omnib...
File chrome/browser/omnibox/omnibox_field_trial.h (right):

https://codereview.chromium.org/348433009/diff/1/chrome/browser/omnibox/omnib...
chrome/browser/omnibox/omnibox_field_trial.h:288: // Returns true if the
AnswersInSuggest feature should be enabled where
nit: "where" -> ", causing"
nit (later): "are provided" -> "to be provided"

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

https://codereview.chromium.org/348433009/diff/1/chrome/browser/search/search...
chrome/browser/search/search.cc:622: bool IsAnswersInSuggestEnabled() {
This  code should not go here.  search.cc is basically entirely about the
EmbeddedSearch field trial.  Put this in omnibox_field_trial.

[Justin Donnelly, 2014-06-20 00:39:21]

I'm making the change to about_flags in a separate change since this change
doesn't depend on that.  But let me know if you feel like they should go
together.

https://codereview.chromium.org/348433009/diff/1/chrome/browser/omnibox/omnib...
File chrome/browser/omnibox/omnibox_field_trial.cc (right):

https://codereview.chromium.org/348433009/diff/1/chrome/browser/omnibox/omnib...
chrome/browser/omnibox/omnibox_field_trial.cc:455: const char
OmniboxFieldTrial::kAnswersInSuggestRule[] = "AnswersInSuggest";
On 2014/06/19 23:50:21, Mark P wrote:
> I admit the above list in the .h / .cc has no sense of order.  (I think its
> partially alphabetic, partially people append when they add.)  Regardless,
> please keep the order of the .h and the .cc consistent.

Oops, sorry about that.  I thought I had this right.  Fixed now.

https://codereview.chromium.org/348433009/diff/1/chrome/browser/omnibox/omnib...
File chrome/browser/omnibox/omnibox_field_trial.h (right):

https://codereview.chromium.org/348433009/diff/1/chrome/browser/omnibox/omnib...
chrome/browser/omnibox/omnibox_field_trial.h:288: // Returns true if the
AnswersInSuggest feature should be enabled where
On 2014/06/19 23:50:21, Mark P wrote:
> nit: "where" -> ", causing"
> nit (later): "are provided" -> "to be provided"

Done.

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

https://codereview.chromium.org/348433009/diff/1/chrome/browser/search/search...
chrome/browser/search/search.cc:622: bool IsAnswersInSuggestEnabled() {
On 2014/06/19 23:50:21, Mark P wrote:
> This  code should not go here.  search.cc is basically entirely about the
> EmbeddedSearch field trial.  Put this in omnibox_field_trial.

Done.

[Mark P, 2014-06-20 00:47:18]

lgtm

[Justin Donnelly, 2014-06-20 00:53:38]

Peter, can you review the changes in c/b/search_engines?

[Peter Kasting, 2014-06-20 00:56:50]

LGTM

https://codereview.chromium.org/348433009/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/ui_thread_search_terms_data.cc (right):

https://codereview.chromium.org/348433009/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/ui_thread_search_terms_data.cc:122: }
Nit: While here: Please fix the violation of the "no else after return" Chromium
style rule -- I suggest the following:

    return OmniboxFieldTrial::EnableAnswersInSuggest() ?
        "chrome-mobile-ext-ansg" : "chrome-mobile-ext";

[Justin Donnelly, 2014-06-20 01:36:42]

https://codereview.chromium.org/348433009/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/ui_thread_search_terms_data.cc (right):

https://codereview.chromium.org/348433009/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/ui_thread_search_terms_data.cc:122: }
On 2014/06/20 00:56:50, Peter Kasting wrote:
> Nit: While here: Please fix the violation of the "no else after return"
Chromium
> style rule -- I suggest the following:
> 
>     return OmniboxFieldTrial::EnableAnswersInSuggest() ?
>         "chrome-mobile-ext-ansg" : "chrome-mobile-ext";

Done.

[Justin Donnelly, 2014-06-20 01:36:56]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2014-06-20 01:40:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdonnelly@chromium.org/348433009/40001

[Justin Donnelly, 2014-06-20 02:13:01]

Fixed failure to apply patch.

[Justin Donnelly, 2014-06-20 02:13:05]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2014-06-20 02:14:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdonnelly@chromium.org/348433009/80001

[commit-bot: I haz the power, 2014-06-20 10:27:58]

Change committed as 278664