Omnibox: Make the Controller Reorder Matches for Inlining

chrome/browser/autocomplete/autocomplete_result_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "chrome/browser/autocomplete/autocomplete_input.h"
 #include "chrome/browser/autocomplete/autocomplete_match.h"
 #include "chrome/browser/autocomplete/autocomplete_provider.h"
 #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"
 
 class AutocompleteResultTest : public testing::Test  {
  public:
   struct TestData {
     // Used to build a url for the AutocompleteMatch. The URL becomes
     // "http://" + ('a' + |url_id|) (e.g. an ID of 2 yields "http://b").
     int url_id;
 
     // ID of the provider.
     int provider_id;
 
     // Relevance score.
     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)
     TemplateURLPrepopulateData::InitCountryCode(
         std::string() /* unknown country code */);
 #endif
     test_util_.SetUp();
     test_util_.VerifyLoad();
   }
 
@@ skipping 18 matching lines @@
   // Creates an AutocompleteResult from |last| and |current|. The two are
   // merged by |CopyOldMatches| and compared by |AssertResultMatches|.
   void RunCopyOldMatchesTest(const TestData* last, size_t last_size,
                              const TestData* current, size_t current_size,
                              const TestData* expected, size_t expected_size);
 
  protected:
   TemplateURLServiceTestUtil test_util_;
 
  private:
+  scoped_ptr<base::FieldTrialList> field_trial_list_;
+
   DISALLOW_COPY_AND_ASSIGN(AutocompleteResultTest);
 };
 
 // static
 void AutocompleteResultTest::PopulateAutocompleteMatch(
     const TestData& data,
     AutocompleteMatch* match) {
   match->provider = reinterpret_cast<AutocompleteProvider*>(data.provider_id);
   match->fill_into_edit = base::IntToString16(data.url_id);
   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
 void AutocompleteResultTest::PopulateAutocompleteMatches(
     const TestData* data,
     size_t count,
     ACMatches* matches) {
   for (size_t i = 0; i < count; ++i) {
     AutocompleteMatch match;
     PopulateAutocompleteMatch(data[i], &match);
@@ skipping 47 matching lines @@
   AutocompleteResult r2;
 
   // Swap with empty shouldn't do anything interesting.
   r1.Swap(&r2);
   EXPECT_EQ(r1.end(), r1.default_match());
   EXPECT_EQ(r2.end(), r2.default_match());
 
   // 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);
   matches.push_back(match);
   r1.AppendMatches(matches);
   r1.SortAndCull(input, test_util_.profile());
   EXPECT_EQ(r1.begin(), r1.default_match());
   EXPECT_EQ("http://a/", r1.alternate_nav_url().spec());
   r1.Swap(&r2);
   EXPECT_TRUE(r1.empty());
@@ skipping 124 matching lines @@
   EXPECT_EQ("http://www.foo.com/s?q=foo",
             result.match_at(0)->destination_url.spec());
   EXPECT_EQ(1300, result.match_at(0)->relevance);
   EXPECT_EQ("http://www.foo.com/s?q=foo2",
             result.match_at(1)->destination_url.spec());
   EXPECT_EQ(1200, result.match_at(1)->relevance);
   EXPECT_EQ("http://www.foo.com/",
             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());
+  }
+}