Omnibox: Create Bundled Field Trial; Convert SearchHistory trial to it

chrome/browser/omnibox/omnibox_field_trial.h

Index: chrome/browser/omnibox/omnibox_field_trial.h
diff --git a/chrome/browser/omnibox/omnibox_field_trial.h b/chrome/browser/omnibox/omnibox_field_trial.h
index d555a9fcfba920f0b34a820936c126dff120f10f..1ea6ed876653f489a23111bbd110f6d8a54acbd1 100644
--- a/chrome/browser/omnibox/omnibox_field_trial.h
+++ b/chrome/browser/omnibox/omnibox_field_trial.h
@@ -9,6 +9,8 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/gtest_prod_util.h"
+#include "chrome/browser/autocomplete/autocomplete_input.h"
 
 // This class manages the Omnibox field trials.
 class OmniboxFieldTrial {
@@ -101,18 +103,35 @@ class OmniboxFieldTrial {
   static bool ShortcutsScoringMaxRelevance(int* max_relevance);
 
   // ---------------------------------------------------------
-  // For the SearchHistory field trial.
+  // For the SearchHistory experiment that's part of the bundled omnibox
+  // field trial.
 
-  // Returns true if the user is in the experiment group that scores
-  // search history query suggestions less aggressively so that they don't
-  // inline.
-  static bool SearchHistoryPreventInlining();
+  // Returns true if the user is in the experiment group that, given the
+  // provided |current_page_classification| context, scores search history
+  // query suggestions less aggressively so that they don't inline.
+  static bool SearchHistoryPreventInlining(
+      AutocompleteInput::PageClassification current_page_classification);
 
-  // Returns true if the user is in the experiment group that disables
-  // all query suggestions from search history.
-  static bool SearchHistoryDisable();
+  // Returns true if the user is in the experiment group that, given the
+  // provided |current_page_classification| context, disables all query
+  // suggestions from search history.
+  static bool SearchHistoryDisable(
+      AutocompleteInput::PageClassification current_page_classification);
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(OmniboxFieldTrialTest,
+                           GetValueOfRuleInPageClassificationContext);
+
+  // Returns the value associated with the |rule| that applies in the context
+  // of |current_page_classification|.  If no such rule exists, looks for that
+  // rule in the global context and return its value if found.  If the rule
+  // remains unfound in the global context, returns the empty string.  For
+  // more details, see the implementation.  How to interpret the value is
+  // left to the caller; this is rule-dependent.
+  static std::string GetValueOfRuleInPageClassificationContext(

[Peter Kasting, 2013/08/01 22:44:48]

Nit: Slightly shorter might be

GetValueForRuleWithPageClassification()

or

GetValueOfRuleForPageClassification()

[Mark P, 2013/08/02 00:44:36]

They are shorter.  I think the word context definitely adds something here and
I'd rather not drop it.  Left as is.

Addendum after reading your other context comments: I think context is an
important notion.  For instance, I can easily see adding an input_type context
as well.  I can easily imagine contexts overlapping and having priorities
between them.  I think it really helps to think of the context as a bigger
notion.  Indeed, I'd rather drop PageClassification from this function name
rather than Context.  (the former can be inferred from the parameter types).

[Peter Kasting, 2013/08/02 00:49:33]

I partly react to this with "then update the name and comments to reflect that
larger context at the time you actually add such a larger context; but for now
you don't have it".  But if you're really committed to this, then I think you
should go ahead and drop "PageClassification" right now, and explicitly note in
your comments that while you only have one piece of context at the moment,
you're planning to add more.

But if you're not planning to add more and that comment is purely hypothetical,
then as I said, I would drop the notion of context for now, because as a reader
it's really confusing.

[Mark P, 2013/08/02 19:50:44]

Next week I'm going to add another piece of context (whether Instant Extended is
enabled), so I revised the comment and function name to put less emphasis on
page classification.

+      const std::string& rule,
+      AutocompleteInput::PageClassification current_page_classification);

[Peter Kasting, 2013/08/01 22:44:48]

Nit: "current" probably not necessary (this function doesn't really care if the
provided classification is in any sense "current")

[Mark P, 2013/08/02 00:44:36]

Good point.  Revised.

+
   DISALLOW_IMPLICIT_CONSTRUCTORS(OmniboxFieldTrial);
 };