Omnibox: Make Bookmarks Set Inline_Autocompletion

chrome/browser/autocomplete/bookmark_provider.cc

Index: chrome/browser/autocomplete/bookmark_provider.cc
diff --git a/chrome/browser/autocomplete/bookmark_provider.cc b/chrome/browser/autocomplete/bookmark_provider.cc
index 73d17370919e2185c555b1a12dc9a25a1baf6e71..7a3de804fbdf2ba499047be641905b51bdfd8c34 100644
--- a/chrome/browser/autocomplete/bookmark_provider.cc
+++ b/chrome/browser/autocomplete/bookmark_provider.cc
@@ -9,7 +9,9 @@
 #include <vector>
 
 #include "base/prefs/pref_service.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/autocomplete/autocomplete_result.h"
+#include "chrome/browser/autocomplete/url_prefix.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/bookmarks/bookmark_title_match.h"
@@ -39,14 +41,9 @@ void BookmarkProvider::Start(const AutocompleteInput& input,
     return;
   matches_.clear();
 
-  // Short-circuit any matching when inline autocompletion is disabled and
-  // we're looking for BEST_MATCH because none of the BookmarkProvider's
-  // matches can score high enough to qualify.
   if (input.text().empty() ||
       ((input.type() != AutocompleteInput::UNKNOWN) &&
-       (input.type() != AutocompleteInput::QUERY)) ||
-      ((input.matches_requested() == AutocompleteInput::BEST_MATCH) &&
-       input.prevent_inline_autocomplete()))
+       (input.type() != AutocompleteInput::QUERY)))
     return;
 
   DoAutocomplete(input,
@@ -94,11 +91,16 @@ void BookmarkProvider::DoAutocomplete(const AutocompleteInput& input,
                                                   &matches);
   if (matches.empty())
     return;  // There were no matches.
+  AutocompleteInput fixed_up_input;
+  // If the input fails to get fixed up, pretend the fixed up version is the
+  // original.
+  if (!FixupUserInput(&fixed_up_input))
+    fixed_up_input = input;
   for (TitleMatches::const_iterator i = matches.begin(); i != matches.end();
        ++i) {
     // Create and score the AutocompleteMatch. If its score is 0 then the
     // match is discarded.
-    AutocompleteMatch match(TitleMatchToACMatch(*i));
+    AutocompleteMatch match(TitleMatchToACMatch(input, fixed_up_input, *i));
     if (match.relevance > 0)
       matches_.push_back(match);
   }
@@ -153,6 +155,8 @@ class ScoringFunctor {
 }  // namespace
 
 AutocompleteMatch BookmarkProvider::TitleMatchToACMatch(
+    const AutocompleteInput& input,
+    const AutocompleteInput& fixed_up_input,
     const BookmarkTitleMatch& title_match) {
   // The AutocompleteMatch we construct is non-deletable because the only
   // way to support this would be to delete the underlying bookmark, which is
@@ -161,16 +165,50 @@ AutocompleteMatch BookmarkProvider::TitleMatchToACMatch(
                           AutocompleteMatchType::BOOKMARK_TITLE);
   const base::string16& title(title_match.node->GetTitle());
   DCHECK(!title.empty());
+
+  // Determine |inline_autocomplete_offset|.

[Peter Kasting, 2014/04/09 00:53:32]

This whole block looks suspiciously similar to what other providers do (or at
least, need to do).  Is there any opportunity to hoist similar code to a common
location?

[Mark P, 2014/04/09 02:44:42]

I thought about this as I was transferred code here.  The regular providers
don't need to do the fixup case, so their logic is simply one BestURLPrefix call
and the variable declarations around it.  No sharing there.

Shortcuts does the fixup case.  But shortcuts doesn't use BestURLPrefix, it uses
something different BestURLPrefixWithWWWCase because the strings it's trying
match against may be missing the scheme but have a www that we want to drop.  I
guess I could share something with Shortcuts, but that code would be a bit odd
because we'd have to take an extra parameter want_www_case, and condition both
its best-url calls on that.  As those calls are really the only thing happening
here, I don't think it's worthwhile to share.  It'd be ugly.

The trim_http line is in common with history_url provider.  That's one line.  I
can replace it with a call that takes those three variables.  I don't think that
will help readability.  (Those three variables don't naturally go together; I
can't think a nice name.  TrimHttp (input, best_prefix,
inline_autocomplete_offset) doesn't look good to me.)

[Mark P, 2014/04/09 20:21:41]

I found a bug this code, causing me to change my mind.  I'm going to take your
advice and put it in a common place so I don't have to remember to make the same
fixes in multiple places.

   const GURL& url(title_match.node->url());
+  const base::string16& url_utf16 = base::UTF8ToUTF16(url.spec());
+  const URLPrefix* best_prefix = URLPrefix::BestURLPrefix(url_utf16,
+                                                          input.text());
+  base::string16 matching_string = input.text();
+  // If we failed to find a best_prefix initially, try again using a fixed-up
+  // version of the user input.  This is especially useful to get about: URLs
+  // to inline against chrome:// URLs.  (about: URLs are fixed up to the
+  // chrome:// scheme.)
+  if ((best_prefix == NULL) && !fixed_up_input.text().empty() &&
+      (fixed_up_input.text() != input.text())) {
+    best_prefix = URLPrefix::BestURLPrefix(url_utf16, fixed_up_input.text());
+    matching_string = fixed_up_input.text();
+  }
+  size_t inline_autocomplete_offset =
+      (best_prefix != NULL) ?
+      (best_prefix->prefix.length() + matching_string.length()) :
+      base::string16::npos;
+
   match.destination_url = url;
+  const bool trim_http = !AutocompleteInput::HasHTTPScheme(input.text()) &&
+      ((best_prefix != NULL) || (inline_autocomplete_offset != 0));

[Peter Kasting, 2014/04/09 00:53:32]

I don't think I understand this conditional.

Wouldn't this trim if the user text was "http" (since we'd get a non-null best
prefix of "")?  Also, if best_prefix is NULL, isn't inline_autocomplete_offset
== base::string16::npos, and thus nonzero, so the second line here always
evaluates to true?

[Mark P, 2014/04/09 02:44:42]

Oops, you're right.  It should be best_prefix == NULL, not best_prefix != NULL.
Done.

   match.contents = net::FormatUrl(url, languages_,
-      net::kFormatUrlOmitAll & net::kFormatUrlOmitHTTP,
-      net::UnescapeRule::SPACES, NULL, NULL, NULL);
+      net::kFormatUrlOmitAll & ~(trim_http ? 0 : net::kFormatUrlOmitHTTP),
+      net::UnescapeRule::SPACES, NULL, NULL, &inline_autocomplete_offset);
   match.contents_class.push_back(
       ACMatchClassification(0, ACMatchClassification::URL));
   match.fill_into_edit =
       AutocompleteInput::FormattedStringWithEquivalentMeaning(url,
                                                               match.contents);
+  if (inline_autocomplete_offset != base::string16::npos) {
+    // |inline_autocomplete_offset| may be beyond the end of the
+    // |fill_into_edit| if the user has typed an URL with a scheme and the
+    // last character typed is a slash.  That slash is removed by the
+    // FormatURLWithOffsets call above.
+    if (inline_autocomplete_offset < match.fill_into_edit.length()) {
+      match.inline_autocompletion =
+          match.fill_into_edit.substr(inline_autocomplete_offset);
+    }
+    match.allowed_to_be_default_match = match.inline_autocompletion.empty() ||
+        !input.prevent_inline_autocomplete();
+  }
   match.description = title;
   match.description_class =
       ClassificationsFromMatch(title_match.match_positions,