Omnibox: Allow HistoryQuickProvider to Reorder Results for Inlining

Currently, if the best HistoryQuickProvider suggestion isn't inlineable, all HistoryQuickProvider suggestions get demoted to non-inlineable scores.  This keeps them in relevance order.

This change adds a parameter to alter this behavior.  The new alternative behavior reorders the HistoryQuickProvider's suggestions so an inlineable one appears first (if possible), and only demotes if necessary.

This parameter is enabled by a command line flag.

I've also made it visible in about:flags.

This change is not yet ready for a field trial.  (I think a field trial would be negative until we tune scoring more.)

BUG=
TEST=


Review URL: https://chromiumcodereview.appspot.com/10532186

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

6 files changed, 97 insertions, 18 deletions

diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd
index fd68740..0427856 100644
--- a/chrome/app/generated_resources.grd
+++ b/chrome/app/generated_resources.grd
@@ -5517,6 +5517,21 @@ Keep your key file in a safe place. You will need it to create new versions of y
       <message name="IDS_FLAGS_OMNIBOX_HISTORY_QUICK_PROVIDER_NEW_SCORING_DISABLED" desc="Option name to use old scoring function in HistoryQuickProvider">
         Old Scoring
       </message>
+      <message name="IDS_FLAGS_OMNIBOX_HISTORY_QUICK_PROVIDER_REORDER_FOR_INLINING_NAME" desc="Title for the flag for omnibox to reorder suggestions in HistoryQuickProvider to make an inlineable appear first">
+        Reorder results for inlining in HistoryQuickProvider
+      </message>
+      <message name="IDS_FLAGS_OMNIBOX_HISTORY_QUICK_PROVIDER_REORDER_FOR_INLINING_DESCRIPTION" desc="Description for the flag for omnibox to reorder suggestions in HistoryQuickProvider to make an inlineable appear first">
+        In omnibox autocomplete, reorder suggestions in HistoryQuickProvider to make an inlineable one appear first.
+      </message>
+      <message name="IDS_FLAGS_OMNIBOX_HISTORY_QUICK_PROVIDER_REORDER_FOR_INLINING_AUTOMATIC" desc="Option name for automatic selection of whether to allow HistoryQuickProvider to reorder suggestions to make an inlineable one appear first">
+        Automatic
+      </message>
+      <message name="IDS_FLAGS_OMNIBOX_HISTORY_QUICK_PROVIDER_REORDER_FOR_INLINING_ENABLED" desc="Option name to allow HistoryQuickProvider to reorder suggestions to make an inlineable one appear first">
+        Reorder
+      </message>
+      <message name="IDS_FLAGS_OMNIBOX_HISTORY_QUICK_PROVIDER_REORDER_FOR_INLINING_DISABLED" desc="Option name to disallow HistoryQuickProvider to reorder suggestions to make an inlineable one appear first">
+        Don't Reorder
+      </message>
       <message name="IDS_FLAGS_OMNIBOX_INLINE_HISTORY_QUICK_PROVIDER_NAME" desc="Title for the flag for omnibox to prohibit the HistoryQuickProvider from inlining suggestions">
         Inline HistoryQuickProvider suggestions
       </message>
diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
index 4706536..16ea65e8 100644
--- a/chrome/browser/about_flags.cc
+++ b/chrome/browser/about_flags.cc
@@ -82,6 +82,19 @@ const Experiment::Choice kOmniboxHistoryQuickProviderNewScoringChoices[] = {
     switches::kOmniboxHistoryQuickProviderNewScoringDisabled }
 };
 
+const Experiment::Choice
+    kOmniboxHistoryQuickProviderReorderForInliningChoices[] = {
+  { IDS_FLAGS_OMNIBOX_HISTORY_QUICK_PROVIDER_REORDER_FOR_INLINING_AUTOMATIC,
+    "",
+    "" },
+  { IDS_FLAGS_OMNIBOX_HISTORY_QUICK_PROVIDER_REORDER_FOR_INLINING_ENABLED,
+    switches::kOmniboxHistoryQuickProviderReorderForInlining,
+    switches::kOmniboxHistoryQuickProviderReorderForInliningEnabled },
+  { IDS_FLAGS_OMNIBOX_HISTORY_QUICK_PROVIDER_REORDER_FOR_INLINING_DISABLED,
+    switches::kOmniboxHistoryQuickProviderReorderForInlining,
+    switches::kOmniboxHistoryQuickProviderReorderForInliningDisabled }
+};
+
 const Experiment::Choice kOmniboxInlineHistoryQuickProviderChoices[] = {
   { IDS_FLAGS_OMNIBOX_INLINE_HISTORY_QUICK_PROVIDER_AUTOMATIC, "", "" },
   { IDS_FLAGS_OMNIBOX_INLINE_HISTORY_QUICK_PROVIDER_ALLOWED,
@@ -423,6 +436,13 @@ const Experiment kExperiments[] = {
     MULTI_VALUE_TYPE(kOmniboxHistoryQuickProviderNewScoringChoices)
   },
   {
+    "omnibox-history-quick-provider-reorder-for-inlining",
+    IDS_FLAGS_OMNIBOX_HISTORY_QUICK_PROVIDER_REORDER_FOR_INLINING_NAME,
+    IDS_FLAGS_OMNIBOX_HISTORY_QUICK_PROVIDER_REORDER_FOR_INLINING_DESCRIPTION,
+    kOsAll,
+    MULTI_VALUE_TYPE(kOmniboxHistoryQuickProviderReorderForInliningChoices)
+  },
+  {
     "omnibox-inline-history-quick-provider",
     IDS_FLAGS_OMNIBOX_INLINE_HISTORY_QUICK_PROVIDER_NAME,
     IDS_FLAGS_OMNIBOX_INLINE_HISTORY_QUICK_PROVIDER_DESCRIPTION,
diff --git a/chrome/browser/autocomplete/history_quick_provider.cc b/chrome/browser/autocomplete/history_quick_provider.cc
index dd7fb1d..f903a10 100644
--- a/chrome/browser/autocomplete/history_quick_provider.cc
+++ b/chrome/browser/autocomplete/history_quick_provider.cc
@@ -45,7 +45,8 @@ bool HistoryQuickProvider::disabled_ = false;
 HistoryQuickProvider::HistoryQuickProvider(ACProviderListener* listener,
                                            Profile* profile)
     : HistoryProvider(listener, profile, "HistoryQuickProvider"),
-      languages_(profile_->GetPrefs()->GetString(prefs::kAcceptLanguages)) {
+      languages_(profile_->GetPrefs()->GetString(prefs::kAcceptLanguages)),
+      reorder_for_inlining_(false) {
   enum InliningOption {
     INLINING_PROHIBITED = 0,
     INLINING_ALLOWED = 1,
@@ -102,6 +103,11 @@ HistoryQuickProvider::HistoryQuickProvider(ACProviderListener* listener,
   UMA_HISTOGRAM_ENUMERATION(
       "Omnibox.InlineHistoryQuickProviderFieldTrialBeacon",
       inlining_option, NUM_OPTIONS);
+
+  reorder_for_inlining_ = CommandLine::ForCurrentProcess()->
+      GetSwitchValueASCII(switches::
+                          kOmniboxHistoryQuickProviderReorderForInlining) ==
+      switches::kOmniboxHistoryQuickProviderReorderForInliningEnabled;
 }
 
 void HistoryQuickProvider::Start(const AutocompleteInput& input,
@@ -151,27 +157,32 @@ void HistoryQuickProvider::DoAutocomplete() {
   if (matches.empty())
     return;
 
+  if (reorder_for_inlining_) {
+    // If we're allowed to reorder results in order to get an
+    // inlineable result to appear first (and hence have a
+    // HistoryQuickProvider suggestion possibly appear first), find
+    // the first inlineable result and then swap it to the front.
+    for (ScoredHistoryMatches::iterator i(matches.begin());
+         (i != matches.end()) &&
+             (i->raw_score >= AutocompleteResult::kLowestDefaultScore);
+         ++i) {
+      if (i->can_inline) {  // this test is only true once because of the break
+        if (i != matches.begin())
+          std::rotate(matches.begin(), i, i + 1);
+        break;
+      }
+    }
+  }
+
   // Loop over every result and add it to matches_.  In the process,
   // guarantee that scores are decreasing.  |max_match_score| keeps
   // track of the highest score we can assign to any later results we
   // see.  Also, if we're not allowing inline autocompletions in
-  // general, artificially reduce the starting |max_match_score|
-  // (which therefore applies to all results) to something low enough
-  // that guarantees no result will be offered as an autocomplete
-  // suggestion.  In addition, even if we allow inlining of
-  // suggestions in general, we also reduce the starting
-  // |max_match_score| if our top suggestion is not inlineable to make
-  // sure it never gets attempted to be offered as an inline
-  // suggestion.  Note that this strategy will allow a funky case:
-  // suppose we're allowing inlining in general.  If the second result
-  // is marked as cannot inline yet has a score that would make it
-  // inlineable, it will keep its score.  This is a bit odd--a
-  // non-inlineable result with a score high enough to make it
-  // eligible for inlining will keep its high score--but it's okay
-  // because there is a higher scoring result that is required to be
-  // shown before this result.  Hence, this result, the second in the
-  // set, will never be inlined.  (The autocomplete UI keeps results
-  // in relevance score order.)
+  // general or the current best suggestion isn't inlineable,
+  // artificially reduce the starting |max_match_score| (which
+  // therefore applies to all results) to something low enough that
+  // guarantees no result will be offered as an autocomplete
+  // suggestion.
   int max_match_score = (PreventInlineAutocomplete(autocomplete_input_) ||
       !matches.begin()->can_inline) ?
       (AutocompleteResult::kLowestDefaultScore - 1) :
diff --git a/chrome/browser/autocomplete/history_quick_provider.h b/chrome/browser/autocomplete/history_quick_provider.h
index cee0e2b..9beba79 100644
--- a/chrome/browser/autocomplete/history_quick_provider.h
+++ b/chrome/browser/autocomplete/history_quick_provider.h
@@ -76,6 +76,24 @@ class HistoryQuickProvider : public HistoryProvider {
   AutocompleteInput autocomplete_input_;
   std::string languages_;
 
+  // True if we're allowed to reorder results depending on
+  // inlineability in order to assign higher relevance scores.
+  // Consider a case where ScoredHistoryMatch provides results x and
+  // y, where x is not inlineable and has a score of 3000 and y is
+  // inlineable and has a score of 2500.  If reorder_for_inlining_ is
+  // false, then x gets demoted to a non-inlineable score (1199) and y
+  // gets demoted to a lower score (1198) because we try to preserve
+  // the order.  On the other hand, if reorder_for_inlining_ is true,
+  // then y keeps its score of 2500 and x gets demoted to 2499 in
+  // order to follow y.  There will not be any problems with an
+  // unexpected inline because the non-inlineable result x scores
+  // lower than the inlineable one.
+  // TODO(mpearson): remove this variable after we're done experimenting.
+  // (This member is meant to only exist for experimentation purposes.
+  // Once we know which behavior is better, we should rip out this variable
+  // and make the best behavior the default.)
+  bool reorder_for_inlining_;
+
   // Only used for testing.
   scoped_ptr<history::InMemoryURLIndex> index_for_testing_;
 
diff --git a/chrome/common/chrome_switches.cc b/chrome/common/chrome_switches.cc
index 23f22e6..a87c46a 100644
--- a/chrome/common/chrome_switches.cc
+++ b/chrome/common/chrome_switches.cc
@@ -928,6 +928,18 @@ const char kOmniboxHistoryQuickProviderNewScoringEnabled[] = "1";
 // 0 means use old scoring ( == current behavior as of 6/2012).
 const char kOmniboxHistoryQuickProviderNewScoringDisabled[] = "0";
 
+// Controls whether HistoryQuickProvider is allowed to reorder results
+// according to inlineability in order to more aggressively assign/keep
+// high relevance scores.
+const char kOmniboxHistoryQuickProviderReorderForInlining[] =
+    "omnibox-history-quick-provider-reorder-for-inlining";
+// The value the kOmniboxHistoryQuickProviderReorderForInlining switch may
+// have, as in "--omnibox-history-quick-provider-reorder-for-inlining=1".
+// 1 means allow reordering results.
+const char kOmniboxHistoryQuickProviderReorderForInliningEnabled[] = "1";
+// 0 means don't allow reordering results ( == current behavior as of 6/2012).
+const char kOmniboxHistoryQuickProviderReorderForInliningDisabled[] = "0";
+
 // Controls whether the omnibox's HistoryQuickProvider is allowed to
 // inline suggestions.
 const char kOmniboxInlineHistoryQuickProvider[] =
diff --git a/chrome/common/chrome_switches.h b/chrome/common/chrome_switches.h
index 5b68da5..a26b43b 100644
--- a/chrome/common/chrome_switches.h
+++ b/chrome/common/chrome_switches.h
@@ -249,6 +249,9 @@ extern const char kNumPacThreads[];
 extern const char kOmniboxHistoryQuickProviderNewScoring[];
 extern const char kOmniboxHistoryQuickProviderNewScoringEnabled[];
 extern const char kOmniboxHistoryQuickProviderNewScoringDisabled[];
+extern const char kOmniboxHistoryQuickProviderReorderForInlining[];
+extern const char kOmniboxHistoryQuickProviderReorderForInliningEnabled[];
+extern const char kOmniboxHistoryQuickProviderReorderForInliningDisabled[];
 extern const char kOmniboxInlineHistoryQuickProvider[];
 extern const char kOmniboxInlineHistoryQuickProviderAllowed[];
 extern const char kOmniboxInlineHistoryQuickProviderProhibited[];