Hardening the 'url::Origin' implementation.

url/origin.cc

Index: url/origin.cc
diff --git a/url/origin.cc b/url/origin.cc
index 8aaa173fa5e0d98728067daca5af80b349e0ba28..fbc61ddd29c36cfbbb06e9626ab49290e511a780 100644
--- a/url/origin.cc
+++ b/url/origin.cc
@@ -4,16 +4,92 @@
 
 #include "url/origin.h"
 
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
+#include "url/url_constants.h"
 
 namespace url {
 
-Origin::Origin() : string_("null") {}
+Origin::Origin() {
+  Init(GURL());
+}
+
+Origin::Origin(const GURL& url) {
+  Init(url);
+}
+
+Origin::Origin(const std::string& scheme,
+               const std::string& host,
+               unsigned short port) {
+  // Special-case 'file://':

[Ryan Sleevi, 2015/05/22 20:43:35]

I don't think this should be necessary - that is, it should be entirely invalid
to supply file:// URLs to a scheme/host/port construct because File URLs are
'empty host', full path.

That is, in this constructor, I suspect the right answer would be something like

DCHECK(IsStandard(...) && !CompareScheme(..., kFileScheme))

Assuming we create & expose a CompareScheme (or perhaps just 'internally' expose
the CompareScheme from url_util, rather than exporting from url::)

[Mike West, 2015/05/28 07:24:29]

Dropped the special-case.

+  if (scheme == kFileScheme) {

[Ryan Sleevi, 2015/05/22 20:43:35]

BUG: The scheme comparisons should operate case-insentively, hence why I
suggested a CompareScheme function above. An equality check like this won't
help.

[Mike West, 2015/05/28 07:24:29]

Switched to using `url::LowerCaseEqualsASCII` to match `GURL::SchemeIs`.

+    scheme_ = kFileScheme;
+    host_.clear();
+    port_ = 0;
+    unique_ = false;
+    return;
+  }
+
+  // Otherwise, pass the data through GURL for normalization:
+  Init(GURL(scheme + kStandardSchemeSeparator + host + ":" +
+            base::IntToString(port)));
+}
+
+Origin::Origin(const std::string& origin) {
+  // Pass the string through GURL for normalization:
+  Init(GURL(origin));
+}
+
+void Origin::Init(const GURL& url) {
+  // Start with a unique origin, parse from there:
+  scheme_.clear();
+  host_.clear();
+  port_ = 0;
+  unique_ = true;
+  serialization_requires_port_ = false;
+
+  // TODO(mkwst): Bring the GURL::GetOrigin replacement logic in here.
+  GURL origin(url.GetOrigin());
+  if (!origin.is_valid())
+    return;
+
+  unique_ = false;
+  scheme_ = origin.scheme();
+
+  if (!origin.IsStandard() || SchemeIs(kFileScheme))
+    return;
+
+  host_ = origin.host();
+  port_ = origin.EffectiveIntPort();
+  serialization_requires_port_ = origin.IntPort() != PORT_UNSPECIFIED;
+}
+
+std::string Origin::serialize() const {
+  if (unique_)
+    return kUniqueOriginSerialization;
+
+  if (host_.empty())
+    return scheme_ + kStandardSchemeSeparator;
+
+  return scheme_ + kStandardSchemeSeparator + host_ +
+         (serialization_requires_port_ ? ":" + base::IntToString(port_) : "");
+}
+
+bool Origin::SchemeIs(const std::string& scheme) const {
+  return scheme_ == scheme;

[Ryan Sleevi, 2015/05/22 20:43:35]

BUG: Case-sensitivity

+}
+
+bool Origin::SchemeIsCryptographic() const {
+  return SchemeIs(kHttpsScheme) || SchemeIs(kWssScheme);

[Ryan Sleevi, 2015/05/22 20:43:35]

We should probably be breaking this out from both GURL and Origin and into an
(internal) free function helper

[Mike West, 2015/05/28 07:24:29]

Internal to what?

Note that content/public/common/origin_util.h exists; I think at least some of
this kind of functionality could either be folded into that set of methods, or
brought back into //url. 

[Ryan Sleevi, 2015/05/28 07:32:53]

Oh, to GURL. Same with the scheme comparisons - a non-exported from GURL set of
functions for working across the two (Origins and GURLs)

+}
+
+bool Origin::IsSameOriginWith(const Origin& other) const {
+  return !unique_ && !other.unique_ && scheme_ == other.scheme_ &&
+         host_ == other.host_ && port_ == other.port_;
+}
 
-Origin::Origin(const std::string& origin) : string_(origin) {
-  DCHECK(origin == "null" || MatchPattern(origin, "?*://?*"));
-  DCHECK_GT(origin.size(), 0u);
-  DCHECK(origin == "file://" || origin[origin.size() - 1] != '/');
+std::ostream& operator<<(std::ostream& out, const url::Origin& url) {
+  return out << url.serialize();
 }
 
 }  // namespace url