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>
 
 #include "base/memory/scoped_ptr.h"
+#include "components/omnibox/autocomplete_input.h"
 #include "components/omnibox/autocomplete_match_type.h"
 #include "components/search_engines/template_url.h"
 #include "ui/base/page_transition_types.h"
 #include "url/gurl.h"
 
 class AutocompleteProvider;
 class SuggestionAnswer;
 class TemplateURL;
 class TemplateURLService;
 
@@ skipping 153 matching lines @@
 
   // Returns |url| altered by stripping off "www.", converting https protocol
   // 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.
-  static GURL GURLToStrippedGURL(const GURL& url,
-                                 TemplateURLService* template_url_service,
-                                 const base::string16& keyword);
+  // proper deduping.  |input_words| is used to decide if the scheme is allowed
+  // to be altered during stripping.  (If the user indicated a desired scheme,

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

Nit: Vague; "If the user indicated a desired scheme" -> "If one of the words in
the input looks like a scheme plus the beginning of this URL's hostname"?

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

Done.

+  // we prevent two URLs with different schemes from having the same stripped
+  // GURL.)
+  static GURL GURLToStrippedGURL(
+      const GURL& url,
+      const std::vector<base::string16>& input_words,
+      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 std::vector<base::string16>& input_words,
+      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,
-                                      TemplateURLService* template_url_service);
+  // AutocompleteInput::canonicalized_url()).  |inputs_words| is the input
+  // split based on spaces into words.

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

Nit: "...is the original input string, tokenized into words using the space
character"?

You might want to include a sentence of your rationale to me about why using
space here was appropriate.

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

Comment is now obsolete.

+  void EnsureUWYTIsAllowedToBeDefault(
+      const GURL& canonical_input_url,
+      const std::vector<base::string16>& input_words,
+      TemplateURLService* template_url_service);
+  // The same as above except only takes an AutocompleteInput and does the
+  // splitting internally.
+  void EnsureUWYTIsAllowedToBeDefault(

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

My one concern with this API is that it will lead to a lot of duplicated effort,
especially with long input strings, since we'll re-split the input on every call
(which could be at least once per generated match per provider that calls this).
 After past "Pasting 10MB of data is slow" bugs I'm keen not to add more
redundant calls.

I wonder if we should be caching this on the AutocompleteInput instead, or
something.

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

I'm not philosophically keen on the notion of caching things in
AutocompleteInput.
I don't know why.  I think I don't like the idea of keeping "value" and "is
already computed" fields.
But I suppose we can simply compute the split version when the AutocompleteInput
is constructed and then use it as needed later, only one field needed.
This seems reasonable.  Is there a better solution?

I'm happy to do this if you want and can't think of anything better.  Let me
know.

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

By "only one field needed" do you mean "only one additional field needed
compared to now"?

I was thinking more about this and I think we could limit the amount of
additional storage needed by keeping a vector of words that only contains those
words that start with "http://" or "https://" plus one additional letter.  That
will mean that for most inputs the vector will be empty and thus we'll have
minimal additional memory cost/copying cost; and it will let us simplify and
speed up the code that looks for matches of these words against various URLs.

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

Yes.
Yes, I can do it this way.  That's definitely more efficient.  If you're willing
to let me do this, I'm okay with it.  I thought you'd be opposed because it
seems so particular / focussed, not more generally useful (as a full word vector
in AutocompleteInput would be).

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

We don't have any other places that need such a vector, though, so there's no
motivation for a generalized solution.  (I'm a big fan of "build what you need
now, reefactor later if your needs change".)

Let's go ahead with this route, it seems maximally efficient.

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

Done.

+      const AutocompleteInput& input,
+      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
   // cases, |keyword| will be cleared, even when our member variable |keyword|
@@ skipping 173 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_