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

components/omnibox/autocomplete_match.h

Index: components/omnibox/autocomplete_match.h
diff --git a/components/omnibox/autocomplete_match.h b/components/omnibox/autocomplete_match.h
index 0464d73ac275e41a2c753bc5a6925828ddc70cbd..53cf8283fc51750c6cb0d40c69924a6ce7981e34 100644
--- a/components/omnibox/autocomplete_match.h
+++ b/components/omnibox/autocomplete_match.h
@@ -10,6 +10,7 @@
 #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"
@@ -183,22 +184,37 @@ struct AutocompleteMatch {
   // 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.