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

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 122 matching lines @@
 void ShortcutsProvider::GetMatches(const AutocompleteInput& input) {
   scoped_refptr<ShortcutsBackend> backend =
       ShortcutsBackendFactory::GetForProfileIfExists(profile_);
   if (!backend.get())
     return;
   // Get the URLs from the shortcuts database with keys that partially or
   // completely match the search term.
   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;
   if (!OmniboxFieldTrial::ShortcutsScoringMaxRelevance(
       input.current_page_classification(), &max_relevance))
     max_relevance = kShortcutsProviderDefaultMaxRelevance;
 
   for (ShortcutsBackend::ShortcutMap::const_iterator it =
            FindFirstMatch(term_string, backend.get());
        it != backend->shortcuts_map().end() &&
            StartsWith(it->first, term_string, true); ++it) {
     // Don't return shortcuts with zero relevance.
     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_);
     }
   }
   // Remove duplicates.  Duplicates don't need to be preserved in the matches
   // because they are only used for deletions, and shortcuts deletes matches
   // based on the URL.
   AutocompleteResult::DedupMatchesByDestination(
       input.current_page_classification(), false, &matches_);
   // Find best matches.
   std::partial_sort(matches_.begin(),
@@ skipping 12 matching lines @@
     if (max_relevance > 1)
       --max_relevance;
   }
 }
 
 AutocompleteMatch ShortcutsProvider::ShortcutToACMatch(
     const history::ShortcutsDatabase::Shortcut& shortcut,
     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;
   match.relevance = relevance;
   match.deletable = true;
   match.fill_into_edit = shortcut.match_core.fill_into_edit;
   match.destination_url = shortcut.match_core.destination_url;
   DCHECK(match.destination_url.is_valid());
   match.contents = shortcut.match_core.contents;
   match.contents_class = AutocompleteMatch::ClassificationsFromString(
@@ skipping 32 matching lines @@
         URLPrefix::GetInlineAutocompleteOffset(
             input.text(), fixed_up_input_text, true, match.fill_into_edit);
     if (inline_autocomplete_offset != base::string16::npos) {
       match.inline_autocompletion =
           match.fill_into_edit.substr(inline_autocomplete_offset);
       match.allowed_to_be_default_match =
           !HistoryProvider::PreventInlineAutocomplete(input) ||
           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()));
     }
   }
 
   // Try to mark pieces of the contents and description as matches if they
   // appear in |input.text()|.
   const base::string16 term_string = base::i18n::ToLower(input.text());
   WordMap terms_map(CreateWordMapForString(term_string));
   if (!terms_map.empty()) {
     match.contents_class = ClassifyAllMatchesInString(term_string, terms_map,
         match.contents, match.contents_class);
@@ skipping 152 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);
 }