Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2544523002:
Pass Connector* into MojoMediaClient::Initialize(). (Closed)
|
Created:
4 years ago by slan Modified:
4 years ago CC:
chromium-reviews, feature-media-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, lcwu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, halliwell+watch_chromium.org, alokp+watch_chromium.org, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPass Connector* into MojoMediaClient::Initialize().
Pass the Connector* provided by the service:media's ServiceContext into
MojoMediaClient::Initialize(), allowing MojoMediaClient implementations
to connect to interfaces exposed by other services. Also makes the
InterfaceProvider passed in CreateInterfaceFactory optional to eliminate
unecessary complexity, and corrects an error in media_manifest.json.
BUG=660736
Committed: https://crrev.com/e18fa6be27931989ea9197d629e467d2d6866fdd
Cr-Commit-Position: refs/heads/master@{#436123}
Patch Set 1 #Patch Set 2 : Rebase. #
Total comments: 14
Patch Set 3 : frame_interfaces renamed as host_interfaces, xhwang@ nits addressed #
Total comments: 4
Patch Set 4 : Add NOTREACHED() to AndroidMojoMediaClient::GetCdmFactory(). #Patch Set 5 : Rebase #Messages
Total messages: 56 (26 generated)
The CQ bit was checked by slan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by slan@chromium.org to run a CQ dry run
Description was changed from ========== Pass Connector* into MojoMediaClient::Initialize(). Pass the Connector* provided by the service:media's ServiceContext into MojoMediaClient::Initialize(), allowing MojoMediaClient implementations to connect to interfaces exposed by other services. Also makes the InterfaceProvider passed in CreateInterfaceFactory optional to eliminate unecessary complexity, and corrects an error in media_manifest.json. BUG=660736 Change-Id: I6b253c74604a6cc0611096703d2309361f16010a ========== to ========== Pass Connector* into MojoMediaClient::Initialize(). Pass the Connector* provided by the service:media's ServiceContext into MojoMediaClient::Initialize(), allowing MojoMediaClient implementations to connect to interfaces exposed by other services. Also makes the InterfaceProvider passed in CreateInterfaceFactory optional to eliminate unecessary complexity, and corrects an error in media_manifest.json. BUG=660736 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
slan@chromium.org changed reviewers: + alokp@chromium.org, xhwang@chromium.org
This will be used to connect to the ProvisionFetcher on Chromecast. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chromecast/ lgtm https://codereview.chromium.org/2544523002/diff/20001/media/mojo/interfaces/m... File media/mojo/interfaces/media_service.mojom (right): https://codereview.chromium.org/2544523002/diff/20001/media/mojo/interfaces/m... media/mojo/interfaces/media_service.mojom:16: service_manager.mojom.InterfaceProvider? frame_interfaces); nit: I do not quite like the name frame_interfaces, which kind of suggests that this interface is somehow tied to render frames. We should make it more generic, something like, remote_interfaces, maybe?
https://codereview.chromium.org/2544523002/diff/20001/media/mojo/interfaces/m... File media/mojo/interfaces/media_service.mojom (right): https://codereview.chromium.org/2544523002/diff/20001/media/mojo/interfaces/m... media/mojo/interfaces/media_service.mojom:16: service_manager.mojom.InterfaceProvider? frame_interfaces); On 2016/11/30 17:36:20, alokp wrote: > nit: I do not quite like the name frame_interfaces, which kind of suggests that > this interface is somehow tied to render frames. We should make it more generic, > something like, remote_interfaces, maybe? client_interfaces? (As Xiaohan mentioned somewhere else, this should be deprecated in favor of a delegate interface)
lgtm with nits https://codereview.chromium.org/2544523002/diff/20001/media/mojo/interfaces/m... File media/mojo/interfaces/media_service.mojom (right): https://codereview.chromium.org/2544523002/diff/20001/media/mojo/interfaces/m... media/mojo/interfaces/media_service.mojom:11: // Request an InterfaceFactory. |frame_interfaces| can optionally be used to nit: Request_s_ https://codereview.chromium.org/2544523002/diff/20001/media/mojo/interfaces/m... media/mojo/interfaces/media_service.mojom:16: service_manager.mojom.InterfaceProvider? frame_interfaces); On 2016/11/30 17:40:39, slan wrote: > On 2016/11/30 17:36:20, alokp wrote: > > nit: I do not quite like the name frame_interfaces, which kind of suggests > that > > this interface is somehow tied to render frames. We should make it more > generic, > > something like, remote_interfaces, maybe? > > client_interfaces? (As Xiaohan mentioned somewhere else, this should be > deprecated in favor of a delegate interface) Naming is hard :) The context is here: https://bugs.chromium.org/p/chromium/issues/detail?id=660573 This interface is provided by whoever calling CreateInterfaceFactory, which may or may not be the final client of the InterfaceFactory. So client_interfaces is a bit ambiguous. I think "remote_interfaces" would be better. Today the only place providing this is in a "frame", hence the current name. Maybe we should just introduce a new term, e.g. "host", which is the caller of CreateInterfaceFactory(), then this would be "host_interfaces". But given the fact that we want to replace this altogether, it's totally up to you whether you want to do the rename or not in this CL. https://codereview.chromium.org/2544523002/diff/20001/media/mojo/services/med... File media/mojo/services/media_manifest.json (right): https://codereview.chromium.org/2544523002/diff/20001/media/mojo/services/med... media/mojo/services/media_manifest.json:7: "media:media": [ "media::mojom::MediaService" ] FYI, we have some very broken test that actually uses media:interface_factory. I am fixing those tests so your change lg. https://codereview.chromium.org/2544523002/diff/20001/media/mojo/services/moj... File media/mojo/services/mojo_media_client.h (right): https://codereview.chromium.org/2544523002/diff/20001/media/mojo/services/moj... media/mojo/services/mojo_media_client.h:43: // this to connect to other services. It is guaranteed to outlive this. s/this/|this| https://codereview.chromium.org/2544523002/diff/20001/media/mojo/services/moj... media/mojo/services/mojo_media_client.h:67: // can be used to request interfaces provided by the remote client. It may be s/by the remote client/remotely? In chrome, the interfaces are provided by the browser process, not the CDM client in the render process. As mentioned above, maybe we should just introduce a new term, e.g. "host", to be whoever calls CreateInterfaceFactory(). Then this will be the host_interfaces.
https://codereview.chromium.org/2544523002/diff/20001/media/mojo/services/and... File media/mojo/services/android_mojo_media_client.cc (right): https://codereview.chromium.org/2544523002/diff/20001/media/mojo/services/and... media/mojo/services/android_mojo_media_client.cc:43: DCHECK(interface_provider) << "InterfaceProvider is not bound!"; Given how flexible people can use mojo media, I imagine it's possible that someone can use AndroidMojoMediaClient in a frame-less context, where this DCEHCK will fire. Can we actually do a if check here, and return nullptr if the interface_provider is null?
The CQ bit was checked by slan@chromium.org to run a CQ dry run
Comments addressed! PTAL https://codereview.chromium.org/2544523002/diff/20001/media/mojo/interfaces/m... File media/mojo/interfaces/media_service.mojom (right): https://codereview.chromium.org/2544523002/diff/20001/media/mojo/interfaces/m... media/mojo/interfaces/media_service.mojom:11: // Request an InterfaceFactory. |frame_interfaces| can optionally be used to On 2016/11/30 18:49:31, xhwang wrote: > nit: Request_s_ Done. https://codereview.chromium.org/2544523002/diff/20001/media/mojo/interfaces/m... media/mojo/interfaces/media_service.mojom:16: service_manager.mojom.InterfaceProvider? frame_interfaces); On 2016/11/30 18:49:31, xhwang wrote: > On 2016/11/30 17:40:39, slan wrote: > > On 2016/11/30 17:36:20, alokp wrote: > > > nit: I do not quite like the name frame_interfaces, which kind of suggests > > that > > > this interface is somehow tied to render frames. We should make it more > > generic, > > > something like, remote_interfaces, maybe? > > > > client_interfaces? (As Xiaohan mentioned somewhere else, this should be > > deprecated in favor of a delegate interface) > > Naming is hard :) > > The context is here: > https://bugs.chromium.org/p/chromium/issues/detail?id=660573 > > This interface is provided by whoever calling CreateInterfaceFactory, which may > or may not be the final client of the InterfaceFactory. So client_interfaces is > a bit ambiguous. I think "remote_interfaces" would be better. Today the only > place providing this is in a "frame", hence the current name. > > Maybe we should just introduce a new term, e.g. "host", which is the caller of > CreateInterfaceFactory(), then this would be "host_interfaces". > > But given the fact that we want to replace this altogether, it's totally up to > you whether you want to do the rename or not in this CL. I like host_interfaces. Done. https://codereview.chromium.org/2544523002/diff/20001/media/mojo/services/and... File media/mojo/services/android_mojo_media_client.cc (right): https://codereview.chromium.org/2544523002/diff/20001/media/mojo/services/and... media/mojo/services/android_mojo_media_client.cc:43: DCHECK(interface_provider) << "InterfaceProvider is not bound!"; On 2016/11/30 18:52:50, xhwang wrote: > Given how flexible people can use mojo media, I imagine it's possible that > someone can use AndroidMojoMediaClient in a frame-less context, where this > DCEHCK will fire. > > Can we actually do a if check here, and return nullptr if the interface_provider > is null? To be honest, if I were a user of this interface, I would not expect that behavior. But I will defer to your judgment. Done. https://codereview.chromium.org/2544523002/diff/20001/media/mojo/services/med... File media/mojo/services/media_manifest.json (right): https://codereview.chromium.org/2544523002/diff/20001/media/mojo/services/med... media/mojo/services/media_manifest.json:7: "media:media": [ "media::mojom::MediaService" ] On 2016/11/30 18:49:31, xhwang wrote: > FYI, we have some very broken test that actually uses media:interface_factory. I > am fixing those tests so your change lg. Acknowledged. https://codereview.chromium.org/2544523002/diff/20001/media/mojo/services/moj... File media/mojo/services/mojo_media_client.h (right): https://codereview.chromium.org/2544523002/diff/20001/media/mojo/services/moj... media/mojo/services/mojo_media_client.h:43: // this to connect to other services. It is guaranteed to outlive this. On 2016/11/30 18:49:31, xhwang wrote: > s/this/|this| Done. https://codereview.chromium.org/2544523002/diff/20001/media/mojo/services/moj... media/mojo/services/mojo_media_client.h:67: // can be used to request interfaces provided by the remote client. It may be On 2016/11/30 18:49:31, xhwang wrote: > s/by the remote client/remotely? > > In chrome, the interfaces are provided by the browser process, not the CDM > client in the render process. > > As mentioned above, maybe we should just introduce a new term, e.g. "host", to > be whoever calls CreateInterfaceFactory(). Then this will be the > host_interfaces. Yes, I like this phrasing. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by slan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, alokp@chromium.org Link to the patchset: https://codereview.chromium.org/2544523002/#ps40001 (title: "frame_interfaces renamed as host_interfaces, xhwang@ nits addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
slan@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng@ for media/mojo/interfaces/media_service.mojom
https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... File media/mojo/services/android_mojo_media_client.cc (right): https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... media/mojo/services/android_mojo_media_client.cc:46: return nullptr; Just trying to understand the change here: does hitting this mean that there's a programming error? Is there any reason why code would be calling this with no host interfaces? Or does this client simply not know?
https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... File media/mojo/services/android_mojo_media_client.cc (right): https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... media/mojo/services/android_mojo_media_client.cc:46: return nullptr; On 2016/12/02 08:52:51, dcheng wrote: > Just trying to understand the change here: does hitting this mean that there's a > programming error? Is there any reason why code would be calling this with no > host interfaces? Or does this client simply not know? |host_interfaces| is, at present, used to provide interfaces specific to the render frame. This change provides flexibility to clients who do not need to provide additional interfaces, or who know nothing about render frames, but still want to consume the "media" service. Xiaohan may be able to provide specifics on what the Android use cases for this are (perhaps some media playback of non-protected content from another context). On Chromecast, we have several applications that will benefit from this flexibility. This pattern deviates from how interfaces requests should be brokered between services (crbug.com/660573). Making this parameter optional takes a step in the right direction for correcting this behavior.
https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... File media/mojo/services/android_mojo_media_client.cc (right): https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... media/mojo/services/android_mojo_media_client.cc:46: return nullptr; On 2016/12/02 16:45:53, slan wrote: > On 2016/12/02 08:52:51, dcheng wrote: > > Just trying to understand the change here: does hitting this mean that there's > a > > programming error? Is there any reason why code would be calling this with no > > host interfaces? Or does this client simply not know? > > |host_interfaces| is, at present, used to provide interfaces specific to the > render frame. This change provides flexibility to clients who do not need to > provide additional interfaces, or who know nothing about render frames, but > still want to consume the "media" service. > > Xiaohan may be able to provide specifics on what the Android use cases for this > are (perhaps some media playback of non-protected content from another context). > On Chromecast, we have several applications that will benefit from this > flexibility. > > This pattern deviates from how interfaces requests should be brokered between > services (crbug.com/660573). Making this parameter optional takes a step in the > right direction for correcting this behavior. dcheng: good point. I think when this happens, it's generally a programming error, thus a DCHECK is typically the recommendation. However, as slan@ mentioned, with the flexibility of mojo, there are many different ways to config the system to use the media service with this client, and I was afraid when the system was misconfiged we could end up here. This is similar to the current warning/failure in mojo connector where a client tries to connect to an interface, but no interface implementation is registered. With that, maybe this will make more sense? if (!host_interfaces) { NOTREACHED() << "Host interface should be provided when using CDM with AndroidMojoMediaClient"; return nullptr; }
https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... File media/mojo/services/android_mojo_media_client.cc (right): https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... media/mojo/services/android_mojo_media_client.cc:46: return nullptr; On 2016/12/02 17:47:48, xhwang wrote: > On 2016/12/02 16:45:53, slan wrote: > > On 2016/12/02 08:52:51, dcheng wrote: > > > Just trying to understand the change here: does hitting this mean that > there's > > a > > > programming error? Is there any reason why code would be calling this with > no > > > host interfaces? Or does this client simply not know? > > > > |host_interfaces| is, at present, used to provide interfaces specific to the > > render frame. This change provides flexibility to clients who do not need to > > provide additional interfaces, or who know nothing about render frames, but > > still want to consume the "media" service. > > > > Xiaohan may be able to provide specifics on what the Android use cases for > this > > are (perhaps some media playback of non-protected content from another > context). > > On Chromecast, we have several applications that will benefit from this > > flexibility. > > > > This pattern deviates from how interfaces requests should be brokered between > > services (crbug.com/660573). Making this parameter optional takes a step in > the > > right direction for correcting this behavior. > > dcheng: good point. I think when this happens, it's generally a programming > error, thus a DCHECK is typically the recommendation. However, as slan@ > mentioned, with the flexibility of mojo, there are many different ways to config > the system to use the media service with this client, and I was afraid when the > system was misconfiged we could end up here. This is similar to the current > warning/failure in mojo connector where a client tries to connect to an > interface, but no interface implementation is registered. > > With that, maybe this will make more sense? > > if (!host_interfaces) { > NOTREACHED() << "Host interface should be provided when using CDM with > AndroidMojoMediaClient"; > return nullptr; > } I feel like a NOTREACHED() will just end up mostly ignored? Also, my uncertainty about what to do here comes from my confusion about the process model in media: I poked around a bit, and I /think/ this is running in the unprivileged process... but I'm not 100% certain either. But in that case, it's the unprivileged process making a programming error, so it would be useful to fail early instead?
On 2016/12/02 18:00:30, dcheng wrote: > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > File media/mojo/services/android_mojo_media_client.cc (right): > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > media/mojo/services/android_mojo_media_client.cc:46: return nullptr; > On 2016/12/02 17:47:48, xhwang wrote: > > On 2016/12/02 16:45:53, slan wrote: > > > On 2016/12/02 08:52:51, dcheng wrote: > > > > Just trying to understand the change here: does hitting this mean that > > there's > > > a > > > > programming error? Is there any reason why code would be calling this with > > no > > > > host interfaces? Or does this client simply not know? > > > > > > |host_interfaces| is, at present, used to provide interfaces specific to the > > > render frame. This change provides flexibility to clients who do not need to > > > provide additional interfaces, or who know nothing about render frames, but > > > still want to consume the "media" service. > > > > > > Xiaohan may be able to provide specifics on what the Android use cases for > > this > > > are (perhaps some media playback of non-protected content from another > > context). > > > On Chromecast, we have several applications that will benefit from this > > > flexibility. > > > > > > This pattern deviates from how interfaces requests should be brokered > between > > > services (crbug.com/660573). Making this parameter optional takes a step in > > the > > > right direction for correcting this behavior. > > > > dcheng: good point. I think when this happens, it's generally a programming > > error, thus a DCHECK is typically the recommendation. However, as slan@ > > mentioned, with the flexibility of mojo, there are many different ways to > config > > the system to use the media service with this client, and I was afraid when > the > > system was misconfiged we could end up here. This is similar to the current > > warning/failure in mojo connector where a client tries to connect to an > > interface, but no interface implementation is registered. > > > > With that, maybe this will make more sense? > > > > if (!host_interfaces) { > > NOTREACHED() << "Host interface should be provided when using CDM with > > AndroidMojoMediaClient"; > > return nullptr; > > } > > I feel like a NOTREACHED() will just end up mostly ignored? In debug build NOTREACHED() is the same as DCHECK(false) so it can't be ignored. If you are talking about release mode, we can add a LOG(ERROR). > Also, my uncertainty about what to do here comes from my confusion about the > process model in media: I poked around a bit, and I /think/ this is running in > the unprivileged process... but I'm not 100% certain either. But in that case, > it's the unprivileged process making a programming error, so it would be useful > to fail early instead? MediaService (including *MojoMediaClient) itself is process-aganostic. This is intentional so that it can be reused in many different ways on different platforms. In Chrome for Android, this is currently running in the GPU process. I am not super clear what you mean by "fail early" here...
On 2016/12/02 18:29:09, xhwang wrote: > On 2016/12/02 18:00:30, dcheng wrote: > > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > > File media/mojo/services/android_mojo_media_client.cc (right): > > > > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > > media/mojo/services/android_mojo_media_client.cc:46: return nullptr; > > On 2016/12/02 17:47:48, xhwang wrote: > > > On 2016/12/02 16:45:53, slan wrote: > > > > On 2016/12/02 08:52:51, dcheng wrote: > > > > > Just trying to understand the change here: does hitting this mean that > > > there's > > > > a > > > > > programming error? Is there any reason why code would be calling this > with > > > no > > > > > host interfaces? Or does this client simply not know? > > > > > > > > |host_interfaces| is, at present, used to provide interfaces specific to > the > > > > render frame. This change provides flexibility to clients who do not need > to > > > > provide additional interfaces, or who know nothing about render frames, > but > > > > still want to consume the "media" service. > > > > > > > > Xiaohan may be able to provide specifics on what the Android use cases for > > > this > > > > are (perhaps some media playback of non-protected content from another > > > context). > > > > On Chromecast, we have several applications that will benefit from this > > > > flexibility. > > > > > > > > This pattern deviates from how interfaces requests should be brokered > > between > > > > services (crbug.com/660573). Making this parameter optional takes a step > in > > > the > > > > right direction for correcting this behavior. > > > > > > dcheng: good point. I think when this happens, it's generally a programming > > > error, thus a DCHECK is typically the recommendation. However, as slan@ > > > mentioned, with the flexibility of mojo, there are many different ways to > > config > > > the system to use the media service with this client, and I was afraid when > > the > > > system was misconfiged we could end up here. This is similar to the current > > > warning/failure in mojo connector where a client tries to connect to an > > > interface, but no interface implementation is registered. > > > > > > With that, maybe this will make more sense? > > > > > > if (!host_interfaces) { > > > NOTREACHED() << "Host interface should be provided when using CDM with > > > AndroidMojoMediaClient"; > > > return nullptr; > > > } > > > > I feel like a NOTREACHED() will just end up mostly ignored? > > In debug build NOTREACHED() is the same as DCHECK(false) so it can't be ignored. > If you are talking about release mode, we can add a LOG(ERROR). > > > Also, my uncertainty about what to do here comes from my confusion about the > > process model in media: I poked around a bit, and I /think/ this is running in > > the unprivileged process... but I'm not 100% certain either. But in that case, > > it's the unprivileged process making a programming error, so it would be > useful > > to fail early instead? > > MediaService (including *MojoMediaClient) itself is process-aganostic. This is > intentional so that it can be reused in many different ways on different > platforms. In Chrome for Android, this is currently running in the GPU process. > > I am not super clear what you mean by "fail early" here... As in "just explode" =) But again, process model / boundaries aren't clear to me here. On Clank, this is in GPU, but if renderer can trigger this code path, then you're right, we have to gracefully fail. If no other process can reach it (and it's internal bookkeeping that the GPU process itself should have been keeping better track of), then that's more reason to only DCHECK.
On 2016/12/02 18:30:52, dcheng wrote: > On 2016/12/02 18:29:09, xhwang wrote: > > On 2016/12/02 18:00:30, dcheng wrote: > > > > > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > > > File media/mojo/services/android_mojo_media_client.cc (right): > > > > > > > > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > > > media/mojo/services/android_mojo_media_client.cc:46: return nullptr; > > > On 2016/12/02 17:47:48, xhwang wrote: > > > > On 2016/12/02 16:45:53, slan wrote: > > > > > On 2016/12/02 08:52:51, dcheng wrote: > > > > > > Just trying to understand the change here: does hitting this mean that > > > > there's > > > > > a > > > > > > programming error? Is there any reason why code would be calling this > > with > > > > no > > > > > > host interfaces? Or does this client simply not know? > > > > > > > > > > |host_interfaces| is, at present, used to provide interfaces specific to > > the > > > > > render frame. This change provides flexibility to clients who do not > need > > to > > > > > provide additional interfaces, or who know nothing about render frames, > > but > > > > > still want to consume the "media" service. > > > > > > > > > > Xiaohan may be able to provide specifics on what the Android use cases > for > > > > this > > > > > are (perhaps some media playback of non-protected content from another > > > > context). > > > > > On Chromecast, we have several applications that will benefit from this > > > > > flexibility. > > > > > > > > > > This pattern deviates from how interfaces requests should be brokered > > > between > > > > > services (crbug.com/660573). Making this parameter optional takes a step > > in > > > > the > > > > > right direction for correcting this behavior. > > > > > > > > dcheng: good point. I think when this happens, it's generally a > programming > > > > error, thus a DCHECK is typically the recommendation. However, as slan@ > > > > mentioned, with the flexibility of mojo, there are many different ways to > > > config > > > > the system to use the media service with this client, and I was afraid > when > > > the > > > > system was misconfiged we could end up here. This is similar to the > current > > > > warning/failure in mojo connector where a client tries to connect to an > > > > interface, but no interface implementation is registered. > > > > > > > > With that, maybe this will make more sense? > > > > > > > > if (!host_interfaces) { > > > > NOTREACHED() << "Host interface should be provided when using CDM with > > > > AndroidMojoMediaClient"; > > > > return nullptr; > > > > } > > > > > > I feel like a NOTREACHED() will just end up mostly ignored? > > > > In debug build NOTREACHED() is the same as DCHECK(false) so it can't be > ignored. > > If you are talking about release mode, we can add a LOG(ERROR). > > > > > Also, my uncertainty about what to do here comes from my confusion about the > > > process model in media: I poked around a bit, and I /think/ this is running > in > > > the unprivileged process... but I'm not 100% certain either. But in that > case, > > > it's the unprivileged process making a programming error, so it would be > > useful > > > to fail early instead? > > > > MediaService (including *MojoMediaClient) itself is process-aganostic. This is > > intentional so that it can be reused in many different ways on different > > platforms. In Chrome for Android, this is currently running in the GPU > process. > > > > I am not super clear what you mean by "fail early" here... > > As in "just explode" =) Got it :) > But again, process model / boundaries aren't clear to me here. On Clank, this is > in GPU, but if renderer can trigger this code path, then you're right, we have > to gracefully fail. If no other process can reach it (and it's internal > bookkeeping that the GPU process itself should have been keeping better track > of), then that's more reason to only DCHECK. For Clank I agree. But as said earlier, you can config the system in many different ways. For example, in theory, you can run this in the browser process (see ENABLE_MOJO_MEDIA_IN_BROWSER_PROCESS), or in some unsandboxed sub-process. In those cases, I am not super confident with a DCHECK...
On 2016/12/02 18:36:17, xhwang wrote: > On 2016/12/02 18:30:52, dcheng wrote: > > On 2016/12/02 18:29:09, xhwang wrote: > > > On 2016/12/02 18:00:30, dcheng wrote: > > > > > > > > > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > > > > File media/mojo/services/android_mojo_media_client.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > > > > media/mojo/services/android_mojo_media_client.cc:46: return nullptr; > > > > On 2016/12/02 17:47:48, xhwang wrote: > > > > > On 2016/12/02 16:45:53, slan wrote: > > > > > > On 2016/12/02 08:52:51, dcheng wrote: > > > > > > > Just trying to understand the change here: does hitting this mean > that > > > > > there's > > > > > > a > > > > > > > programming error? Is there any reason why code would be calling > this > > > with > > > > > no > > > > > > > host interfaces? Or does this client simply not know? > > > > > > > > > > > > |host_interfaces| is, at present, used to provide interfaces specific > to > > > the > > > > > > render frame. This change provides flexibility to clients who do not > > need > > > to > > > > > > provide additional interfaces, or who know nothing about render > frames, > > > but > > > > > > still want to consume the "media" service. > > > > > > > > > > > > Xiaohan may be able to provide specifics on what the Android use cases > > for > > > > > this > > > > > > are (perhaps some media playback of non-protected content from another > > > > > context). > > > > > > On Chromecast, we have several applications that will benefit from > this > > > > > > flexibility. > > > > > > > > > > > > This pattern deviates from how interfaces requests should be brokered > > > > between > > > > > > services (crbug.com/660573). Making this parameter optional takes a > step > > > in > > > > > the > > > > > > right direction for correcting this behavior. > > > > > > > > > > dcheng: good point. I think when this happens, it's generally a > > programming > > > > > error, thus a DCHECK is typically the recommendation. However, as slan@ > > > > > mentioned, with the flexibility of mojo, there are many different ways > to > > > > config > > > > > the system to use the media service with this client, and I was afraid > > when > > > > the > > > > > system was misconfiged we could end up here. This is similar to the > > current > > > > > warning/failure in mojo connector where a client tries to connect to an > > > > > interface, but no interface implementation is registered. > > > > > > > > > > With that, maybe this will make more sense? > > > > > > > > > > if (!host_interfaces) { > > > > > NOTREACHED() << "Host interface should be provided when using CDM with > > > > > AndroidMojoMediaClient"; > > > > > return nullptr; > > > > > } > > > > > > > > I feel like a NOTREACHED() will just end up mostly ignored? > > > > > > In debug build NOTREACHED() is the same as DCHECK(false) so it can't be > > ignored. > > > If you are talking about release mode, we can add a LOG(ERROR). > > > > > > > Also, my uncertainty about what to do here comes from my confusion about > the > > > > process model in media: I poked around a bit, and I /think/ this is > running > > in > > > > the unprivileged process... but I'm not 100% certain either. But in that > > case, > > > > it's the unprivileged process making a programming error, so it would be > > > useful > > > > to fail early instead? > > > > > > MediaService (including *MojoMediaClient) itself is process-aganostic. This > is > > > intentional so that it can be reused in many different ways on different > > > platforms. In Chrome for Android, this is currently running in the GPU > > process. > > > > > > I am not super clear what you mean by "fail early" here... > > > > As in "just explode" =) > > Got it :) > > > But again, process model / boundaries aren't clear to me here. On Clank, this > is > > in GPU, but if renderer can trigger this code path, then you're right, we have > > to gracefully fail. If no other process can reach it (and it's internal > > bookkeeping that the GPU process itself should have been keeping better track > > of), then that's more reason to only DCHECK. > > For Clank I agree. But as said earlier, you can config the system in many > different ways. For example, in theory, you can run this in the browser process > (see ENABLE_MOJO_MEDIA_IN_BROWSER_PROCESS), or in some unsandboxed sub-process. > In those cases, I am not super confident with a DCHECK... Honestly, it seems to me our own code should know what's configured where, and if that's not true, that's kind of ... weird.
On 2016/12/02 18:40:27, dcheng wrote: > On 2016/12/02 18:36:17, xhwang wrote: > > On 2016/12/02 18:30:52, dcheng wrote: > > > On 2016/12/02 18:29:09, xhwang wrote: > > > > On 2016/12/02 18:00:30, dcheng wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > > > > > File media/mojo/services/android_mojo_media_client.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > > > > > media/mojo/services/android_mojo_media_client.cc:46: return nullptr; > > > > > On 2016/12/02 17:47:48, xhwang wrote: > > > > > > On 2016/12/02 16:45:53, slan wrote: > > > > > > > On 2016/12/02 08:52:51, dcheng wrote: > > > > > > > > Just trying to understand the change here: does hitting this mean > > that > > > > > > there's > > > > > > > a > > > > > > > > programming error? Is there any reason why code would be calling > > this > > > > with > > > > > > no > > > > > > > > host interfaces? Or does this client simply not know? > > > > > > > > > > > > > > |host_interfaces| is, at present, used to provide interfaces > specific > > to > > > > the > > > > > > > render frame. This change provides flexibility to clients who do not > > > need > > > > to > > > > > > > provide additional interfaces, or who know nothing about render > > frames, > > > > but > > > > > > > still want to consume the "media" service. > > > > > > > > > > > > > > Xiaohan may be able to provide specifics on what the Android use > cases > > > for > > > > > > this > > > > > > > are (perhaps some media playback of non-protected content from > another > > > > > > context). > > > > > > > On Chromecast, we have several applications that will benefit from > > this > > > > > > > flexibility. > > > > > > > > > > > > > > This pattern deviates from how interfaces requests should be > brokered > > > > > between > > > > > > > services (crbug.com/660573). Making this parameter optional takes a > > step > > > > in > > > > > > the > > > > > > > right direction for correcting this behavior. > > > > > > > > > > > > dcheng: good point. I think when this happens, it's generally a > > > programming > > > > > > error, thus a DCHECK is typically the recommendation. However, as > slan@ > > > > > > mentioned, with the flexibility of mojo, there are many different ways > > to > > > > > config > > > > > > the system to use the media service with this client, and I was afraid > > > when > > > > > the > > > > > > system was misconfiged we could end up here. This is similar to the > > > current > > > > > > warning/failure in mojo connector where a client tries to connect to > an > > > > > > interface, but no interface implementation is registered. > > > > > > > > > > > > With that, maybe this will make more sense? > > > > > > > > > > > > if (!host_interfaces) { > > > > > > NOTREACHED() << "Host interface should be provided when using CDM > with > > > > > > AndroidMojoMediaClient"; > > > > > > return nullptr; > > > > > > } > > > > > > > > > > I feel like a NOTREACHED() will just end up mostly ignored? > > > > > > > > In debug build NOTREACHED() is the same as DCHECK(false) so it can't be > > > ignored. > > > > If you are talking about release mode, we can add a LOG(ERROR). > > > > > > > > > Also, my uncertainty about what to do here comes from my confusion about > > the > > > > > process model in media: I poked around a bit, and I /think/ this is > > running > > > in > > > > > the unprivileged process... but I'm not 100% certain either. But in that > > > case, > > > > > it's the unprivileged process making a programming error, so it would be > > > > useful > > > > > to fail early instead? > > > > > > > > MediaService (including *MojoMediaClient) itself is process-aganostic. > This > > is > > > > intentional so that it can be reused in many different ways on different > > > > platforms. In Chrome for Android, this is currently running in the GPU > > > process. > > > > > > > > I am not super clear what you mean by "fail early" here... > > > > > > As in "just explode" =) > > > > Got it :) > > > > > But again, process model / boundaries aren't clear to me here. On Clank, > this > > is > > > in GPU, but if renderer can trigger this code path, then you're right, we > have > > > to gracefully fail. If no other process can reach it (and it's internal > > > bookkeeping that the GPU process itself should have been keeping better > track > > > of), then that's more reason to only DCHECK. > > > > For Clank I agree. But as said earlier, you can config the system in many > > different ways. For example, in theory, you can run this in the browser > process > > (see ENABLE_MOJO_MEDIA_IN_BROWSER_PROCESS), or in some unsandboxed > sub-process. > > In those cases, I am not super confident with a DCHECK... > > Honestly, it seems to me our own code should know what's configured where, and > if that's not true, that's kind of ... weird. Chromium, including "media/" is used in a lot of products, not limited to the Chrome browser. Now with mojo, it's super easy to move some component that used to run in one process to another process. Here's an example: https://chromiumcodereview.appspot.com/2115603002/. I don't know how security people would think for these cases. But from "media"'s perspective, we probably should never make any process-architecture assumption and always err on the safe side.
On 2016/12/02 18:47:21, xhwang wrote: > On 2016/12/02 18:40:27, dcheng wrote: > > On 2016/12/02 18:36:17, xhwang wrote: > > > On 2016/12/02 18:30:52, dcheng wrote: > > > > On 2016/12/02 18:29:09, xhwang wrote: > > > > > On 2016/12/02 18:00:30, dcheng wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > > > > > > File media/mojo/services/android_mojo_media_client.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > > > > > > media/mojo/services/android_mojo_media_client.cc:46: return nullptr; > > > > > > On 2016/12/02 17:47:48, xhwang wrote: > > > > > > > On 2016/12/02 16:45:53, slan wrote: > > > > > > > > On 2016/12/02 08:52:51, dcheng wrote: > > > > > > > > > Just trying to understand the change here: does hitting this > mean > > > that > > > > > > > there's > > > > > > > > a > > > > > > > > > programming error? Is there any reason why code would be calling > > > this > > > > > with > > > > > > > no > > > > > > > > > host interfaces? Or does this client simply not know? > > > > > > > > > > > > > > > > |host_interfaces| is, at present, used to provide interfaces > > specific > > > to > > > > > the > > > > > > > > render frame. This change provides flexibility to clients who do > not > > > > need > > > > > to > > > > > > > > provide additional interfaces, or who know nothing about render > > > frames, > > > > > but > > > > > > > > still want to consume the "media" service. > > > > > > > > > > > > > > > > Xiaohan may be able to provide specifics on what the Android use > > cases > > > > for > > > > > > > this > > > > > > > > are (perhaps some media playback of non-protected content from > > another > > > > > > > context). > > > > > > > > On Chromecast, we have several applications that will benefit from > > > this > > > > > > > > flexibility. > > > > > > > > > > > > > > > > This pattern deviates from how interfaces requests should be > > brokered > > > > > > between > > > > > > > > services (crbug.com/660573). Making this parameter optional takes > a > > > step > > > > > in > > > > > > > the > > > > > > > > right direction for correcting this behavior. > > > > > > > > > > > > > > dcheng: good point. I think when this happens, it's generally a > > > > programming > > > > > > > error, thus a DCHECK is typically the recommendation. However, as > > slan@ > > > > > > > mentioned, with the flexibility of mojo, there are many different > ways > > > to > > > > > > config > > > > > > > the system to use the media service with this client, and I was > afraid > > > > when > > > > > > the > > > > > > > system was misconfiged we could end up here. This is similar to the > > > > current > > > > > > > warning/failure in mojo connector where a client tries to connect to > > an > > > > > > > interface, but no interface implementation is registered. > > > > > > > > > > > > > > With that, maybe this will make more sense? > > > > > > > > > > > > > > if (!host_interfaces) { > > > > > > > NOTREACHED() << "Host interface should be provided when using CDM > > with > > > > > > > AndroidMojoMediaClient"; > > > > > > > return nullptr; > > > > > > > } > > > > > > > > > > > > I feel like a NOTREACHED() will just end up mostly ignored? > > > > > > > > > > In debug build NOTREACHED() is the same as DCHECK(false) so it can't be > > > > ignored. > > > > > If you are talking about release mode, we can add a LOG(ERROR). > > > > > > > > > > > Also, my uncertainty about what to do here comes from my confusion > about > > > the > > > > > > process model in media: I poked around a bit, and I /think/ this is > > > running > > > > in > > > > > > the unprivileged process... but I'm not 100% certain either. But in > that > > > > case, > > > > > > it's the unprivileged process making a programming error, so it would > be > > > > > useful > > > > > > to fail early instead? > > > > > > > > > > MediaService (including *MojoMediaClient) itself is process-aganostic. > > This > > > is > > > > > intentional so that it can be reused in many different ways on different > > > > > platforms. In Chrome for Android, this is currently running in the GPU > > > > process. > > > > > > > > > > I am not super clear what you mean by "fail early" here... > > > > > > > > As in "just explode" =) > > > > > > Got it :) > > > > > > > But again, process model / boundaries aren't clear to me here. On Clank, > > this > > > is > > > > in GPU, but if renderer can trigger this code path, then you're right, we > > have > > > > to gracefully fail. If no other process can reach it (and it's internal > > > > bookkeeping that the GPU process itself should have been keeping better > > track > > > > of), then that's more reason to only DCHECK. > > > > > > For Clank I agree. But as said earlier, you can config the system in many > > > different ways. For example, in theory, you can run this in the browser > > process > > > (see ENABLE_MOJO_MEDIA_IN_BROWSER_PROCESS), or in some unsandboxed > > sub-process. > > > In those cases, I am not super confident with a DCHECK... > > > > Honestly, it seems to me our own code should know what's configured where, and > > if that's not true, that's kind of ... weird. > > Chromium, including "media/" is used in a lot of products, not limited to the > Chrome browser. Now with mojo, it's super easy to move some component that used > to run in one process to another process. Here's an example: > https://chromiumcodereview.appspot.com/2115603002/. I don't know how security > people would think for these cases. But from "media"'s perspective, we probably > should never make any process-architecture assumption and always err on the safe > side. Right, that was the second part of my question: is this something that we expect to be exposed to lower-privileged processes? Of course, with mojo "anything is possible". But that doesn't necessarily mean we should support it.
On 2016/12/02 19:12:13, dcheng wrote: > On 2016/12/02 18:47:21, xhwang wrote: > > On 2016/12/02 18:40:27, dcheng wrote: > > > On 2016/12/02 18:36:17, xhwang wrote: > > > > On 2016/12/02 18:30:52, dcheng wrote: > > > > > On 2016/12/02 18:29:09, xhwang wrote: > > > > > > On 2016/12/02 18:00:30, dcheng wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > > > > > > > File media/mojo/services/android_mojo_media_client.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > > > > > > > media/mojo/services/android_mojo_media_client.cc:46: return nullptr; > > > > > > > On 2016/12/02 17:47:48, xhwang wrote: > > > > > > > > On 2016/12/02 16:45:53, slan wrote: > > > > > > > > > On 2016/12/02 08:52:51, dcheng wrote: > > > > > > > > > > Just trying to understand the change here: does hitting this > > mean > > > > that > > > > > > > > there's > > > > > > > > > a > > > > > > > > > > programming error? Is there any reason why code would be > calling > > > > this > > > > > > with > > > > > > > > no > > > > > > > > > > host interfaces? Or does this client simply not know? > > > > > > > > > > > > > > > > > > |host_interfaces| is, at present, used to provide interfaces > > > specific > > > > to > > > > > > the > > > > > > > > > render frame. This change provides flexibility to clients who do > > not > > > > > need > > > > > > to > > > > > > > > > provide additional interfaces, or who know nothing about render > > > > frames, > > > > > > but > > > > > > > > > still want to consume the "media" service. > > > > > > > > > > > > > > > > > > Xiaohan may be able to provide specifics on what the Android use > > > cases > > > > > for > > > > > > > > this > > > > > > > > > are (perhaps some media playback of non-protected content from > > > another > > > > > > > > context). > > > > > > > > > On Chromecast, we have several applications that will benefit > from > > > > this > > > > > > > > > flexibility. > > > > > > > > > > > > > > > > > > This pattern deviates from how interfaces requests should be > > > brokered > > > > > > > between > > > > > > > > > services (crbug.com/660573). Making this parameter optional > takes > > a > > > > step > > > > > > in > > > > > > > > the > > > > > > > > > right direction for correcting this behavior. > > > > > > > > > > > > > > > > dcheng: good point. I think when this happens, it's generally a > > > > > programming > > > > > > > > error, thus a DCHECK is typically the recommendation. However, as > > > slan@ > > > > > > > > mentioned, with the flexibility of mojo, there are many different > > ways > > > > to > > > > > > > config > > > > > > > > the system to use the media service with this client, and I was > > afraid > > > > > when > > > > > > > the > > > > > > > > system was misconfiged we could end up here. This is similar to > the > > > > > current > > > > > > > > warning/failure in mojo connector where a client tries to connect > to > > > an > > > > > > > > interface, but no interface implementation is registered. > > > > > > > > > > > > > > > > With that, maybe this will make more sense? > > > > > > > > > > > > > > > > if (!host_interfaces) { > > > > > > > > NOTREACHED() << "Host interface should be provided when using > CDM > > > with > > > > > > > > AndroidMojoMediaClient"; > > > > > > > > return nullptr; > > > > > > > > } > > > > > > > > > > > > > > I feel like a NOTREACHED() will just end up mostly ignored? > > > > > > > > > > > > In debug build NOTREACHED() is the same as DCHECK(false) so it can't > be > > > > > ignored. > > > > > > If you are talking about release mode, we can add a LOG(ERROR). > > > > > > > > > > > > > Also, my uncertainty about what to do here comes from my confusion > > about > > > > the > > > > > > > process model in media: I poked around a bit, and I /think/ this is > > > > running > > > > > in > > > > > > > the unprivileged process... but I'm not 100% certain either. But in > > that > > > > > case, > > > > > > > it's the unprivileged process making a programming error, so it > would > > be > > > > > > useful > > > > > > > to fail early instead? > > > > > > > > > > > > MediaService (including *MojoMediaClient) itself is process-aganostic. > > > This > > > > is > > > > > > intentional so that it can be reused in many different ways on > different > > > > > > platforms. In Chrome for Android, this is currently running in the GPU > > > > > process. > > > > > > > > > > > > I am not super clear what you mean by "fail early" here... > > > > > > > > > > As in "just explode" =) > > > > > > > > Got it :) > > > > > > > > > But again, process model / boundaries aren't clear to me here. On Clank, > > > this > > > > is > > > > > in GPU, but if renderer can trigger this code path, then you're right, > we > > > have > > > > > to gracefully fail. If no other process can reach it (and it's internal > > > > > bookkeeping that the GPU process itself should have been keeping better > > > track > > > > > of), then that's more reason to only DCHECK. > > > > > > > > For Clank I agree. But as said earlier, you can config the system in many > > > > different ways. For example, in theory, you can run this in the browser > > > process > > > > (see ENABLE_MOJO_MEDIA_IN_BROWSER_PROCESS), or in some unsandboxed > > > sub-process. > > > > In those cases, I am not super confident with a DCHECK... > > > > > > Honestly, it seems to me our own code should know what's configured where, > and > > > if that's not true, that's kind of ... weird. > > > > Chromium, including "media/" is used in a lot of products, not limited to the > > Chrome browser. Now with mojo, it's super easy to move some component that > used > > to run in one process to another process. Here's an example: > > https://chromiumcodereview.appspot.com/2115603002/. I don't know how security > > people would think for these cases. But from "media"'s perspective, we > probably > > should never make any process-architecture assumption and always err on the > safe > > side. > > Right, that was the second part of my question: is this something that we expect > to be exposed to lower-privileged processes? Of course, with mojo "anything is > possible". But that doesn't necessarily mean we should support it. Could you elaborate? I don't fully understand the question. What's "this" and "it"? By lower-privileged process, do you mean sandboxed process?
On 2016/12/02 19:18:28, xhwang wrote: > On 2016/12/02 19:12:13, dcheng wrote: > > On 2016/12/02 18:47:21, xhwang wrote: > > > On 2016/12/02 18:40:27, dcheng wrote: > > > > On 2016/12/02 18:36:17, xhwang wrote: > > > > > On 2016/12/02 18:30:52, dcheng wrote: > > > > > > On 2016/12/02 18:29:09, xhwang wrote: > > > > > > > On 2016/12/02 18:00:30, dcheng wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > > > > > > > > File media/mojo/services/android_mojo_media_client.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > > > > > > > > media/mojo/services/android_mojo_media_client.cc:46: return > nullptr; > > > > > > > > On 2016/12/02 17:47:48, xhwang wrote: > > > > > > > > > On 2016/12/02 16:45:53, slan wrote: > > > > > > > > > > On 2016/12/02 08:52:51, dcheng wrote: > > > > > > > > > > > Just trying to understand the change here: does hitting this > > > mean > > > > > that > > > > > > > > > there's > > > > > > > > > > a > > > > > > > > > > > programming error? Is there any reason why code would be > > calling > > > > > this > > > > > > > with > > > > > > > > > no > > > > > > > > > > > host interfaces? Or does this client simply not know? > > > > > > > > > > > > > > > > > > > > |host_interfaces| is, at present, used to provide interfaces > > > > specific > > > > > to > > > > > > > the > > > > > > > > > > render frame. This change provides flexibility to clients who > do > > > not > > > > > > need > > > > > > > to > > > > > > > > > > provide additional interfaces, or who know nothing about > render > > > > > frames, > > > > > > > but > > > > > > > > > > still want to consume the "media" service. > > > > > > > > > > > > > > > > > > > > Xiaohan may be able to provide specifics on what the Android > use > > > > cases > > > > > > for > > > > > > > > > this > > > > > > > > > > are (perhaps some media playback of non-protected content from > > > > another > > > > > > > > > context). > > > > > > > > > > On Chromecast, we have several applications that will benefit > > from > > > > > this > > > > > > > > > > flexibility. > > > > > > > > > > > > > > > > > > > > This pattern deviates from how interfaces requests should be > > > > brokered > > > > > > > > between > > > > > > > > > > services (crbug.com/660573). Making this parameter optional > > takes > > > a > > > > > step > > > > > > > in > > > > > > > > > the > > > > > > > > > > right direction for correcting this behavior. > > > > > > > > > > > > > > > > > > dcheng: good point. I think when this happens, it's generally a > > > > > > programming > > > > > > > > > error, thus a DCHECK is typically the recommendation. However, > as > > > > slan@ > > > > > > > > > mentioned, with the flexibility of mojo, there are many > different > > > ways > > > > > to > > > > > > > > config > > > > > > > > > the system to use the media service with this client, and I was > > > afraid > > > > > > when > > > > > > > > the > > > > > > > > > system was misconfiged we could end up here. This is similar to > > the > > > > > > current > > > > > > > > > warning/failure in mojo connector where a client tries to > connect > > to > > > > an > > > > > > > > > interface, but no interface implementation is registered. > > > > > > > > > > > > > > > > > > With that, maybe this will make more sense? > > > > > > > > > > > > > > > > > > if (!host_interfaces) { > > > > > > > > > NOTREACHED() << "Host interface should be provided when using > > CDM > > > > with > > > > > > > > > AndroidMojoMediaClient"; > > > > > > > > > return nullptr; > > > > > > > > > } > > > > > > > > > > > > > > > > I feel like a NOTREACHED() will just end up mostly ignored? > > > > > > > > > > > > > > In debug build NOTREACHED() is the same as DCHECK(false) so it can't > > be > > > > > > ignored. > > > > > > > If you are talking about release mode, we can add a LOG(ERROR). > > > > > > > > > > > > > > > Also, my uncertainty about what to do here comes from my confusion > > > about > > > > > the > > > > > > > > process model in media: I poked around a bit, and I /think/ this > is > > > > > running > > > > > > in > > > > > > > > the unprivileged process... but I'm not 100% certain either. But > in > > > that > > > > > > case, > > > > > > > > it's the unprivileged process making a programming error, so it > > would > > > be > > > > > > > useful > > > > > > > > to fail early instead? > > > > > > > > > > > > > > MediaService (including *MojoMediaClient) itself is > process-aganostic. > > > > This > > > > > is > > > > > > > intentional so that it can be reused in many different ways on > > different > > > > > > > platforms. In Chrome for Android, this is currently running in the > GPU > > > > > > process. > > > > > > > > > > > > > > I am not super clear what you mean by "fail early" here... > > > > > > > > > > > > As in "just explode" =) > > > > > > > > > > Got it :) > > > > > > > > > > > But again, process model / boundaries aren't clear to me here. On > Clank, > > > > this > > > > > is > > > > > > in GPU, but if renderer can trigger this code path, then you're right, > > we > > > > have > > > > > > to gracefully fail. If no other process can reach it (and it's > internal > > > > > > bookkeeping that the GPU process itself should have been keeping > better > > > > track > > > > > > of), then that's more reason to only DCHECK. > > > > > > > > > > For Clank I agree. But as said earlier, you can config the system in > many > > > > > different ways. For example, in theory, you can run this in the browser > > > > process > > > > > (see ENABLE_MOJO_MEDIA_IN_BROWSER_PROCESS), or in some unsandboxed > > > > sub-process. > > > > > In those cases, I am not super confident with a DCHECK... > > > > > > > > Honestly, it seems to me our own code should know what's configured where, > > and > > > > if that's not true, that's kind of ... weird. > > > > > > Chromium, including "media/" is used in a lot of products, not limited to > the > > > Chrome browser. Now with mojo, it's super easy to move some component that > > used > > > to run in one process to another process. Here's an example: > > > https://chromiumcodereview.appspot.com/2115603002/. I don't know how > security > > > people would think for these cases. But from "media"'s perspective, we > > probably > > > should never make any process-architecture assumption and always err on the > > safe > > > side. > > > > Right, that was the second part of my question: is this something that we > expect > > to be exposed to lower-privileged processes? Of course, with mojo "anything is > > possible". But that doesn't necessarily mean we should support it. > > Could you elaborate? I don't fully understand the question. What's "this" and > "it"? By lower-privileged process, do you mean sandboxed process? Ultimately, what's not clear to me is the call graph for GetCdmFactory(): is this designed to be itself exposed via IPC? For example, on Clank, AndroidMojoMediaClient lives in the GPU process right now. Can a renderer process send a IPC to the GPU that will cause it to eventually call GetCdmFactory()? If so, then I agree that we need to handle this robustly and not DCHECK() at all. However, if this is never the case and GetCdmFactory() is only ever meant to be callable from within the same process, a DCHECK() should be perfectly reasonable.
On 2016/12/02 19:23:27, dcheng wrote: > On 2016/12/02 19:18:28, xhwang wrote: > > On 2016/12/02 19:12:13, dcheng wrote: > > > On 2016/12/02 18:47:21, xhwang wrote: > > > > On 2016/12/02 18:40:27, dcheng wrote: > > > > > On 2016/12/02 18:36:17, xhwang wrote: > > > > > > On 2016/12/02 18:30:52, dcheng wrote: > > > > > > > On 2016/12/02 18:29:09, xhwang wrote: > > > > > > > > On 2016/12/02 18:00:30, dcheng wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > > > > > > > > > File media/mojo/services/android_mojo_media_client.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > > > > > > > > > media/mojo/services/android_mojo_media_client.cc:46: return > > nullptr; > > > > > > > > > On 2016/12/02 17:47:48, xhwang wrote: > > > > > > > > > > On 2016/12/02 16:45:53, slan wrote: > > > > > > > > > > > On 2016/12/02 08:52:51, dcheng wrote: > > > > > > > > > > > > Just trying to understand the change here: does hitting > this > > > > mean > > > > > > that > > > > > > > > > > there's > > > > > > > > > > > a > > > > > > > > > > > > programming error? Is there any reason why code would be > > > calling > > > > > > this > > > > > > > > with > > > > > > > > > > no > > > > > > > > > > > > host interfaces? Or does this client simply not know? > > > > > > > > > > > > > > > > > > > > > > |host_interfaces| is, at present, used to provide interfaces > > > > > specific > > > > > > to > > > > > > > > the > > > > > > > > > > > render frame. This change provides flexibility to clients > who > > do > > > > not > > > > > > > need > > > > > > > > to > > > > > > > > > > > provide additional interfaces, or who know nothing about > > render > > > > > > frames, > > > > > > > > but > > > > > > > > > > > still want to consume the "media" service. > > > > > > > > > > > > > > > > > > > > > > Xiaohan may be able to provide specifics on what the Android > > use > > > > > cases > > > > > > > for > > > > > > > > > > this > > > > > > > > > > > are (perhaps some media playback of non-protected content > from > > > > > another > > > > > > > > > > context). > > > > > > > > > > > On Chromecast, we have several applications that will > benefit > > > from > > > > > > this > > > > > > > > > > > flexibility. > > > > > > > > > > > > > > > > > > > > > > This pattern deviates from how interfaces requests should be > > > > > brokered > > > > > > > > > between > > > > > > > > > > > services (crbug.com/660573). Making this parameter optional > > > takes > > > > a > > > > > > step > > > > > > > > in > > > > > > > > > > the > > > > > > > > > > > right direction for correcting this behavior. > > > > > > > > > > > > > > > > > > > > dcheng: good point. I think when this happens, it's generally > a > > > > > > > programming > > > > > > > > > > error, thus a DCHECK is typically the recommendation. However, > > as > > > > > slan@ > > > > > > > > > > mentioned, with the flexibility of mojo, there are many > > different > > > > ways > > > > > > to > > > > > > > > > config > > > > > > > > > > the system to use the media service with this client, and I > was > > > > afraid > > > > > > > when > > > > > > > > > the > > > > > > > > > > system was misconfiged we could end up here. This is similar > to > > > the > > > > > > > current > > > > > > > > > > warning/failure in mojo connector where a client tries to > > connect > > > to > > > > > an > > > > > > > > > > interface, but no interface implementation is registered. > > > > > > > > > > > > > > > > > > > > With that, maybe this will make more sense? > > > > > > > > > > > > > > > > > > > > if (!host_interfaces) { > > > > > > > > > > NOTREACHED() << "Host interface should be provided when > using > > > CDM > > > > > with > > > > > > > > > > AndroidMojoMediaClient"; > > > > > > > > > > return nullptr; > > > > > > > > > > } > > > > > > > > > > > > > > > > > > I feel like a NOTREACHED() will just end up mostly ignored? > > > > > > > > > > > > > > > > In debug build NOTREACHED() is the same as DCHECK(false) so it > can't > > > be > > > > > > > ignored. > > > > > > > > If you are talking about release mode, we can add a LOG(ERROR). > > > > > > > > > > > > > > > > > Also, my uncertainty about what to do here comes from my > confusion > > > > about > > > > > > the > > > > > > > > > process model in media: I poked around a bit, and I /think/ this > > is > > > > > > running > > > > > > > in > > > > > > > > > the unprivileged process... but I'm not 100% certain either. But > > in > > > > that > > > > > > > case, > > > > > > > > > it's the unprivileged process making a programming error, so it > > > would > > > > be > > > > > > > > useful > > > > > > > > > to fail early instead? > > > > > > > > > > > > > > > > MediaService (including *MojoMediaClient) itself is > > process-aganostic. > > > > > This > > > > > > is > > > > > > > > intentional so that it can be reused in many different ways on > > > different > > > > > > > > platforms. In Chrome for Android, this is currently running in the > > GPU > > > > > > > process. > > > > > > > > > > > > > > > > I am not super clear what you mean by "fail early" here... > > > > > > > > > > > > > > As in "just explode" =) > > > > > > > > > > > > Got it :) > > > > > > > > > > > > > But again, process model / boundaries aren't clear to me here. On > > Clank, > > > > > this > > > > > > is > > > > > > > in GPU, but if renderer can trigger this code path, then you're > right, > > > we > > > > > have > > > > > > > to gracefully fail. If no other process can reach it (and it's > > internal > > > > > > > bookkeeping that the GPU process itself should have been keeping > > better > > > > > track > > > > > > > of), then that's more reason to only DCHECK. > > > > > > > > > > > > For Clank I agree. But as said earlier, you can config the system in > > many > > > > > > different ways. For example, in theory, you can run this in the > browser > > > > > process > > > > > > (see ENABLE_MOJO_MEDIA_IN_BROWSER_PROCESS), or in some unsandboxed > > > > > sub-process. > > > > > > In those cases, I am not super confident with a DCHECK... > > > > > > > > > > Honestly, it seems to me our own code should know what's configured > where, > > > and > > > > > if that's not true, that's kind of ... weird. > > > > > > > > Chromium, including "media/" is used in a lot of products, not limited to > > the > > > > Chrome browser. Now with mojo, it's super easy to move some component that > > > used > > > > to run in one process to another process. Here's an example: > > > > https://chromiumcodereview.appspot.com/2115603002/. I don't know how > > security > > > > people would think for these cases. But from "media"'s perspective, we > > > probably > > > > should never make any process-architecture assumption and always err on > the > > > safe > > > > side. > > > > > > Right, that was the second part of my question: is this something that we > > expect > > > to be exposed to lower-privileged processes? Of course, with mojo "anything > is > > > possible". But that doesn't necessarily mean we should support it. > > > > Could you elaborate? I don't fully understand the question. What's "this" and > > "it"? By lower-privileged process, do you mean sandboxed process? > > Ultimately, what's not clear to me is the call graph for GetCdmFactory(): is > this designed to be itself exposed via IPC? > > For example, on Clank, AndroidMojoMediaClient lives in the GPU process right > now. Can a renderer process send a IPC to the GPU that will cause it to > eventually call GetCdmFactory()? If so, then I agree that we need to handle this > robustly and not DCHECK() at all. > > However, if this is never the case and GetCdmFactory() is only ever meant to be > callable from within the same process, a DCHECK() should be perfectly > reasonable. Thanks for the clarification. GetCdmFactory() can be triggered by a mojo call in a remote process on media::mojom::InterfaceFactoryPtr::CreateCdm(): https://cs.chromium.org/chromium/src/media/mojo/services/interface_factory_im...
On 2016/12/02 19:29:52, xhwang wrote: > On 2016/12/02 19:23:27, dcheng wrote: > > On 2016/12/02 19:18:28, xhwang wrote: > > > On 2016/12/02 19:12:13, dcheng wrote: > > > > On 2016/12/02 18:47:21, xhwang wrote: > > > > > On 2016/12/02 18:40:27, dcheng wrote: > > > > > > On 2016/12/02 18:36:17, xhwang wrote: > > > > > > > On 2016/12/02 18:30:52, dcheng wrote: > > > > > > > > On 2016/12/02 18:29:09, xhwang wrote: > > > > > > > > > On 2016/12/02 18:00:30, dcheng wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > > > > > > > > > > File media/mojo/services/android_mojo_media_client.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2544523002/diff/40001/media/mojo/services/and... > > > > > > > > > > media/mojo/services/android_mojo_media_client.cc:46: return > > > nullptr; > > > > > > > > > > On 2016/12/02 17:47:48, xhwang wrote: > > > > > > > > > > > On 2016/12/02 16:45:53, slan wrote: > > > > > > > > > > > > On 2016/12/02 08:52:51, dcheng wrote: > > > > > > > > > > > > > Just trying to understand the change here: does hitting > > this > > > > > mean > > > > > > > that > > > > > > > > > > > there's > > > > > > > > > > > > a > > > > > > > > > > > > > programming error? Is there any reason why code would be > > > > calling > > > > > > > this > > > > > > > > > with > > > > > > > > > > > no > > > > > > > > > > > > > host interfaces? Or does this client simply not know? > > > > > > > > > > > > > > > > > > > > > > > > |host_interfaces| is, at present, used to provide > interfaces > > > > > > specific > > > > > > > to > > > > > > > > > the > > > > > > > > > > > > render frame. This change provides flexibility to clients > > who > > > do > > > > > not > > > > > > > > need > > > > > > > > > to > > > > > > > > > > > > provide additional interfaces, or who know nothing about > > > render > > > > > > > frames, > > > > > > > > > but > > > > > > > > > > > > still want to consume the "media" service. > > > > > > > > > > > > > > > > > > > > > > > > Xiaohan may be able to provide specifics on what the > Android > > > use > > > > > > cases > > > > > > > > for > > > > > > > > > > > this > > > > > > > > > > > > are (perhaps some media playback of non-protected content > > from > > > > > > another > > > > > > > > > > > context). > > > > > > > > > > > > On Chromecast, we have several applications that will > > benefit > > > > from > > > > > > > this > > > > > > > > > > > > flexibility. > > > > > > > > > > > > > > > > > > > > > > > > This pattern deviates from how interfaces requests should > be > > > > > > brokered > > > > > > > > > > between > > > > > > > > > > > > services (crbug.com/660573). Making this parameter > optional > > > > takes > > > > > a > > > > > > > step > > > > > > > > > in > > > > > > > > > > > the > > > > > > > > > > > > right direction for correcting this behavior. > > > > > > > > > > > > > > > > > > > > > > dcheng: good point. I think when this happens, it's > generally > > a > > > > > > > > programming > > > > > > > > > > > error, thus a DCHECK is typically the recommendation. > However, > > > as > > > > > > slan@ > > > > > > > > > > > mentioned, with the flexibility of mojo, there are many > > > different > > > > > ways > > > > > > > to > > > > > > > > > > config > > > > > > > > > > > the system to use the media service with this client, and I > > was > > > > > afraid > > > > > > > > when > > > > > > > > > > the > > > > > > > > > > > system was misconfiged we could end up here. This is similar > > to > > > > the > > > > > > > > current > > > > > > > > > > > warning/failure in mojo connector where a client tries to > > > connect > > > > to > > > > > > an > > > > > > > > > > > interface, but no interface implementation is registered. > > > > > > > > > > > > > > > > > > > > > > With that, maybe this will make more sense? > > > > > > > > > > > > > > > > > > > > > > if (!host_interfaces) { > > > > > > > > > > > NOTREACHED() << "Host interface should be provided when > > using > > > > CDM > > > > > > with > > > > > > > > > > > AndroidMojoMediaClient"; > > > > > > > > > > > return nullptr; > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > I feel like a NOTREACHED() will just end up mostly ignored? > > > > > > > > > > > > > > > > > > In debug build NOTREACHED() is the same as DCHECK(false) so it > > can't > > > > be > > > > > > > > ignored. > > > > > > > > > If you are talking about release mode, we can add a LOG(ERROR). > > > > > > > > > > > > > > > > > > > Also, my uncertainty about what to do here comes from my > > confusion > > > > > about > > > > > > > the > > > > > > > > > > process model in media: I poked around a bit, and I /think/ > this > > > is > > > > > > > running > > > > > > > > in > > > > > > > > > > the unprivileged process... but I'm not 100% certain either. > But > > > in > > > > > that > > > > > > > > case, > > > > > > > > > > it's the unprivileged process making a programming error, so > it > > > > would > > > > > be > > > > > > > > > useful > > > > > > > > > > to fail early instead? > > > > > > > > > > > > > > > > > > MediaService (including *MojoMediaClient) itself is > > > process-aganostic. > > > > > > This > > > > > > > is > > > > > > > > > intentional so that it can be reused in many different ways on > > > > different > > > > > > > > > platforms. In Chrome for Android, this is currently running in > the > > > GPU > > > > > > > > process. > > > > > > > > > > > > > > > > > > I am not super clear what you mean by "fail early" here... > > > > > > > > > > > > > > > > As in "just explode" =) > > > > > > > > > > > > > > Got it :) > > > > > > > > > > > > > > > But again, process model / boundaries aren't clear to me here. On > > > Clank, > > > > > > this > > > > > > > is > > > > > > > > in GPU, but if renderer can trigger this code path, then you're > > right, > > > > we > > > > > > have > > > > > > > > to gracefully fail. If no other process can reach it (and it's > > > internal > > > > > > > > bookkeeping that the GPU process itself should have been keeping > > > better > > > > > > track > > > > > > > > of), then that's more reason to only DCHECK. > > > > > > > > > > > > > > For Clank I agree. But as said earlier, you can config the system in > > > many > > > > > > > different ways. For example, in theory, you can run this in the > > browser > > > > > > process > > > > > > > (see ENABLE_MOJO_MEDIA_IN_BROWSER_PROCESS), or in some unsandboxed > > > > > > sub-process. > > > > > > > In those cases, I am not super confident with a DCHECK... > > > > > > > > > > > > Honestly, it seems to me our own code should know what's configured > > where, > > > > and > > > > > > if that's not true, that's kind of ... weird. > > > > > > > > > > Chromium, including "media/" is used in a lot of products, not limited > to > > > the > > > > > Chrome browser. Now with mojo, it's super easy to move some component > that > > > > used > > > > > to run in one process to another process. Here's an example: > > > > > https://chromiumcodereview.appspot.com/2115603002/. I don't know how > > > security > > > > > people would think for these cases. But from "media"'s perspective, we > > > > probably > > > > > should never make any process-architecture assumption and always err on > > the > > > > safe > > > > > side. > > > > > > > > Right, that was the second part of my question: is this something that we > > > expect > > > > to be exposed to lower-privileged processes? Of course, with mojo > "anything > > is > > > > possible". But that doesn't necessarily mean we should support it. > > > > > > Could you elaborate? I don't fully understand the question. What's "this" > and > > > "it"? By lower-privileged process, do you mean sandboxed process? > > > > Ultimately, what's not clear to me is the call graph for GetCdmFactory(): is > > this designed to be itself exposed via IPC? > > > > For example, on Clank, AndroidMojoMediaClient lives in the GPU process right > > now. Can a renderer process send a IPC to the GPU that will cause it to > > eventually call GetCdmFactory()? If so, then I agree that we need to handle > this > > robustly and not DCHECK() at all. > > > > However, if this is never the case and GetCdmFactory() is only ever meant to > be > > callable from within the same process, a DCHECK() should be perfectly > > reasonable. > > Thanks for the clarification. GetCdmFactory() can be triggered by a mojo call in > a remote process on media::mojom::InterfaceFactoryPtr::CreateCdm(): > > https://cs.chromium.org/chromium/src/media/mojo/services/interface_factory_im... Got it, thank you for explaining =) I think a NOTREACHED() would be nice, but in that case, there is no choice but to handle this failure gracefully. LGTM
(removing old comments since we are reaching the max char limit)
> Got it, thank you for explaining =)
> I think a NOTREACHED() would be nice, but in that case, there is no choice but
> to handle this failure gracefully.
>
> LGTM
Thanks!
I still think the following is the best (with an extra NOTREACHED), so that a
dev that accidentally mis-configed the system would be a clear signal what went
wrong.
if (!host_interfaces) {
NOTREACHED() << "Host interface should be provided when using CDM with
AndroidMojoMediaClient";
return nullptr;
}
On 2016/12/02 19:58:20, xhwang wrote:
> (removing old comments since we are reaching the max char limit)
>
> > Got it, thank you for explaining =)
> > I think a NOTREACHED() would be nice, but in that case, there is no choice
but
> > to handle this failure gracefully.
> >
> > LGTM
>
> Thanks!
>
> I still think the following is the best (with an extra NOTREACHED), so that a
> dev that accidentally mis-configed the system would be a clear signal what
went
> wrong.
>
> if (!host_interfaces) {
> NOTREACHED() << "Host interface should be provided when using CDM with
> AndroidMojoMediaClient";
> return nullptr;
> }
Done. Thanks for the thoughtful discussion, both.
The CQ bit was checked by slan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, dcheng@chromium.org, alokp@chromium.org Link to the patchset: https://codereview.chromium.org/2544523002/#ps60001 (title: "Add NOTREACHED() to AndroidMojoMediaClient::GetCdmFactory().")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for media/mojo/services/media_manifest.json:
While running git apply --index -p1;
error: patch failed: media/mojo/services/media_manifest.json:4
error: media/mojo/services/media_manifest.json: patch does not apply
Patch: media/mojo/services/media_manifest.json
Index: media/mojo/services/media_manifest.json
diff --git a/media/mojo/services/media_manifest.json
b/media/mojo/services/media_manifest.json
index
f1c7205dd08b15595b8853b862dd6122c1155663..64c6194d6bf9f20d861874cac51450167c28cd56
100644
--- a/media/mojo/services/media_manifest.json
+++ b/media/mojo/services/media_manifest.json
@@ -4,8 +4,7 @@
"interface_provider_specs": {
"service_manager:connector": {
"provides": {
- "media:media": [ "media::mojom::MediaService" ],
- "media:interface_factory": [ "media::mojom::InterfaceFactory" ]
+ "media:media": [ "media::mojom::MediaService" ]
},
"requires": {
"*": [ "app" ]
The CQ bit was checked by slan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, dcheng@chromium.org, alokp@chromium.org Link to the patchset: https://codereview.chromium.org/2544523002/#ps80001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480722479382170,
"parent_rev": "c326065399008990261787abd828bb219175f0bf", "commit_rev":
"2e91555ddb9e6af79b3a0e9c2da7b318633821d3"}
Message was sent while issue was closed.
Description was changed from ========== Pass Connector* into MojoMediaClient::Initialize(). Pass the Connector* provided by the service:media's ServiceContext into MojoMediaClient::Initialize(), allowing MojoMediaClient implementations to connect to interfaces exposed by other services. Also makes the InterfaceProvider passed in CreateInterfaceFactory optional to eliminate unecessary complexity, and corrects an error in media_manifest.json. BUG=660736 ========== to ========== Pass Connector* into MojoMediaClient::Initialize(). Pass the Connector* provided by the service:media's ServiceContext into MojoMediaClient::Initialize(), allowing MojoMediaClient implementations to connect to interfaces exposed by other services. Also makes the InterfaceProvider passed in CreateInterfaceFactory optional to eliminate unecessary complexity, and corrects an error in media_manifest.json. BUG=660736 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Pass Connector* into MojoMediaClient::Initialize(). Pass the Connector* provided by the service:media's ServiceContext into MojoMediaClient::Initialize(), allowing MojoMediaClient implementations to connect to interfaces exposed by other services. Also makes the InterfaceProvider passed in CreateInterfaceFactory optional to eliminate unecessary complexity, and corrects an error in media_manifest.json. BUG=660736 ========== to ========== Pass Connector* into MojoMediaClient::Initialize(). Pass the Connector* provided by the service:media's ServiceContext into MojoMediaClient::Initialize(), allowing MojoMediaClient implementations to connect to interfaces exposed by other services. Also makes the InterfaceProvider passed in CreateInterfaceFactory optional to eliminate unecessary complexity, and corrects an error in media_manifest.json. BUG=660736 Committed: https://crrev.com/e18fa6be27931989ea9197d629e467d2d6866fdd Cr-Commit-Position: refs/heads/master@{#436123} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e18fa6be27931989ea9197d629e467d2d6866fdd Cr-Commit-Position: refs/heads/master@{#436123} |
