Omnibox: Make the Controller Reorder Matches for Inlining

chrome/browser/autocomplete/autocomplete_result_unittest.cc

Index: chrome/browser/autocomplete/autocomplete_result_unittest.cc
diff --git a/chrome/browser/autocomplete/autocomplete_result_unittest.cc b/chrome/browser/autocomplete/autocomplete_result_unittest.cc
index 3038c27a017dd8d6ca05996518140d6486edf7cd..f1b3935057bf368798769a7bfbb5ebb442853003 100644
--- a/chrome/browser/autocomplete/autocomplete_result_unittest.cc
+++ b/chrome/browser/autocomplete/autocomplete_result_unittest.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/autocomplete/autocomplete_result.h"
 
+#include "base/memory/scoped_ptr.h"
+#include "base/metrics/field_trial.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -13,6 +15,8 @@
 #include "chrome/browser/search_engines/template_url_prepopulate_data.h"
 #include "chrome/browser/search_engines/template_url_service.h"
 #include "chrome/browser/search_engines/template_url_service_test_util.h"
+#include "chrome/common/metrics/entropy_provider.h"
+#include "chrome/common/metrics/variations/variations_util.h"
 #include "chrome/test/base/testing_profile.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -30,7 +34,13 @@ class AutocompleteResultTest : public testing::Test  {
     int relevance;
   };
 
-  AutocompleteResultTest() {}
+  AutocompleteResultTest() {
+    // Destroy the existing FieldTrialList before creating a new one to avoid
+    // a DCHECK.
+    field_trial_list_.reset();
+    field_trial_list_.reset(new base::FieldTrialList(
+        new metrics::SHA1EntropyProvider("foo")));
+  }
 
   virtual void SetUp() OVERRIDE {
 #if defined(OS_ANDROID)
@@ -69,6 +79,8 @@ class AutocompleteResultTest : public testing::Test  {
   TemplateURLServiceTestUtil test_util_;
 
  private:
+  scoped_ptr<base::FieldTrialList> field_trial_list_;
+
   DISALLOW_COPY_AND_ASSIGN(AutocompleteResultTest);
 };
 
@@ -81,6 +93,7 @@ void AutocompleteResultTest::PopulateAutocompleteMatch(
   std::string url_id(1, data.url_id + 'a');
   match->destination_url = GURL("http://" + url_id);
   match->relevance = data.relevance;
+  match->allowed_to_be_default_match = true;
 }
 
 // static
@@ -148,6 +161,7 @@ TEST_F(AutocompleteResultTest, Swap) {
   // Swap with a single match.
   ACMatches matches;
   AutocompleteMatch match;
+  match.allowed_to_be_default_match = true;
   AutocompleteInput input(ASCIIToUTF16("a"), string16::npos, string16(), GURL(),
                           AutocompleteInput::INVALID_SPEC, false, false, false,
                           AutocompleteInput::ALL_MATCHES);
@@ -292,3 +306,62 @@ TEST_F(AutocompleteResultTest, SortAndCullDuplicateSearchURLs) {
             result.match_at(2)->destination_url.spec());
   EXPECT_EQ(900, result.match_at(2)->relevance);
 }
+
+TEST_F(AutocompleteResultTest, SortAndCullReorderForDefaultMatch) {
+  TestData data[] = {
+    { 0, 0, 1300 },
+    { 1, 0, 1200 },
+    { 2, 0, 1100 },
+    { 3, 0, 1000 }
+  };
+
+  // Must be the same as kBundledExperimentFieldTrialName

[msw, 2013/08/07 20:09:21]

Is this duplication of constants common/necessary? This file's field trial code
is pretty cryptic and beyond me. Couldn't AutocompleteResult have a simple
boolean member |reorder_for_legal_default_match| or similar, generally set by
some field trial code, but also modifiable for testing without hacking up some
fake field trial settings?

[Mark P, 2013/08/07 22:13:05]

Alexei asked a similar question on a different code review.  Here was
my answer:
He said "Fair enough, SGTM."

I'm willing to back off if pushed, but that was my reasoning.
|reorder_for_legal_default_match| changes depending on the input so having
it as a modifiable boolean member seems difficult because the tests will have
to inject the right setting at the right time.  I guess we could have a function
in this class that overrides its value (permanently) for testing.

I actually like this rule-setting code because it tests exactly how the code
is used: we have to put rules in exactly this cryptic form in the field trial
config file.

[Peter Kasting, 2013/08/07 22:20:01]

FWIW, I also asked about this kind of thing.

I don't think it's problematic to declare these in a .h and atop them put a
comment like "// Exposed publicly for the sake of unittests."  That makes it
clear that they're not particularly critical to understanding the implementation
unless you want to know how tests interact with the code.  I think this would be
an overall improvement.

+  // defined in omnibox_field_trial.cc.
+  const std::string kTrialName = "OmniboxBundledExperimentV1";
+  // Must be the same as kReorderForLegalDefaultMatchRule defined in
+  // omnibox_field_trial.cc.
+  const std::string kRuleName = "ReorderForLegalDefaultMatch";
+  // Must be the same as the constant used in
+  // OmniboxFieldTrial::ReorderForLegalDefaultMatch().
+  const std::string kReorderEnabled = "ReorderForLegalDefaultMatch";
+  std::map<std::string, std::string> params;
+  // Enable reorder for omnibox inputs on the user's homepage.
+  params[kRuleName + ":3"] = kReorderEnabled;
+  ASSERT_TRUE(chrome_variations::AssociateVariationParams(
+      kTrialName, "A", params));
+  base::FieldTrialList::CreateFieldTrial(kTrialName, "A");
+
+  {
+    // Check that reorder doesn't do anything if the top result
+    // is already a legal default match (which is the default from
+    // PopulateAutocompleteMatches()).
+    ACMatches matches;
+    PopulateAutocompleteMatches(data, arraysize(data), &matches);
+    AutocompleteResult result;
+    result.AppendMatches(matches);
+    AutocompleteInput input(string16(), string16::npos, string16(), GURL(),
+                            AutocompleteInput::HOMEPAGE, false, false, false,
+                            AutocompleteInput::ALL_MATCHES);
+    result.SortAndCull(input, test_util_.profile());
+    AssertResultMatches(result, data, 4);
+  }
+
+  {
+    // Check that reorder swaps up a result appropriately.
+    ACMatches matches;
+    PopulateAutocompleteMatches(data, arraysize(data), &matches);
+    matches[0].allowed_to_be_default_match = false;
+    matches[1].allowed_to_be_default_match = false;
+    AutocompleteResult result;
+    result.AppendMatches(matches);
+    AutocompleteInput input(string16(), string16::npos, string16(), GURL(),
+                            AutocompleteInput::HOMEPAGE, false, false, false,
+                            AutocompleteInput::ALL_MATCHES);
+    result.SortAndCull(input, test_util_.profile());
+    ASSERT_EQ(4U, result.size());
+    EXPECT_EQ("http://c/", result.match_at(0)->destination_url.spec());
+    EXPECT_EQ("http://a/", result.match_at(1)->destination_url.spec());
+    EXPECT_EQ("http://b/", result.match_at(2)->destination_url.spec());
+    EXPECT_EQ("http://d/", result.match_at(3)->destination_url.spec());
+  }
+}