Omnibox: Create Disable-Inlining Experiment

Sets up a experiment that, when active, demotes all suggestions with
inline autocompletions so that a suggestion without one (or, more precisely,
a suggestion with an empty inline autocompletion) appears first / within
the omnibox.

BUG=

Review URL: https://codereview.chromium.org/303013002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@273966 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 225 insertions, 2 deletions

diff --git a/chrome/browser/autocomplete/autocomplete_result.cc b/chrome/browser/autocomplete/autocomplete_result.cc
index b4637f6..a65ca2b 100644
--- a/chrome/browser/autocomplete/autocomplete_result.cc
+++ b/chrome/browser/autocomplete/autocomplete_result.cc
@@ -8,6 +8,7 @@
 #include <iterator>
 
 #include "base/logging.h"
+#include "base/metrics/histogram.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/autocomplete/autocomplete_input.h"
 #include "chrome/browser/autocomplete/autocomplete_match.h"
@@ -92,6 +93,18 @@ bool DestinationSort::operator()(const AutocompleteMatch& elem1,
   return elem1.stripped_destination_url < elem2.stripped_destination_url;
 }
 
+// Returns true if |match| is allowed to the default match taking into account
+// whether we're supposed to (and able to) demote all matches with inline
+// autocompletions.
+bool AllowedToBeDefaultMatchAccountingForDisableInliningExperiment(
+    const AutocompleteMatch& match,
+    const bool has_legal_default_match_without_completion) {
+  return match.allowed_to_be_default_match &&
+      (!OmniboxFieldTrial::DisableInlining() ||
+       !has_legal_default_match_without_completion ||
+       match.inline_autocompletion.empty());
+}
+
 };  // namespace
 
 // static
@@ -177,16 +190,37 @@ void AutocompleteResult::SortAndCull(const AutocompleteInput& input,
   DedupMatchesByDestination(input.current_page_classification(), true,
                             &matches_);
 
+  // If the result set has at least one legal default match without an inline
+  // autocompletion, then in the disable inlining experiment it will be okay
+  // to demote all matches with inline autocompletions.  On the other hand, if
+  // the experiment is active but there is no legal match without an inline
+  // autocompletion, then we'll pretend the experiment is not active and not
+  // demote the matches with an inline autocompletion.  In other words, an
+  // alternate name for this variable is
+  // allowed_to_demote_matches_with_inline_autocompletion.
+  bool has_legal_default_match_without_completion = false;
+  for (AutocompleteResult::iterator it = matches_.begin();
+       (it != matches_.end()) && !has_legal_default_match_without_completion;
+       ++it) {
+    if (it->allowed_to_be_default_match && it->inline_autocompletion.empty())
+      has_legal_default_match_without_completion = true;
+  }
+  UMA_HISTOGRAM_BOOLEAN("Omnibox.HasLegalDefaultMatchWithoutCompletion",
+                        has_legal_default_match_without_completion);
+
   // Sort and trim to the most relevant kMaxMatches matches.
   size_t max_num_matches = std::min(kMaxMatches, matches_.size());
   CompareWithDemoteByType comparing_object(input.current_page_classification());
   std::sort(matches_.begin(), matches_.end(), comparing_object);
-  if (!matches_.empty() && !matches_.begin()->allowed_to_be_default_match) {
+  if (!matches_.empty() &&
+      !AllowedToBeDefaultMatchAccountingForDisableInliningExperiment(
+          *matches_.begin(), has_legal_default_match_without_completion)) {
     // Top match is not allowed to be the default match.  Find the most
     // relevant legal match and shift it to the front.
     for (AutocompleteResult::iterator it = matches_.begin() + 1;
          it != matches_.end(); ++it) {
-      if (it->allowed_to_be_default_match) {
+      if (AllowedToBeDefaultMatchAccountingForDisableInliningExperiment(
+              *it, has_legal_default_match_without_completion)) {
         std::rotate(matches_.begin(), it, it + 1);
         break;
       }
diff --git a/chrome/browser/autocomplete/autocomplete_result_unittest.cc b/chrome/browser/autocomplete/autocomplete_result_unittest.cc
index 56a51ad..28ab9a5 100644
--- a/chrome/browser/autocomplete/autocomplete_result_unittest.cc
+++ b/chrome/browser/autocomplete/autocomplete_result_unittest.cc
@@ -557,6 +557,165 @@ TEST_F(AutocompleteResultTest, SortAndCullReorderForDefaultMatch) {
   }
 }
 
+
+
+TEST_F(AutocompleteResultTest, SortAndCullWithDisableInlining) {
+  TestData data[] = {
+    { 0, 0, 1300 },
+    { 1, 0, 1200 },
+    { 2, 0, 1100 },
+    { 3, 0, 1000 }
+  };
+
+  {
+    // Check that with the field trial disabled, we keep keep the first match
+    // first even if it has an inline autocompletion.
+    ACMatches matches;
+    PopulateAutocompleteMatches(data, arraysize(data), &matches);
+    matches[0].inline_autocompletion = base::ASCIIToUTF16("completion");
+    AutocompleteResult result;
+    result.AppendMatches(matches);
+    AutocompleteInput input(base::string16(), base::string16::npos,
+                            base::string16(), GURL(),
+                            AutocompleteInput::HOME_PAGE, false, false, false,
+                            true);
+    result.SortAndCull(input, test_util_.profile());
+    AssertResultMatches(result, data, 4);
+  }
+
+  // Enable the field trial to disable inlining.
+  {
+    std::map<std::string, std::string> params;
+    params[OmniboxFieldTrial::kDisableInliningRule] = "true";
+    ASSERT_TRUE(chrome_variations::AssociateVariationParams(
+        OmniboxFieldTrial::kBundledExperimentFieldTrialName, "D", params));
+  }
+  base::FieldTrialList::CreateFieldTrial(
+      OmniboxFieldTrial::kBundledExperimentFieldTrialName, "D");
+
+  {
+    // Now the first match should be demoted past the second.
+    ACMatches matches;
+    PopulateAutocompleteMatches(data, arraysize(data), &matches);
+    matches[0].inline_autocompletion = base::ASCIIToUTF16("completion");
+    AutocompleteResult result;
+    result.AppendMatches(matches);
+    AutocompleteInput input(base::string16(), base::string16::npos,
+                            base::string16(), GURL(),
+                            AutocompleteInput::HOME_PAGE, false, false, false,
+                            true);
+    result.SortAndCull(input, test_util_.profile());
+    ASSERT_EQ(4U, result.size());
+    EXPECT_EQ("http://b/", result.match_at(0)->destination_url.spec());
+    EXPECT_EQ("http://a/", result.match_at(1)->destination_url.spec());
+    EXPECT_EQ("http://c/", result.match_at(2)->destination_url.spec());
+    EXPECT_EQ("http://d/", result.match_at(3)->destination_url.spec());
+  }
+
+  {
+    // But if there was no inline autocompletion on the first match, then
+    // the order should stay the same.  This is true even if there are
+    // inline autocompletions elsewhere.
+    ACMatches matches;
+    PopulateAutocompleteMatches(data, arraysize(data), &matches);
+    matches[2].inline_autocompletion = base::ASCIIToUTF16("completion");
+    AutocompleteResult result;
+    result.AppendMatches(matches);
+    AutocompleteInput input(base::string16(), base::string16::npos,
+                            base::string16(), GURL(),
+                            AutocompleteInput::HOME_PAGE, false, false, false,
+                            true);
+    result.SortAndCull(input, test_util_.profile());
+    AssertResultMatches(result, data, 4);
+  }
+
+  {
+    // Try a more complicated situation.
+    ACMatches matches;
+    PopulateAutocompleteMatches(data, arraysize(data), &matches);
+    matches[0].allowed_to_be_default_match = false;
+    matches[1].inline_autocompletion = base::ASCIIToUTF16("completion");
+    AutocompleteResult result;
+    result.AppendMatches(matches);
+    AutocompleteInput input(base::string16(), base::string16::npos,
+                            base::string16(), GURL(),
+                            AutocompleteInput::HOME_PAGE, false, false, false,
+                            true);
+    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());
+  }
+
+  {
+    // Try another complicated situation.
+    ACMatches matches;
+    PopulateAutocompleteMatches(data, arraysize(data), &matches);
+    matches[0].inline_autocompletion = base::ASCIIToUTF16("completion");
+    matches[1].allowed_to_be_default_match = false;
+    AutocompleteResult result;
+    result.AppendMatches(matches);
+    AutocompleteInput input(base::string16(), base::string16::npos,
+                            base::string16(), GURL(),
+                            AutocompleteInput::HOME_PAGE, false, false, false,
+                            true);
+    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());
+  }
+
+  {
+    // Check that disaster doesn't strike if we can't demote the top inline
+    // autocompletion because every match either has a completion or isn't
+    // allowed to be the default match.  In this case, we should leave
+    // everything untouched.
+    ACMatches matches;
+    PopulateAutocompleteMatches(data, arraysize(data), &matches);
+    matches[0].inline_autocompletion = base::ASCIIToUTF16("completion");
+    matches[1].allowed_to_be_default_match = false;
+    matches[2].allowed_to_be_default_match = false;
+    matches[3].inline_autocompletion = base::ASCIIToUTF16("completion");
+    AutocompleteResult result;
+    result.AppendMatches(matches);
+    AutocompleteInput input(base::string16(), base::string16::npos,
+                            base::string16(), GURL(),
+                            AutocompleteInput::HOME_PAGE, false, false, false,
+                            true);
+    result.SortAndCull(input, test_util_.profile());
+    AssertResultMatches(result, data, 4);
+  }
+
+  {
+    // Check a similar situation, except in this case the top match is not
+    // allowed to the default match, so it still needs to be demoted so we
+    // get a legal default match first.  That match will have an inline
+    // autocompletion because we don't have any better options.
+    ACMatches matches;
+    PopulateAutocompleteMatches(data, arraysize(data), &matches);
+    matches[0].allowed_to_be_default_match = false;
+    matches[1].inline_autocompletion = base::ASCIIToUTF16("completion");
+    matches[2].allowed_to_be_default_match = false;
+    matches[3].inline_autocompletion = base::ASCIIToUTF16("completion");
+    AutocompleteResult result;
+    result.AppendMatches(matches);
+    AutocompleteInput input(base::string16(), base::string16::npos,
+                            base::string16(), GURL(),
+                            AutocompleteInput::HOME_PAGE, false, false, false,
+                            true);
+    result.SortAndCull(input, test_util_.profile());
+    ASSERT_EQ(4U, result.size());
+    EXPECT_EQ("http://b/", result.match_at(0)->destination_url.spec());
+    EXPECT_EQ("http://a/", result.match_at(1)->destination_url.spec());
+    EXPECT_EQ("http://c/", result.match_at(2)->destination_url.spec());
+    EXPECT_EQ("http://d/", result.match_at(3)->destination_url.spec());
+  }
+}
+
 TEST_F(AutocompleteResultTest, ShouldHideTopMatch) {
   base::FieldTrialList::CreateFieldTrial("InstantExtended",
                                          "Group1 hide_verbatim:1");
diff --git a/chrome/browser/omnibox/omnibox_field_trial.cc b/chrome/browser/omnibox/omnibox_field_trial.cc
index 0512978..ecffd9f 100644
--- a/chrome/browser/omnibox/omnibox_field_trial.cc
+++ b/chrome/browser/omnibox/omnibox_field_trial.cc
@@ -425,6 +425,12 @@ bool OmniboxFieldTrial::BookmarksIndexURLsValue() {
       kBookmarksIndexURLsRule) == "true";
 }
 
+bool OmniboxFieldTrial::DisableInlining() {
+  return chrome_variations::GetVariationParamValue(
+      kBundledExperimentFieldTrialName,
+      kDisableInliningRule) == "true";
+}
+
 const char OmniboxFieldTrial::kBundledExperimentFieldTrialName[] =
     "OmniboxBundledExperimentV1";
 const char OmniboxFieldTrial::kShortcutsScoringMaxRelevanceRule[] =
@@ -439,6 +445,7 @@ const char OmniboxFieldTrial::kHQPAllowMatchInSchemeRule[] =
 const char OmniboxFieldTrial::kZeroSuggestRule[] = "ZeroSuggest";
 const char OmniboxFieldTrial::kZeroSuggestVariantRule[] = "ZeroSuggestVariant";
 const char OmniboxFieldTrial::kBookmarksIndexURLsRule[] = "BookmarksIndexURLs";
+const char OmniboxFieldTrial::kDisableInliningRule[] = "DisableInlining";
 
 const char OmniboxFieldTrial::kHUPNewScoringEnabledParam[] =
     "HUPExperimentalScoringEnabled";
diff --git a/chrome/browser/omnibox/omnibox_field_trial.h b/chrome/browser/omnibox/omnibox_field_trial.h
index 95ed7d6..df79dd5 100644
--- a/chrome/browser/omnibox/omnibox_field_trial.h
+++ b/chrome/browser/omnibox/omnibox_field_trial.h
@@ -270,6 +270,18 @@ class OmniboxFieldTrial {
   static bool BookmarksIndexURLsValue();
 
   // ---------------------------------------------------------
+  // For the DisableInlining experiment that's part of the bundled omnibox
+  // field trial.
+
+  // Returns true if AutocompleteResult should prevent any suggestion with
+  // a non-empty |inline_autocomplete| from being the default match.  In
+  // other words, prevent an inline autocompletion from appearing as the
+  // top suggestion / within the omnibox itself, reordering matches as
+  // necessary to make this true.  Returns false if the experiment isn't
+  // active.
+  static bool DisableInlining();
+
+  // ---------------------------------------------------------
   // Exposed publicly for the sake of unittests.
   static const char kBundledExperimentFieldTrialName[];
   // Rule names used by the bundled experiment.
@@ -283,6 +295,7 @@ class OmniboxFieldTrial {
   static const char kZeroSuggestRule[];
   static const char kZeroSuggestVariantRule[];
   static const char kBookmarksIndexURLsRule[];
+  static const char kDisableInliningRule[];
 
   // Parameter names used by the HUP new scoring experiments.
   static const char kHUPNewScoringEnabledParam[];
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index 79d08d0..02b5ade 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -17357,6 +17357,16 @@ Therefore, the affected-histogram name has to have at least one dot in it.
   </summary>
 </histogram>
 
+<histogram name="Omnibox.HasLegalDefaultMatchWithoutCompletion" enum="Boolean">
+  <owner>mpearson@chromium.org</owner>
+  <summary>
+    Whether there was at least one legal default match without an
+    |inline_autocompletion|.  Recorded every time
+    AutocompleteResult::SortAndCull() is called, which could happen multiple
+    times on each keystroke.
+  </summary>
+</histogram>
+
 <histogram name="Omnibox.Paste" units="count">
   <owner>mpearson@chromium.org</owner>
   <summary>