Omnibox: Bug Fixes for Shortcuts Inlining

chrome/browser/autocomplete/shortcuts_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/shortcuts_provider.h"
 
 #include <algorithm>
 #include <cmath>
 #include <map>
 #include <vector>
@@ skipping 10 matching lines @@
 #include "chrome/browser/autocomplete/autocomplete_input.h"
 #include "chrome/browser/autocomplete/autocomplete_provider_listener.h"
 #include "chrome/browser/autocomplete/autocomplete_result.h"
 #include "chrome/browser/autocomplete/url_prefix.h"
 #include "chrome/browser/history/history_notifications.h"
 #include "chrome/browser/history/history_service.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/history/shortcuts_backend_factory.h"
 #include "chrome/browser/omnibox/omnibox_field_trial.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/net/url_fixer_upper.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "url/url_parse.h"
 
 namespace {
 
 class DestinationURLEqualsURL {
  public:
   explicit DestinationURLEqualsURL(const GURL& url) : url_(url) {}
   bool operator()(const AutocompleteMatch& match) const {
@@ skipping 180 matching lines @@
       // the default list didn't have a match or the default list's match
       // was shorter than it could've been.
       if (URLPrefix::PrefixMatch(www_prefix, match.fill_into_edit, term_string))
         best_prefix = &www_prefix;
     }
     if (best_prefix != NULL) {
       match.inline_autocompletion = match.fill_into_edit.substr(
           best_prefix->prefix.length() + term_string.length());
       match.allowed_to_be_default_match =
           !prevent_inline_autocomplete || match.inline_autocompletion.empty();
+    } else {
+      // Special case to allow URL-like inputs that need to be fixed up to
+      // inline against URL shortcuts.  This is especially userful to get

[Peter Kasting, 2013/12/17 19:56:49]

Nit: userful -> useful

[Mark P, 2013/12/17 20:36:18]

Done.

+      // about: URLs to inline against chrome:// shortcuts.  (about: URLs
+      // are fixed up to the chrome:// scheme.)

[Peter Kasting, 2013/12/17 19:56:49]

What about if the fill_into_edit is about:foo and the term_string is chrome://f?
 Should this be able to match?  If so we might need to fix up both
fill_into_edit and term_string?

[Mark P, 2013/12/17 20:36:18]

This will not happen.  There's no way to select a match from the omnibox
dropdown that has a fill_into_edit of about:foo.  All fill_into_edits (whether
from HistoryURL or BuiltinProvider) are already fixed up.

+      GURL url =
+          URLFixerUpper::FixupURL(UTF16ToUTF8(term_string), std::string());
+      if (url.is_valid()) {

[Mark P, 2013/12/17 19:29:02]

Alternatively, do I want to skip is_valid() and simply use
possibly_invalid_spec()?  I'm not sure.

[Peter Kasting, 2013/12/17 19:56:49]

I think so.  "about:" might get fixed to "chrome://" which is probably invalid
but would be useful to get here.

[Mark P, 2013/12/17 20:36:18]

Done.  I thought I tested this case but apparently I didn't...  I thought I
tested it before I saw at least one shortcut could be inlined against about:. 
Now I look closer and I find another problem:
about:
gets fixed up to
chrome://version

This allows chrome://version to inline (and hence fixes the bug by making sure a
non-search can inline) but it's misleading in that all the other shortcuts get
marked as non-inlineable.  In other words, no matter how good a chrome://omnibox
shortcut is for me, it'll never be returned for about:.  I'm not sure how much I
should be concerned about this new discovery.  I believe it should only affect
"about:", "about:/", and "about://".

Note that "about", "abou", "abo", will never match against BuiltinProvider
shortcuts (which are always stored in the form chrome://).  We would need more
special code to deal handle this.

+        base::string16 url_spec_utf16 = ASCIIToUTF16(url.spec());
+        if ((term_string[term_string.length() - 1] != char16('/')) &&
+            (url_spec_utf16[url_spec_utf16.length() - 1] == char16('/'))) {
+          // If the url.spec() added a trailing '/' that wasn't in the input
+          // (e.g., at the end of a hostname in a URL such as chrome://version),
+          // remove it for the purposes of calculating whether this can be
+          // the default match.  (Fill_into_edit may be missing the trailing

[Peter Kasting, 2013/12/17 19:56:49]

Nit: Fill -> fill

[Mark P, 2013/12/17 20:36:18]

Done.

+          // slash in some cases.)
+          url_spec_utf16 =
+              url_spec_utf16.substr(0, url_spec_utf16.length() - 1);
+        }
+        if (StartsWith(match.fill_into_edit, url_spec_utf16, false)) {
+          match.inline_autocompletion = match.fill_into_edit.substr(
+              url_spec_utf16.length());
+          match.allowed_to_be_default_match = !prevent_inline_autocomplete ||
+              match.inline_autocompletion.empty();
+        }
+      }

[Mark P, 2013/12/17 19:29:02]

I think this strategy works but I'm not positive.  I think the simplest case
where it might be able fail is this: I'm trying to come up with an example where
we need this FixupURL() strategy but want to match against a fill_into_edit that
lacks a scheme.  Can you come up with a failure example like this?

[Peter Kasting, 2013/12/17 19:56:49]

Check i18n domain names and escaped versus unescaped paths.

[Mark P, 2013/12/17 20:36:18]

Seems to work.  I can only manage to get shortcut's database to have properly
escaped fill_into_edit (because other providers set the fill_into_edit
correctly), so when I fixup the user input, it matches correctly.

     }
   }
 
   // Try to mark pieces of the contents and description as matches if they
   // appear in |term_string|.
   WordMap terms_map(CreateWordMapForString(term_string));
   if (!terms_map.empty()) {
     match.contents_class = ClassifyAllMatchesInString(term_string, terms_map,
         match.contents, match.contents_class);
     match.description_class = ClassifyAllMatchesInString(term_string, terms_map,
@@ skipping 151 matching lines @@
   // (1.0 / each 5 additional hits), up to a maximum of 5x as long.
   const double kMaxDecaySpeedDivisor = 5.0;
   const double kNumUsesPerDecaySpeedDivisorIncrement = 5.0;
   double decay_divisor = std::min(kMaxDecaySpeedDivisor,
       (shortcut.number_of_hits + kNumUsesPerDecaySpeedDivisorIncrement - 1) /
       kNumUsesPerDecaySpeedDivisorIncrement);
 
   return static_cast<int>((base_score / exp(decay_exponent / decay_divisor)) +
       0.5);
 }