Omnibox: SearchProvider: Fix Navsuggestions in Prevent-Inlining Mode

chrome/browser/autocomplete/search_provider.cc

Index: chrome/browser/autocomplete/search_provider.cc
diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc
index 331a498f53666329f59a7096842933c0306eec03..389938df0072ae35678a7339f33253920625131a 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -1509,8 +1509,11 @@ int SearchProvider::CalculateRelevanceForHistory(
 
 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();
@@ -1543,16 +1546,19 @@ AutocompleteMatch SearchProvider::NavigationToMatch(
     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();