Omnibox: Fix URL-What-You-Typed Allowed-To-Be-Default-Match Issues

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 94192c4ef2206e2703eceea9a12f262825434d1c..5e1b7b11f4447306afe7e355224da9758727e292 100644
--- a/chrome/browser/autocomplete/shortcuts_provider.cc
+++ b/chrome/browser/autocomplete/shortcuts_provider.cc
@@ -140,8 +140,9 @@ void ShortcutsProvider::GetMatches(const AutocompleteInput& input) {
   base::string16 term_string(base::i18n::ToLower(input.text()));
   DCHECK(!term_string.empty());
 
-  const GURL& input_as_gurl =
-      url_fixer::FixupURL(base::UTF16ToUTF8(input.text()), std::string());
+  const GURL& stripped_canonical_input_url =
+      AutocompleteMatch::GURLToStrippedGURL(
+          input.canonicalized_url(), profile_, base::string16());
   const base::string16 fixed_up_input(FixupUserInput(input).second);
 
   int max_relevance;
@@ -157,7 +158,8 @@ void ShortcutsProvider::GetMatches(const AutocompleteInput& input) {
     int relevance = CalculateScore(term_string, it->second, max_relevance);
     if (relevance) {
       matches_.push_back(ShortcutToACMatch(it->second, relevance, input,
-                                           fixed_up_input, input_as_gurl));
+                                           fixed_up_input,
+                                           stripped_canonical_input_url));
       matches_.back().ComputeStrippedDestinationURL(profile_);
     }
   }
@@ -190,7 +192,7 @@ AutocompleteMatch ShortcutsProvider::ShortcutToACMatch(
     int relevance,
     const AutocompleteInput& input,
     const base::string16& fixed_up_input_text,
-    const GURL& input_as_gurl) {
+    const GURL& stripped_canonical_input_url) {
   DCHECK(!input.text().empty());
   AutocompleteMatch match;
   match.provider = this;
@@ -243,12 +245,15 @@ AutocompleteMatch ShortcutsProvider::ShortcutToACMatch(
           match.inline_autocompletion.empty();
     } else {
       // Also allow a user's input to be marked as default if it would be fixed
-      // up to the same thing as the fill_into_edit.  This handles cases like
+      // up to the same thing as the fill_into_edit (or something that would be
+      // dupped against the fill_into_edit match).  This handles cases like
       // the user input containing a trailing slash absent in fill_into_edit.
       match.allowed_to_be_default_match =
-          (input_as_gurl ==
-           url_fixer::FixupURL(base::UTF16ToUTF8(match.fill_into_edit),
-                               std::string()));
+          (stripped_canonical_input_url ==
+           AutocompleteMatch::GURLToStrippedGURL(
+               url_fixer::FixupURL(base::UTF16ToUTF8(match.fill_into_edit),

[Peter Kasting, 2014/06/30 23:02:58]

Why are we computing the stripped fixed-up fill_into_edit, instead of just
looking at |match|'s stripped destination URL as the other providers you've
changed do?

[Mark P, 2014/06/30 23:27:36]

Because when I originally wrote this code, you insisted that we set
allowed_to_be_default_match using the URL indicated by fill_into_edit and not
match's destination URL.  When I wrote this revision, I kept that property.

If you wish to take back that assertion, I can comply.

[Peter Kasting, 2014/06/30 23:31:25]

I think it doesn't make sense to be inconsistent.  Some providers use the
destination URL, this one uses the fill_into_edit.  When we talked about this
last week, my memory is that we eventually agreed that the destination URL was
the right thing to use.  If so, we should use it here too.

This may make the code here similar enough to the other two places that you can
hoist all three into AutocompleteMatch, though I'm not sure.

[Mark P, 2014/07/01 17:45:33]

Yes, the code looks very similar.  I can refactor it into a helper function in
AutocompleteMatch if you want (EnsureUWYTIsAllowedToBeDefault()).  I thought for
a bit about whether I can put this code somewhere so that it gets called
automatically on all matches from all providers, which would be nice, but I
think this code needs to be called within the providers so providers can react
to the results of it.  e.g., SearchProvider will know about the correct
allowed_to_be_default_match state when checking constraints.

Sounds good?

+                                   std::string()),
+               profile_, base::string16()));
     }
   }