DO NOT LAND

DO NOT LAND

Starting to restructure 'url::Origin', and to drop usage of
'GURL::GetOrigin()' from //net.

BUG=490074

[Mike West, 2015-05-21 15:00:54]

mkwst@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Mike West, 2015-05-21 15:00:55]

Exploring what it would take to get rid of `GetOrigin` in //net. This isn't for
landing, but up as a discussion piece while we figure out what `Origin` should
mean, where it should live, and how it should be used.

[Ryan Sleevi, 2015-05-22 02:50:04]

Thanks for taking the time to whip this together. I'm guessing you based it on
looking in //net for things that deal with origins.

I think this highlights another interesting dichotomy - those features of the
platform that need to treat Origins as first-class (e.g. null origins aren't
equivalent, file handling, etc) and those that are using "origins" as internal
tuples for (scheme, host, port), in which null origins ARE to fold into the
"everything else" bucket.

I left some comments that hopefully provide a bit more illustration of the split
I mention, and why the behaviour changes here are... not easy.

This is somewhat of the same issue with the SchemeIsCryptographic change (which,
different from SchemeIsSecure, removed support for filesystem), in that if the
reviewer was paying attention, they had to make sure that the behaviour change
was side-effect free. Here, the behaviour change is how invalid URLs are
handled, and it's... tricky... to know what should happen

https://codereview.chromium.org/1151843002/diff/20001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/1151843002/diff/20001/net/base/net_util.cc#ne...
net/base/net_util.cc:417: return base::StringPrintf("%s:%d",
origin.host().c_str(), origin.port());
Except Origin.host == HostNoBrackets() == BAY SPLOSIONS

https://codereview.chromium.org/1151843002/diff/20001/net/ftp/ftp_auth_cache.h
File net/ftp/ftp_auth_cache.h (right):

https://codereview.chromium.org/1151843002/diff/20001/net/ftp/ftp_auth_cache....
net/ftp/ftp_auth_cache.h:49: void Remove(const url::Origin& origin, const
AuthCredentials& credentials);
So, overall, changes like this make me... Generally nervous. And I guess this
probably highlights some of my unease re: url::Origin

Yes, if you get rid of GURL::GetOrigin(), then this is necessary. But this comes
with it a non-trivial, subtle behaviour change that has to be evaluated on a
case by case basis. That is, we have to make sure that you can't pass a 'null'
origin here, or someone could flood the cache. In the existing behaviour, all
such invalid origins would collapse into a single origin.

You can argue it's good ("but they COULD BE different") or bad (if anyone can
get junk in, they can do a DoS)

That's why I'm nervous about the overall approach. I absolutely don't think a
bombing run like this can be rubberstamped - every one of these calls has to be
checked for callers to see if they're handling user input, if bad origins could
be provided, what the expectations are, etc.

https://codereview.chromium.org/1151843002/diff/20001/net/http/http_auth_cach...
File net/http/http_auth_cache.cc (right):

https://codereview.chromium.org/1151843002/diff/20001/net/http/http_auth_cach...
net/http/http_auth_cache.cc:49: origin.SchemeIs("wss"));
These all make me sad, because they all are strictly worse (re: constants and
such) than the original code.

https://codereview.chromium.org/1151843002/diff/20001/url/origin.cc
File url/origin.cc (right):

https://codereview.chromium.org/1151843002/diff/20001/url/origin.cc#newcode27
url/origin.cc:27: host_ = "";
scheme_.clear()
host_.clear()

https://codereview.chromium.org/1151843002/diff/20001/url/origin.cc#newcode35
url/origin.cc:35: // Ok. We have an origin.
No pronouns in comments or the boogey-sleevi will get you. Not to be confused
with the boogy-sleevi, who will get down.

https://codereview.chromium.org/1151843002/diff/20001/url/origin.cc#newcode40
url/origin.cc:40: }
API wise, I don't think this is right. You're encoding assumptions here re:
standard URLs.

More importantly, why can't you just use origin.scheme()?

I suspect what you really want is something like

scheme_ = origin.scheme()

if (!origin.SchemeIsStandard() || origin.SchemeIsFile())
  return;

host_ = origin.HostNoBrackets();  <-- I'm not sure how I feel about this, for
the record.

https://codereview.chromium.org/1151843002/diff/20001/url/origin.cc#newcode46
url/origin.cc:46: is_default_port_ = origin.EffectiveIntPort() !=
origin.IntPort();
BUG: I don't think this does what you think it does

If the GURL didn't specify a port, origin.IntPort() == PORT_UNSPECIFIED
If the scheme has a default port, origin.EffectiveIntPort != PORT_UNSPECIFIED

https://codereview.chromium.org/1151843002/diff/20001/url/origin.cc#newcode58
url/origin.cc:58: }
url code uses the URL constants -
https://code.google.com/p/chromium/codesearch#chromium/src/url/url_constants....

https://codereview.chromium.org/1151843002/diff/20001/url/origin.h
File url/origin.h (right):

https://codereview.chromium.org/1151843002/diff/20001/url/origin.h#newcode23
url/origin.h:23: explicit Origin(const std::string& origin);
Not a fan of having a string constructor - gives us no opportunity to validate
that origin is wellformed.

For that matter, having a GURL constructor is problematic if !url.is_valid() -
there's no way to surface that the origin is invalid/unexpressable.

https://codereview.chromium.org/1151843002/diff/20001/url/origin.h#newcode41
url/origin.h:41: std::string string() const { return serialize(); }
Generally not a fan of classes managing their own serialization; seems to
violate the single-responsibility model

https://codereview.chromium.org/1151843002/diff/20001/url/origin.h#newcode44
url/origin.h:44: bool operator!=(const Origin& other) const;
Blink vs Chrome will strike again -
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Operator_Over...

[Ryan Sleevi, 2016-05-16 20:58:04]

On 2015/05/22 02:50:04, Ryan Sleevi wrote:
> Thanks for taking the time to whip this together. I'm guessing you based it on
> looking in //net for things that deal with origins.

Is this something you're still working on? Can/should we close out this CL?
Trying to clean up my dashboard