Omnibox: Fix URL-What-You-Typed Allowed-To-Be-Default-Match Issues

chrome/browser/autocomplete/search_provider_unittest.cc

 // Copyright 2012 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 "chrome/browser/autocomplete/search_provider.h"
 
 #include <string>
 
 #include "base/command_line.h"
 #include "base/metrics/field_trial.h"
@@ skipping 2189 matching lines @@
   } cases[] = {
     // Do not inline matches that do not contain the input; trim http as needed.
     { "x",                 "http://www.abc.com",
                                   "www.abc.com",  std::string(), false, false },
     { "https:",            "http://www.abc.com",
                                   "www.abc.com",  std::string(), false, false },
     { "http://www.abc.com/a", "http://www.abc.com",
                               "http://www.abc.com",  std::string(), false,
                                                                     false },
     { "http://www.abc.com",   "https://www.abc.com",
-                              "https://www.abc.com", std::string(), false,
-                                                                    false },
+                              "https://www.abc.com", std::string(), true,
+                                                                    true },

[Peter Kasting, 2014/07/09 00:46:43]

Hmm.  This seems like a bad outcome.  I think if the user explicitly types a
scheme, we should only allow to be default a match that matches that scheme.

I wonder if there are already deduping bugs as a result of this -- like, if the
HUP returns both http://foo.com (because you typed it) and https://foo.com
(because you visited it), maybe one gets deduped to the other.  Or maybe we're
relying today on "no provider will suggest the latter when you type the former",
in which case we should just ensure this test tests what providers could
actually do.

[Mark P, 2014/07/09 19:54:17]

I agree.  Yet [see below]
You're right: we're already doing dedupping like this.  I think what's saving us
is that generally if the user types http://whatever, no provider is likely to
suggest https://whatever, and vice versa.

Do you want me to remove the tests in this unit test that involve scheme
changes?

Also, if you want, I could add something to the autocomplete_result.cc code:
      if (AutocompleteMatch::IsSearchType(default_match_->type)) {
        DCHECK_NE(metrics::OmniboxInputType::URL, input.type()) << debug_info;
      } else {
        DCHECK_NE(metrics::OmniboxInputType::FORCED_QUERY, input.type())
            << debug_info;
      }
I could add a block within the else clause that checks
  if ((input.type() == URL) && !LowerCaseEqualsASCII(input.scheme(),
url::kHttpScheme)) {
    DCHECK_EQ(input.canonicalized_url().scheme(),
default_match_->destination_url.scheme());
  }
i.e., if the user explicitly typed a non-http scheme, then make sure the
destination_url keeps the same scheme.
(input.scheme() may be http if the user typed it or if the user didn't type a
scheme at all.  FixupURL and Parse infer the http scheme on inputs where no
exist.  So non-http means the user must've explicitly typed a scheme.)
(It would be harder to simply say "if user explicitly typed a scheme then make
sure the destination URL scheme matches."  Also, I don't think the http->https
transition is a problem, which is probably the most likely transition that comes
from http.)
Frankly, I don't think this test will catch much if anything but I can add it if
you want.

[Peter Kasting, 2014/07/09 21:05:03]

Yeah, I think we should.
I think the only test that would make sense to add is the one you note would be
harder: if the user explicitly typed a scheme, ensure the default match has that
scheme.

I don't know why that would be hard to implement, though.  Don't you just need
to check if AutocompleteInput::parts.scheme.is_nonempty()?  As for defaulting to
https when the user explicitly typed http://... hmm.  I feel like this should
probably be an error; the case where it might not is if the host is HSTS-pinned,
and somehow the omnibox code knows that (it doesn't today).

[Mark P, 2014/07/09 22:31:36]

Done.
Oh, I guess you're right.  I assumed that parts would correspond to
canonicalized_url and have lost memory of what was originally empty or
non-empty.  i.e., I noticed that scheme() in autocomplete_input is set (to
"http") even when not typed explicitly.  I assumed autocomplete_input's parts
would agree.  Apparently it doesn't so I can do what we both want.

Done.

     { "http://abc.com",       "ftp://abc.com",
                               "ftp://abc.com",       std::string(), false,
                                                                     false },
     { "https://www.abc.com",  "http://www.abc.com",
-                                     "www.abc.com",  std::string(), false,
-                                                                    false },
+                                     "www.abc.com",  std::string(), true,
+                                                                    true },
     { "ftp://abc.com",        "http://abc.com",
                                      "abc.com",      std::string(), false,
                                                                     false },
 
     // Do not inline matches with invalid input prefixes; trim http as needed.
     { "ttp",              "http://www.abc.com",
                                  "www.abc.com", std::string(), false, false },
     { "://w",             "http://www.abc.com",
                                  "www.abc.com", std::string(), false, false },
     { "ww.",              "http://www.abc.com",
@@ skipping 27 matching lines @@
                              "http://www.abc.com", std::string(), true, true },
 
     // Inputs with trailing whitespace should inline when possible.
     { "abc.com ",      "http://www.abc.com",
                               "www.abc.com",      std::string(), true,  true },
     { "abc.com/ ",     "http://www.abc.com",
                               "www.abc.com",      std::string(), true,  true },
     { "abc.com ",      "http://www.abc.com/bar",
                               "www.abc.com/bar",  "/bar",        false, false },
 
+    // A suggestion that's equivalent to what the input gets fixed up to
+    // should be inlined.
+    { "abc.com:",      "http://abc.com/",
+                              "abc.com",      "", true, true },
+    { "abc.com:",      "http://www.abc.com",
+                              "www.abc.com",  "", true, true },
+
     // Inline matches when the input is a leading substring of the scheme.
     { "h",             "http://www.abc.com",
                        "http://www.abc.com", "ttp://www.abc.com", true, false },
     { "http",          "http://www.abc.com",
                        "http://www.abc.com", "://www.abc.com",    true, false },
 
     // Inline matches when the input is a leading substring of the full URL.
     { "http:",             "http://www.abc.com",
                            "http://www.abc.com", "//www.abc.com", true, false },
     { "http://w",          "http://www.abc.com",
@@ skipping 921 matching lines @@
 
   // The expiration time is always updated.
   provider_->GetSessionToken();
   base::TimeTicks expiration_time_1 = provider_->token_expiration_time_;
   base::PlatformThread::Sleep(kSmallDelta);
   provider_->GetSessionToken();
   base::TimeTicks expiration_time_2 = provider_->token_expiration_time_;
   EXPECT_GT(expiration_time_2, expiration_time_1);
   EXPECT_GE(expiration_time_2, expiration_time_1 + kSmallDelta);
 }