Hardening the 'url::Origin' implementation.

url/origin.h

Index: url/origin.h
diff --git a/url/origin.h b/url/origin.h
index 777e4e1ef481bc062557b2fc8d7fe2437f784c0d..acebc7ee969e84d2a0cf11bfcbdd43e2ccd7d68e 100644
--- a/url/origin.h
+++ b/url/origin.h
@@ -7,27 +7,74 @@
 
 #include <string>
 
+#include "url/gurl.h"
 #include "url/url_export.h"
 
 namespace url {
 
-// Origin represents a Web Origin serialized to a string.
-// See RFC6454 for details.
+// Origin represents a scheme/host/port tuple, as described in RFC6454.

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

At the risk of being a pain, I think it would be great to expand this
documentation here, especially if we expect to build more code on top of it.

// Represents a tuple of scheme/host/port, as described in RFC 6454
---^ nit, I like spaces in my RFC #s ;)
//
// Conceptually, an origin is just a tuple of scheme/host/port, but
// as described in RFC 6454, there are a number of subtle behaviours
// to be aware of.
//
// The first is that not everything that can be expressed as a URL
// has an origin. [something about scheme's without hier-parts, like data URIs,
and the nuances thereof]
//
// The second is the concept of the "null" origin [something something]
//
// etc
//
// Usage:
// - If you're trying to do X
//   [sample]
// - If you're trying to do Y
//   [sample]

(Please don't take the above, as I think I did a poor job at phrasing, but I
wanted to try to illustrate the type of concepts). 

I think there's still a lot of sharp edges in this API, which is why I'm still
sort of uneasy, but the best way I suppose to smooth those edges is to both
document them and show, idiomatically in the documentation, how to avoid those
edges. Things like should you be checking if the GURL is valid beforehand, etc.

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

Makes sense.
In Blink::SecurityOrigin, we call this concept "unique", which seems to map well
to RFC 6454's "globally unique origin" terminology. "null" is just the
serialization of a globally unique origin. I'd prefer to be consistent with
Blink here, and call it "unique".

 class URL_EXPORT Origin {
  public:
+  // Creates a unique Origin.
   Origin();
-  explicit Origin(const std::string& origin);
 
-  const std::string& string() const { return string_; }
+  // Creates an origin from a scheme/host/port tuple. If the scheme, host, or
+  // port are invalid, a unique origin will be created.
+  Origin(const std::string& scheme,
+         const std::string& host,
+         unsigned short port);
+
+  // Creates an origin for a given URL, as specified in
+  // https://url.spec.whatwg.org/#origin. Invalid URLs are parsed as unique
+  // origins, and non-standard URLs will be parsed as ([scheme], '', 0).
+  explicit Origin(const GURL& url);
+
+  // Returns true if the origin is "the same" as |other|, as defined in Section
+  // 5 of RFC6454: https://tools.ietf.org/html/rfc6454#section-5. Note that
+  // unique origins are not "the same" as any other origin, including
+  // themselves.
+  bool IsSameOriginWith(const Origin& other) const;
+
+  bool SchemeIs(const std::string& scheme) const;
+  bool SchemeIsCryptographic() const;
+
+  std::string scheme() const { return scheme_; }
+  std::string host() const { return host_; }
+  unsigned short port() const { return port_; }
+  bool unique() const { return unique_; }

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

I'm not sure what the utility of this is, nor if it's the right thing to expose.
Maybe the documentation comments above will sort of address this.

For example, is this trying to express whether it's the null origin? Is that the
concept we should be exposing? Or perhaps we should be exposing whether or not
the GURL was valid to begin with (assuming invalid GURL == null origin ==
unique).

I don't recall from the other CL you checking this. If there isn't concrete code
you can point to needing this, I'd actually prefer we not include it at all, at
least not until we can actually demonstrate and explain the need.

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

Happy to drop it. If it turns out that we need it, adding it back is trivial. I
know we use the SecurityOrigin analog in a number of places in Blink, but
perhaps we don't need it in //content, //net, or //chrome.

 
-  bool IsSameAs(const Origin& that) const {
-    return string_ == that.string_;
-  }
+  // Returns a serialization of the origin, suitable for passing around via IPC.
+  // This is _not_ the serialization of the origin which ought to be displayed
+  // to a user in browser UI.

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

s/browser//

//net, and below (including URL) have zero knowledge or requirement that they be
used as or in a browser.

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

Got it. Expanded this comment to point to the FAQ about rendering origins to
users.

+  //
+  // This is an implementation of the algorithm defined in RFC6454:
+  // https://tools.ietf.org/html/rfc6454#section-6, with the following

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

Follow-up expansion: Is this serializing to Unicode or ASCII? It's not obvious
from the signature() which


[Secret Answer: If you only allow the inputs to be GURLs, then it's ASCII. If
you let the host be supplied by the caller, and you don't force
canonicalization, you can't really reason about it other than documenting "Do
this" and "Don't do this". As someone who has fought a long hell during reviews
of trying to clarify when brackets are needed and when they are for hostnames
(not if giving to DNS, are if using in URL), I suspect the U-Label/A-Label hell
of hostnames will bite you here if you don't explicitly canonicalize
scheme/host/port bits yourself and document explicitly as such]

+  // divergences:
+  //
+  // 1. Origins with a scheme of 'file' serialize to 'file://'.
+  // 2. Origins with a

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

Incomplete?

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

Killed this; we're not divergent.

+  // 3. IPv6 addresses serialize with bracketed hostnames: 'http://[::1]'.

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

Why is this a divergence? This is exactly what's required of IPv6 addresses in
an origin. The host-part of a URL is always bracketed (7.1 references RFC 3986,
which incorporated the changes from RFC2732)

+  std::string serialize() const;

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

Are you still unhappy with serialization being defined here? Should this be
"ToString"?

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

Well, naming, s/serialize/Serialize()/, but if the goal is RFC 6454, then I
guess I don't get to grumble.

The concern I have is that people will assume it's for IPC serialization, and
that's maybe-true/maybe-not/hard-to-tell (e.g. it'd be a lossy operation, as
data URIs would serialize to 'null', for example)

+
+  // TODO(mkwst): Remove tese once blink::WebSerializedOrigin is gone.

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

s/these/

+  explicit Origin(const std::string& origin);

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

STYLE: This belongs at line 31, even if it's deprecated.

Rather than grouping these two, you should document this more fully, exactly how
it behaves w/r/t invalid inputs, etc.

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

Moved, and incorporated the documentation into the big block at the top.

+  std::string string() const { return serialize(); }

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

Naming: ToString() / AsString()

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

Can't rename or remove until Blink doesn't use 'string()' anymore.

 
  private:
+  void Init(const GURL& url);
+
+  std::string scheme_;
+  std::string host_;
+  unsigned short port_;
+  bool unique_;
+  bool serialization_requires_port_;
+
   std::string string_;
 };
 
+// Stream operator so Origin can be used in assertion statements.
+URL_EXPORT std::ostream& operator<<(std::ostream& out, const url::Origin& url);
+
 }  // namespace url
 
 #endif  // URL_ORIGIN_H_