Add suborigin logic to url::Origin

Add suborigin logic to url::Origin

In order for the browser to correctly reason about suborigins, this adds
support to url::Origin to parse and understand suborigins. It separates
requests for hosts and schemes from the embedded suborigin
serialization, while keeping the same-origin policy checks intact so a
suborigin is a different origin from other suborigins at the same
physical origin.

This updates url/origin.* so that given a suborigin encoded in a GURL,
will correctly deserialize the suborigin and store it accordingly, while
the scheme/host/port tuple is left with the real scheme/host/port.
Additionally, removes the content/public/common/origin_util.h functions
for using suborigins since url::Origin should now be used in their
stead.

BUG=336894, 649893
Committed: https://crrev.com/0448040f47ea1ba6a5c6ee03c82eff41854f0e0e
Cr-Commit-Position: refs/heads/master@{#427254}

[jww, 2016-10-09 19:21:12]

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

[jww, 2016-10-09 19:21:13]

Mike, can you review this CL, focusing on url/, which is the core? I'll ask
msramek@ and nasko@ for chrome/browser/ and content/ reviews, respectively,
after you've taken a look.

[jww, 2016-10-09 19:21:34]

On 2016/10/09 19:21:13, jww wrote:
> Mike, can you review this CL, focusing on url/, which is the core? I'll ask
> msramek@ and nasko@ for chrome/browser/ and content/ reviews, respectively,
> after you've taken a look.

s/the core/the core of this CL/

[jww, 2016-10-09 19:21:52]

The CQ bit was checked by jww@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-09 19:21:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-09 20:54:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-09 20:54:59]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-10-11 07:40:41]

This looks like a reasonable approach, thanks for putting it together.

I guess the core question for me is whether or not this is the state in which we
ought to leave things. This implementation of suborigins seems tightly coupled
to URL and origin processing. Perhaps so tightly coupled that implementing the
whole thing on `url::Origin` is really the right thing to do.

It would be worth looping in Anne to get his opinion on the implications for the
URL and HTML specifications.

https://codereview.chromium.org/2403713002/diff/20001/content/public/common/u...
File content/public/common/url_constants.h (left):

https://codereview.chromium.org/2403713002/diff/20001/content/public/common/u...
content/public/common/url_constants.h:78: CONTENT_EXPORT extern const char
kHttpsSuboriginScheme[];
If we're moving in this direction, we really ought to file a bug against URL and
HTML to see about upstreaming this change. It's pointing to a pretty tight
integration of suborigins with origins themselves in terms of parsing,
serialization, comparison, etc.

https://codereview.chromium.org/2403713002/diff/20001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/2403713002/diff/20001/url/gurl.h#newcode244
url/gurl.h:244: SchemeIs(url::kHttpsSuboriginScheme);
This needs tests.

https://codereview.chromium.org/2403713002/diff/20001/url/gurl.h#newcode253
url/gurl.h:253: bool SchemeIsSuborigin() const {
Ditto.

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

https://codereview.chromium.org/2403713002/diff/20001/url/origin.h#newcode124
url/origin.h:124: // exists, returning only the physical origin.. For example,
when
Nit: ..

https://codereview.chromium.org/2403713002/diff/20001/url/origin_unittest.cc
File url/origin_unittest.cc (right):

https://codereview.chromium.org/2403713002/diff/20001/url/origin_unittest.cc#...
url/origin_unittest.cc:265: EXPECT_EQ(test_case.is_same_origin,
origin1.IsSameOriginWith(origin2));
Can you add a self-comparison (which presumably will always be true?)?

https://codereview.chromium.org/2403713002/diff/20001/url/url_canon_stdurl.cc
File url/url_canon_stdurl.cc (right):

https://codereview.chromium.org/2403713002/diff/20001/url/url_canon_stdurl.cc...
url/url_canon_stdurl.cc:129: default_port = 80;
443? Both of these should have tests as well.

[jww, 2016-10-11 18:16:10]

Thanks, Mike! I think I've addressed your comments.

On 2016/10/11 07:40:41, Mike West wrote:
> This looks like a reasonable approach, thanks for putting it together.
> 
> I guess the core question for me is whether or not this is the state in which
we
> ought to leave things. This implementation of suborigins seems tightly coupled
> to URL and origin processing. Perhaps so tightly coupled that implementing the
> whole thing on `url::Origin` is really the right thing to do.
> 
> It would be worth looping in Anne to get his opinion on the implications for
the
> URL and HTML specifications.
Indeed. Anne brought this up in
https://github.com/w3c/webappsec-suborigins/issues/38. I have a personal action
item to work on this integration, and I thought there was a suborigin spec issue
for it as well, but apparently not. I'd *like* to keep the suborigin spec as
separated out as possible from the URL and HTML specs, but I agree that it's
inevitable that we're going to have to update some of the serialization and
origin portions in terms of those specs as well.

In any case, for now, tightly coupling with url::Origin is a necessity, even if
I'd like it to be separate in the future.
> 
>
https://codereview.chromium.org/2403713002/diff/20001/content/public/common/u...
> File content/public/common/url_constants.h (left):
> 
>
https://codereview.chromium.org/2403713002/diff/20001/content/public/common/u...
> content/public/common/url_constants.h:78: CONTENT_EXPORT extern const char
> kHttpsSuboriginScheme[];
> If we're moving in this direction, we really ought to file a bug against URL
and
> HTML to see about upstreaming this change. It's pointing to a pretty tight
> integration of suborigins with origins themselves in terms of parsing,
> serialization, comparison, etc.
> 
> https://codereview.chromium.org/2403713002/diff/20001/url/gurl.h
> File url/gurl.h (right):
> 
> https://codereview.chromium.org/2403713002/diff/20001/url/gurl.h#newcode244
> url/gurl.h:244: SchemeIs(url::kHttpsSuboriginScheme);
> This needs tests.
> 
> https://codereview.chromium.org/2403713002/diff/20001/url/gurl.h#newcode253
> url/gurl.h:253: bool SchemeIsSuborigin() const {
> Ditto.
> 
> https://codereview.chromium.org/2403713002/diff/20001/url/origin.h
> File url/origin.h (right):
> 
> https://codereview.chromium.org/2403713002/diff/20001/url/origin.h#newcode124
> url/origin.h:124: // exists, returning only the physical origin.. For example,
> when
> Nit: ..
> 
> https://codereview.chromium.org/2403713002/diff/20001/url/origin_unittest.cc
> File url/origin_unittest.cc (right):
> 
>
https://codereview.chromium.org/2403713002/diff/20001/url/origin_unittest.cc#...
> url/origin_unittest.cc:265: EXPECT_EQ(test_case.is_same_origin,
> origin1.IsSameOriginWith(origin2));
> Can you add a self-comparison (which presumably will always be true?)?
> 
> https://codereview.chromium.org/2403713002/diff/20001/url/url_canon_stdurl.cc
> File url/url_canon_stdurl.cc (right):
> 
>
https://codereview.chromium.org/2403713002/diff/20001/url/url_canon_stdurl.cc...
> url/url_canon_stdurl.cc:129: default_port = 80;
> 443? Both of these should have tests as well.

[jww, 2016-10-11 18:16:27]

Oops, here are my actual response to your comments.

https://codereview.chromium.org/2403713002/diff/20001/content/public/common/u...
File content/public/common/url_constants.h (left):

https://codereview.chromium.org/2403713002/diff/20001/content/public/common/u...
content/public/common/url_constants.h:78: CONTENT_EXPORT extern const char
kHttpsSuboriginScheme[];
On 2016/10/11 07:40:41, Mike West wrote:
> If we're moving in this direction, we really ought to file a bug against URL
and
> HTML to see about upstreaming this change. It's pointing to a pretty tight
> integration of suborigins with origins themselves in terms of parsing,
> serialization, comparison, etc.

Indeed, that was a suggestion by Anne in the serialization spec issue. I have a
TODO for potential URL integration. I'm still not convinced it's the right way
to go, though.

https://codereview.chromium.org/2403713002/diff/20001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/2403713002/diff/20001/url/gurl.h#newcode244
url/gurl.h:244: SchemeIs(url::kHttpsSuboriginScheme);
On 2016/10/11 07:40:41, Mike West wrote:
> This needs tests.

Done.

https://codereview.chromium.org/2403713002/diff/20001/url/gurl.h#newcode253
url/gurl.h:253: bool SchemeIsSuborigin() const {
On 2016/10/11 07:40:41, Mike West wrote:
> Ditto.

Done.

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

https://codereview.chromium.org/2403713002/diff/20001/url/origin.h#newcode124
url/origin.h:124: // exists, returning only the physical origin.. For example,
when
On 2016/10/11 07:40:41, Mike West wrote:
> Nit: ..

Done.

https://codereview.chromium.org/2403713002/diff/20001/url/origin_unittest.cc
File url/origin_unittest.cc (right):

https://codereview.chromium.org/2403713002/diff/20001/url/origin_unittest.cc#...
url/origin_unittest.cc:265: EXPECT_EQ(test_case.is_same_origin,
origin1.IsSameOriginWith(origin2));
On 2016/10/11 07:40:41, Mike West wrote:
> Can you add a self-comparison (which presumably will always be true?)?

Done.

https://codereview.chromium.org/2403713002/diff/20001/url/url_canon_stdurl.cc
File url/url_canon_stdurl.cc (right):

https://codereview.chromium.org/2403713002/diff/20001/url/url_canon_stdurl.cc...
url/url_canon_stdurl.cc:129: default_port = 80;
On 2016/10/11 07:40:41, Mike West wrote:
> 443? Both of these should have tests as well.

Done.

[jww, 2016-10-11 21:45:16]

jww@chromium.org changed reviewers:
+ msramek@chromium.org, nasko@chromium.org

[jww, 2016-10-11 21:45:17]

nasko@, can you take a look at the content/ bits?
msramek@, can you look at chrome/browser/?

Thanks!

[msramek, 2016-10-12 13:38:12]

browsing_data/ LGTM

[nasko, 2016-10-12 17:36:28]

I won't be able to get to this CL likely today.

[Mike West, 2016-10-13 09:50:36]

//url LGTM. Thanks for taking another pass.

[nasko, 2016-10-13 22:16:44]

nasko@chromium.org changed reviewers:
+ michaeln@chromium.org

[nasko, 2016-10-13 22:16:45]

The CL looks good and I've left some comments.

Adding michaeln@ to review the content/browser/dom_storage/ code for OWNERS.
I'll stamp it once he has it approved.

https://codereview.chromium.org/2403713002/diff/40001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/2403713002/diff/40001/url/gurl.h#newcode252
url/gurl.h:252: // Returns true if the scheme indicates a suborigin URL.
nit: An URL can't really be suborigin, just the serialized version of an Origin.
It will be good to make the comment explicit that the GURL is for serialized
Origin not something real on the web.

https://codereview.chromium.org/2403713002/diff/40001/url/gurl_unittest.cc
File url/gurl_unittest.cc (right):

https://codereview.chromium.org/2403713002/diff/40001/url/gurl_unittest.cc#ne...
url/gurl_unittest.cc:688:
EXPECT_TRUE(GURL("HTTPS-SO://foo.bar.com/").SchemeIsCryptographic());
nit: I'd add mixed case instead of/in addition to all capitals.

https://codereview.chromium.org/2403713002/diff/40001/url/origin.cc
File url/origin.cc (right):

https://codereview.chromium.org/2403713002/diff/40001/url/origin.cc#newcode56
url/origin.cc:56: }
Why not keep the same code in AddSuboriginToUrl? There you use the tertiary
operator and no DCHECK, here you have if statement and DCHECK. I like DCHECKs in
general :).

https://codereview.chromium.org/2403713002/diff/40001/url/origin.cc#newcode71
url/origin.cc:71: suborigin_ = "";
Why not add this to the initializer list? That way, you can just return straight
away and you will get the default unique_ = true and empty string for
suborigin_.

https://codereview.chromium.org/2403713002/diff/40001/url/origin.cc#newcode126
url/origin.cc:126: return tuple_.Serialize();
This part of the comment is a bit confusing "so that alone is what is needed to
serialize for the suborigin". Did you mean something like "so that alone is what
is needed to serialize without the suborigin"?

https://codereview.chromium.org/2403713002/diff/40001/url/url_canon_unittest.cc
File url/url_canon_unittest.cc (right):

https://codereview.chromium.org/2403713002/diff/40001/url/url_canon_unittest....
url/url_canon_unittest.cc:2176: TEST(URLCanonTest, DefaultPortForScheme) {
Hmm, this test case doesn't belong logically with change for suborigins. I'd
personally commit it in a separate CL.

[jww, 2016-10-14 01:08:58]

Thanks, nasko!

https://codereview.chromium.org/2403713002/diff/40001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/2403713002/diff/40001/url/gurl.h#newcode252
url/gurl.h:252: // Returns true if the scheme indicates a suborigin URL.
On 2016/10/13 22:16:45, nasko (slow) wrote:
> nit: An URL can't really be suborigin, just the serialized version of an
Origin.
> It will be good to make the comment explicit that the GURL is for serialized
> Origin not something real on the web.

I've updated the wording, but you might want to double check that it's clear
enough.

https://codereview.chromium.org/2403713002/diff/40001/url/gurl_unittest.cc
File url/gurl_unittest.cc (right):

https://codereview.chromium.org/2403713002/diff/40001/url/gurl_unittest.cc#ne...
url/gurl_unittest.cc:688:
EXPECT_TRUE(GURL("HTTPS-SO://foo.bar.com/").SchemeIsCryptographic());
On 2016/10/13 22:16:45, nasko (slow) wrote:
> nit: I'd add mixed case instead of/in addition to all capitals.

Done.

https://codereview.chromium.org/2403713002/diff/40001/url/origin.cc
File url/origin.cc (right):

https://codereview.chromium.org/2403713002/diff/40001/url/origin.cc#newcode56
url/origin.cc:56: }
On 2016/10/13 22:16:45, nasko (slow) wrote:
> Why not keep the same code in AddSuboriginToUrl? There you use the tertiary
> operator and no DCHECK, here you have if statement and DCHECK. I like DCHECKs
in
> general :).

Done.

https://codereview.chromium.org/2403713002/diff/40001/url/origin.cc#newcode71
url/origin.cc:71: suborigin_ = "";
On 2016/10/13 22:16:45, nasko (slow) wrote:
> Why not add this to the initializer list? That way, you can just return
straight
> away and you will get the default unique_ = true and empty string for
> suborigin_.

Done.

https://codereview.chromium.org/2403713002/diff/40001/url/origin.cc#newcode126
url/origin.cc:126: return tuple_.Serialize();
On 2016/10/13 22:16:45, nasko (slow) wrote:
> This part of the comment is a bit confusing "so that alone is what is needed
to
> serialize for the suborigin". Did you mean something like "so that alone is
what
> is needed to serialize without the suborigin"?

Yes, that's what I meant. I tried to clarify the comment; let me know if it's no
good.

https://codereview.chromium.org/2403713002/diff/40001/url/url_canon_unittest.cc
File url/url_canon_unittest.cc (right):

https://codereview.chromium.org/2403713002/diff/40001/url/url_canon_unittest....
url/url_canon_unittest.cc:2176: TEST(URLCanonTest, DefaultPortForScheme) {
On 2016/10/13 22:16:45, nasko (slow) wrote:
> Hmm, this test case doesn't belong logically with change for suborigins. I'd
> personally commit it in a separate CL.

How so? I added Suborigins to DefaultPortForScheme in this CL (and it is a
necessary addition for serialization), and Mike pointed out that I ought to unit
test that change. Unfortunately, there wasn't already a unit test for
DefaultPortForScheme, so it felt natural to add the other schemes while adding
the unit test for suborigins.

[nasko, 2016-10-14 14:12:41]

Thanks!

https://codereview.chromium.org/2403713002/diff/40001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/2403713002/diff/40001/url/gurl.h#newcode252
url/gurl.h:252: // Returns true if the scheme indicates a suborigin URL.
On 2016/10/14 01:08:57, jww wrote:
> On 2016/10/13 22:16:45, nasko (slow) wrote:
> > nit: An URL can't really be suborigin, just the serialized version of an
> Origin.
> > It will be good to make the comment explicit that the GURL is for serialized
> > Origin not something real on the web.
> 
> I've updated the wording, but you might want to double check that it's clear
> enough.

Acknowledged.

https://codereview.chromium.org/2403713002/diff/40001/url/url_canon_unittest.cc
File url/url_canon_unittest.cc (right):

https://codereview.chromium.org/2403713002/diff/40001/url/url_canon_unittest....
url/url_canon_unittest.cc:2176: TEST(URLCanonTest, DefaultPortForScheme) {
On 2016/10/14 01:08:58, jww wrote:
> On 2016/10/13 22:16:45, nasko (slow) wrote:
> > Hmm, this test case doesn't belong logically with change for suborigins. I'd
> > personally commit it in a separate CL.
> 
> How so? I added Suborigins to DefaultPortForScheme in this CL (and it is a
> necessary addition for serialization), and Mike pointed out that I ought to
unit
> test that change. Unfortunately, there wasn't already a unit test for
> DefaultPortForScheme, so it felt natural to add the other schemes while adding
> the unit test for suborigins.

It is totally fine if you want to commit it this way, I just missed the "nit:"
prefix in my comment. I was just stating personal preference on committing
chunks of code.

https://codereview.chromium.org/2403713002/diff/80001/url/origin.cc
File url/origin.cc (right):

https://codereview.chromium.org/2403713002/diff/80001/url/origin.cc#newcode132
url/origin.cc:132: // physical origin. Thus the tuple is all all that is needed
to serialize the
nit: "all all".

[jww, 2016-10-14 14:28:19]

Ah, got it. I just uploaded https://codereview.chromium.org/2419083002/ to add
the DefaultSchemeHostPort unit test separately and will modify this CL
accordingly once this goes through.

[michaeln, 2016-10-17 20:06:59]

i'm not so familiar with this, pardon the stupid questions,  i'd like understand
wassup

https://codereview.chromium.org/2403713002/diff/120001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

https://codereview.chromium.org/2403713002/diff/120001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl.cc:218: if
(origin.IsSameOriginWith(origin) ||
What is the desired behavior if an "origin_url" explicitly expresses a
suborigin?

a. only the data for the specific suborigin of physorigin is deleted?
b. all data for the physorigin is deleted (all suborigins)?

Looks like this method does (b), is that right? And is that the desired outcome?

I see this comment...
// Suborigin storage should be deleted in tandem with the physical origin's
// storage. https://w3c.github.io/webappsec-suborigins/

My question is a little different, should all physorigin storage be deleted
given only a suborigin as input?

Is there a way to JUST delete a suborigin and leave the others alone? Does there
need to be?

If the inputs to the method can only contain PhysOrigins, can we have a DCHECK
to indicate that?

[jww, 2016-10-18 06:44:19]

https://codereview.chromium.org/2403713002/diff/120001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

https://codereview.chromium.org/2403713002/diff/120001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl.cc:218: if
(origin.IsSameOriginWith(origin) ||
Great point, Michael! In short, yes, you can yet an origin_url which is a
serialized suborigin. Frankly, I hadn't thought about this direction (origin_url
contains a suborigin) explicitly before. Now that I'm considering it, it should
delete the entire physical origin since, from the user's perspective, they're
the same. So I'll update the comments to reflect this, and add a test as well.

On 2016/10/17 20:06:59, michaeln wrote:
> What is the desired behavior if an "origin_url" explicitly expresses a
> suborigin?
> 
> a. only the data for the specific suborigin of physorigin is deleted?
> b. all data for the physorigin is deleted (all suborigins)?
> 
> Looks like this method does (b), is that right? And is that the desired
outcome?
> 
> I see this comment...
> // Suborigin storage should be deleted in tandem with the physical origin's
> // storage. https://w3c.github.io/webappsec-suborigins/
> 
> My question is a little different, should all physorigin storage be deleted
> given only a suborigin as input?
> 
> Is there a way to JUST delete a suborigin and leave the others alone? Does
there
> need to be?
> 
> If the inputs to the method can only contain PhysOrigins, can we have a DCHECK
> to indicate that?
>

[michaeln, 2016-10-19 00:17:30]

I'm not sure about the semantic squishiness of sub vs phys origin? 

>
https://codereview.chromium.org/2403713002/diff/120001/content/browser/dom_st...
> content/browser/dom_storage/dom_storage_context_impl.cc:218: if
> (origin.IsSameOriginWith(origin) ||
> Great point, Michael! In short, yes, you can yet an origin_url which is a
> serialized suborigin. Frankly, I hadn't thought about this direction
(origin_url
> contains a suborigin) explicitly before. Now that I'm considering it, it
should
> delete the entire physical origin since, from the user's perspective, they're
> the same. So I'll update the comments to reflect this, and add a test as well.

That seems unexpected to me? How can I delete just SUB1?

SUB1 = https-so://sub1.host.com
SUB2 = https-so://sub2.host.com
PHYS = https://host.com

Deleting PHYS --> deletes all three makes sense (assuming you know about
suborigins), but when deleting only SUB1, I think I'd be surprised to see SUB2
and PHYS also go away. Seems like your adding the ability to logically separate
the data but not adding the ability for the caller (or ultimately the user) to
take advantage of that separation?

Here's another option for this method that gives the caller the ability to take
advantage of the separation of data.
* given a suborigin, only delete that exact suborigin's data
* given a physorigin, delete it and all it's suborigins

If caller's if this method explicit want the semantics you describe, that's
easily enough accomplished...

content->DeleteLocalStorage(origin.GetPhysicalOrigin().GetURL());

wdty?

[jww, 2016-10-19 00:27:24]

On 2016/10/19 00:17:30, michaeln wrote:
> I'm not sure about the semantic squishiness of sub vs phys origin? 
Indeed, it's quite confusing (in part because we're teasing out some ideas that
we have implicitly defined in the past but didn't make explicit, e.g. that the
parsed scheme/host/port of a URL isn't necessarily it's origin, a la blob: and
filesystem:).

"Physical origin" refers to the (scheme, host, port) tuple that defines an
origin today. The origin is the (scheme, host, port) tuple plus a suborigin
namespace. Note that if origin O has an empty suborigin (which is what you get
if you don't use the suborigin header), origin O is same-origin with the
physical origin of origin O, but we still talk about "origin O having a physical
origin". This is because, as best as possible, we 
> 
> >
>
https://codereview.chromium.org/2403713002/diff/120001/content/browser/dom_st...
> > content/browser/dom_storage/dom_storage_context_impl.cc:218: if
> > (origin.IsSameOriginWith(origin) ||
> > Great point, Michael! In short, yes, you can yet an origin_url which is a
> > serialized suborigin. Frankly, I hadn't thought about this direction
> (origin_url
> > contains a suborigin) explicitly before. Now that I'm considering it, it
> should
> > delete the entire physical origin since, from the user's perspective,
they're
> > the same. So I'll update the comments to reflect this, and add a test as
well.
> 
> That seems unexpected to me? How can I delete just SUB1?
> 
> SUB1 = https-so://sub1.host.com
> SUB2 = https-so://sub2.host.com
> PHYS = https://host.com
> 
> Deleting PHYS --> deletes all three makes sense (assuming you know about
> suborigins), but when deleting only SUB1, I think I'd be surprised to see SUB2
> and PHYS also go away. Seems like your adding the ability to logically
separate
> the data but not adding the ability for the caller (or ultimately the user) to
> take advantage of that separation?
> 
> Here's another option for this method that gives the caller the ability to
take
> advantage of the separation of data.
> * given a suborigin, only delete that exact suborigin's data
> * given a physorigin, delete it and all it's suborigins
> 
> If caller's if this method explicit want the semantics you describe, that's
> easily enough accomplished...
> 
> content->DeleteLocalStorage(origin.GetPhysicalOrigin().GetURL());
> 
> wdty?

I don't agree, at least right now. One of the principles of suborigins is that
users should not see or deal with suborigins; they're an application-level
concept. So in this case, if the browser is deleting their history and storage,
for example, there shouldn't be a difference between deleting the history and
storage of the suborigin vs an empty suborigin. Say the user goes to a page,
they click on the site settings for the page, and they say "blow away storage."
It doesn't matter to them if that page is in a suborigin (that's an
implementation detail); they should have all of their data for that physical
origin blown away.

Maybe there's some future where we want to have a more fine grained separation,
but I don't know exactly what we'd need that for, and I don't think it's where
we want to be right now.

[michaeln, 2016-10-19 20:36:02]

> > I'm not sure about the semantic squishiness of sub vs phys origin? 
> Indeed, it's quite confusing

Great, so let's make it less magical more explicit when code reading --> less
confusing.

> I don't agree, at least right now. One of the principles of suborigins is that
> users should not see or deal with suborigins; they're an application-level
> concept. So in this case, if the browser is deleting their history and
storage,
> for example, there shouldn't be a difference between deleting the history and
> storage of the suborigin vs an empty suborigin. Say the user goes to a page,
> they click on the site settings for the page, and they say "blow away
storage."
> It doesn't matter to them if that page is in a suborigin (that's an
> implementation detail); they should have all of their data for that physical
> origin blown away.

I don't disagree with this policy for how this is exposed to end-users, if
blurring the distinction is the right answer for users, great.  My argument is
with blurring the distinction to programmers and having it not be clear in code
what is going on. People need to know of the distinction to be able to maintain
correctness.

If the higher logic always wants delete them in lock step, thats a fine policy,
I'm just saying let's make that more clear and less confusing. Here's a concrete
suggestion for how to to that, rename the existing method to...

void DeleteLocalStorageForPhysicalOrigin(physorign) {
  DCHECK(physorign.suborigin().empty())
  ...
}


Add a new inline method to the context object for compat with existing callsites

void DeleteLocalStorage(ori) {
  DeleteLocalStorageForPhysicalOrigin(orig.GetPhysicalOrigin());
}

It might be beneficial to update the callsites to use the ForPhysicalOrigin()
method directly (and not have the compat method) to make it super clear at
callsites what is going on. I realize that would make the change a little
larger, but it makes it very clear that you can't delete just a suborigin's data
independently (which is what you want).
 
Updating the public api to reflect this behavior is a good thing.

https://codereview.chromium.org/2403713002/diff/180001/url/origin.h
File url/origin.h (right):

https://codereview.chromium.org/2403713002/diff/180001/url/origin.h#newcode145
url/origin.h:145: bool IsSamePhysicalOriginWith(const Origin& other) const;
Might be nice to have an Origin GetPhysicalOrigin() method too.

[michaeln, 2016-10-19 20:52:50]

ultimately... with layering in mind, the context api should *let* callers delete
just a suborigin if they so desire (and the entire physorigin too as a seperate
method), higher level logic should clearly express what it wants

[michaeln, 2016-10-19 21:11:39]

https://codereview.chromium.org/2403713002/diff/180001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_local_storage_helper.cc (right):

https://codereview.chromium.org/2403713002/diff/180001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_local_storage_helper.cc:139:
BrowsingDataLocalStorageHelper::DeleteOrigin(origin_url);
not directly related to your cl, but maybe you know?

Do you know why this 'canned' class calls thru to its base class? It seems like
the intent of the canned class is to mock up a helper rather than provide an
interface to the actual LocalStorage repository. This method will call thru to
the real thing and that doesn't seem quite right?

[jww, 2016-10-19 22:11:23]

OK, that makes sense. I'll update the CL accordingly in a bit to hopefully make
the API clearer. (Some other responses  below.)

https://codereview.chromium.org/2403713002/diff/180001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_local_storage_helper.cc (right):

https://codereview.chromium.org/2403713002/diff/180001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_local_storage_helper.cc:139:
BrowsingDataLocalStorageHelper::DeleteOrigin(origin_url);
On 2016/10/19 21:11:39, michaeln wrote:
> not directly related to your cl, but maybe you know?
> 
> Do you know why this 'canned' class calls thru to its base class? It seems
like
> the intent of the canned class is to mock up a helper rather than provide an
> interface to the actual LocalStorage repository. This method will call thru to
> the real thing and that doesn't seem quite right?

I'm not sure, but I *think* what's going on is that it essentially keeps the
pending info around, and all of the other methods it overrides are merely
information retrieval, so there's no reason to go to the underlying storage
context. However, this method is destructive, so it needs to pass on the
deletion to the storage context. That's my best guess at any rate.

https://codereview.chromium.org/2403713002/diff/180001/url/origin.h
File url/origin.h (right):

https://codereview.chromium.org/2403713002/diff/180001/url/origin.h#newcode145
url/origin.h:145: bool IsSamePhysicalOriginWith(const Origin& other) const;
On 2016/10/19 20:36:02, michaeln wrote:
> Might be nice to have an Origin GetPhysicalOrigin() method too.

Indeed. Per a suggestion from nasko@, I'm thinking it may make sense to replace
this with just:
   Origin GetPhysicalOrigin() const;
and then callers can use IsWithOriginWith() on the result of that value.

However, I'd rather do that in a follow-up CL, as I've just started thinking
about that possibility. Is that OK?

[michaeln, 2016-10-19 22:32:37]

sgtm, thnx!

[jww, 2016-10-20 06:03:47]

Hey Michael, let me know if this clarifies the API a bit.

https://codereview.chromium.org/2403713002/diff/180001/url/origin.h
File url/origin.h (right):

https://codereview.chromium.org/2403713002/diff/180001/url/origin.h#newcode145
url/origin.h:145: bool IsSamePhysicalOriginWith(const Origin& other) const;
On 2016/10/19 22:11:23, jww wrote:
> On 2016/10/19 20:36:02, michaeln wrote:
> > Might be nice to have an Origin GetPhysicalOrigin() method too.
> 
> Indeed. Per a suggestion from nasko@, I'm thinking it may make sense to
replace
> this with just:
>    Origin GetPhysicalOrigin() const;
> and then callers can use IsWithOriginWith() on the result of that value.
> 
> However, I'd rather do that in a follow-up CL, as I've just started thinking
> about that possibility. Is that OK?

Changed my mind and added it in this CL. I think it simplifies some
unnecessarily messy code.

[michaeln, 2016-10-21 23:07:12]

Sorry for moving the target on you :(

https://codereview.chromium.org/2403713002/diff/200001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

https://codereview.chromium.org/2403713002/diff/200001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl.cc:209:
DCHECK(origin.suborigin().empty());
if we redefine the semantics as described i the .h file, we should remove this
dcheck

https://codereview.chromium.org/2403713002/diff/200001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl.cc:226:
DeleteAndClearStorageNamespaceForOrigin(origin_candidate_url, local);
call DeleteLocalStorage here and on line 232

https://codereview.chromium.org/2403713002/diff/200001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl.cc:235: void
DOMStorageContextImpl::DeleteLocalStorage(const GURL& origin_url) {
make this method do what DeleteAndClearStorageNamespaceForOrigin does and remove
that method

https://codereview.chromium.org/2403713002/diff/200001/content/public/browser...
File content/public/browser/dom_storage_context.h (right):

https://codereview.chromium.org/2403713002/diff/200001/content/public/browser...
content/public/browser/dom_storage_context.h:54: virtual void
DeleteLocalStorage(const GURL& origin) = 0;
Looking at this again i think we should retain the ability to delete a singular
area, and add a seperate function to delete the multiple areas related to a
physorigin. The latter function builds on the former function. There are very
few callsites, so updating to use DeleteLocalStorageForPhysicalOrigin() where
appropiate is easy enough.

DeleteLocalStorage() deletes just one specific <schm,host,port> identified
DOMStorageArea (the function you're CL is currently calling
DeleteAndClearStorageNamespaceForOrigin) 

and 

DeleteLocalStorageForPhysicalOrigin() deletes all areas (top level and sub
origins) for the ori.PhysOrigin of the origin provided.

[jww, 2016-10-22 00:43:13]

Thanks, Michael. Those changes definitely make sense, especially once I reviewed
the call sites more closely. Let me know how it looks now!

https://codereview.chromium.org/2403713002/diff/200001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

https://codereview.chromium.org/2403713002/diff/200001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl.cc:209:
DCHECK(origin.suborigin().empty());
On 2016/10/21 23:07:12, michaeln wrote:
> if we redefine the semantics as described i the .h file, we should remove this
> dcheck

Done.

https://codereview.chromium.org/2403713002/diff/200001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl.cc:226:
DeleteAndClearStorageNamespaceForOrigin(origin_candidate_url, local);
On 2016/10/21 23:07:12, michaeln wrote:
> call DeleteLocalStorage here and on line 232

Done.

https://codereview.chromium.org/2403713002/diff/200001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl.cc:235: void
DOMStorageContextImpl::DeleteLocalStorage(const GURL& origin_url) {
On 2016/10/21 23:07:12, michaeln wrote:
> make this method do what DeleteAndClearStorageNamespaceForOrigin does and
remove
> that method

Done.

https://codereview.chromium.org/2403713002/diff/200001/content/public/browser...
File content/public/browser/dom_storage_context.h (right):

https://codereview.chromium.org/2403713002/diff/200001/content/public/browser...
content/public/browser/dom_storage_context.h:54: virtual void
DeleteLocalStorage(const GURL& origin) = 0;
On 2016/10/21 23:07:12, michaeln wrote:
> Looking at this again i think we should retain the ability to delete a
singular
> area, and add a seperate function to delete the multiple areas related to a
> physorigin. The latter function builds on the former function. There are very
> few callsites, so updating to use DeleteLocalStorageForPhysicalOrigin() where
> appropiate is easy enough.
> 
> DeleteLocalStorage() deletes just one specific <schm,host,port> identified
> DOMStorageArea (the function you're CL is currently calling
> DeleteAndClearStorageNamespaceForOrigin) 
> 
> and 
> 
> DeleteLocalStorageForPhysicalOrigin() deletes all areas (top level and sub
> origins) for the ori.PhysOrigin of the origin provided.
> 

Done.

[michaeln, 2016-10-24 21:10:03]

lgtm, thnx!

https://codereview.chromium.org/2403713002/diff/220001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_local_storage_helper.cc (right):

https://codereview.chromium.org/2403713002/diff/220001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_local_storage_helper.cc:81:
origin.GetPhysicalOrigin().GetURL());
no need to call GetPhysOrigin anymore

https://codereview.chromium.org/2403713002/diff/220001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_impl.h (right):

https://codereview.chromium.org/2403713002/diff/220001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl.h:227: void
DeleteAndClearStorageNamespaceForOrigin(const GURL& origin_url,
this can be deleted too

https://codereview.chromium.org/2403713002/diff/220001/content/public/browser...
File content/public/browser/dom_storage_context.h (right):

https://codereview.chromium.org/2403713002/diff/220001/content/public/browser...
content/public/browser/dom_storage_context.h:44: // contains a suborigin,
deletes the empty suborigin as well.
i'm not sure the examples help or hurt clarity? consider simpler wording?

// Deletes the local storage data for the physical origin of |origin_url|,
// including the data for the top level origin and all suborigins.

If you really want to clarify how it behaves given a suborigin as input, maybe
say DeleteLocalStorageForPhysicalOrigin(ori) is functionally equivalent to 
DeleteLocalStorageForPhysicalOrigin(ori.GetPhysicalOrigin())

https://codereview.chromium.org/2403713002/diff/220001/content/public/browser...
content/public/browser/dom_storage_context.h:56: // suborigin.
the top part of the comment is stale

[jww, 2016-10-24 21:22:54]

Thanks, Michael.

nasko@, should be ready for your review now.

https://codereview.chromium.org/2403713002/diff/220001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_local_storage_helper.cc (right):

https://codereview.chromium.org/2403713002/diff/220001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_local_storage_helper.cc:81:
origin.GetPhysicalOrigin().GetURL());
On 2016/10/24 21:10:03, michaeln wrote:
> no need to call GetPhysOrigin anymore

Done.

https://codereview.chromium.org/2403713002/diff/220001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_impl.h (right):

https://codereview.chromium.org/2403713002/diff/220001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl.h:227: void
DeleteAndClearStorageNamespaceForOrigin(const GURL& origin_url,
On 2016/10/24 21:10:03, michaeln wrote:
> this can be deleted too

Done.

https://codereview.chromium.org/2403713002/diff/220001/content/public/browser...
File content/public/browser/dom_storage_context.h (right):

https://codereview.chromium.org/2403713002/diff/220001/content/public/browser...
content/public/browser/dom_storage_context.h:44: // contains a suborigin,
deletes the empty suborigin as well.
On 2016/10/24 21:10:03, michaeln wrote:
> i'm not sure the examples help or hurt clarity? consider simpler wording?
> 
> // Deletes the local storage data for the physical origin of |origin_url|,
> // including the data for the top level origin and all suborigins.
> 
> If you really want to clarify how it behaves given a suborigin as input, maybe
> say DeleteLocalStorageForPhysicalOrigin(ori) is functionally equivalent to 
> DeleteLocalStorageForPhysicalOrigin(ori.GetPhysicalOrigin())

Done.

https://codereview.chromium.org/2403713002/diff/220001/content/public/browser...
content/public/browser/dom_storage_context.h:56: // suborigin.
On 2016/10/24 21:10:03, michaeln wrote:
> the top part of the comment is stale

Given the simplification suggestion, I basically reverted this to the original
description.

[nasko, 2016-10-24 23:33:56]

content/ LGTM

[jww, 2016-10-25 00:06:01]

The CQ bit was checked by jww@chromium.org

[jww, 2016-10-25 00:06:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, mkwst@chromium.org, michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/2403713002/#ps260001
(title: "Rebase on ToT")

[commit-bot: I haz the power, 2016-10-25 00:06:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 00:12:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 00:12:05]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jww, 2016-10-25 00:53:57]

The CQ bit was checked by jww@chromium.org

[jww, 2016-10-25 00:53:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, michaeln@chromium.org, mkwst@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2403713002/#ps280001
(title: "Fix unit test")

[commit-bot: I haz the power, 2016-10-25 00:54:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 02:50:58]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-10-25 02:53:58]

Description was changed from

==========
Add suborigin logic to url::Origin

In order for the browser to correctly reason about suborigins, this adds
support to url::Origin to parse and understand suborigins. It separates
requests for hosts and schemes from the embedded suborigin
serialization, while keeping the same-origin policy checks intact so a
suborigin is a different origin from other suborigins at the same
physical origin.

This updates url/origin.* so that given a suborigin encoded in a GURL,
will correctly deserialize the suborigin and store it accordingly, while
the scheme/host/port tuple is left with the real scheme/host/port.
Additionally, removes the content/public/common/origin_util.h functions
for using suborigins since url::Origin should now be used in their
stead.

BUG=336894,649893
==========

to

==========
Add suborigin logic to url::Origin

In order for the browser to correctly reason about suborigins, this adds
support to url::Origin to parse and understand suborigins. It separates
requests for hosts and schemes from the embedded suborigin
serialization, while keeping the same-origin policy checks intact so a
suborigin is a different origin from other suborigins at the same
physical origin.

This updates url/origin.* so that given a suborigin encoded in a GURL,
will correctly deserialize the suborigin and store it accordingly, while
the scheme/host/port tuple is left with the real scheme/host/port.
Additionally, removes the content/public/common/origin_util.h functions
for using suborigins since url::Origin should now be used in their
stead.

BUG=336894,649893

Committed: https://crrev.com/0448040f47ea1ba6a5c6ee03c82eff41854f0e0e
Cr-Commit-Position: refs/heads/master@{#427254}
==========

[commit-bot: I haz the power, 2016-10-25 02:54:00]

Patchset 15 (id:??) landed as
https://crrev.com/0448040f47ea1ba6a5c6ee03c82eff41854f0e0e
Cr-Commit-Position: refs/heads/master@{#427254}