Omnibox: Bug Fixes for Shortcuts Inlining

chrome/browser/autocomplete/shortcuts_provider.cc

Index: chrome/browser/autocomplete/shortcuts_provider.cc
diff --git a/chrome/browser/autocomplete/shortcuts_provider.cc b/chrome/browser/autocomplete/shortcuts_provider.cc
index b476f0fe2c96ea2d15624c8d0921df9148d94fee..b467d583b1d48f79ec4365cd96d9b45685576620 100644
--- a/chrome/browser/autocomplete/shortcuts_provider.cc
+++ b/chrome/browser/autocomplete/shortcuts_provider.cc
@@ -28,6 +28,7 @@
 #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"
@@ -228,6 +229,32 @@ AutocompleteMatch ShortcutsProvider::ShortcutToACMatch(
           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.

     }
   }