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}
marcosholgado · Feb 10, 2017 · 12bca18 · 12bca18
1 parent 9d60f1f
commit 12bca18
Show file tree

Hide file tree

Showing 8 changed files with 92 additions and 100 deletions.
diff --git 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 <about:blank>, <about:blank?foo>,
     // <about:blank/#foo> and <about:srcdoc>.
-    if (url::IsAboutBlank(url) || url == kAboutSrcDocURL)
+    if (url.IsAboutBlank() || url == kAboutSrcDocURL)
       return true;
     // URLs like <about:version>, <about:crash>, <view-source:...> 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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