Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(232)

Issue 1838903007: media: Add mojo MediaPermission service

Created:
4 years, 8 months ago by xhwang
Modified:
4 years, 8 months ago
CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, feature-media-reviews_chromium.org, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, posciak+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

media: Add mojo MediaPermission service The CDM in the MojoMediaApplication needs to check media permissions in some cases. Today in the non-mojo world the check is done in the browser process in some ad-hoc places, in BrowserCdmManager on Android and ChromeCast, and in PlatformVerificationFlow on ChromeOS. Now with MojoMediaApplication, we have a unified way to host out-of-process CDMs, we'd like to have a unified way to check the permission as well, e.g. passing the MediaPermission to the CDM running in MojoMediaApplication. Note that our CDMs know neither content nor mojo. So to request permission, it can only talk to media::MediaPermission. Since permission is hosted in the browser in content, we need some extra plumbing. With this CL, now we have these interfaces and classes: - media::MediaPermission: C++ interface in media/ - (new) media::interfaces::MediaPermission: mojo interface in media/ - content::mojom::PermissionService: mojo interface in content/ - content::PermissionServiceContext: hosts content::mojom::PermissionService - (moved) content::MediaPermissionDispatcher: conent::mojom::PermissionService -> media::MediaPermission - (new) media::MediaPermissionImpl: media::MediaPermission -> media::interfaces::MediaPermission - (new) media::MojoMediaPermission: media::interfaces::MediaPermission -> media::MediaPermission This is how we plumb media::MediaPermission to a CDM running in the MojoMediaApplication: PermissionServiceContext -> MediaPermissionDispatcher -> MediaPermissionImpl -> mojo IPC -> MojoMediaPermission -> CDM In the next CL: - Create MojoMediaPermission in AndroidMojoMediaClient. - Pass MojoMediaPermission to AndroidCdmFactory and MediaDrmBridge. - Check protected media identifier permission in MediaDrmBridge. An alternative solution is to create a MediaPermissionDispatcher in the process where the MojoMediaApplication is hosted, and plumb it through to the CDM. With the ServiceRegistry in GPU process, this is prototyped in https://chromiumcodereview.appspot.com/1837143002/. It seems to have less layers involved. However, this solution is not generic. We need to do this differently when hosting the MojoMediaApplication in different processes. Therefore, the current approach is preferred. BUG=559839 TEST=Manually tested to verify the permission check is plumbed through. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+357 lines, -222 lines) Patch
M content/browser/frame_host/frame_mojo_shell.h View 3 chunks +5 lines, -1 line 0 comments Download
M content/browser/frame_host/frame_mojo_shell.cc View 2 chunks +14 lines, -7 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 chunk +4 lines, -2 lines 0 comments Download
A content/browser/media/media_permission_impl.h View 1 chunk +64 lines, -0 lines 1 comment Download
A content/browser/media/media_permission_impl.cc View 1 chunk +97 lines, -0 lines 1 comment Download
A + content/common/media/media_permission_dispatcher.h View 2 chunks +3 lines, -3 lines 0 comments Download
A + content/common/media/media_permission_dispatcher.cc View 1 chunk +1 line, -1 line 0 comments Download
M content/content_browser.gypi View 1 chunk +2 lines, -0 lines 0 comments Download
M content/content_common.gypi View 1 chunk +2 lines, -0 lines 0 comments Download
M content/content_renderer.gypi View 1 chunk +0 lines, -2 lines 0 comments Download
D content/renderer/media/media_permission_dispatcher.h View 1 chunk +0 lines, -74 lines 0 comments Download
D content/renderer/media/media_permission_dispatcher.cc View 1 chunk +0 lines, -131 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 2 chunks +1 line, -1 line 0 comments Download
M media/mojo/interfaces/BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
A media/mojo/interfaces/media_permission.mojom View 1 chunk +24 lines, -0 lines 0 comments Download
M media/mojo/services/BUILD.gn View 1 chunk +2 lines, -0 lines 0 comments Download
A media/mojo/services/mojo_media_permission.h View 1 chunk +57 lines, -0 lines 0 comments Download
A media/mojo/services/mojo_media_permission.cc View 1 chunk +80 lines, -0 lines 0 comments Download

Messages

Total messages: 14 (4 generated)
xhwang
I hope the CL description is self-explanatory. It's mostly trying to convert one interface to ...
4 years, 8 months ago (2016-03-30 07:04:36 UTC) #3
xhwang
https://chromiumcodereview.appspot.com/1838903007/diff/1/content/browser/media/media_permission_impl.cc File content/browser/media/media_permission_impl.cc (right): https://chromiumcodereview.appspot.com/1838903007/diff/1/content/browser/media/media_permission_impl.cc#newcode19 content/browser/media/media_permission_impl.cc:19: NativeType ConvertMediaPermissionType(MojoType type) { rockot: What's the new recommendation ...
4 years, 8 months ago (2016-03-30 07:11:34 UTC) #4
mlamouri (slow - plz ping)
Why do you create a new MediaPermission instead of using the existing PermissionService?
4 years, 8 months ago (2016-03-31 21:54:40 UTC) #6
xhwang
On 2016/03/31 21:54:40, Mounir Lamouri wrote: > Why do you create a new MediaPermission instead ...
4 years, 8 months ago (2016-03-31 22:26:19 UTC) #7
xhwang
On 2016/03/31 22:26:19, xhwang wrote: > On 2016/03/31 21:54:40, Mounir Lamouri wrote: > > Why ...
4 years, 8 months ago (2016-04-01 03:32:25 UTC) #8
xhwang
+jam since rockot is OOO at the moment. This is not super high priority but ...
4 years, 8 months ago (2016-04-01 17:07:19 UTC) #10
jam
yes the goal is to have a low level permission service as part of moving ...
4 years, 8 months ago (2016-04-01 23:17:02 UTC) #11
xhwang
On 2016/04/01 23:17:02, jam wrote: > yes the goal is to have a low level ...
4 years, 8 months ago (2016-04-01 23:20:44 UTC) #12
jam
On 2016/04/01 23:20:44, xhwang (OOO Apr4 - Apr8) wrote: > On 2016/04/01 23:17:02, jam wrote: ...
4 years, 8 months ago (2016-04-04 15:37:51 UTC) #13
xhwang
4 years, 8 months ago (2016-04-04 16:16:58 UTC) #14
On 2016/04/04 15:37:51, jam wrote:
> On 2016/04/01 23:20:44, xhwang (OOO Apr4 - Apr8) wrote:
> > On 2016/04/01 23:17:02, jam wrote:
> > > yes the goal is to have a low level permission service as part of moving
> > towards
> > > components using mojo, see
> > >
> >
>
https://docs.google.com/document/d/1Q40FQiphP1SZNr010ZERqo9aKSY3ozcMjDl7NjeMf....
> > > it could live in components/permission
> > > 
> > > also see https://codereview.chromium.org/1771743002/#msg28 where we
discuss
> > this
> > > for the cl that moved this from content to blink yesterday. until then,
> since
> > > the only consumer of that interface was blink we wanted to wait to move it
> to
> > a
> > > low-level directory once we see what other callers need and we can move
the
> > > implementation there. Given that you need this now, perhaps we can reorder
> the
> > > tasks to unblock you. i.e. start by moving permission.mojom &
> > > permission_status.mojom to components/permission. that unblocks you.
> > > 
> > > then when we find someone to work on the permission component, they can
> change
> > > the enum to either a string or some token that is returned by a
registration
> > > mechanism so that the list of permissions isn't contained in one central
> > place.
> > > and then move the implementation from content&chrome to there, so that we
> can
> > > eventually run this permission service without chrome running.
> > 
> > Great! Thank you!!!!!
> > 
> > I assume moving the PermissionService mojom isn't too hard. Is it possible
to
> do
> > this fairly soon so I can land my work in M51? (Merge back to M51 shortly
> after
> > branch cut should be fine.)
> 
> yes we can do it anytime. can you do this?

I am OOO this week. I can do this next week if not done by anyone else. Thanks!

Powered by Google App Engine
This is Rietveld 408576698