From 12bca18ba5d9a9756cbe87bbc10c92ea7e402f9a Mon Sep 17 00:00:00 2001 From: clamy Date: Fri, 10 Feb 2017 07:33:07 -0800 Subject: [PATCH] Move IsAboutBlank from url_utils to GURL https://codereview.chromium.org/2644133002/ introduced a IsAboutBlank function to recognize urls of the form about:blank/#foo & about:blank?foo. It was mistaknely placed in url_utils.h which should not depend on GURL. This CL moves the function to GURL.h. BUG=575210 Review-Url: https://codereview.chromium.org/2686853004 Cr-Commit-Position: refs/heads/master@{#449624} --- .../child_process_security_policy_impl.cc | 3 +- content/common/navigation_params.cc | 3 +- url/gurl.cc | 13 ++++ url/gurl.h | 4 + url/gurl_unittest.cc | 73 ++++++++++++++++++ url/url_util.cc | 16 ---- url/url_util.h | 6 -- url/url_util_unittest.cc | 74 ------------------- 8 files changed, 92 insertions(+), 100 deletions(-) diff --git a/content/browser/child_process_security_policy_impl.cc b/content/browser/child_process_security_policy_impl.cc index 3e8868aa682761..3cf58583613241 100644 --- a/content/browser/child_process_security_policy_impl.cc +++ b/content/browser/child_process_security_policy_impl.cc @@ -30,7 +30,6 @@ #include "storage/browser/fileapi/isolated_context.h" #include "storage/common/fileapi/file_system_util.h" #include "url/gurl.h" -#include "url/url_util.h" namespace content { @@ -629,7 +628,7 @@ bool ChildProcessSecurityPolicyImpl::CanRequestURL( if (IsPseudoScheme(url.scheme())) { // Every child process can request , , // and . - if (url::IsAboutBlank(url) || url == kAboutSrcDocURL) + if (url.IsAboutBlank() || url == kAboutSrcDocURL) return true; // URLs like , , shouldn't be // requestable by any child process. Also, this case covers diff --git a/content/common/navigation_params.cc b/content/common/navigation_params.cc index 0d64f3b8be26cc..b232ce3bd64685 100644 --- a/content/common/navigation_params.cc +++ b/content/common/navigation_params.cc @@ -12,7 +12,6 @@ #include "content/public/common/url_constants.h" #include "url/gurl.h" #include "url/url_constants.h" -#include "url/url_util.h" namespace content { @@ -22,7 +21,7 @@ bool ShouldMakeNetworkRequestForURL(const GURL& url) { // Javascript URLs, about:blank, srcdoc should not send a request // to the network stack. - return !url::IsAboutBlank(url) && !url.SchemeIs(url::kJavaScriptScheme) && + return !url.IsAboutBlank() && !url.SchemeIs(url::kJavaScriptScheme) && !url.is_empty() && !url.SchemeIs(url::kContentIDScheme) && url != content::kAboutSrcDocURL; } diff --git a/url/gurl.cc b/url/gurl.cc index f521692f931da5..c9dad0600b2801 100644 --- a/url/gurl.cc +++ b/url/gurl.cc @@ -348,6 +348,19 @@ bool GURL::IsStandard() const { return url::IsStandard(spec_.data(), parsed_.scheme); } +bool GURL::IsAboutBlank() const { + if (!SchemeIs(url::kAboutScheme)) + return false; + + if (has_host() || has_username() || has_password() || has_port()) + return false; + + if (path() != url::kAboutBlankPath && path() != url::kAboutBlankWithHashPath) + return false; + + return true; +} + bool GURL::SchemeIs(base::StringPiece lower_ascii_scheme) const { DCHECK(base::IsStringASCII(lower_ascii_scheme)); DCHECK(base::ToLowerASCII(lower_ascii_scheme) == lower_ascii_scheme); diff --git a/url/gurl.h b/url/gurl.h index d7d575b1e1633d..ae2c40889f9aa6 100644 --- a/url/gurl.h +++ b/url/gurl.h @@ -203,6 +203,10 @@ class URL_EXPORT GURL { // by calling SchemeIsFile[System]. bool IsStandard() const; + // Returns true when the url is of the form about:blank, about:blank?foo or + // about:blank/#foo. + bool IsAboutBlank() const; + // Returns true if the given parameter (should be lower-case ASCII to match // the canonicalized scheme) is the scheme for this URL. Do not include a // colon. diff --git a/url/gurl_unittest.cc b/url/gurl_unittest.cc index 24dee6c2a65aec..258ea03e0b2d7e 100644 --- a/url/gurl_unittest.cc +++ b/url/gurl_unittest.cc @@ -740,4 +740,77 @@ TEST(GURLTest, ContentAndPathForNonStandardURLs) { } } +TEST(GURLTest, IsAboutBlank) { + const std::string kAboutBlankUrls[] = {"about:blank", "about:blank?foo", + "about:blank/#foo", + "about:blank?foo#foo"}; + for (const auto& url : kAboutBlankUrls) + EXPECT_TRUE(GURL(url).IsAboutBlank()) << url; + + const std::string kNotAboutBlankUrls[] = { + "http:blank", "about:blan", "about://blank", + "about:blank/foo", "about://:8000/blank", "about://foo:foo@/blank", + "foo@about:blank", "foo:bar@about:blank", "about:blank:8000"}; + for (const auto& url : kNotAboutBlankUrls) + EXPECT_FALSE(GURL(url).IsAboutBlank()) << url; +} + +TEST(GURLTest, EqualsIgnoringRef) { + const struct { + const char* url_a; + const char* url_b; + bool are_equals; + } kTestCases[] = { + // No ref. + {"http://a.com", "http://a.com", true}, + {"http://a.com", "http://b.com", false}, + + // Same Ref. + {"http://a.com#foo", "http://a.com#foo", true}, + {"http://a.com#foo", "http://b.com#foo", false}, + + // Different Refs. + {"http://a.com#foo", "http://a.com#bar", true}, + {"http://a.com#foo", "http://b.com#bar", false}, + + // One has a ref, the other doesn't. + {"http://a.com#foo", "http://a.com", true}, + {"http://a.com#foo", "http://b.com", false}, + + // Empty refs. + {"http://a.com#", "http://a.com#", true}, + {"http://a.com#", "http://a.com", true}, + + // URLs that differ only by their last character. + {"http://aaa", "http://aab", false}, + {"http://aaa#foo", "http://aab#foo", false}, + + // Different size of the part before the ref. + {"http://123#a", "http://123456#a", false}, + + // Blob URLs + {"blob:http://a.com#foo", "blob:http://a.com#foo", true}, + {"blob:http://a.com#foo", "blob:http://a.com#bar", true}, + {"blob:http://a.com#foo", "blob:http://b.com#bar", false}, + + // Filesystem URLs + {"filesystem:http://a.com#foo", "filesystem:http://a.com#foo", true}, + {"filesystem:http://a.com#foo", "filesystem:http://a.com#bar", true}, + {"filesystem:http://a.com#foo", "filesystem:http://b.com#bar", false}, + }; + + for (const auto& test_case : kTestCases) { + SCOPED_TRACE(testing::Message() + << std::endl + << "url_a = " << test_case.url_a << std::endl + << "url_b = " << test_case.url_b << std::endl); + // A versus B. + EXPECT_EQ(test_case.are_equals, + GURL(test_case.url_a).EqualsIgnoringRef(GURL(test_case.url_b))); + // B versus A. + EXPECT_EQ(test_case.are_equals, + GURL(test_case.url_b).EqualsIgnoringRef(GURL(test_case.url_a))); + } +} + } // namespace url diff --git a/url/url_util.cc b/url/url_util.cc index 83cf71f3de14d4..a79e114fed36ae 100644 --- a/url/url_util.cc +++ b/url/url_util.cc @@ -10,7 +10,6 @@ #include "base/debug/leak_annotations.h" #include "base/logging.h" #include "base/strings/string_util.h" -#include "url/gurl.h" #include "url/url_canon_internal.h" #include "url/url_constants.h" #include "url/url_file.h" @@ -645,21 +644,6 @@ bool IsReferrerScheme(const char* spec, const Component& scheme) { return DoIsInSchemes(spec, scheme, &unused_scheme_type, *referrer_schemes); } -bool IsAboutBlank(const GURL& url) { - if (!url.SchemeIs(url::kAboutScheme)) - return false; - - if (url.has_host() || url.has_username() || url.has_password() || - url.has_port()) { - return false; - } - - if (url.path() != kAboutBlankPath && url.path() != kAboutBlankWithHashPath) - return false; - - return true; -} - bool FindAndCompareScheme(const char* str, int str_len, const char* compare, diff --git a/url/url_util.h b/url/url_util.h index 08d660890dddff..d0a8f22b6ff962 100644 --- a/url/url_util.h +++ b/url/url_util.h @@ -15,8 +15,6 @@ #include "url/url_constants.h" #include "url/url_export.h" -class GURL; - namespace url { // Init ------------------------------------------------------------------------ @@ -168,10 +166,6 @@ URL_EXPORT bool GetStandardSchemeType(const char* spec, const Component& scheme, SchemeType* type); -// Check whether the url is of the form about:blank, about:blank?foo or -// about:blank/#foo. -URL_EXPORT bool IsAboutBlank(const GURL& url); - // Hosts ---------------------------------------------------------------------- // Returns true if the |canonicalized_host| matches or is in the same domain as diff --git a/url/url_util_unittest.cc b/url/url_util_unittest.cc index e51b0241a3ccb9..a706f5a08a04bb 100644 --- a/url/url_util_unittest.cc +++ b/url/url_util_unittest.cc @@ -6,7 +6,6 @@ #include "base/macros.h" #include "testing/gtest/include/gtest/gtest.h" -#include "url/gurl.h" #include "url/third_party/mozilla/url_parse.h" #include "url/url_canon.h" #include "url/url_canon_stdstring.h" @@ -418,77 +417,4 @@ TEST(URLUtilTest, TestDomainIs) { } } -TEST(URLUtilTest, IsAboutBlank) { - const std::string kAboutBlankUrls[] = {"about:blank", "about:blank?foo", - "about:blank/#foo", - "about:blank?foo#foo"}; - for (const auto& url : kAboutBlankUrls) - EXPECT_TRUE(IsAboutBlank(GURL(url))); - - const std::string kNotAboutBlankUrls[] = { - "http:blank", "about:blan", "about://blank", - "about:blank/foo", "about://:8000/blank", "about://foo:foo@/blank", - "foo@about:blank", "foo:bar@about:blank", "about:blank:8000"}; - for (const auto& url : kNotAboutBlankUrls) - EXPECT_FALSE(IsAboutBlank(GURL(url))); -} - -TEST(URLUtilTest, EqualsIgnoringRef) { - const struct { - const char* url_a; - const char* url_b; - bool are_equals; - } kTestCases[] = { - // No ref. - {"http://a.com", "http://a.com", true}, - {"http://a.com", "http://b.com", false}, - - // Same Ref. - {"http://a.com#foo", "http://a.com#foo", true}, - {"http://a.com#foo", "http://b.com#foo", false}, - - // Different Refs. - {"http://a.com#foo", "http://a.com#bar", true}, - {"http://a.com#foo", "http://b.com#bar", false}, - - // One has a ref, the other doesn't. - {"http://a.com#foo", "http://a.com", true}, - {"http://a.com#foo", "http://b.com", false}, - - // Empty refs. - {"http://a.com#", "http://a.com#", true}, - {"http://a.com#", "http://a.com", true}, - - // URLs that differ only by their last character. - {"http://aaa", "http://aab", false}, - {"http://aaa#foo", "http://aab#foo", false}, - - // Different size of the part before the ref. - {"http://123#a", "http://123456#a", false}, - - // Blob URLs - {"blob:http://a.com#foo", "blob:http://a.com#foo", true}, - {"blob:http://a.com#foo", "blob:http://a.com#bar", true}, - {"blob:http://a.com#foo", "blob:http://b.com#bar", false}, - - // Filesystem URLs - {"filesystem:http://a.com#foo", "filesystem:http://a.com#foo", true}, - {"filesystem:http://a.com#foo", "filesystem:http://a.com#bar", true}, - {"filesystem:http://a.com#foo", "filesystem:http://b.com#bar", false}, - }; - - for (const auto& test_case : kTestCases) { - SCOPED_TRACE(testing::Message() - << std::endl - << "url_a = " << test_case.url_a << std::endl - << "url_b = " << test_case.url_b << std::endl); - // A versus B. - EXPECT_EQ(test_case.are_equals, - GURL(test_case.url_a).EqualsIgnoringRef(GURL(test_case.url_b))); - // B versus A. - EXPECT_EQ(test_case.are_equals, - GURL(test_case.url_b).EqualsIgnoringRef(GURL(test_case.url_a))); - } -} - } // namespace url