Omnibox: Make HUP Scoring More Sane in Prevent-Inline Mode

chrome/browser/autocomplete/history_url_provider.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/history_url_provider.h"
 
 #include <algorithm>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
@@ skipping 550 matching lines @@
   // url_db can be NULL if it hasn't finished initializing (or failed to
   // initialize).  In this case all we can do is fall back on the second
   // pass.
   //
   // TODO(pkasting): We should just block here until this loads.  Any time
   // someone unloads the history backend, we'll get inconsistent inline
   // autocomplete behavior here.
   if (url_db) {
     DoAutocomplete(NULL, url_db, params.get());
     matches_.clear();
-    PromoteMatchIfNecessary(*params);
+    PromoteMatchesIfNecessary(*params);
     // NOTE: We don't reset |params| here since at least the |promote_type|
     // field on it will be read by the second pass -- see comments in
     // DoAutocomplete().
   }
 
   // Pass 2: Ask the history service to call us back on the history thread,
   // where we can read the full on-disk DB.
   if (input.want_asynchronous_matches()) {
     done_ = false;
     params_ = params.release();  // This object will be destroyed in
@@ skipping 177 matching lines @@
   SortAndDedupMatches(&params->matches);
 
   // Try to create a shorter suggestion from the best match.
   // We consider the what you typed match eligible for display when it's
   // navigable and there's a reasonable chance the user intended to do
   // something other than search.  We use a variety of heuristics to determine
   // this, e.g. whether the user explicitly typed a scheme, or if omnibox
   // searching has been disabled by policy. In the cases where we've parsed as
   // UNKNOWN, we'll still show an accidental search infobar if need be.
   VisitClassifier classifier(this, params->input, db);
-  bool have_what_you_typed_match =
+  params->have_what_you_typed_match =
       (params->input.type() != metrics::OmniboxInputType::QUERY) &&
       ((params->input.type() != metrics::OmniboxInputType::UNKNOWN) ||
        (classifier.type() == VisitClassifier::UNVISITED_INTRANET) ||
        !params->trim_http ||
        (AutocompleteInput::NumNonHostComponents(params->input.parts()) > 0) ||
        !params->default_search_provider);
   const bool have_shorter_suggestion_suitable_for_inline_autocomplete =
-      PromoteOrCreateShorterSuggestion(db, have_what_you_typed_match, params);
+      PromoteOrCreateShorterSuggestion(db, params);
 
   // Check whether what the user typed appears in history.
   const bool can_check_history_for_exact_match =
       // Checking what_you_typed_match.is_history_what_you_typed_match tells us
       // whether SuggestExactInput() succeeded in constructing a valid match.
       params->what_you_typed_match.is_history_what_you_typed_match &&
       // Additionally, in the case where the user has typed "foo.com" and
       // visited (but not typed) "foo/", and the input is "foo", the first pass
       // will fall into the FRONT_HISTORY_MATCH case for "foo.com" but the
       // second pass can suggest the exact input as a better URL.  Since we need
       // both passes to agree, and since during the first pass there's no way to
       // know about "foo/", ensure that if the promote type was set to
       // FRONT_HISTORY_MATCH during the first pass, the second pass will not
       // consider the exact suggestion to be in history and therefore will not
       // suggest the exact input as a better match.  (Note that during the first
       // pass, this conditional will always succeed since |promote_type| is
       // initialized to NEITHER.)
       (params->promote_type != HistoryURLProviderParams::FRONT_HISTORY_MATCH);
   params->exact_suggestion_is_in_history = can_check_history_for_exact_match &&
       FixupExactSuggestion(db, classifier, params);
 
   // If we succeeded in fixing up the exact match based on the user's history,
   // we should treat it as the best match regardless of input type.  If not,
   // then we check whether there's an inline autocompletion we can create from
   // this input, so we can promote that as the best match.
   if (params->exact_suggestion_is_in_history) {
     params->promote_type = HistoryURLProviderParams::WHAT_YOU_TYPED_MATCH;
-  } else if (!params->prevent_inline_autocomplete && !params->matches.empty() &&
-      (have_shorter_suggestion_suitable_for_inline_autocomplete ||
-       CanPromoteMatchForInlineAutocomplete(params->matches[0]))) {
+  } else if (!params->matches.empty() &&

[Peter Kasting, 2015/03/24 22:02:43]

Rather than just removing the |prevent_inline_autocomplete| check here, I wonder
if we should change it to:

  } else if ((!params->prevent_inline_autocomplete ||
              params->have_what_you_typed_match) && ...

I'm worrying about whether we could promote an inline-autocompleted match when
inline autocomplete is disabled, and we don't have a what-you-typed match to
replace it as potential default.

Or is this unnecessary because in all the cases where this would happen, the
inline-autocompleted match is marked as "can't be default" and the search
provider is guaranteed to return a "can be default" SWYT match?  If that's true,
then we should keep the code as you wrote it and not as I'm suggesting, or we'll
fail to reap the full benefit of your scoring change because we still won't
promote these good history matches as high in these cases.

[Mark P, 2015/03/24 22:35:46]

This is not a problem.  Promoting has to do with scores.  It doesn't matter
whether we promoted a match or not.  The promoted inline autocomplete match will
not be allowed to be the default match, hence the reorder code in
AutocompleteController will reorder results to put a legal default match first. 
SearchProvider will provide such a match if no other provider does.

I answered this first paragraph question of yours without reading the second.  I
believe in the second paragraph you hypothesized the same answer I gave.

[Peter Kasting, 2015/03/24 23:05:19]

That is fine as long as SearchProvider always actually _does_ provide such a
match.

My worry was that it may not be providing a match in every case where
!have_what_you_typed_match.  I'm more concerned about this than I used to be
because of things like variable verbatim -- I want to make sure that even a
downranked verbatim search score, which scores below more then kMaxMatches other
scores, will still be put back at the top of the list.

I think this is probably OK (or we would probably have issues today in any cases
where (prevent_inline_autocomplete && !have_what_you_typed_match)) but I'm
trying to make sure I've thought through every angle.

There is one weird edge case I can think of.  If we have no default search
provider, and the user types a QUERY, then have_what_you_typed_match will be
false, and any promotion here will result in Bad Things -- really, returning any
results at all will result in Bad Things, since none of them will be allowed to
be default, and we'll hit the DCHECK in the controller that there be at least
one defaultable match among the returned matches.  I think that's already a
problem today, though, since if we could promote this match, then the behavior
today would be to return it as a non-promoted match, so we'd still hit that
DCHECK.  And hopefully we can't have a match here to promote, since hopefully
QUERY means "this can't be fixed up to anything that we can produce a prefix
match for" -- but I'm not sure that's the case, now that we do things like
marking "calculator-like" inputs as QUERY in AutocompleteInput::Parse().

So I guess we don't need to try to address that since I think this patch isn't
making the issue any worse.

[Mark P, 2015/03/25 19:04:57]

SearchProvider is always supposed to provide a legal default match (regardless
of input type).  If it doesn't, it's a bug.  We found one such bug recently
(default search engine got renamed/deleted in the middle of processing the
request).  But I think these bugs are rare or non-existent anymore, and
orthogonal to this change.  This change does not change when/how often we put
the UWYT match in HistoryURLProvider's result set.
It will.  We always pull up a legal default match, even if it wouldn't normally
be in the top X items in the dropdown, and only later resize the list.
https://code.google.com/p/chromium/codesearch#chromium/src/components/omnibox...
I understand.  Asking someone else to think through these questions with you is
a good idea.
I agree.  If the user doesn't have a default search provider, lots of bad things
can happen, and would happen, already today.  This change doesn't make that
situation any worse.

+             (have_shorter_suggestion_suitable_for_inline_autocomplete ||
+              CanPromoteMatchForInlineAutocomplete(params->matches[0]))) {
     params->promote_type = HistoryURLProviderParams::FRONT_HISTORY_MATCH;

[Peter Kasting, 2015/03/24 23:05:19]

OK, so let's put something like this comment here:

    // Note that we promote this inline-autocompleted match even when
    // params->prevent_inline_autocomplete is true.  This is safe because in
    // this case the match will be marked as "not allowed to be default", and
    // a non-inlined match that is "allowed to be default" will be reordered
    // above it by the controller.  We ensure there is such a match in two ways:
    //   * If params->have_what_you_typed_match is true, we force the
    //     what-you-typed match to be added in this case.  See comments in
    //     PromoteMatchesIfNecessary().
    //   * Otherwise, we should have some sort of QUERY or UNKNOWN input that
    //     the SearchProvider will provide a defaultable WYT match for.

[Mark P, 2015/03/25 19:04:57]

Okay, that's a nice clarification to add.  Added with only trivial edits.

   } else {
     // Failed to promote any URLs.  Use the What You Typed match, if we have it.
-    params->promote_type = have_what_you_typed_match ?
+    params->promote_type = params->have_what_you_typed_match ?
         HistoryURLProviderParams::WHAT_YOU_TYPED_MATCH :
         HistoryURLProviderParams::NEITHER;
   }
 
   const size_t max_results =
       kMaxMatches + (params->exact_suggestion_is_in_history ? 1 : 0);
   if (backend) {
     // Remove redirects and trim list to size.  We want to provide up to
     // kMaxMatches results plus the What You Typed result, if it was added to
     // params->matches above.
     CullRedirects(backend, &params->matches, max_results);
   } else if (params->matches.size() > max_results) {
     // Simply trim the list to size.
     params->matches.resize(max_results);
   }
 }
 
-void HistoryURLProvider::PromoteMatchIfNecessary(
+void HistoryURLProvider::PromoteMatchesIfNecessary(
     const HistoryURLProviderParams& params) {
   if (params.promote_type == HistoryURLProviderParams::NEITHER)
     return;
-  matches_.push_back(
-      (params.promote_type == HistoryURLProviderParams::WHAT_YOU_TYPED_MATCH) ?
-      params.what_you_typed_match :
-      HistoryMatchToACMatch(params, 0, INLINE_AUTOCOMPLETE,
-                            CalculateRelevance(INLINE_AUTOCOMPLETE, 0)));
+  if (params.promote_type == HistoryURLProviderParams::FRONT_HISTORY_MATCH) {
+    matches_.push_back(
+        HistoryMatchToACMatch(params, 0, INLINE_AUTOCOMPLETE,
+                              CalculateRelevance(INLINE_AUTOCOMPLETE, 0)));
+  }
+  // Add the what-you-typed match if we're explicitly told to or if we might
+  // need it (because inline autocompletion is disabled, i.e., no other URL
+  // suggestion is allowed to be the default match) and it's eligible for
+  // display.

[Peter Kasting, 2015/03/24 22:02:43]

After thinking more, this code is correct.

[Mark P, 2015/03/24 22:35:46]

Acknowledged.

[Peter Kasting, 2015/03/24 23:05:19]

Let's make the comment here something like this:

  // There are two cases where we need to add the what-you-typed-match:
  //   * If params.promote_type is WHAT_YOU_TYPED_MATCH, we're being explicitly
  //     directed to.
  //   * If params.have_what_you_typed_match is true, then params.promote_type
  //     can't be NEITHER (see code near the end of DoAutocomplete()), so if
  //     it's not WHAT_YOU_TYPED_MATCH, it must be FRONT_HISTORY_MATCH, and
  //     we'll have promoted the history match above.  If
  //     params.prevent_inline_autocomplete is also true, then this match
  //     will be marked "not allowed to be default", and we need to add the
  //     what-you-typed match to ensure there's a defaultable match for the
  //     controller to promote.  (If params.have_what_you_typed_match is false,
  //     the SearchProvider should take care of adding this defaultable match.)

[Mark P, 2015/03/25 19:04:57]

Done with trivial edits.

+  if ((params.promote_type == HistoryURLProviderParams::WHAT_YOU_TYPED_MATCH) ||
+      (params.prevent_inline_autocomplete &&
+       params.have_what_you_typed_match)) {
+    matches_.push_back(params.what_you_typed_match);
+  }
 }
 
 void HistoryURLProvider::QueryComplete(
     HistoryURLProviderParams* params_gets_deleted) {
   // Ensure |params_gets_deleted| gets deleted on exit.
   scoped_ptr<HistoryURLProviderParams> params(params_gets_deleted);
 
   // If the user hasn't already started another query, clear our member pointer
   // so we can't write into deleted memory.
   if (params_ == params_gets_deleted)
     params_ = NULL;
 
   // Don't send responses for queries that have been canceled.
   if (params->cancel_flag.IsSet())
     return;  // Already set done_ when we canceled, no need to set it again.
 
   // Don't modify |matches_| if the query failed, since it might have a default
   // match in it, whereas |params->matches| will be empty.
   if (!params->failed) {
     matches_.clear();
-    PromoteMatchIfNecessary(*params);
+    PromoteMatchesIfNecessary(*params);
 
     // Determine relevance of highest scoring match, if any.
     int relevance = matches_.empty() ?
         CalculateRelevance(NORMAL,
                            static_cast<int>(params->matches.size() - 1)) :
         matches_[0].relevance;
 
     // Convert the history matches to autocomplete matches.  If we promoted the
     // first match, skip over it.
     const size_t first_match =
@@ skipping 80 matching lines @@
   const size_t registry_length =
       net::registry_controlled_domains::GetRegistryLength(
           host,
           net::registry_controlled_domains::EXCLUDE_UNKNOWN_REGISTRIES,
           net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES);
   return registry_length == 0 && db->IsTypedHost(host);
 }
 
 bool HistoryURLProvider::PromoteOrCreateShorterSuggestion(
     history::URLDatabase* db,
-    bool have_what_you_typed_match,
     HistoryURLProviderParams* params) {
   if (params->matches.empty())
     return false;  // No matches, nothing to do.
 
   // Determine the base URL from which to search, and whether that URL could
   // itself be added as a match.  We can add the base iff it's not "effectively
   // the same" as any "what you typed" match.
   const history::HistoryMatch& match = params->matches[0];
   GURL search_base = ConvertToHostOnly(match, params->input.text());
-  bool can_add_search_base_to_matches = !have_what_you_typed_match;
+  bool can_add_search_base_to_matches = !params->have_what_you_typed_match;
   if (search_base.is_empty()) {
     // Search from what the user typed when we couldn't reduce the best match
     // to a host.  Careful: use a substring of |match| here, rather than the
     // first match in |params|, because they might have different prefixes.  If
     // the user typed "google.com", params->what_you_typed_match will hold
     // "http://google.com/", but |match| might begin with
     // "http://www.google.com/".
     // TODO: this should be cleaned up, and is probably incorrect for IDN.
     std::string new_match = match.url_info.url().possibly_invalid_spec().
         substr(0, match.input_location + params->input.text().length());
@@ skipping 173 matching lines @@
     AutocompleteMatch::ClassifyLocationInString(base::string16::npos, 0,
         match.contents.length(), ACMatchClassification::URL,
         &match.contents_class);
   }
   match.description = info.title();
   match.description_class =
       ClassifyDescription(params.input.text(), match.description);
   RecordAdditionalInfoFromUrlRow(info, &match);
   return match;
 }