Commit cd966170 authored by Kevin Bailey's avatar Kevin Bailey Committed by Commit Bot

[omnibox] Make GURLToStrippedGURL() remove refs

Refs (or hashes) are useful for navigating, but not useful for
comparing URLs. We compare URLs when deduping URLs in Omnibox
matches, and when testing eligibility for switching to already
open tabs. This CL changes GURLToStrippedGURL() to remove refs
from the stripped version of destination URLs (only used for
comparison) and when testing URLs for tab switching.

We only remove refs when the input doesn't specify a ref.
This is critical to proper deduping.

Also added unit test.

Bug: 780835
Change-Id: Ifef8d8b9d2efd4394b598342c13ef8aa439089ce
Reviewed-on: https://chromium-review.googlesource.com/1025953
Commit-Queue: Kevin Bailey <krb@chromium.org>
Reviewed-by: default avatarPeter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555760}
parent 41c3ad10
......@@ -6,6 +6,7 @@
#include <memory>
#include "base/strings/utf_string_conversions.h"
#include "chrome/common/pref_names.h"
#include "chrome/test/base/testing_profile.h"
#include "components/prefs/pref_service.h"
......@@ -15,6 +16,29 @@
#include "testing/gtest/include/gtest/gtest.h"
#include "url/gurl.h"
namespace {
class TestSchemeClassifier : public AutocompleteSchemeClassifier {
public:
TestSchemeClassifier() = default;
~TestSchemeClassifier() override = default;
// Overridden from AutocompleteInputSchemeClassifier:
metrics::OmniboxInputType GetInputTypeForScheme(
const std::string& scheme) const override;
private:
DISALLOW_COPY_AND_ASSIGN(TestSchemeClassifier);
};
metrics::OmniboxInputType TestSchemeClassifier::GetInputTypeForScheme(
const std::string& scheme) const {
return scheme.empty() ? metrics::OmniboxInputType::INVALID
: metrics::OmniboxInputType::URL;
}
}; // namespace
class ChromeAutocompleteProviderClientTest : public testing::Test {
public:
void SetUp() override {
......@@ -71,3 +95,50 @@ TEST_F(ChromeAutocompleteProviderClientTest,
EXPECT_FALSE(service_worker_context_
.start_service_worker_for_navigation_hint_called());
}
TEST_F(ChromeAutocompleteProviderClientTest, TestStrippedURLsAreEqual) {
struct {
const char* url1;
const char* url2;
const char* input;
bool equal;
} test_cases[] = {
// Sanity check cases.
{"http://google.com", "http://google.com", "", true},
{"http://google.com", "http://www.google.com", "", true},
{"http://google.com", "http://facebook.com", "", false},
{"http://google.com", "https://google.com", "", true},
// Because we provided scheme, must match in scheme.
{"http://google.com", "https://google.com", "http://google.com", false},
// Ignore ref if not in input.
{"http://drive.google.com/doc/blablabla#page=10",
"http://drive.google.com/doc/blablabla#page=111", "", true},
{"http://drive.google.com/doc/blablabla#page=10",
"http://drive.google.com/doc/blablabla#page=111",
"http://drive.google.com/doc/blablabla", true},
{"file:///usr/local/bin/tuxpenguin#ref1",
"file:///usr/local/bin/tuxpenguin#ref2", "", true},
{"file:///usr/local/bin/tuxpenguin#ref1",
"file:///usr/local/bin/tuxpenguin#ref2",
"file:///usr/local/bin/tuxpenguin", true},
// Do not ignore ref if in input.
{"http://drive.google.com/doc/blablabla#page=10",
"http://drive.google.com/doc/blablabla#page=111",
"http://drive.google.com/doc/blablabla#p", false},
{"file:///usr/local/bin/tuxpenguin#ref1",
"file:///usr/local/bin/tuxpenguin#ref2",
"file:///usr/local/bin/tuxpenguin#r", false}};
for (const auto& test_case : test_cases) {
SCOPED_TRACE(std::string(test_case.url1) + " vs " + test_case.url2 +
", input '" + test_case.input + "'");
AutocompleteInput input(base::ASCIIToUTF16(test_case.input),
test_case.input[0]
? metrics::OmniboxEventProto::OTHER
: metrics::OmniboxEventProto::BLANK,
TestSchemeClassifier());
EXPECT_EQ(test_case.equal,
client_->StrippedURLsAreEqual(GURL(test_case.url1),
GURL(test_case.url2), &input));
}
}
......@@ -174,6 +174,21 @@ std::string AutocompleteInput::TypeToString(metrics::OmniboxInputType type) {
return std::string();
}
// static
void AutocompleteInput::ParseFilePath(const base::string16& text,
size_t offset,
url::Parsed* parts) {
parts->path.begin = offset;
size_t hash_offset = text.find('#', offset);
if (hash_offset != text.npos) {
parts->path.len = hash_offset - offset;
parts->ref.begin = hash_offset + 1;
parts->ref.len = text.size() - parts->ref.begin;
} else {
parts->path.len = text.size() - offset;
}
}
// static
metrics::OmniboxInputType AutocompleteInput::Parse(
const base::string16& text,
......@@ -213,6 +228,8 @@ metrics::OmniboxInputType AutocompleteInput::Parse(
// A user might or might not type a scheme when entering a file URL. In
// either case, |parsed_scheme_utf8| will tell us that this is a file URL,
// but |parts->scheme| might be empty, e.g. if the user typed "C:\foo".
ParseFilePath(text, parts->scheme.is_nonempty() ? parts->scheme.end() : 0,
parts);
return metrics::OmniboxInputType::URL;
}
......
......@@ -61,6 +61,11 @@ class AutocompleteInput {
// Converts |type| to a string representation. Used in logging.
static std::string TypeToString(metrics::OmniboxInputType type);
// Parses the |path| and |ref| fields of |parts| from a file path input.
static void ParseFilePath(const base::string16& text,
size_t offset,
url::Parsed* parts);
// Parses |text| (including an optional |desired_tld|) and returns the type of
// input this will be interpreted as. |scheme_classifier| is used to check
// the scheme in |text| is known and registered in the current environment.
......
......@@ -521,6 +521,11 @@ GURL AutocompleteMatch::GURLToStrippedGURL(
needs_replacement = true;
}
if (!input.parts().ref.is_nonempty() && url.has_ref()) {
replacements.ClearRef();
needs_replacement = true;
}
if (needs_replacement)
stripped_destination_url = stripped_destination_url.ReplaceComponents(
replacements);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment