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

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 8ead62e723d8dad0cccf6424efd8abfcb4feb7df..5f54dd97d72cb8ded68993a9b66866f405baffc1 100644
--- a/chrome/browser/autocomplete/shortcuts_provider.cc
+++ b/chrome/browser/autocomplete/shortcuts_provider.cc
@@ -15,6 +15,7 @@
 #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"
@@ -143,6 +144,8 @@ void ShortcutsProvider::GetMatches(const AutocompleteInput& input) {
   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() &&
@@ -151,8 +154,9 @@ 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));
-      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
@@ -183,7 +187,8 @@ 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;
@@ -236,7 +241,7 @@ AutocompleteMatch ShortcutsProvider::ShortcutToACMatch(
     }
   }
   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