Omnibox: Allow Bundled Omnibox Field Trial to Examine Instant Extended

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 2bec78bc0ccf477943baf275e0d459b0f0edaa24..4506bfc301ee440f97f340311740e0067e715653 100644
--- a/chrome/browser/omnibox/omnibox_field_trial.cc
+++ b/chrome/browser/omnibox/omnibox_field_trial.cc
@@ -11,6 +11,7 @@
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "chrome/browser/search/search.h"
 #include "chrome/common/metrics/metrics_util.h"
 #include "chrome/common/metrics/variations/variation_ids.h"
 #include "chrome/common/metrics/variations/variations_util.h"
@@ -270,19 +271,29 @@ void OmniboxFieldTrial::GetDemotionsByType(
 // 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)>
+//   key=<Rule>:<AutocompleteInput::PageClassification (as an int)>:<whether

[Peter Kasting, 2013/08/09 20:45:45]

Nit: Consider breaking after each colon for readability (and maybe noting there
are no linebreaks in the real rules)

[Mark P, 2013/08/09 21:55:01]

Nice idea: it makes the elements more readable and makes it easier to add them
in the future.

+//       InstantExtended is enabled (as a 1 or 0)>
 //   value=<arbitrary string>
-// The AutocompleteInput::PageClassification can also be "*", which means
-// this rule applies in all page classification contexts.
+// Both the AutocompleteInput::PageClassification and the InstantExtended
+// entires can be "*", which means this rule applies regardless of relevant
+// context.

[Peter Kasting, 2013/08/09 20:45:45]

Nit: regardless of relevant context -> for all values of the matching portion of
the context?

[Mark P, 2013/08/09 21:55:01]

Sure.  I spent some time trying to choose these words and ended up with
"regardless".  Your phrasing is better.  Replaced.

 // One example parameter is
-//   key=SearchHistory:6
+//   key=SearchHistory:6:1
 //   value=PreventInlining
 // This means in page classification context 6 (a search result page doing
-// search term replacement), the SearchHistory experiment should
-// PreventInlining.
+// search term replacement) with InstantExtended enabled, the SearchHistory
+// experiment should PreventInlining.
+//
+// When an exact match to the rule in the current context is missing, we
+// give preference to a wildcard rule that matches the instant extended
+// context over a wildcard rule that matches the page classification
+// context.  Hopefully, though, users will write their field trial configs
+// so as not to rely on this backoff order.
 //
 // In short, this function tries to find the value associated with key
-// |rule|:|page_classification|, failing that it looks up |rule|:*,
+// |rule|:|page_classification|:|instant_extended|, failing that it looks up
+// |rule|:*:|instant_extended|, failing that it looks up
+// |rule|:|page_classification|:*, failing that it looks up |rule|:*:*,
 // and failing that it returns the empty string.
 std::string OmniboxFieldTrial::GetValueForRuleInContext(
     const std::string& rule,
@@ -292,13 +303,24 @@ std::string OmniboxFieldTrial::GetValueForRuleInContext(
                                              &params)) {
     return std::string();
   }
+  const std::string page_classification_str =
+      base::IntToString(static_cast<int>(page_classification));
+  const std::string instant_extended =
+      chrome::IsInstantExtendedAPIEnabled() ? "1" : "0";
   // Look up rule in this exact context.
-  std::map<std::string, std::string>::iterator it =
-      params.find(rule + ":" + base::IntToString(
-          static_cast<int>(page_classification)));
+  std::map<std::string, std::string>::iterator it = params.find(
+      rule + ":" + page_classification_str + ":" + instant_extended);
+  if (it != params.end())
+    return it->second;
+  // Backoff to the global page classification context.

[Peter Kasting, 2013/08/09 20:45:45]

Nit: Backoff -> Fall back (2 places)

[Mark P, 2013/08/09 21:55:01]

Done.

+  it = params.find(rule + ":*:" + instant_extended);
+  if (it != params.end())
+    return it->second;
+  // Backoff to the global instant extended context.
+  it = params.find(rule + ":" + page_classification_str + ":*");
   if (it != params.end())
     return it->second;
   // Look up rule in the global context.
-  it = params.find(rule + ":*");
+  it = params.find(rule + ":*:*");
   return (it != params.end()) ? it->second : std::string();
 }