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..a96d957cac4bb14a66458f62d9d718a84a85fc01 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,34 @@ 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, in the

[Peter Kasting, 2013/07/31 20:14:09]

Nit: last "in" -> "given"?  (2 places)

[Mark P, 2013/07/31 21:59:28]

Done.

+  // 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, in 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,
+                           GetConsequencesOfRuleInPageClassificationContext);
+
+  // Returns the "consequences" for the |rule| that applies in the context

[Peter Kasting, 2013/07/31 20:14:09]

Nit: Putting "consequences" in quotes and never defining what it means leaves
the reader still unsure how this function is actually used.  I might hoist most
of the comments from the .cc file to here, since that background is all useful
in the .h file, rather than being low-level implementation detail that's only of
interest in deciphering the implementation.  I'd also probably use
"GetRuleValue" or "GetValueOfRule" or "GetValueForRule" instead of using the
word "consequences" anywhere.

[Mark P, 2013/07/31 21:59:28]

I think with the newly rewritten comment and removing the reference to
consequences things read pretty well.  I don't feel the need to drag the long
formatting of rules comment into the header.

+  // of |current_page_classification|.  If no such rule exists, looks for
+  // that rule in the global context and return it if found.  If the rule
+  // remains unfound in the global context, returns the empty string.  For
+  // more details, see the implementation.
+  static std::string GetConsequencesOfRuleInPageClassificationContext(
+      AutocompleteInput::PageClassification current_page_classification,
+      const std::string& rule);
+
   DISALLOW_IMPLICIT_CONSTRUCTORS(OmniboxFieldTrial);
 };