Omnibox: SearchProvider: Fix Navsuggestions in Prevent-Inlining Mode

chrome/browser/autocomplete/search_provider.cc

 // Copyright 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/search_provider.h"
 
 #include <algorithm>
 #include <cmath>
 
 #include "base/callback.h"
@@ skipping 1491 matching lines @@
   int base_score;
   if (is_primary_provider)
     base_score = (input_.type() == AutocompleteInput::URL) ? 750 : 1050;
   else
     base_score = 200;
   return std::max(0, base_score - score_discount);
 }
 
 AutocompleteMatch SearchProvider::NavigationToMatch(
     const NavigationResult& navigation) {
-  const base::string16& input = navigation.from_keyword_provider() ?
+  const base::string16& full_input = navigation.from_keyword_provider() ?
       keyword_input_.text() : input_.text();
+  // Actual input to use when matching, discarding trailing slash if any.

[Mark P, 2014/02/19 00:26:19]

This helps handle cases like input doodle.com/ with a navsuggestion formatted
URL of doodle.com

[Peter Kasting, 2014/02/19 20:39:32]

What about e.g. matches where the "www." is only present on one side, or matches
with escaping or punycode/Unicode differences?

I'm worried that if we want to check for "equivalent URLs", we need something
smarter than this.

[Mark P, 2014/02/19 23:23:01]

The code (below) uses BestUrlPrefix, exactly like all the other providers, to
decide what is inlineable.  This means for instance if the user types doodle.com
and the navsuggestion URL is http://www.doodle.com, that will be identified as
inlineable.  Likewise, if the user types www.doodle.com, it will also be.  But
if the user types www.doodle.com and the navsuggestion URL is merely
http://doodle.com/, it cannot be identified as inlineable.  All this is like
HistoryURL (and other) providers.
I think we do this equivalent URLs check right enough (see above).  The messed
up time is fixed by this '/' handling is to better deal with (unnecessary)
trailing slashes.

I find it odd that we need this code.  HistoryURL provider turns out not to need
it because it returns results based on matching against the URL (not the URL
with the trimmed slash).  HistoryQuick provider turns out not to need it because
it does substring matching against the URL (not the URL with the trimmed slash).

This makes me think of an alternate implementation.  For the purpose of
calculating allowed_to_be_default_match, rather than trimming the optional
trailing slash on the input and matching it against formatted_url (which lacks
the trailing slash and is used to calculate match_conents), leave the input
as-is and match it against a (lightly-formatted) URL, one that keeps the
trailing slash.  Would you prefer this approach?

Note that this approach might be slightly more complex because we'd still want
to compute inline_autocomplete_offset and inline_autocompletion using
formatted_url, just not allowed_to_be_default_match.  (In other words, we'd do
some of the code below twice, once with each version of the URL.)  Obviously I
can add helper functions add needed.

+  const base::string16 input = (*(full_input.rbegin()) == '/') ?
+      full_input.substr(0, full_input.length() - 1) : full_input;
   AutocompleteMatch match(this, navigation.relevance(), false,
                           AutocompleteMatchType::NAVSUGGEST);
   match.destination_url = navigation.url();
 
   // First look for the user's input inside the formatted url as it would be
   // without trimming the scheme, so we can find matches at the beginning of the
   // scheme.
   const URLPrefix* prefix =
       URLPrefix::BestURLPrefix(navigation.formatted_url(), input);
   size_t match_start = (prefix == NULL) ?
@@ skipping 12 matching lines @@
           net::FormatUrl(navigation.url(), languages, format_types,
                          net::UnescapeRule::SPACES, NULL, NULL,
                          &inline_autocomplete_offset));
   // Preserve the forced query '?' prefix in |match.fill_into_edit|.
   // Otherwise, user edits to a suggestion would show non-Search results.
   if (input_.type() == AutocompleteInput::FORCED_QUERY) {
     match.fill_into_edit.insert(0, base::ASCIIToUTF16("?"));
     if (inline_autocomplete_offset != base::string16::npos)
       ++inline_autocomplete_offset;
   }
-  if (!input_.prevent_inline_autocomplete() &&
-      (inline_autocomplete_offset != base::string16::npos)) {
+  if (inline_autocomplete_offset != base::string16::npos) {

[Mark P, 2014/02/19 00:26:19]

The restructuring in this block allows handling simple cases such as input
doodle.com with navsuggestion formatted_url doodle.com (identical).  (This was
broken before.)  The new structure also plays nicely with "/"-handling code
above, so fixes those cases too.

     DCHECK(inline_autocomplete_offset <= match.fill_into_edit.length());
+    match.inline_autocompletion =
+        match.fill_into_edit.substr(inline_autocomplete_offset);
     // A navsuggestion can only be the default match when there is no
     // keyword provider active, lest it appear first and break the user
-    // out of keyword mode.
+    // out of keyword mode.  It can also only be default when we're not
+    // preventing inline autocompletion (unless the inline autocompletion
+    // would be empty).
     match.allowed_to_be_default_match =
-        (providers_.GetKeywordProviderURL() == NULL);
-    match.inline_autocompletion =
-        match.fill_into_edit.substr(inline_autocomplete_offset);
+        (providers_.GetKeywordProviderURL() == NULL) &&
+        (!input_.prevent_inline_autocomplete() ||
+         match.inline_autocompletion.empty());

[Peter Kasting, 2014/02/19 20:39:32]

Does this mean that an input of "doodle.com" won't allow a navsuggestion of
"doodle.com/" to be default, because we'd have a slash as the inline
autocompletion?

[Mark P, 2014/02/19 23:23:01]

There's some subtlety here.
Technically, you're right: in prevent_inline_autocomplete mode, then doodle.com
will not allow a navsuggestion of doodle.com/ to be marked as inlineable.  But
note that because right now we're using formatted_url (which is always computed
in a way to drop the trailing slash on hostnames) in order to compute
formatted_url and inline_autocompletion, then this will never happen.

There might be some concern about an input doodle.com/abc and the navsuggestion
doodle.com/abc/ not being marked as inlineable.  That might be another issue
worth talking about (outside the scope of this which clearly needs to be fixed
ASAP).


Also note, I just realized HistoryQuick provider is already bad on this front:
doodle.com against the URL suggestion doodle.com/ will not be marked as
inlineable in prevent_inline_autocomplete mode.  (HQP uses the URL with the
slash for this decision.)  This sounds like another issue.  I'll file a bug.

   }
 
   match.contents = navigation.match_contents();
   match.contents_class = navigation.match_contents_class();
   match.description = navigation.description();
   AutocompleteMatch::ClassifyMatchInString(input, match.description,
       ACMatchClassification::NONE, &match.description_class);
 
   match.RecordAdditionalInfo(
       kRelevanceFromServerKey,
@@ skipping 42 matching lines @@
     it->set_relevance(max_query_relevance);
     it->set_relevance_from_server(relevance_from_server);
   }
 }
 
 void SearchProvider::UpdateDone() {
   // We're done when the timer isn't running, there are no suggest queries
   // pending, and we're not waiting on Instant.
   done_ = !timer_.IsRunning() && (suggest_results_pending_ == 0);
 }