Move geolocation and permission mojoms into WebKit/public/platform.

Move geolocation and permission mojoms into WebKit/public/platform.

BUG=593607, 596209
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/fcd191dc45a3114ffbe1305b38b27e19b91a32c7
Cr-Commit-Position: refs/heads/master@{#384536}

[Sam McNally, 2016-03-07 06:23:21]

Description was changed from

==========
Move geolocation and permission mojoms into WebKit/public/platform.

BUG=561879
==========

to

==========
Move geolocation and permission mojoms into WebKit/public/platform.

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

[Sam McNally, 2016-03-09 06:28:20]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2016-03-09 06:40:12]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2016-03-09 06:40:23]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2016-03-09 06:40:38]

Patchset #1 (id:60001) has been deleted

[Sam McNally, 2016-03-09 23:58:47]

sammc@chromium.org changed reviewers:
+ esprehn@chromium.org

[Sam McNally, 2016-03-09 23:58:48]

PTAL at the blink side.

[Michael van Ouwerkerk, 2016-03-10 10:40:52]

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

[Michael van Ouwerkerk, 2016-03-10 10:40:53]

geolocation lgtm

https://codereview.chromium.org/1771743002/diff/80001/content/browser/geoloca...
File content/browser/geolocation/geolocation_service_context.h (right):

https://codereview.chromium.org/1771743002/diff/80001/content/browser/geoloca...
content/browser/geolocation/geolocation_service_context.h:27:
mojo::InterfaceRequest<blink::mojom::GeolocationService> request);
IWYU: #include
"third_party/WebKit/public/platform/modules/geolocation/geolocation.mojom.h"

[mlamouri (slow - plz ping), 2016-03-12 06:27:30]

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

[mlamouri (slow - plz ping), 2016-03-12 06:27:31]

https://codereview.chromium.org/1771743002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/permissions/permission.mojom
(right):

https://codereview.chromium.org/1771743002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/permissions/permission.mojom:12: };
Why did you change from import to included?

[Sam McNally, 2016-03-14 02:12:42]

Patchset #3 (id:120001) has been deleted

[Sam McNally, 2016-03-14 02:42:10]

https://codereview.chromium.org/1771743002/diff/80001/content/browser/geoloca...
File content/browser/geolocation/geolocation_service_context.h (right):

https://codereview.chromium.org/1771743002/diff/80001/content/browser/geoloca...
content/browser/geolocation/geolocation_service_context.h:27:
mojo::InterfaceRequest<blink::mojom::GeolocationService> request);
On 2016/03/10 10:40:53, Michael van Ouwerkerk wrote:
> IWYU: #include
> "third_party/WebKit/public/platform/modules/geolocation/geolocation.mojom.h"

Done.

https://codereview.chromium.org/1771743002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/permissions/permission.mojom
(right):

https://codereview.chromium.org/1771743002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/permissions/permission.mojom:12: };
On 2016/03/12 06:27:31, Mounir Lamouri wrote:
> Why did you change from import to included?

It seemed simpler. Taking another look, quite a few users do only need
PermissionStatus. I've changed it back.

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

Are we fine to have a type used all over content/ and chrome/ and other content
embedders (blimp/, android_webview/ cast/ and third parties) be defined in
Blink? My understanding was that Blink includes should be exceptional. We are
making it on purpose common here.

https://codereview.chromium.org/1771743002/diff/160001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/1771743002/diff/160001/content/browser/DEPS#n...
content/browser/DEPS:72:
"+third_party/WebKit/public/platform/modules/permissions/permission_status.mojom.h",
If we move basic types like PermissionStatus to Blink, shouldn't we reflect that
kind of new design policy to DEPS? (ie. allow Blink headers to be used?).

[Sam McNally, 2016-03-19 00:32:17]

On 2016/03/18 14:24:59, Mounir Lamouri wrote:
> Are we fine to have a type used all over content/ and chrome/ and other
content
> embedders (blimp/, android_webview/ cast/ and third parties) be defined in
> Blink? My understanding was that Blink includes should be exceptional. We are
> making it on purpose common here.

Yes. This is where mojom consumed by Blink that don't belong somewhere else
(e.g. //device) are going to be. See
https://groups.google.com/a/chromium.org/d/topic/chromium-mojo/ZR2YlRV7Uxs/di....

https://codereview.chromium.org/1771743002/diff/160001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/1771743002/diff/160001/content/browser/DEPS#n...
content/browser/DEPS:72:
"+third_party/WebKit/public/platform/modules/permissions/permission_status.mojom.h",
On 2016/03/18 14:24:59, Mounir Lamouri wrote:
> If we move basic types like PermissionStatus to Blink, shouldn't we reflect
that
> kind of new design policy to DEPS? (ie. allow Blink headers to be used?).

I don't think all Blink public platform headers should be useable from
everywhere.

[Sam McNally, 2016-03-19 00:39:05]

Description was changed from

==========
Move geolocation and permission mojoms into WebKit/public/platform.

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

to

==========
Move geolocation and permission mojoms into WebKit/public/platform.

BUG=561879, 596209
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Sam McNally, 2016-03-19 00:39:43]

Description was changed from

==========
Move geolocation and permission mojoms into WebKit/public/platform.

BUG=561879, 596209
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move geolocation and permission mojoms into WebKit/public/platform.

BUG=593607, 596209
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[mlamouri (slow - plz ping), 2016-03-21 14:19:56]

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

[mlamouri (slow - plz ping), 2016-03-21 14:19:57]

I guess my concern is at a higher level than the code you want me to review.
lgtm, then.

+jochen@ in case of he has opinions.

[Sam McNally, 2016-03-22 00:38:22]

esprehn: ping

[jochen (gone - plz use gerrit), 2016-03-24 14:32:44]

I defer to esprehn@ but my take is that it's fine to use a blink type all over
the place

[Sam McNally, 2016-03-29 01:40:17]

Patchset #5 (id:180001) has been deleted

[Sam McNally, 2016-03-29 01:40:30]

Patchset #5 (id:200001) has been deleted

[esprehn, 2016-03-31 04:50:02]

From my perspective the permission stuff is a layer below blink, I don't
understand why a PDF plugin wouldn't need the same permission prompt powers.

I defer to jam@ for where he wants to put this stuff. Having it inside blink
seems very weird to me.

lgtm for now though, burning down the walls between content and blink is a
higher priority than figuring out where the mojoms go.

[Sam McNally, 2016-03-31 09:06:00]

Patchset #8 (id:280001) has been deleted

[Sam McNally, 2016-03-31 09:21:08]

sammc@chromium.org changed reviewers:
+ dcheng@chromium.org, jam@chromium.org

[Sam McNally, 2016-03-31 09:21:09]

+dcheng for the mojoms

+jam for pretty much everything else

[jam, 2016-03-31 18:22:06]

On 2016/03/31 04:50:02, esprehn wrote:
> From my perspective the permission stuff is a layer below blink, I don't
> understand why a PDF plugin wouldn't need the same permission prompt powers.
> 
> I defer to jam@ for where he wants to put this stuff. Having it inside blink
> seems very weird to me.
> 
> lgtm for now though, burning down the walls between content and blink is a
> higher priority than figuring out where the mojoms go.

Right, the permission stuff will eventually be below blink. See the chrome
servicification design doc,
https://docs.google.com/document/d/1Q40FQiphP1SZNr010ZERqo9aKSY3ozcMjDl7NjeMf...,
which includes it among other low level shared services. Also the "Location of
cross-module mojom files" thread on chromium-mojo@ & platform-architecture-dev@

We aim to be conservative in how we define these shared interfaces. i.e. we
don't want to rush to move any of the initial set of shared services listed
above until
1) we are confident that the mojom is generic
2) the implementation of the mojom is separated out of chrome so that it can
live beside the mojom

For this specific mojom, the above aren't satisfied because no one has started
working on making a self contained service around this yet:
1) the fact that it has to list all the possible values is problematic, i.e.
this is specific to blink. so we'll probably want to generalize that, so it's
reusable from arbitrary apps that consume the service.
2) the implementation is in chrome/ for the most part, with a mostly small
proxying layer in content that should just be removed.

Given that blink is the only consumer right now, let's put the mojom in blink's
public directory as it's comparable to other delegate interfaces there. Then
when we get to making a self contained service for the permission code, whoever
is working on that will convert blink to using it which will include moving the
mojom out.

[jam, 2016-03-31 18:28:11]

On 2016/03/19 00:32:17, Sam McNally wrote:
> On 2016/03/18 14:24:59, Mounir Lamouri wrote:
> > Are we fine to have a type used all over content/ and chrome/ and other
> content
> > embedders (blimp/, android_webview/ cast/ and third parties) be defined in
> > Blink? My understanding was that Blink includes should be exceptional. We
are
> > making it on purpose common here.

We've always allowed enum & POD headers, and now mojoms. This is to avoid
wrapping all blink structs and enums with a wrapper type.

see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/DEP...
and
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/DE...

> 
> Yes. This is where mojom consumed by Blink that don't belong somewhere else
> (e.g. //device) are going to be. See
>
https://groups.google.com/a/chromium.org/d/topic/chromium-mojo/ZR2YlRV7Uxs/di....
> 
> https://codereview.chromium.org/1771743002/diff/160001/content/browser/DEPS
> File content/browser/DEPS (right):
> 
>
https://codereview.chromium.org/1771743002/diff/160001/content/browser/DEPS#n...
> content/browser/DEPS:72:
>
"+third_party/WebKit/public/platform/modules/permissions/permission_status.mojom.h",
> On 2016/03/18 14:24:59, Mounir Lamouri wrote:
> > If we move basic types like PermissionStatus to Blink, shouldn't we reflect
> that
> > kind of new design policy to DEPS? (ie. allow Blink headers to be used?).
> 
> I don't think all Blink public platform headers should be useable from
> everywhere.

correct, the deps rules linked above enforce that

[jam, 2016-03-31 19:45:52]

lgtm

https://codereview.chromium.org/1771743002/diff/300001/content/browser/permis...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/1771743002/diff/300001/content/browser/permis...
content/browser/permissions/permission_service_impl.cc:24: case
PermissionName::GEOLOCATION:
(not for this cl)

this duplicate enum seems unnecessary; we should remove it. the only difference
is BACKGROUND_SYNC which isn't in the mojom, seems fine to add it there.

https://codereview.chromium.org/1771743002/diff/300001/content/browser/permis...
File content/browser/permissions/permission_service_impl.h (right):

https://codereview.chromium.org/1771743002/diff/300001/content/browser/permis...
content/browser/permissions/permission_service_impl.h:26: class
PermissionServiceImpl : public blink::mojom::PermissionService {
(not for this cl)

this class doesn't have much logic in it. it seems that we can remove it
altogether and move the implementation to the application layer.

[dcheng, 2016-03-31 20:34:32]

mojom lgtm (I didn't really look at anything else)

[Sam McNally, 2016-03-31 22:27:13]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-03-31 22:27:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvanouwerkerk@chromium.org, mlamouri@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1771743002/#ps300001
(title: " ")

[commit-bot: I haz the power, 2016-03-31 22:27:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1771743002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1771743002/300001

[commit-bot: I haz the power, 2016-03-31 22:35:41]

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

[commit-bot: I haz the power, 2016-03-31 22:35:45]

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

[Sam McNally, 2016-04-01 00:19:06]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-04-01 00:19:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, esprehn@chromium.org, jam@chromium.org,
dcheng@chromium.org, mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/1771743002/#ps320001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-01 00:19:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1771743002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1771743002/320001

[commit-bot: I haz the power, 2016-04-01 00:28:49]

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

[commit-bot: I haz the power, 2016-04-01 00:28:52]

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

[Sam McNally, 2016-04-01 00:35:33]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-04-01 00:35:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, esprehn@chromium.org, jam@chromium.org,
dcheng@chromium.org, mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/1771743002/#ps340001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-01 00:36:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1771743002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1771743002/340001

[Sam McNally, 2016-04-01 03:59:08]

The CQ bit was unchecked by sammc@chromium.org

[Sam McNally, 2016-04-01 03:59:43]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2016-04-01 04:00:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1771743002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1771743002/340001

[commit-bot: I haz the power, 2016-04-01 06:37:50]

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

[commit-bot: I haz the power, 2016-04-01 06:37:52]

Exceeded global retry quota

[Sam McNally, 2016-04-01 08:06:42]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2016-04-01 08:06:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1771743002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1771743002/340001

[commit-bot: I haz the power, 2016-04-01 10:03:12]

Description was changed from

==========
Move geolocation and permission mojoms into WebKit/public/platform.

BUG=593607, 596209
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move geolocation and permission mojoms into WebKit/public/platform.

BUG=593607, 596209
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-04-01 10:03:15]

Committed patchset #10 (id:340001)

[commit-bot: I haz the power, 2016-04-01 10:04:57]

Description was changed from

==========
Move geolocation and permission mojoms into WebKit/public/platform.

BUG=593607, 596209
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move geolocation and permission mojoms into WebKit/public/platform.

BUG=593607, 596209
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/fcd191dc45a3114ffbe1305b38b27e19b91a32c7
Cr-Commit-Position: refs/heads/master@{#384536}
==========

[commit-bot: I haz the power, 2016-04-01 10:05:00]

Patchset 10 (id:??) landed as
https://crrev.com/fcd191dc45a3114ffbe1305b38b27e19b91a32c7
Cr-Commit-Position: refs/heads/master@{#384536}