Have Permission{Manager,Service} use Origin.

Have Permission{Manager,Service} use Origin.

Not GURL.

BUG=527149
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

[palmer, 2016-02-24 01:45:54]

Description was changed from

==========
Have Permission{Manager,Service} use Origin.

Not GURL.

BUG=527149
==========

to

==========
Have Permission{Manager,Service} use Origin.

Not GURL.

BUG=527149
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[palmer, 2016-02-24 01:47:49]

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

[commit-bot: I haz the power, 2016-02-24 01:50:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1726323002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1726323002/1

[commit-bot: I haz the power, 2016-02-24 02:17:50]

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

[commit-bot: I haz the power, 2016-02-24 02:17:51]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[palmer, 2016-03-03 01:00:09]

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

[commit-bot: I haz the power, 2016-03-03 01:00:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1726323002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1726323002/40001

[commit-bot: I haz the power, 2016-03-03 01:14:57]

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

[commit-bot: I haz the power, 2016-03-03 01:14:59]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[palmer, 2016-03-03 01:32:43]

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

[palmer, 2016-03-03 01:32:44]

Mike, could you please take a look at this and see if you think it's on the
right track? Thanks.

[Mike West, 2016-03-03 05:39:15]

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

[commit-bot: I haz the power, 2016-03-03 05:39:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1726323002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1726323002/60001

[Mike West, 2016-03-03 05:41:08]

On 2016/03/03 at 05:39:15, Mike West wrote:
> The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

Thank you for doing this grungy, grungy work. One question about the changes to
`url::Origin`, but this otherwise LGTM.

[Mike West, 2016-03-03 05:42:39]

On 2016/03/03 at 05:41:08, Mike West wrote:
> On 2016/03/03 at 05:39:15, Mike West wrote:
> > The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
> 
> Thank you for doing this grungy, grungy work. One question about the changes
to `url::Origin`, but this otherwise LGTM.

Actually, another question: should "empty" origins "==" each other? Are they
same-origin with each other? I think so.

I'd appreciate it if you added some tests for the new bits and pieces in
`url/origin_test.cc`. :)

[commit-bot: I haz the power, 2016-03-03 05:58:20]

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

[commit-bot: I haz the power, 2016-03-03 05:58:22]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Mike West, 2016-03-03 09:20:02]

https://codereview.chromium.org/1726323002/diff/60001/chrome/browser/geolocat...
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

https://codereview.chromium.org/1726323002/diff/60001/chrome/browser/geolocat...
chrome/browser/geolocation/geolocation_permission_context.cc:78:
GURL(requesting_frame.Serialize()), allowed);
Nit: Would you mind adding something like "// TODO(mkwst): Update
TabSpecificContentSettings::OnGeolocationPermissionset to use url::Origin.
crbug.com/XXXXXX" here and elsewhere so we call out what's left to do?

https://codereview.chromium.org/1726323002/diff/60001/url/origin.h
File url/origin.h (right):

https://codereview.chromium.org/1726323002/diff/60001/url/origin.h#newcode116
url/origin.h:116: bool is_empty() const { return empty_; }
If we're going to distinguish between unique origins and empty origins, should
we also distinguish the serializations? ("" vs "null")?

[palmer, 2016-03-03 21:32:57]

https://codereview.chromium.org/1726323002/diff/60001/chrome/browser/geolocat...
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

https://codereview.chromium.org/1726323002/diff/60001/chrome/browser/geolocat...
chrome/browser/geolocation/geolocation_permission_context.cc:78:
GURL(requesting_frame.Serialize()), allowed);
On 2016/03/03 09:20:02, Mike West wrote:
> Nit: Would you mind adding something like "// TODO(mkwst): Update
> TabSpecificContentSettings::OnGeolocationPermissionset to use url::Origin.
> crbug.com/XXXXXX" here and elsewhere so we call out what's left to do?

Done.

https://codereview.chromium.org/1726323002/diff/60001/url/origin.h
File url/origin.h (right):

https://codereview.chromium.org/1726323002/diff/60001/url/origin.h#newcode116
url/origin.h:116: bool is_empty() const { return empty_; }
On 2016/03/03 09:20:02, Mike West wrote:
> If we're going to distinguish between unique origins and empty origins, should
> we also distinguish the serializations? ("" vs "null")?

Done.

[palmer, 2016-03-03 21:33:37]

> Actually, another question: should "empty" origins "==" each other? Are they
> same-origin with each other? I think so.

Done.

> I'd appreciate it if you added some tests for the new bits and pieces in
> `url/origin_test.cc`. :)

Coming up!

[palmer, 2016-03-03 22:11:49]

palmer@chromium.org changed reviewers:
+ avi@chromium.org, dtrainor@chromium.org, jochen@chromium.org

[palmer, 2016-03-03 22:11:49]

This CL looks huge, but it's really about refactoring a widely-used interface
from taking a general type (GURL) to a more specific type (url::Origin). So
hopefully it's not quite as daunting as it seems...

(famous last words)

dtrainor: for blimp/OWNERS
avi: for content/OWNERS
jochen: for chrome/OWNERS
sgurun: for android_webview/OWNERS

[palmer, 2016-03-03 22:13:02]

palmer@chromium.org changed reviewers:
+ sgurun@chromium.org

[palmer, 2016-03-03 22:13:03]

Oops, forgot to add sgurun.

Additionally, some of the changes are due to "git cl format".

[Avi (use Gerrit), 2016-03-03 22:16:27]

content LGTM

[sgurun-gerrit only, 2016-03-03 22:21:01]

On 2016/03/03 22:16:27, Avi wrote:
> content LGTM

lgtm

[palmer, 2016-03-03 23:18:05]

https://codereview.chromium.org/1726323002/diff/120001/url/origin.h
File url/origin.h (right):

https://codereview.chromium.org/1726323002/diff/120001/url/origin.h#newcode119
url/origin.h:119: bool empty() const { return empty_; }
mkwst: Maybe we should name this |was_constructed_from_empty_url| instead of
|empty|? It's kind of a long name, but, it's kind of a weird concept. (And
rarely used.)

[Mike West, 2016-03-04 06:02:35]

On 2016/03/03 at 23:18:05, palmer wrote:
> https://codereview.chromium.org/1726323002/diff/120001/url/origin.h
> File url/origin.h (right):
> 
> https://codereview.chromium.org/1726323002/diff/120001/url/origin.h#newcode119
> url/origin.h:119: bool empty() const { return empty_; }
> mkwst: Maybe we should name this |was_constructed_from_empty_url| instead of
|empty|? It's kind of a long name, but, it's kind of a weird concept. (And
rarely used.)

Eh. I'd prefer to rename it `is_empty` so that it matches GURL exactly. That
seems more valuable than having a longer, more detailed name.

[Mike West, 2016-03-04 06:09:57]

https://codereview.chromium.org/1726323002/diff/120001/url/origin_unittest.cc
File url/origin_unittest.cc (right):

https://codereview.chromium.org/1726323002/diff/120001/url/origin_unittest.cc...
url/origin_unittest.cc:108: const char* empties[] = {"yay", ""};
Hrm. Why is "GURL('yay')" empty? I would have expected an invalid URL to produce
a unique origin. Though maybe I'm misremembering what GURL does? Is
`GURL('yay')` both invalid and empty? I expect it to be invalid but non-empty...

https://codereview.chromium.org/1726323002/diff/120001/url/origin_unittest.cc...
url/origin_unittest.cc:250: EXPECT_TRUE(origin.IsSameOriginWith(origin));
Nit: `EXPECT_TRUE(origin.empty());`

[mlamouri (slow - plz ping), 2016-03-05 22:20:16]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-03-05 22:20:18]

permissions code lgtm.

Though, shouldn't bug 515797 be fixed in order for this to land?

https://codereview.chromium.org/1726323002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/1726323002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:63: const url::Origin&
requesting_frame,
ditto

https://codereview.chromium.org/1726323002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:231:
PermissionUmaUtil::PermissionDismissed(permission_type_, requesting_url);
Should you open a bug for PermissionUmaUtil to use url::Origin?

https://codereview.chromium.org/1726323002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.h (right):

https://codereview.chromium.org/1726323002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.h:77: const url::Origin&
requesting_frame,
Can you rename |requesting_origin| or |requesting| if you prefer.

[jochen (gone - plz use gerrit), 2016-03-07 12:02:24]

lgtm

[David Trainor- moved to gerrit, 2016-03-07 18:55:33]

blimp/ lgtm

[palmer, 2016-03-08 01:20:00]

https://codereview.chromium.org/1726323002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/1726323002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:63: const url::Origin&
requesting_frame,
On 2016/03/05 22:20:18, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/1726323002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:231:
PermissionUmaUtil::PermissionDismissed(permission_type_, requesting_url);
> Should you open a bug for PermissionUmaUtil to use url::Origin?

https://bugs.chromium.org/p/chromium/issues/detail?id=592834

https://codereview.chromium.org/1726323002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.h (right):

https://codereview.chromium.org/1726323002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.h:77: const url::Origin&
requesting_frame,
On 2016/03/05 22:20:18, Mounir Lamouri wrote:
> Can you rename |requesting_origin| or |requesting| if you prefer.

Done.

https://codereview.chromium.org/1726323002/diff/120001/url/origin_unittest.cc
File url/origin_unittest.cc (right):

https://codereview.chromium.org/1726323002/diff/120001/url/origin_unittest.cc...
url/origin_unittest.cc:108: const char* empties[] = {"yay", ""};
> Hrm. Why is "GURL('yay')" empty? I would have expected an invalid URL to
produce
> a unique origin. Though maybe I'm misremembering what GURL does? Is
> `GURL('yay')` both invalid and empty? I expect it to be invalid but
non-empty...

GURL("yay") is invalid and empty.

I'm not sure why, or if that is correct or desirable. But that's how it is now.
:)

https://codereview.chromium.org/1726323002/diff/120001/url/origin_unittest.cc...
url/origin_unittest.cc:250: EXPECT_TRUE(origin.IsSameOriginWith(origin));
On 2016/03/04 06:09:56, Mike West wrote:
> Nit: `EXPECT_TRUE(origin.empty());`

Done.

[palmer, 2016-03-08 01:20:58]

mkwst: What do you think of mlamouri's comment in
https://codereview.chromium.org/1726323002/#msg31:

"""Though, shouldn't bug 515797 be fixed in order for this to land?"""

Are you close to closing that bug?

[palmer, 2016-03-09 20:56:47]

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

[commit-bot: I haz the power, 2016-03-09 20:58:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1726323002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1726323002/160001

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

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

[commit-bot: I haz the power, 2016-03-09 21:19:11]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[palmer, 2016-03-09 22:10:59]

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

[commit-bot: I haz the power, 2016-03-09 22:11:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1726323002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1726323002/180001

[commit-bot: I haz the power, 2016-03-09 22:29:07]

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

[commit-bot: I haz the power, 2016-03-09 22:29:08]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[mlamouri (slow - plz ping), 2016-03-16 11:27:39]

On 2016/03/08 at 01:20:58, palmer wrote:
> mkwst: What do you think of mlamouri's comment in
https://codereview.chromium.org/1726323002/#msg31:
> 
> """Though, shouldn't bug 515797 be fixed in order for this to land?"""
> 
> Are you close to closing that bug?

It seems to me that we might end up regressing the permissions behaviour for
file:// if we don't fix that but I would be happy to defer to whatever mkwst@
thinks is right.

[palmer, 2016-03-17 01:17:09]

> > """Though, shouldn't bug 515797 be fixed in order for this to land?"""
> > 
> > Are you close to closing that bug?
> 
> It seems to me that we might end up regressing the permissions behaviour for
> file:// if we don't fix that but I would be happy to defer to whatever mkwst@
> thinks is right.

I am getting these Blink test failures¸e.g. ./out/Debug/content_shell
--run-layout-test --no-sandbox
http/tests/permissions/chromium/test-change-event-worker.html. |location| is
'test-change-event-worker.html' but location.origin is ''. I'm not sure yet if
that is due to this CL directly, or due to bug 515797. Any clues appreciated;
I'll start gnawing on this some more tomorrow.

[mlamouri (slow - plz ping), 2016-03-21 14:18:24]

On 2016/03/17 at 01:17:09, palmer wrote:
> > > """Though, shouldn't bug 515797 be fixed in order for this to land?"""
> > > 
> > > Are you close to closing that bug?
> > 
> > It seems to me that we might end up regressing the permissions behaviour for
> > file:// if we don't fix that but I would be happy to defer to whatever
mkwst@
> > thinks is right.
> 
> I am getting these Blink test failures¸e.g. ./out/Debug/content_shell
--run-layout-test --no-sandbox
http/tests/permissions/chromium/test-change-event-worker.html. |location| is
'test-change-event-worker.html' but location.origin is ''. I'm not sure yet if
that is due to this CL directly, or due to bug 515797. Any clues appreciated;
I'll start gnawing on this some more tomorrow.

This is expected. This test is meant to be run with
./blink/tools/run_layout_tests.py (which will start an HTTP server). You can run
it by creating the HTTP server yourself. If you don't, you will have a file://
location but it is expected to have some real non-local origin. The test should
be able to handle localhost:xxxx.

[palmer, 2016-03-28 23:00:34]

> This is expected. This test is meant to be run with
> ./blink/tools/run_layout_tests.py (which will start an HTTP server). You can
run
> it by creating the HTTP server yourself. If you don't, you will have a file://
> location but it is expected to have some real non-local origin. The test
should
> be able to handle localhost:xxxx.

Thanks for the clue. :) But, I'm not sure I understand, then. Don't the trybots
run the tests with /blink/tools/run_layout_tests.py and using an HTTP server?

[palmer, 2016-03-28 23:01:50]

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

[commit-bot: I haz the power, 2016-03-28 23:02:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1726323002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1726323002/260001

[commit-bot: I haz the power, 2016-03-28 23:19:20]

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

[commit-bot: I haz the power, 2016-03-28 23:19:22]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[mlamouri (slow - plz ping), 2016-03-31 21:41:12]

On 2016/03/28 at 23:00:34, palmer wrote:
> > This is expected. This test is meant to be run with
> > ./blink/tools/run_layout_tests.py (which will start an HTTP server). You can
run
> > it by creating the HTTP server yourself. If you don't, you will have a
file://
> > location but it is expected to have some real non-local origin. The test
should
> > be able to handle localhost:xxxx.
> 
> Thanks for the clue. :) But, I'm not sure I understand, then. Don't the
trybots run the tests with /blink/tools/run_layout_tests.py and using an HTTP
server?

Yes, they do that. Though, in your comment you said you do
`./out/Debug/content_shell --run-layout-test --no-sandbox
http/tests/permissions/chromium/test-change-event-worker.html` which isn't
running an HTTP server, right?