Allow url::SchemeHostPort to hold non-file scheme without port

url/scheme_host_port.cc

Index: url/scheme_host_port.cc
diff --git a/url/scheme_host_port.cc b/url/scheme_host_port.cc
index c2fe830e377fa2edde0260265fd5a6b013fd4d35..b65e26cfed6d7b43fc2bc606b4108ba1ab41af71 100644
--- a/url/scheme_host_port.cc
+++ b/url/scheme_host_port.cc
@@ -7,7 +7,9 @@
 #include <string.h>
 
 #include "base/logging.h"
+#include "base/macros.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
 #include "url/gurl.h"
 #include "url/url_canon.h"
 #include "url/url_canon_stdstring.h"
@@ -16,19 +18,39 @@
 
 namespace url {
 
+const char* kSchemesWithAuthorityOfHostPortPair[] = {
+  kHttpScheme,
+  kHttpsScheme,
+  kFtpScheme,
+  kFileScheme,
+  kGopherScheme,
+  kWsScheme,
+  kWssScheme,
+};
+
+static bool IsSchemeWithAuthorityOfHostPortPair(base::StringPiece scheme) {
+  for (size_t i = 0; i < arraysize(kSchemesWithAuthorityOfHostPortPair); ++i) {
+    if (base::LowerCaseEqualsASCII(
+            scheme, kSchemesWithAuthorityOfHostPortPair[i]))
+      return true;
+  }
+  return false;
+}

[Ryan Sleevi, 2015/08/07 01:35:36]

We already have such a registry. It's called a Standard URL in GURL - aka one
that conforms to the generic URL syntax.

[tyoshino (SeeGerritForStatus), 2015/08/07 02:47:54]

url::IsStandard() which is used by GURL is no longer working as a method to
check if URLs with the given scheme is expected to follow the generic URI
syntax. It may still be working for checking "syntax" (the reg-name production),
but at least the host component doesn't always have a string representing a
host. In content::RegisterContentSchemes, various Chrome specific addresses are
registered as standard scheme. On binaries where ChromeContentClient is used,
chrome-native, chrome-search, chrome-extension-resource, chrome-distiller,
chrome-extension are registered as standard schemes to url_util.

Like Blob URL, the filesystem URI also have non-host data in the authority
subcomponent when parsed following the generic URI syntax. But it's in the
default list of standard schemes used by DoIsStandard().

So, to know if we want to apply host canonicalization algorithm or not, we
shouldn't use IsStandard(), but have something else. This could be placed in
url_util.h.

[tyoshino (SeeGerritForStatus), 2015/08/07 06:10:23]

This is not a problem in SchemeHostPort as it doesn't accept filesystem URLs.

[tyoshino (SeeGerritForStatus), 2015/08/07 07:57:39]

To be clear,

GURL is using url::Canonicalize() to set up parsed_. KURL is doing so, too.
url::Canonicalize() is using url::IsStandard() to switch between Parse.*URL
functions defined in third_party/mozilla/url_parse.h. Not based on whether "//"
is include or not. Once ParseStandardURL() is called, it ignores "//". This
means that GURL/KURL are using IsStandard() as a way to check if the following
URL string follows the

  "//" authority path-abempty

construction defined in the generic URI syntax or not. Not a way to check if the
following URL string conforms to any of the hier-part constructions.

Let's say like "conforms to the URI construction with authority" or something to
avoid confusion.

+
 SchemeHostPort::SchemeHostPort() : port_(0) {
 }
 
-SchemeHostPort::SchemeHostPort(base::StringPiece scheme,
-                               base::StringPiece host,
-                               uint16 port)
-    : scheme_(scheme.data(), scheme.length()),
-      host_(host.data(), host.length()),
-      port_(port) {
+void SchemeHostPort::Clear() {
+  scheme_.clear();
+  host_.clear();
+  port_ = 0;
+}
+
+void SchemeHostPort::CanonicalizeHost(base::StringPiece host,

[Ryan Sleevi, 2015/08/07 01:35:36]

STYLE: Declaration order should match definition order. This should be moved.

[tyoshino (SeeGerritForStatus), 2015/08/07 06:10:23]

Done.

+                                      std::string* canon_host) {
   // Try to canonicalize the host (copy/pasted from net/base. :( ).
   const url::Component raw_host_component(0, static_cast<int>(host.length()));
-  std::string canon_host;
-  url::StdStringCanonOutput canon_host_output(&canon_host);
+  url::StdStringCanonOutput canon_host_output(canon_host);
   url::CanonHostInfo host_info;
   url::CanonicalizeHostVerbose(host.data(), raw_host_component,
                                &canon_host_output, &host_info);
@@ -37,32 +59,44 @@ SchemeHostPort::SchemeHostPort(base::StringPiece scheme,
       host_info.family != url::CanonHostInfo::BROKEN) {
     // Success!  Assert that there's no extra garbage.
     canon_host_output.Complete();
-    DCHECK_EQ(host_info.out_host.len, static_cast<int>(canon_host.length()));
+    DCHECK_EQ(host_info.out_host.len, static_cast<int>(canon_host->length()));
   } else {
     // Empty host, or canonicalization failed.
-    canon_host.clear();
+    canon_host->clear();
   }
+}
 
-  // Return an invalid SchemeHostPort object if any of the following conditions
-  // hold:
-  //
-  // 1. The provided scheme is non-standard, 'blob:', or 'filesystem:'.
-  // 2. The provided host is non-canonical.
-  // 3. The scheme is 'file' and the port is non-zero.
-  // 4. The scheme is not 'file', and the port is zero or the host is empty.
-  bool isUnsupportedScheme =

[Ryan Sleevi, 2015/08/07 01:35:36]

blah. How'd I miss these variable names during review. They're all wrong :(

-      !url::IsStandard(scheme.data(),
+SchemeHostPort::SchemeHostPort(base::StringPiece scheme,
+                               base::StringPiece host,
+                               uint16 port)
+    : scheme_(scheme.data(), scheme.length()),
+      host_(host.data(), host.length()),
+      port_(port) {
+  if (!url::IsStandard(scheme.data(),

[Ryan Sleevi, 2015/08/07 01:35:36]

You've removed all of the documentation that explains this logic. Please provide
ample and clear reasoning for each of these.

[tyoshino (SeeGerritForStatus), 2015/08/07 06:10:23]

The comment in the .h file also explains almost the same thing as the comment we
had here. And, after refactoring, the method calls newly introduced helped
improve descriptiveness of the code. So, I removed it.

I've re-added some to have better readability.

                        url::Component(0, static_cast<int>(scheme.length()))) ||
-      scheme == kFileSystemScheme || scheme == kBlobScheme;
-  bool isNoncanonicalHost = host != canon_host;
-  bool isFileSchemeWithPort = scheme == kFileScheme && port != 0;
-  bool isNonFileSchemeWithoutPortOrHost =
-      scheme != kFileScheme && (port == 0 || host.empty());
-  if (isUnsupportedScheme || isNoncanonicalHost || isFileSchemeWithPort ||
-      isNonFileSchemeWithoutPortOrHost) {
-    scheme_.clear();
-    host_.clear();
-    port_ = 0;
+      scheme == kFileSystemScheme || scheme == kBlobScheme) {
+    Clear();
+    return;
+  }
+
+  if (!IsSchemeWithAuthorityOfHostPortPair(scheme))
+    return;
+
+  if (scheme == kFileScheme) {
+    if (port != 0) {
+      Clear();
+      return;
+    }
+  } else if (port == 0 || host.empty()) {
+    Clear();
+    return;
+  }
+
+  std::string canon_host;
+  CanonicalizeHost(host, &canon_host);
+  if (host != canon_host) {
+    Clear();
+    return;
   }
 }
 
@@ -103,7 +137,9 @@ std::string SchemeHostPort::Serialize() const {
   result.append(kStandardSchemeSeparator);
   result.append(host_);
 
-  if (scheme_ != kFileScheme && !is_default_port) {
+  if (IsSchemeWithAuthorityOfHostPortPair(scheme_) &&
+      scheme_ != kFileScheme &&
+      !is_default_port) {
     result.push_back(':');
     result.append(base::IntToString(port_));
   }