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

components/omnibox/autocomplete_match.h

 // 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.
 
 #ifndef COMPONENTS_OMNIBOX_AUTOCOMPLETE_MATCH_H_
 #define COMPONENTS_OMNIBOX_AUTOCOMPLETE_MATCH_H_
 
 #include <map>
 #include <string>
 #include <vector>
@@ skipping 156 matching lines @@
 
   // A static version GetTemplateURL() that takes the match's keyword and
   // match's hostname as parameters.  In short, returns the TemplateURL
   // associated with |keyword| if it exists; otherwise returns the TemplateURL
   // associated with |host| if it exists.
   static TemplateURL* GetTemplateURLWithKeyword(
       TemplateURLService* template_url_service,
       const base::string16& keyword,
       const std::string& host);
 
   // Returns |url| altered by stripping off "www.", converting https protocol

[Peter Kasting, 2015/04/25 00:03:12]

It strikes me that there's probably a similar issue with "www."-stripping: if
the user only typed foo.com, then probably foo.com and www.foo.com should be
duped.  But if the user typed www.foo.com, then "foo.com" is not the same thing
and shouldn't be duped.

I doubt this is as critical of a distinction, but it seems like if we're fixing
one issue, we should fix the other as well.

Technically, this probably applies to query params too -- if the user explicitly
adds "&foo=1", he won't want it stripped -- but in practice this seems rare.

[Mark P, 2015/04/27 22:26:05]

I admit this is a distinction.  However, I know from watching user studies that
people often add www. unnecessarily, and I don't want this mistake to cause
problems.  In particular, I think the number of people who add www. when they
shouldn't is much larger than the number of people who care that adding www.
means that don't want the non-www. version simultaneously/dupped together.
I agree.

   // to http, and stripping excess query parameters.  These conversions are
   // merely to allow comparisons to remove likely duplicates; these URLs are
   // not used as actual destination URLs.  If |template_url_service| is not
   // NULL, it is used to get a template URL corresponding to this match.  If
   // the match's keyword is known, it can be passed in.  Otherwise, it can be
   // left empty and the template URL (if any) is determined from the
   // destination's hostname.  The template URL is used to strip off query args
   // other than the search terms themselves that would otherwise prevent doing
-  // proper deduping.
+  // proper deduping.  If |scheme_explicitly_entered| then the scheme is not
+  // allowed to be altered during stripping, thus preventing two URLs with
+  // different schemes from having the same stripped GURL.

[Peter Kasting, 2015/04/25 00:03:12]

Hmm, this doesn't seem quite right.

Consider user input of some version of "foo.com" and matches for various origins
from foo.com and bar.com.

Whether the user explicitly types a scheme should affect deduping of foo.com
matches, but I don't think it should affect deduping of bar.com ones.  That is:

"foo.com" -> user doesn't care about http://foo.com vs. https://foo.com
"http[s]://foo.com" -> user does care about the difference;
...but in both cases, user doesn't care about http://bar.com vs. https://bar.com

Thinking about this more, in conjunction with my comment above about "www.", I
realize that this issue is this:

Given a particular user input, we might normally mark as dupes matches where one
match is inlineable and one is not.  Instead we should be unwilling to dupe two
matches if their inlineability differs.

In the case above, when typing "foo.com", all variants of foo.com (http vs.
https, www. vs. no-www.) are inlineable.  So it's safe to dupe one against
another.  Likewise, all variants of bar.com are non-inlineable, and again, it's
safe to dupe.

But if the user types "http://foo.com", then the two "http" forms of "foo.com"
are still inlineable, but the "https" forms aren't.  We should allow duping
within each pair, but not across pairs.  And if the user types
"https://www.foo.com", then none of the other forms can be inlined against that,
so none should be duped against it.  Meanwhile, none of these inputs changes the
(non-)inlineability of the bar.com matches, so those should all still be
dupeable.

So, basically, I think the answer is to pass in sufficient info about the user's
typing to understand whether some of the things we'd normally strip were
explicitly present in the original typing _and the hostnames are the same_, and
if so, don't strip those things.

[Mark P, 2015/04/27 22:26:05]

Astute analysis.

Here's a follow-up question: if a user types "https foo", do you want to prevent
http/https matches getting dupped together?

   static GURL GURLToStrippedGURL(const GURL& url,
+                                 const bool scheme_explicitly_entered,
                                  TemplateURLService* template_url_service,
                                  const base::string16& keyword);
 
   // Computes the stripped destination URL (via GURLToStrippedGURL()) and
   // stores the result in |stripped_destination_url|.
-  void ComputeStrippedDestinationURL(TemplateURLService* template_url_service);
+  void ComputeStrippedDestinationURL(const bool scheme_explicitly_entered,
+                                     TemplateURLService* template_url_service);
 
   // Sets |allowed_to_be_default_match| to true if this match is effectively
   // the URL-what-you-typed match (i.e., would be dupped against the UWYT
   // match when AutocompleteResult merges matches).  |canonical_input_url| is
   // the AutocompleteInput interpreted as a URL (i.e.,
   // AutocompleteInput::canonicalized_url()).
   void EnsureUWYTIsAllowedToBeDefault(const GURL& canonical_input_url,
+                                      const bool scheme_explicitly_entered,
                                       TemplateURLService* template_url_service);
 
   // Gets data relevant to whether there should be any special keyword-related
   // UI shown for this match.  If this match represents a selected keyword, i.e.
   // the UI should be "in keyword mode", |keyword| will be set to the keyword
   // and |is_keyword_hint| will be set to false.  If this match has a non-NULL
   // |associated_keyword|, i.e. we should show a "Press [tab] to search ___"
   // hint and allow the user to toggle into keyword mode, |keyword| will be set
   // to the associated keyword and |is_keyword_hint| will be set to true.  Note
   // that only one of these states can be in effect at once.  In all other
@@ skipping 178 matching lines @@
       const base::string16& text,
       const ACMatchClassifications& classifications) const;
 #endif
 };
 
 typedef AutocompleteMatch::ACMatchClassification ACMatchClassification;
 typedef std::vector<ACMatchClassification> ACMatchClassifications;
 typedef std::vector<AutocompleteMatch> ACMatches;
 
 #endif  // COMPONENTS_OMNIBOX_AUTOCOMPLETE_MATCH_H_