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

chrome/browser/omnibox/omnibox_field_trial.cc

Index: chrome/browser/omnibox/omnibox_field_trial.cc
diff --git a/chrome/browser/omnibox/omnibox_field_trial.cc b/chrome/browser/omnibox/omnibox_field_trial.cc
index 09c2d1a72638da218e1c89c055ff345124d21b2d..060fde3ed7efd65ef973379312b67635551fec6f 100644
--- a/chrome/browser/omnibox/omnibox_field_trial.cc
+++ b/chrome/browser/omnibox/omnibox_field_trial.cc
@@ -22,7 +22,10 @@ const char kHUPCreateShorterMatchFieldTrialName[] =
     "OmniboxHUPCreateShorterMatch";
 const char kStopTimerFieldTrialName[] = "OmniboxStopTimer";
 const char kShortcutsScoringFieldTrialName[] = "OmniboxShortcutsScoring";
-const char kSearchHistoryFieldTrialName[] = "OmniboxSearchHistory";
+const char kBundledExperimentFieldTrialName[] = "OmniboxBundledExperimentV1";
+
+// Rule names used by the bundled experiment.
+const char kSearchHistoryRule[] = "SearchHistory";
 
 // The autocomplete dynamic field trial name prefix.  Each field trial is
 // configured dynamically and is retrieved automatically by Chrome during
@@ -218,12 +221,52 @@ bool OmniboxFieldTrial::ShortcutsScoringMaxRelevance(int* max_relevance) {
   return true;
 }
 
-bool OmniboxFieldTrial::SearchHistoryPreventInlining() {
-  return (base::FieldTrialList::FindFullName(kSearchHistoryFieldTrialName) ==
-          "PreventInlining");
+bool OmniboxFieldTrial::SearchHistoryPreventInlining(
+    AutocompleteInput::PageClassification current_page_classification) {
+  return OmniboxFieldTrial::GetValueOfRuleInPageClassificationContext(
+      kSearchHistoryRule, current_page_classification) == "PreventInlining";
+}
+
+bool OmniboxFieldTrial::SearchHistoryDisable(
+    AutocompleteInput::PageClassification current_page_classification) {
+  return OmniboxFieldTrial::GetValueOfRuleInPageClassificationContext(
+      kSearchHistoryRule, current_page_classification) == "Disable";
 }
 
-bool OmniboxFieldTrial::SearchHistoryDisable() {
-  return (base::FieldTrialList::FindFullName(kSearchHistoryFieldTrialName) ==
-          "Disable");
+// Background and implementation details:

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

Honestly, I still think this comment, as-is, is clearer than the comment you
currently have in the .h file.  I'd still probably just replace that comment
with this one.

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

Added a new opening paragraph to the .h that imports some of the explanation
found here but not the implementation details (format of key-value pairs).

+//
+// Each experiment group in any field trial can come with an optional set of
+// parameters (key-value pairs).  In the bundled omnibox experiment
+// (kBundledExperimentFieldTrialName), each experiment group comes with a
+// list of parameters in the form:
+//   key=<Rule>:<AutocompleteInput::PageClassification (as an int)>
+//   value=<arbitrary string>
+// The AutocompleteInput::PageClassification can also be "*", which means
+// this rule applies in all contexts.

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

Nit: "in all contexts" -> "for all classifications"

(The theme of my suggested function rename and the comments here and below is
that I'm trying to move away from using the word "context", as I think it adds
verbosity but not clarity.)

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

see other comment about context.

+// One example parameter is
+//   key=SearchHistory:6
+//   value=PreventInlining
+// This means in context 6 (a search result page doing search term replacement),
+// the SearchHistory experiment should PreventInlining.
+//
+// In short, this function tries to find the value associated with key
+// |rule|:|current_page_classification|, failing that it looks up |rule|:*,
+// and failing that it returns the empty string.
+std::string OmniboxFieldTrial::GetValueOfRuleInPageClassificationContext(
+    const std::string& rule,
+    AutocompleteInput::PageClassification current_page_classification) {
+  std::map<std::string, std::string> params;
+  if (!chrome_variations::GetVariationParams(kBundledExperimentFieldTrialName,
+                                             &params)) {
+    return std::string();
+  }
+  // Look up rule in this exact context.

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

Nit: "in this exact context" -> "for the provided classification"

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

see other comment about context.

+  std::map<std::string, std::string>::iterator it =
+      params.find(rule + ":" + base::IntToString(
+          static_cast<int>(current_page_classification)));
+  if (it != params.end())
+    return it->second;
+  // Look up rule in the global context.

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

Nit: Maybe "See if we have a fallback rule for all classifications."

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

see other comment about context.

+  it = params.find(rule + ":*");
+  return (it != params.end()) ? it->second : std::string();
 }