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

chrome/browser/omnibox/omnibox_field_trial_unittest.cc

Index: chrome/browser/omnibox/omnibox_field_trial_unittest.cc
diff --git a/chrome/browser/omnibox/omnibox_field_trial_unittest.cc b/chrome/browser/omnibox/omnibox_field_trial_unittest.cc
index b40be13d9daaf55a1113e9a5c48a5d698b48c121..367346efc0081a3dc4eadff46c27aa39ef2dca59 100644
--- a/chrome/browser/omnibox/omnibox_field_trial_unittest.cc
+++ b/chrome/browser/omnibox/omnibox_field_trial_unittest.cc
@@ -8,6 +8,7 @@
 #include "base/metrics/field_trial.h"
 #include "base/strings/string16.h"
 #include "chrome/common/metrics/entropy_provider.h"
+#include "chrome/common/metrics/variations/variations_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 class OmniboxFieldTrialTest : public testing::Test {
@@ -129,3 +130,52 @@ TEST_F(OmniboxFieldTrialTest, ZeroSuggestFieldTrial) {
     EXPECT_TRUE(OmniboxFieldTrial::InZeroSuggestFieldTrial());
   }
 }
+
+TEST_F(OmniboxFieldTrialTest, GetValueOfRuleInPageClassificationContext) {
+  // Must be the same as kBundledExperimentFieldTrialName
+  // defined in omnibox_field_trial.cc.
+  const std::string kTrialName = "OmniboxBundledExperimentV1";
+  {
+    std::map<std::string, std::string> params;
+    params["rule1:1"] = "rule1-1-value";

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

Nit: Might want EOL comments like "// NEW_TAB_PAGE" to clarify what "1"/"3"/"4"
correspond to

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

Done.

+    params["rule1:3"] = "rule1-3-value";
+    params["rule1:*"] = "rule1-*-value";
+    params["rule2:*"] = "rule2-*-value";
+    params["rule3:4"] = "rule3-4-value";
+    params["unrecognized"] = "unrecognized-value";
+    ASSERT_TRUE(chrome_variations::AssociateVariationParams(
+        kTrialName, "A", params));
+  }
+
+  base::FieldTrialList::CreateFieldTrial(kTrialName, "A");
+  EXPECT_EQ("rule1-1-value",

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

Tiny nit: All lines of args at the same nesting level should be aligned (so,
you'd probably need to put this alone on a subsequent, indented-4 line).

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

Done.

+      OmniboxFieldTrial::GetValueOfRuleInPageClassificationContext(
+          "rule1", AutocompleteInput::NEW_TAB_PAGE));  // exact match

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

Nit: Rather than these EOL comments, how about block comments like:

  // Test exact matches for particular rule/classification pairs.
  EXPECT...
  ...

  // Test mismatches for rules with global "wildcard" fallbacks.
  ...

  // Test mismatches for rules without global fallbacks.
  ...

  // Test nonexistent rules.
  ...

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

I think this test is more easily understood by looking at each set of cases
about a particular rule.  That's the current order.  Otherwise, for instance, to
examine all tests in each of your particular categories, one would have to keep
all rules in one's head.  This way one only has to keep one set of rules in
one's head at once.

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

OK, then break the test into blocks that have to deal with each rule, and put
comments like "// Tests for rule 1." or something atop each block.

I'm not as sold as you on this ordering, but at least then it's not an
intimidating wall of verbiage that all looks the same, which is how the function
reads today.

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

Done.

+  EXPECT_EQ("rule1-*-value",
+      OmniboxFieldTrial::GetValueOfRuleInPageClassificationContext(
+          "rule1", AutocompleteInput::BLANK));  // fallback to global
+  EXPECT_EQ("rule1-3-value",
+      OmniboxFieldTrial::GetValueOfRuleInPageClassificationContext(
+          "rule1", AutocompleteInput::HOMEPAGE));  // exact match
+  EXPECT_EQ("rule1-*-value",
+      OmniboxFieldTrial::GetValueOfRuleInPageClassificationContext(
+          "rule1", AutocompleteInput::OTHER));  // fallback to global
+  EXPECT_EQ("rule2-*-value",
+      OmniboxFieldTrial::GetValueOfRuleInPageClassificationContext(
+          "rule2", AutocompleteInput::HOMEPAGE));  // fallback to global
+  EXPECT_EQ("rule2-*-value",
+      OmniboxFieldTrial::GetValueOfRuleInPageClassificationContext(
+          "rule2", AutocompleteInput::OTHER));  // fallback to global
+  EXPECT_EQ("",
+      OmniboxFieldTrial::GetValueOfRuleInPageClassificationContext(
+          "rule3", AutocompleteInput::BLANK));  // no global
+  EXPECT_EQ("",
+      OmniboxFieldTrial::GetValueOfRuleInPageClassificationContext(
+          "rule3", AutocompleteInput::HOMEPAGE));  // no global
+  EXPECT_EQ("rule3-4-value",
+      OmniboxFieldTrial::GetValueOfRuleInPageClassificationContext(
+          "rule3", AutocompleteInput::OTHER));  // exact match
+  EXPECT_EQ("",
+      OmniboxFieldTrial::GetValueOfRuleInPageClassificationContext(
+          "rule4", AutocompleteInput::OTHER));  // no rule at all
+}