Omnibox - Do Not Allow HTTP/HTTPS Equivalence if User Explicitly Entered A Scheme

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>
 
 #include "base/i18n/break_iterator.h"
 #include "base/i18n/case_conversion.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "chrome/browser/autocomplete/history_provider.h"
 #include "chrome/browser/autocomplete/shortcuts_backend_factory.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
@@ skipping 108 matching lines @@
   base::string16 term_string(base::i18n::ToLower(input.text()));
   DCHECK(!term_string.empty());
 
   int max_relevance;
   if (!OmniboxFieldTrial::ShortcutsScoringMaxRelevance(
       input.current_page_classification(), &max_relevance))
     max_relevance = kShortcutsProviderDefaultMaxRelevance;
   TemplateURLService* template_url_service =
       TemplateURLServiceFactory::GetForProfile(profile_);
   const base::string16 fixed_up_input(FixupUserInput(input).second);
+  std::vector<base::string16> words;
+  base::SplitString(input.text(), ' ', &words);

[Peter Kasting, 2015/06/06 01:31:23]

Hmmm.

Is it realy necessary for the shortcuts system to be doing all this fixup?  At a
glance it looks to me like this is basically what the controller will be doing
anyway: computing the stripped destination URL and then de-duping by
destination.  I don't really see why ShortcutsProvider needs to be a special
snowflake in regards to pre-deleting these before returning, but the other
providers don't.

It seems like if it's important quality-wise that we get kMaxMatches
post-deduped matches here, the same could be said for other providers, and
perhaps the answer would be to expose the relevant computation/de-duping on
AutocompleteResult/AutocompleteController/somewhere as a method all providers
could (and should) call before returning.

[Mark P, 2015/06/06 17:10:11]

No.  Prior to fixing bug 260799, Shortcuts did not do any duplicate elimination,
not even for URLs that are identical and just associated with different shortcut
strings.  I can make the duplicate elimination in ShortcutsProvider more naive
by using exact URLs if you want.  That'll simplify the API changes necessary
because no one (besides AutocompleteResult) will have to explicitly call
ComputeStrippedDestinationUrl().

I don't think it's important quality-wise to do correct full dedupping here
(as long as shortcuts is smart about how it dedups query strings if I switch
to the exact URL comparison--I should check this).

[Peter Kasting, 2015/06/06 18:38:15]

I'm wondering if we can go even further and avoid doing any de-duping at all
without a significant quality impact.

[Mark P, 2015/06/06 20:23:06]

Look at the pointed-to bug.  For almost any input, avoiding de-dupping between
shortcuts means ShortcutsProvider will only return one URL (three times, each
associated with different shortcut text).  I think this would be a quality loss.

[Peter Kasting, 2015/06/08 20:26:59]

OK.  Can we add comments about why it's more important for the shortcuts
provider to do this than most providers?

[Mark P, 2015/06/09 19:29:38]

Okay, I added some comments about why it's important to do this (below in this
file).

   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));
-      matches_.back().ComputeStrippedDestinationURL(template_url_service);
+                                           fixed_up_input, words));
+      matches_.back().ComputeStrippedDestinationURL(words,
+                                                    template_url_service);
     }
   }
   // 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(),
       matches_.begin() +
@@ skipping 10 matching lines @@
     it->relevance = max_relevance;
     if (max_relevance > 1)
       --max_relevance;
   }
 }
 
 AutocompleteMatch ShortcutsProvider::ShortcutToACMatch(
     const ShortcutsDatabase::Shortcut& shortcut,
     int relevance,
     const AutocompleteInput& input,
-    const base::string16& fixed_up_input_text) {
+    const base::string16& fixed_up_input_text,
+    const std::vector<base::string16>& words) {
   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 @@
             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();
     }
   }
   match.EnsureUWYTIsAllowedToBeDefault(
-      input.canonicalized_url(),
+      input.canonicalized_url(), words,

[Peter Kasting, 2015/06/06 01:31:23]

Instead of passing |words| to this function we could just call the version of
EnsureUWYTIsAllowedToBeDefault() that takes an AutocompleteInput.  I believe
this is the only place where you use the URL-and-vector version of that
function, so this would also eliminate a method from the class API.

This would be a duplication of effort with what you do above in GetMatches(),
but if you eliminate that code, that issue goes away.

[Mark P, 2015/06/06 17:10:11]

I'm passing |words| here so as to above the version that takes |input|.  This
function is called in a loop and don't want to have to continually split |input|
on |words| if I call the latter rather than the former.

[Peter Kasting, 2015/06/06 18:38:16]

OK, but this seems like an argument in favor of my suggestion of caching the
words on the AutocompleteInput.

[Mark P, 2015/06/06 20:23:06]

Yes, it is.

       TemplateURLServiceFactory::GetForProfile(profile_));
 
   // 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);
     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);
 }