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

url/scheme_host_port.cc

 // Copyright 2015 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 "url/scheme_host_port.h"
 
 #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"
 #include "url/url_constants.h"
 #include "url/url_util.h"
 
 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);
 
   if (host_info.out_host.is_nonempty() &&
       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();
-  }
-
-  // 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(),
-                       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;
+    canon_host->clear();
   }
 }
 
+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) {
+    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;
+  }
+}
+
 SchemeHostPort::SchemeHostPort(const GURL& url) : port_(0) {
   if (!url.is_valid() || !url.IsStandard())
     return;
 
   // These schemes do not follow the generic URL syntax, so we treat them as
   // invalid (scheme, host, port) tuples (even though such URLs' _Origin_ might
   // have a (scheme, host, port) tuple, they themselves do not).
   if (url.SchemeIsBlob() || url.SchemeIsFileSystem())
     return;
 
@@ skipping 17 matching lines @@
     return result;
 
   bool is_default_port =
       port_ == url::DefaultPortForScheme(scheme_.data(),
                                          static_cast<int>(scheme_.length()));
 
   result.append(scheme_);
   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_));
   }
 
   return result;
 }
 
 bool SchemeHostPort::Equals(const SchemeHostPort& other) const {
   return port_ == other.port() && scheme_ == other.scheme() &&
          host_ == other.host();
 }
 
 bool SchemeHostPort::operator<(const SchemeHostPort& other) const {
   if (port_ != other.port_)
     return port_ < other.port_;
   if (scheme_ != other.scheme_)
     return scheme_ < other.scheme_;
   if (host_ != other.host_)
     return host_ < other.host_;
   return false;
 }
 
 }  // namespace url