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

components/omnibox/autocomplete_match.cc

 // Copyright 2014 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 "components/omnibox/autocomplete_match.h"
 
 #include "base/i18n/time_formatting.h"
 #include "base/logging.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.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 "components/omnibox/autocomplete_provider.h"
 #include "components/omnibox/suggestion_answer.h"
 #include "components/search_engines/template_url.h"
 #include "components/search_engines/template_url_service.h"
 #include "grit/components_scaled_resources.h"
 
 namespace {
 
 bool IsTrivialClassification(const ACMatchClassifications& classifications) {
   return classifications.empty() ||
       ((classifications.size() == 1) &&
        (classifications.back().style == ACMatchClassification::NONE));
 }
 
+// Returns true if one of the input words includes a scheme and followed by
+// something that matches at the beginning of the URL's hostname.  This
+// indicates that the user desires this URL loaded with this scheme in

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

Nit: "This suggests that the user may want this particular scheme, and the
caller should not consider another URL like this one but with a different scheme
to be a duplicate."

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

Done.

+// particular.
+bool InputIncludesSchemePlusPartOfHost(
+    const std::vector<base::string16>& input_words,
+    const GURL& url) {
+  // Find scheme separator and see if the stuff following it matches the
+  // beginning of the URL (after the scheme).
+  const base::string16& separator_as_utf16 =
+      base::UTF8ToUTF16(url::kStandardSchemeSeparator);
+  const base::string16& url_spec_without_scheme_as_utf16 =
+      base::UTF8ToUTF16(url.spec().substr(
+          url.scheme().length() +
+          std::string(url::kStandardSchemeSeparator).length()));
+  for (auto it : input_words) {
+    const size_t pos = it.find(separator_as_utf16);
+    if ((pos != base::string16::npos) && (pos > 0) &&

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

I'm still thinking about this (pos > 0) check.

I think we should actually look for "http://" and "https://" instead of just
"<anything>://".  If I type in "ftp://mozilla.com", it seems OK -- preferable,
even -- to dupe http://mozilla.com and https://mozilla.com together.  This is
even more true if I type garbage like "!@#://mozilla.com".

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

Your proposal requires explicitly looking at the scheme and making decisions
based on it.  In other words, if the users enters "http" or "https" you want to
disable dedupping those two things.  There is no where to implement your code
without explicitly using those URLs constants.  Do you really want me to do such
a narrow solution?  i.e., it won't apply by default to new protocols we
introduce.

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

Yes.  This should be in sync with the actual scheme manipulation we do in
ComputeStrippedDestinationURL().  Right now we only convert HTTPS->HTTP, so
those are the schemes we should look at here.  If we were to add more schemes
there we would want to add them here too.  (We could even add a comment in one
or both of these places talking about the need to stay in sync.)

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

Acknowledged.

+        StartsWith(url_spec_without_scheme_as_utf16,
+                  it.substr(pos + separator_as_utf16.length()), false)) {
+      return true;
+    }
+  }
+  return false;
+}
+
 }  // namespace
 
 // AutocompleteMatch ----------------------------------------------------------
 
 // static
 const base::char16 AutocompleteMatch::kInvalidChars[] = {
   '\n', '\r', '\t',
   0x2028,  // Line separator
   0x2029,  // Paragraph separator
   0
@@ skipping 324 matching lines @@
     return NULL;
   TemplateURL* template_url = keyword.empty() ?
       NULL : template_url_service->GetTemplateURLForKeyword(keyword);
   return (template_url || host.empty()) ?
       template_url : template_url_service->GetTemplateURLForHost(host);
 }
 
 // static
 GURL AutocompleteMatch::GURLToStrippedGURL(
     const GURL& url,
+    const std::vector<base::string16>& input_words,
     TemplateURLService* template_url_service,
     const base::string16& keyword) {
   if (!url.is_valid())
     return url;
 
   GURL stripped_destination_url = url;
 
   // If the destination URL looks like it was generated from a TemplateURL,
   // remove all substitutions other than the search terms.  This allows us
   // to eliminate cases like past search URLs from history that differ only
@@ skipping 25 matching lines @@
 
   // Remove the www. prefix from the host.
   static const char prefix[] = "www.";
   static const size_t prefix_len = arraysize(prefix) - 1;
   std::string host = stripped_destination_url.host();
   if (host.compare(0, prefix_len, prefix) == 0) {
     replacements.SetHostStr(base::StringPiece(host).substr(prefix_len));
     needs_replacement = true;
   }
 
-  // Replace https protocol with http protocol.
-  if (stripped_destination_url.SchemeIs(url::kHttpsScheme)) {
+  // Possibly replace https protocol with http protocol.

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

Nit: "Replace https protocol with http, as long as the user didn't explicitly
specify one of the two."  (Goes along with my above suggestion)

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

Done.

+  if (!InputIncludesSchemePlusPartOfHost(input_words, url) &&
+      stripped_destination_url.SchemeIs(url::kHttpsScheme)) {
     replacements.SetScheme(url::kHttpScheme,
                            url::Component(0, strlen(url::kHttpScheme)));
     needs_replacement = true;
   }
 
   if (needs_replacement)
     stripped_destination_url = stripped_destination_url.ReplaceComponents(
         replacements);
   return stripped_destination_url;
 }
 
 void AutocompleteMatch::ComputeStrippedDestinationURL(
+    const std::vector<base::string16>& input_words,
     TemplateURLService* template_url_service) {
-  stripped_destination_url =
-      GURLToStrippedGURL(destination_url, template_url_service, keyword);
+  stripped_destination_url = GURLToStrippedGURL(
+      destination_url, input_words, template_url_service, keyword);
 }
 
 void AutocompleteMatch::EnsureUWYTIsAllowedToBeDefault(
     const GURL& canonical_input_url,
+    const std::vector<base::string16>& input_words,
     TemplateURLService* template_url_service) {
   if (!allowed_to_be_default_match) {
     const GURL& stripped_canonical_input_url =
         AutocompleteMatch::GURLToStrippedGURL(
-            canonical_input_url, template_url_service, base::string16());
-    ComputeStrippedDestinationURL(template_url_service);
+            canonical_input_url, input_words,
+            template_url_service, base::string16());
+    ComputeStrippedDestinationURL(input_words, template_url_service);
     allowed_to_be_default_match =
         stripped_canonical_input_url == stripped_destination_url;
   }
 }
 
+void AutocompleteMatch::EnsureUWYTIsAllowedToBeDefault(
+    const AutocompleteInput& input,
+    TemplateURLService* template_url_service) {
+  std::vector<base::string16> words;
+  base::SplitString(input.text(), ' ', &words);
+  EnsureUWYTIsAllowedToBeDefault(
+      input.canonicalized_url(), words, template_url_service);
+}
+
 void AutocompleteMatch::GetKeywordUIState(
     TemplateURLService* template_url_service,
     base::string16* keyword,
     bool* is_keyword_hint) const {
   *is_keyword_hint = associated_keyword.get() != NULL;
   keyword->assign(*is_keyword_hint ? associated_keyword->keyword :
       GetSubstitutingExplicitlyInvokedKeyword(template_url_service));
 }
 
 base::string16 AutocompleteMatch::GetSubstitutingExplicitlyInvokedKeyword(
@@ skipping 97 matching lines @@
         << " is unsorted in relation to last offset of " << last_offset
         << ". Provider: " << provider_name << ".";
     DCHECK_LT(i->offset, text.length())
         << " Classification of [" << i->offset << "," << text.length()
         << "] is out of bounds for \"" << text << "\". Provider: "
         << provider_name << ".";
     last_offset = i->offset;
   }
 }
 #endif