Move permission.mojom from WebKit/public/platform/ to components/

Move permission.mojom from WebKit/public/platform/ to components/

Mojo permission interface is needed by other parts of Chromium, e.g. media.
Hence move it to a more common place.

BUG=604503
TEST=No functionality change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/1b751521d3f027742f8f2b95fc814820977fc7a4
Cr-Commit-Position: refs/heads/master@{#391185}

[xhwang, 2016-04-25 21:23:30]

Description was changed from

==========
Move permission.mojom from WebKit/public/platform/ to components/

Mojo permission interface is needed by other parts of Chromium, e.g. media.
Hence move it to a more common place.

BUG=604503
TEST=No functionality change.
==========

to

==========
Move permission.mojom from WebKit/public/platform/ to components/

Mojo permission interface is needed by other parts of Chromium, e.g. media.
Hence move it to a more common place.

BUG=604503
TEST=No functionality change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[xhwang, 2016-04-25 21:30:24]

xhwang@chromium.org changed reviewers:
+ jam@chromium.org, mlamouri@chromium.org, sammc@chromium.org

[xhwang, 2016-04-25 21:30:25]

Please see CL description and bug for details.

This CL is not complete. Currently content/{browser|renderer|common} compiles in
GN builds. It definitely needs more polish to be complete. But before I go
further, I'd like to have you help check whether the current approach makes
sense. So please take a look at the new components/permission/* files to see
whether the file location, naming etc lgty. Thanks!

[haraken, 2016-04-26 08:01:33]

haraken@chromium.org changed reviewers:
+ esprehn@chromium.org, haraken@chromium.org, rockot@chromium.org,
yzshen@chromium.org

[haraken, 2016-04-26 08:01:33]

+esprehn +rockot +yzshen (about where we should put the mojom files)

[Ken Rockot(use gerrit already), 2016-04-26 16:06:35]

On 2016/04/26 at 08:01:33, haraken wrote:
> +esprehn +rockot +yzshen (about where we should put the mojom files)

Hmm. Seems incorrect to move this entirely out of content when it encapsulates
concepts (namely, specific permission types) which are still clearly specific to
the web layer.

If we're going to introduce a permission component, it should be agnostic to the
types of permissions it can host (e.g. we should use string names or something),
and an embedder (content for now; later probably //services/user) can deal with
registering supported permission types and behaviors.

[jam, 2016-04-26 16:31:43]

On 2016/04/26 16:06:35, Ken Rockot wrote:
> On 2016/04/26 at 08:01:33, haraken wrote:
> > +esprehn +rockot +yzshen (about where we should put the mojom files)
> 
> Hmm. Seems incorrect to move this entirely out of content when it encapsulates
> concepts (namely, specific permission types) which are still clearly specific
to
> the web layer.
> 
> If we're going to introduce a permission component, it should be agnostic to
the
> types of permissions it can host (e.g. we should use string names or
something),
> and an embedder (content for now; later probably //services/user) can deal
with
> registering supported permission types and behaviors.

Yeah that was my preferred solution as well, but given that we want to access
this now in src/media as well as blink, it seemed preferable to switch the order
around to avoid duplicating this interface. more discussion at
https://codereview.chromium.org/1838903007/#msg11

[yzshen1, 2016-04-26 16:57:11]

On 2016/04/26 16:31:43, jam wrote:
> On 2016/04/26 16:06:35, Ken Rockot wrote:
> > On 2016/04/26 at 08:01:33, haraken wrote:
> > > +esprehn +rockot +yzshen (about where we should put the mojom files)
> > 
> > Hmm. Seems incorrect to move this entirely out of content when it
encapsulates
> > concepts (namely, specific permission types) which are still clearly
specific
> to
> > the web layer.
> > 
> > If we're going to introduce a permission component, it should be agnostic to
> the
> > types of permissions it can host (e.g. we should use string names or
> something),
> > and an embedder (content for now; later probably //services/user) can deal
> with
> > registering supported permission types and behaviors.
> 
> Yeah that was my preferred solution as well, but given that we want to access
> this now in src/media as well as blink, it seemed preferable to switch the
order
> around to avoid duplicating this interface. more discussion at
> https://codereview.chromium.org/1838903007/#msg11

I noticed that I have been CC-ed about where we should put the mojom files.
John's opinion SGTM and I don't have anything to add. :)

[xhwang, 2016-04-26 23:33:04]

On 2016/04/26 16:57:11, yzshen1 wrote:
> On 2016/04/26 16:31:43, jam wrote:
> > On 2016/04/26 16:06:35, Ken Rockot wrote:
> > > On 2016/04/26 at 08:01:33, haraken wrote:
> > > > +esprehn +rockot +yzshen (about where we should put the mojom files)
> > > 
> > > Hmm. Seems incorrect to move this entirely out of content when it
> encapsulates
> > > concepts (namely, specific permission types) which are still clearly
> specific
> > to
> > > the web layer.
> > > 
> > > If we're going to introduce a permission component, it should be agnostic
to
> > the
> > > types of permissions it can host (e.g. we should use string names or
> > something),
> > > and an embedder (content for now; later probably //services/user) can deal
> > with
> > > registering supported permission types and behaviors.
> > 
> > Yeah that was my preferred solution as well, but given that we want to
access
> > this now in src/media as well as blink, it seemed preferable to switch the
> order
> > around to avoid duplicating this interface. more discussion at
> > https://codereview.chromium.org/1838903007/#msg11
> 
> I noticed that I have been CC-ed about where we should put the mojom files.
> John's opinion SGTM and I don't have anything to add. :)

This change still makes sense to me barring the legit concerns Ken has. But now
it
seems too much effort to fix my small issue :( I looked at a previous approach I
had (https://chromiumcodereview.appspot.com/1837143002) and now it actually
makes
more sense. I'll continue that work to see whether I could have a better
solution
there without having to move permission services to components/. If that doesn't
work I'll come back to this CL.

Thanks all for the comments.

[mlamouri (slow - plz ping), 2016-04-27 13:00:58]

Looks generally fine. I have one question though. Should we call the components
"permissions" given that we have chrome/browser/permissions/? It sounds error
prone to have chrome/browser/permissions/ and components/permission/.

https://codereview.chromium.org/1921553003/diff/1/content/content_renderer.gypi
File content/content_renderer.gypi (right):

https://codereview.chromium.org/1921553003/diff/1/content/content_renderer.gy...
content/content_renderer.gypi:12:
#'../components/permission/permission.gyp:permission_mojo_bindings',
what did you mean to do here?

[xhwang, 2016-04-27 21:04:29]

On 2016/04/26 23:33:04, xhwang wrote:
> On 2016/04/26 16:57:11, yzshen1 wrote:
> > On 2016/04/26 16:31:43, jam wrote:
> > > On 2016/04/26 16:06:35, Ken Rockot wrote:
> > > > On 2016/04/26 at 08:01:33, haraken wrote:
> > > > > +esprehn +rockot +yzshen (about where we should put the mojom files)
> > > > 
> > > > Hmm. Seems incorrect to move this entirely out of content when it
> > encapsulates
> > > > concepts (namely, specific permission types) which are still clearly
> > specific
> > > to
> > > > the web layer.
> > > > 
> > > > If we're going to introduce a permission component, it should be
agnostic
> to
> > > the
> > > > types of permissions it can host (e.g. we should use string names or
> > > something),
> > > > and an embedder (content for now; later probably //services/user) can
deal
> > > with
> > > > registering supported permission types and behaviors.
> > > 
> > > Yeah that was my preferred solution as well, but given that we want to
> access
> > > this now in src/media as well as blink, it seemed preferable to switch the
> > order
> > > around to avoid duplicating this interface. more discussion at
> > > https://codereview.chromium.org/1838903007/#msg11
> > 
> > I noticed that I have been CC-ed about where we should put the mojom files.
> > John's opinion SGTM and I don't have anything to add. :)
> 
> This change still makes sense to me barring the legit concerns Ken has. But
now
> it
> seems too much effort to fix my small issue :( I looked at a previous approach
I
> had (https://chromiumcodereview.appspot.com/1837143002) and now it actually
> makes
> more sense. I'll continue that work to see whether I could have a better
> solution
> there without having to move permission services to components/. If that
doesn't
> work I'll come back to this CL.
> 
> Thanks all for the comments.

I am back to this CL. I tried the other approach and it didn't work well*.

jam: I looked at previous discussions around low level services and your design
doc.
Does it make sense to put permissions.mojom in //services/permissions/? Note
that
the interface implementation will still be mostly in content/chrome.

* More details: I tried to host the permission service in GpuProcessHost so that
we
can connect to the permission service from the GPU process. But apparently
PermissionServiceContext depends on BrowserContext [1], while GPU process is
shared by all tabs so it should know nothing about BrowserContext.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/pe...

[xhwang, 2016-04-27 21:06:43]

On 2016/04/27 13:00:58, Mounir Lamouri wrote:
> Looks generally fine. I have one question though. Should we call the
components
> "permissions" given that we have chrome/browser/permissions/? It sounds error
> prone to have chrome/browser/permissions/ and components/permission/.

"permissions" sgtm. I'll rename it.
 
>
https://codereview.chromium.org/1921553003/diff/1/content/content_renderer.gypi
> File content/content_renderer.gypi (right):
> 
>
https://codereview.chromium.org/1921553003/diff/1/content/content_renderer.gy...
> content/content_renderer.gypi:12:
> #'../components/permission/permission.gyp:permission_mojo_bindings',
> what did you mean to do here?

I was trying to get the gyp build working but then I messed it up. I'll revisit
to see what I need to do here.

[xhwang, 2016-04-28 22:25:36]

rebase only

[xhwang, 2016-04-29 00:18:36]

It seems now I have too many reviewers on this CL. I'll move everybody to CC and
re-add reviewers as necessary.

[xhwang, 2016-04-29 00:18:58]

Description was changed from

==========
Move permission.mojom from WebKit/public/platform/ to components/

Mojo permission interface is needed by other parts of Chromium, e.g. media.
Hence move it to a more common place.

BUG=604503
TEST=No functionality change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move permission.mojom from WebKit/public/platform/ to components/

Mojo permission interface is needed by other parts of Chromium, e.g. media.
Hence move it to a more common place.

BUG=604503
TEST=No functionality change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[xhwang, 2016-04-29 00:18:58]

xhwang@chromium.org changed reviewers:
- esprehn@chromium.org, haraken@chromium.org, jam@chromium.org,
mlamouri@chromium.org, rockot@chromium.org, sammc@chromium.org,
yzshen@chromium.org

[xhwang, 2016-04-29 00:22:04]

xhwang@chromium.org changed reviewers:
+ dcheng@chromium.org, jam@chromium.org, mlamouri@chromium.org

[xhwang, 2016-04-29 00:22:05]

jam@chromium.org: Please do a general review.

mlamouri@chromium.org: Please review changes as permission owners.

dcheng@chromium.org: Please do a security review (mojom/ipc).

[xhwang, 2016-04-29 04:29:13]

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

[commit-bot: I haz the power, 2016-04-29 04:29:24]

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

[commit-bot: I haz the power, 2016-04-29 06:03:34]

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

[commit-bot: I haz the power, 2016-04-29 06:03:35]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-04-29 12:43:26]

Assuming there was no other change than s/permission/permissions/ since the
previous version, lgtm.

[xhwang, 2016-05-02 18:25:18]

jam / dcheng: kindly ping!

[dcheng, 2016-05-02 18:28:53]

Mostly just skimmed.

RS LGTM for *.mojom changes (since it looks like it's just a move). Let me know
if I missed anything =)

https://codereview.chromium.org/1921553003/diff/80001/content/renderer/media/...
File content/renderer/media/media_permission_dispatcher.cc (right):

https://codereview.chromium.org/1921553003/diff/80001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher.cc:130:
permissions::mojom::PermissionStatus::GRANTED);
Huh, is this clang-format? I'm surprised it didn't wrap this to the next line.

[xhwang, 2016-05-02 18:35:31]

https://codereview.chromium.org/1921553003/diff/80001/content/renderer/media/...
File content/renderer/media/media_permission_dispatcher.cc (right):

https://codereview.chromium.org/1921553003/diff/80001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher.cc:130:
permissions::mojom::PermissionStatus::GRANTED);
On 2016/05/02 18:28:53, dcheng wrote:
> Huh, is this clang-format? I'm surprised it didn't wrap this to the next line.

Apparently yes :( I've stopped manual formatting for long time...

[dcheng, 2016-05-02 21:30:46]

https://codereview.chromium.org/1921553003/diff/80001/content/renderer/media/...
File content/renderer/media/media_permission_dispatcher.cc (right):

https://codereview.chromium.org/1921553003/diff/80001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher.cc:130:
permissions::mojom::PermissionStatus::GRANTED);
On 2016/05/02 at 18:35:31, xhwang wrote:
> On 2016/05/02 18:28:53, dcheng wrote:
> > Huh, is this clang-format? I'm surprised it didn't wrap this to the next
line.
> 
> Apparently yes :( I've stopped manual formatting for long time...

I've filed a bug upstream: https://llvm.org/bugs/show_bug.cgi?id=27608

But this is fine (I am a strong advocate of not fighting clang-format): it's
mostly just surprising =)

[jam, 2016-05-02 23:54:12]

On 2016/04/27 21:04:29, xhwang wrote:
> On 2016/04/26 23:33:04, xhwang wrote:
> > On 2016/04/26 16:57:11, yzshen1 wrote:
> > > On 2016/04/26 16:31:43, jam wrote:
> > > > On 2016/04/26 16:06:35, Ken Rockot wrote:
> > > > > On 2016/04/26 at 08:01:33, haraken wrote:
> > > > > > +esprehn +rockot +yzshen (about where we should put the mojom files)
> > > > > 
> > > > > Hmm. Seems incorrect to move this entirely out of content when it
> > > encapsulates
> > > > > concepts (namely, specific permission types) which are still clearly
> > > specific
> > > > to
> > > > > the web layer.
> > > > > 
> > > > > If we're going to introduce a permission component, it should be
> agnostic
> > to
> > > > the
> > > > > types of permissions it can host (e.g. we should use string names or
> > > > something),
> > > > > and an embedder (content for now; later probably //services/user) can
> deal
> > > > with
> > > > > registering supported permission types and behaviors.
> > > > 
> > > > Yeah that was my preferred solution as well, but given that we want to
> > access
> > > > this now in src/media as well as blink, it seemed preferable to switch
the
> > > order
> > > > around to avoid duplicating this interface. more discussion at
> > > > https://codereview.chromium.org/1838903007/#msg11
> > > 
> > > I noticed that I have been CC-ed about where we should put the mojom
files.
> > > John's opinion SGTM and I don't have anything to add. :)
> > 
> > This change still makes sense to me barring the legit concerns Ken has. But
> now
> > it
> > seems too much effort to fix my small issue :( I looked at a previous
approach
> I
> > had (https://chromiumcodereview.appspot.com/1837143002) and now it actually
> > makes
> > more sense. I'll continue that work to see whether I could have a better
> > solution
> > there without having to move permission services to components/. If that
> doesn't
> > work I'll come back to this CL.
> > 
> > Thanks all for the comments.
> 
> I am back to this CL. I tried the other approach and it didn't work well*.
> 
> jam: I looked at previous discussions around low level services and your
design
> doc.
> Does it make sense to put permissions.mojom in //services/permissions/? Note
> that
> the interface implementation will still be mostly in content/chrome.

I think components/ is the best place now, as per
https://code.google.com/p/chromium/codesearch#chromium/src/services/README.md...
we envision services/ as a place for services that could be in their own process
eventually (at least on desktop).

(reviewing the cl now)
> 
> * More details: I tried to host the permission service in GpuProcessHost so
that
> we
> can connect to the permission service from the GPU process. But apparently
> PermissionServiceContext depends on BrowserContext [1], while GPU process is
> shared by all tabs so it should know nothing about BrowserContext.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/pe...

[jam, 2016-05-03 03:32:46]

lgtm

https://codereview.chromium.org/1921553003/diff/80001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/1921553003/diff/80001/chrome/browser/DEPS#new...
chrome/browser/DEPS:103: "+components/permissions/permission_status.mojom.h",
not needed since there's a +components at top

https://codereview.chromium.org/1921553003/diff/80001/content/DEPS
File content/DEPS (right):

https://codereview.chromium.org/1921553003/diff/80001/content/DEPS#newcode2
content/DEPS:2: # from src/chrome in src/content. Also, we may only depend on
mojo interface
actually you can just remove the second sentence altogether, it's not true
anymore.

https://codereview.chromium.org/1921553003/diff/80001/content/DEPS#newcode115
content/DEPS:115: "+components/permissions",
put this up near the other components/ lines

https://codereview.chromium.org/1921553003/diff/80001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/1921553003/diff/80001/content/browser/DEPS#ne...
content/browser/DEPS:3: # depend on components which we share with the mojo
html_viewer.
remove this second sentence too please

https://codereview.chromium.org/1921553003/diff/80001/content/browser/DEPS#ne...
content/browser/DEPS:76: "+components/permissions/permission_status.mojom.h",
these lines aren't needed anymore

https://codereview.chromium.org/1921553003/diff/80001/content/common/DEPS
File content/common/DEPS (right):

https://codereview.chromium.org/1921553003/diff/80001/content/common/DEPS#new...
content/common/DEPS:35: "+components/permissions/permission_status.mojom.h",
not needed

[xhwang, 2016-05-03 04:49:24]

comments addressed

[xhwang, 2016-05-03 04:53:32]

rebase only

[xhwang, 2016-05-03 05:04:12]

https://chromiumcodereview.appspot.com/1921553003/diff/80001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://chromiumcodereview.appspot.com/1921553003/diff/80001/chrome/browser/D...
chrome/browser/DEPS:103: "+components/permissions/permission_status.mojom.h",
On 2016/05/03 03:32:46, jam wrote:
> not needed since there's a +components at top

Done.

https://chromiumcodereview.appspot.com/1921553003/diff/80001/content/DEPS
File content/DEPS (right):

https://chromiumcodereview.appspot.com/1921553003/diff/80001/content/DEPS#new...
content/DEPS:2: # from src/chrome in src/content. Also, we may only depend on
mojo interface
On 2016/05/03 03:32:46, jam wrote:
> actually you can just remove the second sentence altogether, it's not true
> anymore.

Done.

https://chromiumcodereview.appspot.com/1921553003/diff/80001/content/DEPS#new...
content/DEPS:115: "+components/permissions",
On 2016/05/03 03:32:46, jam wrote:
> put this up near the other components/ lines

Moved up. But I don't see other components/ lines.

https://chromiumcodereview.appspot.com/1921553003/diff/80001/content/browser/...
File content/browser/DEPS (right):

https://chromiumcodereview.appspot.com/1921553003/diff/80001/content/browser/...
content/browser/DEPS:3: # depend on components which we share with the mojo
html_viewer.
On 2016/05/03 03:32:46, jam wrote:
> remove this second sentence too please

Done.

https://chromiumcodereview.appspot.com/1921553003/diff/80001/content/browser/...
content/browser/DEPS:76: "+components/permissions/permission_status.mojom.h",
On 2016/05/03 03:32:46, jam wrote:
> these lines aren't needed anymore

Done.

https://chromiumcodereview.appspot.com/1921553003/diff/80001/content/common/DEPS
File content/common/DEPS (right):

https://chromiumcodereview.appspot.com/1921553003/diff/80001/content/common/D...
content/common/DEPS:35: "+components/permissions/permission_status.mojom.h",
On 2016/05/03 03:32:46, jam wrote:
> not needed

Done.

[xhwang, 2016-05-03 05:50:47]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2016-05-03 05:50:48]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
dcheng@chromium.org, mlamouri@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1921553003/#ps140001 (title: "fix gn
check")

[commit-bot: I haz the power, 2016-05-03 05:51:00]

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

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

Description was changed from

==========
Move permission.mojom from WebKit/public/platform/ to components/

Mojo permission interface is needed by other parts of Chromium, e.g. media.
Hence move it to a more common place.

BUG=604503
TEST=No functionality change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move permission.mojom from WebKit/public/platform/ to components/

Mojo permission interface is needed by other parts of Chromium, e.g. media.
Hence move it to a more common place.

BUG=604503
TEST=No functionality change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-05-03 08:17:24]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-03 08:18:55]

Description was changed from

==========
Move permission.mojom from WebKit/public/platform/ to components/

Mojo permission interface is needed by other parts of Chromium, e.g. media.
Hence move it to a more common place.

BUG=604503
TEST=No functionality change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move permission.mojom from WebKit/public/platform/ to components/

Mojo permission interface is needed by other parts of Chromium, e.g. media.
Hence move it to a more common place.

BUG=604503
TEST=No functionality change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/1b751521d3f027742f8f2b95fc814820977fc7a4
Cr-Commit-Position: refs/heads/master@{#391185}
==========

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

Patchset 8 (id:??) landed as
https://crrev.com/1b751521d3f027742f8f2b95fc814820977fc7a4
Cr-Commit-Position: refs/heads/master@{#391185}

[henrika (OOO until Aug 14), 2016-05-03 09:18:42]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1940223002/ by henrika@chromium.org.

The reason for reverting is: Breaks Chrome building:

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%....