Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2675743002:
PPAPI: Make output protection API work with mus+ash (Closed)
|
Created:
3 years, 10 months ago by Peng Modified:
3 years, 10 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, oshima+watch_chromium.org, darin (slow to review), davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPPAPI: Make output protection API work with mus+ash
With mustash, mus process is responsible for controlling
the display hardware instead of browser process. So the
pepper output protection impl in browser process can not
access DisplayConfigurator directly anymore. This CL adds
several content protection related mojo IPC. The output
protection impl will use those mojo IPC to access the
DisplayConfigurator in mus process.
PS. Netflix uses the pepper output protection API.
BUG=684003
Review-Url: https://codereview.chromium.org/2675743002
Cr-Commit-Position: refs/heads/master@{#450858}
Committed: https://chromium.googlesource.com/chromium/src/+/ae7972cadaacc1220283a8536061350046e82a3a
Patch Set 1 #Patch Set 2 : Update #Patch Set 3 : Update #Patch Set 4 : Update #Patch Set 5 : Fix build issues #Patch Set 6 : Fix try bot #
Total comments: 4
Patch Set 7 : Fix review issues #
Total comments: 2
Patch Set 8 : Update #
Total comments: 2
Patch Set 9 : Update #
Total comments: 17
Patch Set 10 : Update #Patch Set 11 : Update #Patch Set 12 : Update #
Total comments: 18
Patch Set 13 : Update #
Total comments: 8
Patch Set 14 : Update #Patch Set 15 : Fix build error #
Total comments: 4
Patch Set 16 : Update #Patch Set 17 : Add it in ui's manifest #
Total comments: 2
Patch Set 18 : Fix review issues #Patch Set 19 : Rebase #
Total comments: 2
Patch Set 20 : Fix review issue #
Total comments: 14
Patch Set 21 : Addressed review issues #Messages
Total messages: 106 (58 generated)
The CQ bit was checked by penghuang@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 checked by penghuang@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...
Description was changed from ========== ContentProtection ContentProtection BUG= ========== to ========== Make netflix work with mustash With mustash, mus process is responsible for controlling the display hardware instead of browser process. So we have to add several content protection related mojo IPC between mus and browser process. BUG=684003 ==========
penghuang@chromium.org changed reviewers: + oshima@chromium.org
Hi Oshima, PTAL. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by penghuang@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: This issue passed the CQ dry run.
oshima@chromium.org changed reviewers: + xhwang@chromium.org
The change looks fine, but I want xhwang@ who worked on this to take a look once. https://codereview.chromium.org/2675743002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_delegate.cc (right): https://codereview.chromium.org/2675743002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_delegate.cc:154: display_id_(0), nit: shouldn't this use kInvalidDisplayId? ditto for other places that uses 0.
xhwang@chromium.org changed reviewers: + kcwu@chromium.org
+kcwu, who is the best person to review this change.
BTW, in the CL title/description, please provide summary/detail on what this CL is doing. AFAICT, this change is not Netflix specific.
Could you give me a link explain what is mustash? This is the first time I heard it. https://codereview.chromium.org/2675743002/diff/100001/services/ui/public/int... File services/ui/public/interfaces/display/display_controller.mojom (right): https://codereview.chromium.org/2675743002/diff/100001/services/ui/public/int... services/ui/public/interfaces/display/display_controller.mojom:61: RegisterContentProtectionClient() => (uint64 client_id); client_id is int64 or uint64? this is inconsistent.
Description was changed from ========== Make netflix work with mustash With mustash, mus process is responsible for controlling the display hardware instead of browser process. So we have to add several content protection related mojo IPC between mus and browser process. BUG=684003 ========== to ========== PPAPI: Make output protection API work with mus+ash With mustash, mus process is responsible for controlling the display hardware instead of browser process. So the pepper output protection impl in browser process can not access DisplayConfigurator directly anymore. This CL adds several content protection related mojo IPC. The output protection impl will use those mojo IPC to access the DisplayConfigurator in mus process. PS. Netflix uses the pepper output protection API. BUG=684003 ==========
https://codereview.chromium.org/2675743002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_delegate.cc (right): https://codereview.chromium.org/2675743002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_delegate.cc:154: display_id_(0), On 2017/02/06 21:51:23, oshima wrote: > nit: shouldn't this use kInvalidDisplayId? > > ditto for other places that uses 0. Done. https://codereview.chromium.org/2675743002/diff/100001/services/ui/public/int... File services/ui/public/interfaces/display/display_controller.mojom (right): https://codereview.chromium.org/2675743002/diff/100001/services/ui/public/int... services/ui/public/interfaces/display/display_controller.mojom:61: RegisterContentProtectionClient() => (uint64 client_id); On 2017/02/07 01:35:45, kcwu wrote: > client_id is int64 or uint64? this is inconsistent. Done.
On 2017/02/07 01:35:45, kcwu wrote: > Could you give me a link explain what is mustash? This is the first time I heard > it. Please check go/mus+ash for more detail. > > https://codereview.chromium.org/2675743002/diff/100001/services/ui/public/int... > File services/ui/public/interfaces/display/display_controller.mojom (right): > > https://codereview.chromium.org/2675743002/diff/100001/services/ui/public/int... > services/ui/public/interfaces/display/display_controller.mojom:61: > RegisterContentProtectionClient() => (uint64 client_id); > client_id is int64 or uint64? this is inconsistent.
The CQ bit was checked by penghuang@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: This issue passed the CQ dry run.
On 2017/02/07 15:13:38, Peng wrote: > On 2017/02/07 01:35:45, kcwu wrote: > > Could you give me a link explain what is mustash? This is the first time I > heard > > it. > > Please check go/mus+ash for more detail. > > > > > > https://codereview.chromium.org/2675743002/diff/100001/services/ui/public/int... > > File services/ui/public/interfaces/display/display_controller.mojom (right): > > > > > https://codereview.chromium.org/2675743002/diff/100001/services/ui/public/int... > > services/ui/public/interfaces/display/display_controller.mojom:61: > > RegisterContentProtectionClient() => (uint64 client_id); > > client_id is int64 or uint64? this is inconsistent. kcwu & xhwang, kindly ping.
https://codereview.chromium.org/2675743002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_delegate.cc (right): https://codereview.chromium.org/2675743002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_delegate.cc:274: controller_->EnableProtection(display_id_, desired_method_mask_, should be new_display_id.
https://codereview.chromium.org/2675743002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_delegate.cc (right): https://codereview.chromium.org/2675743002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_delegate.cc:274: controller_->EnableProtection(display_id_, desired_method_mask_, On 2017/02/08 23:06:02, kcwu wrote: > should be new_display_id. Done
https://codereview.chromium.org/2675743002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_delegate.cc (right): https://codereview.chromium.org/2675743002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_delegate.cc:272: bool result = InitializeControllerIfNecessary(); IIUC, I feel this could be DCHECK(controller_); instead. Otherwise, if controller_ may change, line 276 need to be modified as something like if (old_controller) old_controller_->EnableProtection(display_id_, display::CONTENT_PROTECTION_METHOD_NONE, ...); What do you think?
https://codereview.chromium.org/2675743002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_delegate.cc (right): https://codereview.chromium.org/2675743002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_delegate.cc:272: bool result = InitializeControllerIfNecessary(); On 2017/02/08 23:29:53, kcwu wrote: > IIUC, I feel this could be > DCHECK(controller_); > instead. > > Otherwise, if controller_ may change, line 276 need to be modified as something > like > if (old_controller) > old_controller_->EnableProtection(display_id_, > display::CONTENT_PROTECTION_METHOD_NONE, ...); > > What do you think? Done.
lgtm
Hi Scott, PTAL. Thanks.
penghuang@chromium.org changed reviewers: + sky@chromium.org
Hi Scott, PTAL. Thanks.
rjkroege@chromium.org changed reviewers: + rjkroege@chromium.org
https://codereview.chromium.org/2675743002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_delegate.cc (right): https://codereview.chromium.org/2675743002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_delegate.cc:46: // Display content protection controller for running on Aura. On Aura? This code already presumes Aura. This is quite a bit of code. Perhaps you could extract into a separate file? https://codereview.chromium.org/2675743002/diff/160001/services/ui/public/int... File services/ui/public/interfaces/display/display_controller.mojom (right): https://codereview.chromium.org/2675743002/diff/160001/services/ui/public/int... services/ui/public/interfaces/display/display_controller.mojom:56: // ========================= Content Protection ============================== I don't think that this code belongs here. display_controller.mojom is a privileged interface for clients such as ash to manage the display hardware. Requesting content protection would seem to be a general service that (apparently) should be available to lower-privilege clients such as the browser. In particular: in the future, blink may wish to request display protection yes? I think this needs to be a separate interface and the client (browser) ought to be prepared for mus-ws to indicate that protection is not available or that the client is not allowed to request it. https://codereview.chromium.org/2675743002/diff/160001/services/ui/public/int... services/ui/public/interfaces/display/display_controller.mojom:60: [Sync] Can this be not sync? https://codereview.chromium.org/2675743002/diff/160001/ui/display/manager/chr... File ui/display/manager/chromeos/display_configurator.h (right): https://codereview.chromium.org/2675743002/diff/160001/ui/display/manager/chr... ui/display/manager/chromeos/display_configurator.h:58: typedef base::Callback<void(bool /* success */, using in new code?
Some drive-by comments https://codereview.chromium.org/2675743002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_delegate.cc (right): https://codereview.chromium.org/2675743002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_delegate.cc:46: // Display content protection controller for running on Aura. On 2017/02/09 00:44:43, rjkroege wrote: > On Aura? This code already presumes Aura. Maybe call this ControllerAsh instead (since it gets the DisplayConfigurator from ash) > > This is quite a bit of code. Perhaps you could extract into a separate file? Separate files sound like a good idea. https://codereview.chromium.org/2675743002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_delegate.cc:52: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); I suggest using a ThreadChecker instead. (and may be DCHECK() in the ctor that it's being created on the UI thread) ditto for below in ControllerMus. https://codereview.chromium.org/2675743002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_delegate.cc:211: callback.Run(false, 0, 0); It looks like both calls to InitializeControllerIfNecessary() can happen asynchronously, since both already gets an async callback. So it shouldn't be difficult to make the RegisterContentProtectionClient() call in mojom async, right?
https://codereview.chromium.org/2675743002/diff/160001/services/ui/public/int... File services/ui/public/interfaces/display/display_controller.mojom (right): https://codereview.chromium.org/2675743002/diff/160001/services/ui/public/int... services/ui/public/interfaces/display/display_controller.mojom:56: // ========================= Content Protection ============================== On 2017/02/09 00:44:43, rjkroege wrote: > I don't think that this code belongs here. display_controller.mojom is a > privileged interface for clients such as ash to manage the display hardware. > Requesting content protection would seem to be a general service that > (apparently) should be available to lower-privilege clients such as the browser. > In particular: in the future, blink may wish to request display protection yes? > > I think this needs to be a separate interface and the client (browser) ought to > be prepared for mus-ws to indicate that protection is not available or that the > client is not allowed to request it. > This interface can not only enable content protection, but also disable content protection. So I don't think we want to every clients have the right to disable the display content protection. So in pepper, this API is made a as private API, and it is only accessible to plugins provided by Google. Consider this fact, probably we want to make this mojo interface as a privileged interface too. @kwcu, could you please give us more detail about this API? Should we make it a general service or keep it as a private and privileged service? Thanks.
https://codereview.chromium.org/2675743002/diff/160001/services/ui/public/int... File services/ui/public/interfaces/display/display_controller.mojom (right): https://codereview.chromium.org/2675743002/diff/160001/services/ui/public/int... services/ui/public/interfaces/display/display_controller.mojom:56: // ========================= Content Protection ============================== On 2017/02/09 14:27:15, Peng wrote: > On 2017/02/09 00:44:43, rjkroege wrote: > > I don't think that this code belongs here. display_controller.mojom is a > > privileged interface for clients such as ash to manage the display hardware. > > Requesting content protection would seem to be a general service that > > (apparently) should be available to lower-privilege clients such as the > browser. > > In particular: in the future, blink may wish to request display protection > yes? > > > > I think this needs to be a separate interface and the client (browser) ought > to > > be prepared for mus-ws to indicate that protection is not available or that > the > > client is not allowed to request it. > > > > This interface can not only enable content protection, but also disable content > protection. So I don't think we want to every clients have the right to disable > the display content protection. So in pepper, this API is made a as private API, > and it is only accessible to plugins provided by Google. Consider this fact, > probably we want to make this mojo interface as a privileged interface too. > > @kwcu, could you please give us more detail about this API? Should we make it a > general service or keep it as a private and privileged service? Thanks. This interface should be only accessed from trusted code because it fully trust parameters like |client_id| from the client. It is possible converting it to general service since it is mojo now, |client_id| can be kept on trusted callee side. However, so far there is no known request to open it as general service.
I just looked at the mojom. https://codereview.chromium.org/2675743002/diff/160001/services/ui/public/int... File services/ui/public/interfaces/display/display_controller.mojom (right): https://codereview.chromium.org/2675743002/diff/160001/services/ui/public/int... services/ui/public/interfaces/display/display_controller.mojom:60: [Sync] On 2017/02/09 00:44:43, rjkroege wrote: > Can this be not sync? +1 https://codereview.chromium.org/2675743002/diff/160001/services/ui/public/int... services/ui/public/interfaces/display/display_controller.mojom:61: RegisterContentProtectionClient() => (uint64 client_id); This seems like separate functions than the rest of the functions on this interface. Is there a reason these functions need to be on DisplayController? Why is the client_id necessary?
On 2017/02/09 14:27:15, Peng wrote: > https://codereview.chromium.org/2675743002/diff/160001/services/ui/public/int... > File services/ui/public/interfaces/display/display_controller.mojom (right): > > https://codereview.chromium.org/2675743002/diff/160001/services/ui/public/int... > services/ui/public/interfaces/display/display_controller.mojom:56: // > ========================= Content Protection ============================== > On 2017/02/09 00:44:43, rjkroege wrote: > > I don't think that this code belongs here. display_controller.mojom is a > > privileged interface for clients such as ash to manage the display hardware. > > Requesting content protection would seem to be a general service that > > (apparently) should be available to lower-privilege clients such as the > browser. > > In particular: in the future, blink may wish to request display protection > yes? > > > > I think this needs to be a separate interface and the client (browser) ought > to > > be prepared for mus-ws to indicate that protection is not available or that > the > > client is not allowed to request it. > > > > This interface can not only enable content protection, but also disable content > protection. So I don't think we want to every clients have the right to disable > the display content protection. So in pepper, this API is made a as private API, > and it is only accessible to plugins provided by Google. Consider this fact, > probably we want to make this mojo interface as a privileged interface too. But should it be the same interface as the one used to control low level hardware display details? > > @kwcu, could you please give us more detail about this API? Should we make it a > general service or keep it as a private and privileged service? Thanks.
Patchset #10 (id:180001) has been deleted
Patchset #10 (id:200001) has been deleted
https://codereview.chromium.org/2675743002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_delegate.cc (right): https://codereview.chromium.org/2675743002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_delegate.cc:46: // Display content protection controller for running on Aura. On 2017/02/09 02:51:10, sadrul wrote: > On 2017/02/09 00:44:43, rjkroege wrote: > > On Aura? This code already presumes Aura. > > Maybe call this ControllerAsh instead (since it gets the DisplayConfigurator > from ash) > > > > > This is quite a bit of code. Perhaps you could extract into a separate file? > > Separate files sound like a good idea. Done. https://codereview.chromium.org/2675743002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_delegate.cc:52: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2017/02/09 02:51:10, sadrul wrote: > I suggest using a ThreadChecker instead. (and may be DCHECK() in the ctor that > it's being created on the UI thread) > > ditto for below in ControllerMus. Done. https://codereview.chromium.org/2675743002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_delegate.cc:211: callback.Run(false, 0, 0); On 2017/02/09 02:51:10, sadrul wrote: > It looks like both calls to InitializeControllerIfNecessary() can happen > asynchronously, since both already gets an async callback. So it shouldn't be > difficult to make the RegisterContentProtectionClient() call in mojom async, > right? I removed sync RegisterContentProtectionClient() mojo method, and client_id_ will be keeped in mus process. https://codereview.chromium.org/2675743002/diff/160001/services/ui/public/int... File services/ui/public/interfaces/display/display_controller.mojom (right): https://codereview.chromium.org/2675743002/diff/160001/services/ui/public/int... services/ui/public/interfaces/display/display_controller.mojom:60: [Sync] On 2017/02/09 00:44:43, rjkroege wrote: > Can this be not sync? I removed this method. And keep the client_id in service side. https://codereview.chromium.org/2675743002/diff/160001/services/ui/public/int... services/ui/public/interfaces/display/display_controller.mojom:61: RegisterContentProtectionClient() => (uint64 client_id); On 2017/02/09 20:59:07, sky wrote: > This seems like separate functions than the rest of the functions on this > interface. Is there a reason these functions need to be on DisplayController? > > Why is the client_id necessary? I removed the client_id, and moved those method to a new interface. https://codereview.chromium.org/2675743002/diff/160001/ui/display/manager/chr... File ui/display/manager/chromeos/display_configurator.h (right): https://codereview.chromium.org/2675743002/diff/160001/ui/display/manager/chr... ui/display/manager/chromeos/display_configurator.h:58: typedef base::Callback<void(bool /* success */, On 2017/02/09 00:44:43, rjkroege wrote: > using in new code? Yes. To use the same signature with mojo interface.
The CQ bit was checked by penghuang@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: This issue passed the CQ dry run.
Also, you have a lot of reviewers here, please list who is expected to review what files. Thanks. https://codereview.chromium.org/2675743002/diff/260001/services/ui/display/ou... File services/ui/display/output_protection.cc (right): https://codereview.chromium.org/2675743002/diff/260001/services/ui/display/ou... services/ui/display/output_protection.cc:16: display_configurator_->UnregisterContentProtectionClient(client_id_); Does unregister automatically revert? https://codereview.chromium.org/2675743002/diff/260001/services/ui/public/int... File services/ui/public/interfaces/display/output_protection.mojom (right): https://codereview.chromium.org/2675743002/diff/260001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:9: interface OutputProtection { Do we care about multiple clients trying to use this interface at once? Should only one client be allowed to use it at a time? https://codereview.chromium.org/2675743002/diff/260001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:19: EnableContentProtection(int64 display_id, Isn't this more of SetContentProtectionMethod() ? Also, I assume the content protection reverts to non when the interface is closed. If not it should, please document that. https://codereview.chromium.org/2675743002/diff/260001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:20: uint32 desired_method_mask) => (bool success); Please define the enum in the mojom. https://codereview.chromium.org/2675743002/diff/260001/ui/display/manager/chr... File ui/display/manager/chromeos/display_configurator.h (right): https://codereview.chromium.org/2675743002/diff/260001/ui/display/manager/chr... ui/display/manager/chromeos/display_configurator.h:49: kInvalidClientId = 0, enums use ALL_CAPS style. https://codereview.chromium.org/2675743002/diff/260001/ui/display/manager/chr... ui/display/manager/chromeos/display_configurator.h:60: typedef base::Callback<void(bool /* success */, Use using?
xhwang@chromium.org changed reviewers: - xhwang@chromium.org
sky: Good point! I am deferring the review to kcwu@, so removing me from the reviewer list.
https://codereview.chromium.org/2675743002/diff/260001/services/ui/display/ou... File services/ui/display/output_protection.cc (right): https://codereview.chromium.org/2675743002/diff/260001/services/ui/display/ou... services/ui/display/output_protection.cc:16: display_configurator_->UnregisterContentProtectionClient(client_id_); On 2017/02/10 19:24:56, sky wrote: > Does unregister automatically revert? I checked the code of DisplayConfigurator. Seems the requested mask is associated to given client id. I assume, when the client is unregistered, the associated mask will be reverted. @kcwu, could you please confirm it? https://codereview.chromium.org/2675743002/diff/260001/services/ui/public/int... File services/ui/public/interfaces/display/output_protection.mojom (right): https://codereview.chromium.org/2675743002/diff/260001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:9: interface OutputProtection { On 2017/02/10 19:24:56, sky wrote: > Do we care about multiple clients trying to use this interface at once? Should > only one client be allowed to use it at a time? I am not sure. But I checked the netflix, it will call QueryContentProtectionStatus() repeatedly to make sure the current display is protected. If the display is not protected, I think netflix will call EnableContentProtection() again to enable the protection or stop playing. kcwu@, Could you please give us more detail? Thanks https://codereview.chromium.org/2675743002/diff/260001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:19: EnableContentProtection(int64 display_id, On 2017/02/10 19:24:56, sky wrote: > Isn't this more of SetContentProtectionMethod() ? > > Also, I assume the content protection reverts to non when the interface is > closed. If not it should, please document that. I assume it too. kcwu@, Could you please confirm it? https://codereview.chromium.org/2675743002/diff/260001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:20: uint32 desired_method_mask) => (bool success); On 2017/02/10 19:24:56, sky wrote: > Please define the enum in the mojom. For the desired_method_mask? But chrome doesn't use enum for it, chrome just uses uint32_t. And the ContentProtectionMethod[1] is never used as function args type. [1] https://cs.chromium.org/chromium/src/ui/display/types/display_constants.h?typ... https://codereview.chromium.org/2675743002/diff/260001/ui/display/manager/chr... File ui/display/manager/chromeos/display_configurator.h (right): https://codereview.chromium.org/2675743002/diff/260001/ui/display/manager/chr... ui/display/manager/chromeos/display_configurator.h:49: kInvalidClientId = 0, On 2017/02/10 19:24:56, sky wrote: > enums use ALL_CAPS style. Done. https://codereview.chromium.org/2675743002/diff/260001/ui/display/manager/chr... ui/display/manager/chromeos/display_configurator.h:60: typedef base::Callback<void(bool /* success */, On 2017/02/10 19:24:56, sky wrote: > Use using? Done.
https://codereview.chromium.org/2675743002/diff/260001/services/ui/display/ou... File services/ui/display/output_protection.cc (right): https://codereview.chromium.org/2675743002/diff/260001/services/ui/display/ou... services/ui/display/output_protection.cc:16: display_configurator_->UnregisterContentProtectionClient(client_id_); On 2017/02/10 20:35:47, Peng wrote: > On 2017/02/10 19:24:56, sky wrote: > > Does unregister automatically revert? > > I checked the code of DisplayConfigurator. Seems the requested mask > is associated to given client id. I assume, when the client is > unregistered, the associated mask will be reverted. > > @kcwu, could you please confirm it? Yes, your understanding is correct. https://codereview.chromium.org/2675743002/diff/260001/services/ui/public/int... File services/ui/public/interfaces/display/output_protection.mojom (right): https://codereview.chromium.org/2675743002/diff/260001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:9: interface OutputProtection { On 2017/02/10 20:35:47, Peng wrote: > On 2017/02/10 19:24:56, sky wrote: > > Do we care about multiple clients trying to use this interface at once? Should > > only one client be allowed to use it at a time? > > I am not sure. But I checked the netflix, it will call > QueryContentProtectionStatus() repeatedly to make sure the current display is > protected. If the display is not protected, I think netflix will call > EnableContentProtection() again to enable the protection or stop playing. > > kcwu@, Could you please give us more detail? Thanks The api allows multiple clients at the same time. The implementation should enable protection if at least one client request protection. The protection may be disabled due to connection changed or other reasons, so the client will query status to make sure the protection is correctly enabled. https://codereview.chromium.org/2675743002/diff/260001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:19: EnableContentProtection(int64 display_id, On 2017/02/10 20:35:47, Peng wrote: > On 2017/02/10 19:24:56, sky wrote: > > Isn't this more of SetContentProtectionMethod() ? > > > > Also, I assume the content protection reverts to non when the interface is > > closed. If not it should, please document that. > > I assume it too. > > kcwu@, Could you please confirm it? Yes. When the interface is closed, the request from the said client will be removed. But the protection maybe still be enabled if there are still other clients requesting protection. About naming, I agree now, "set" seems better than "enable". I forgot why I named it "enable" at that time. https://codereview.chromium.org/2675743002/diff/260001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:20: uint32 desired_method_mask) => (bool success); On 2017/02/10 20:35:47, Peng wrote: > On 2017/02/10 19:24:56, sky wrote: > > Please define the enum in the mojom. > > For the desired_method_mask? But chrome doesn't use enum for it, chrome just > uses uint32_t. > And the ContentProtectionMethod[1] is never used as function args type. > > [1] > https://cs.chromium.org/chromium/src/ui/display/types/display_constants.h?typ... It is a bitmask in order to support other protection mechanism. However, so far we only support HDCP. https://codereview.chromium.org/2675743002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_controller_ash.h (right): https://codereview.chromium.org/2675743002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_controller_ash.h:22: void QueryStatus( // OutputProtectionDelegate::Controller: https://codereview.chromium.org/2675743002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_controller_mus.h (right): https://codereview.chromium.org/2675743002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_controller_mus.h:22: void QueryStatus( // OutputProtectionDelegate::Controlle: https://codereview.chromium.org/2675743002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_delegate.cc (right): https://codereview.chromium.org/2675743002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_delegate.cc:59: controller_.reset(); Is this line necessary? unique_ptr will be auto reset in dtor. https://codereview.chromium.org/2675743002/diff/280001/ui/display/manager/chr... File ui/display/manager/chromeos/display_configurator.h (right): https://codereview.chromium.org/2675743002/diff/280001/ui/display/manager/chr... ui/display/manager/chromeos/display_configurator.h:47: enum : uint64_t { Why use enum instead of a constant? And why use uint64_t directly instead of a type?
The CQ bit was checked by penghuang@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by penghuang@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...
I only looked at services ui, that looks good. One question holding back approval, I don't see any updates to manifest files. How come? https://codereview.chromium.org/2675743002/diff/320001/services/ui/display/ou... File services/ui/display/output_protection.h (right): https://codereview.chromium.org/2675743002/diff/320001/services/ui/display/ou... services/ui/display/output_protection.h:23: OutputProtection(DisplayConfigurator* display_configurator); explicit https://codereview.chromium.org/2675743002/diff/320001/services/ui/public/int... File services/ui/public/interfaces/display/output_protection.mojom (right): https://codereview.chromium.org/2675743002/diff/320001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:20: // Returns true when the protection request has been made. Returns true on success? Also, document why it might fail.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2675743002/diff/260001/services/ui/public/int... File services/ui/public/interfaces/display/output_protection.mojom (right): https://codereview.chromium.org/2675743002/diff/260001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:9: interface OutputProtection { On 2017/02/11 01:45:21, kcwu wrote: > On 2017/02/10 20:35:47, Peng wrote: > > On 2017/02/10 19:24:56, sky wrote: > > > Do we care about multiple clients trying to use this interface at once? > Should > > > only one client be allowed to use it at a time? > > > > I am not sure. But I checked the netflix, it will call > > QueryContentProtectionStatus() repeatedly to make sure the current display is > > protected. If the display is not protected, I think netflix will call > > EnableContentProtection() again to enable the protection or stop playing. > > > > kcwu@, Could you please give us more detail? Thanks > > The api allows multiple clients at the same time. The implementation should > enable protection if at least one client request protection. > The protection may be disabled due to connection changed or other reasons, so > the client will query status to make sure the protection is correctly enabled. Refined the document. https://codereview.chromium.org/2675743002/diff/260001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:19: EnableContentProtection(int64 display_id, On 2017/02/11 01:45:21, kcwu wrote: > On 2017/02/10 20:35:47, Peng wrote: > > On 2017/02/10 19:24:56, sky wrote: > > > Isn't this more of SetContentProtectionMethod() ? > > > > > > Also, I assume the content protection reverts to non when the interface is > > > closed. If not it should, please document that. > > > > I assume it too. > > > > kcwu@, Could you please confirm it? > > Yes. When the interface is closed, the request from the said client will be > removed. But the protection maybe still be enabled if there are still other > clients requesting protection. > About naming, I agree now, "set" seems better than "enable". I forgot why I > named it "enable" at that time. Refined the document. And renamed it to SetContentProtection. https://codereview.chromium.org/2675743002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_controller_ash.h (right): https://codereview.chromium.org/2675743002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_controller_ash.h:22: void QueryStatus( On 2017/02/11 01:45:21, kcwu wrote: > // OutputProtectionDelegate::Controller: Done. https://codereview.chromium.org/2675743002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_controller_mus.h (right): https://codereview.chromium.org/2675743002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_controller_mus.h:22: void QueryStatus( On 2017/02/11 01:45:21, kcwu wrote: > // OutputProtectionDelegate::Controlle: Done. https://codereview.chromium.org/2675743002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_delegate.cc (right): https://codereview.chromium.org/2675743002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_delegate.cc:59: controller_.reset(); On 2017/02/11 01:45:21, kcwu wrote: > Is this line necessary? unique_ptr will be auto reset in dtor. Done. https://codereview.chromium.org/2675743002/diff/280001/ui/display/manager/chr... File ui/display/manager/chromeos/display_configurator.h (right): https://codereview.chromium.org/2675743002/diff/280001/ui/display/manager/chr... ui/display/manager/chromeos/display_configurator.h:47: enum : uint64_t { On 2017/02/11 01:45:21, kcwu wrote: > Why use enum instead of a constant? And why use uint64_t directly instead of a > type? > The static constant member value causes the link errors when I use the vale in DCHECK() in debug build. I changed the ContentProtectionClientId to uint64 because below reasons: 1. It avoids defining ContentProtectionClientId in mojom, and type casting. 2. Using uint64 in more readable. It is very easier to understand what's the real type of the client_id. 3. Replacing 'DisplayConfigurator::DisplayConfigurator client_id' to 'uint64_t client_id' makes the code less and beautiful. :) WDYT? https://codereview.chromium.org/2675743002/diff/320001/services/ui/display/ou... File services/ui/display/output_protection.h (right): https://codereview.chromium.org/2675743002/diff/320001/services/ui/display/ou... services/ui/display/output_protection.h:23: OutputProtection(DisplayConfigurator* display_configurator); On 2017/02/13 17:05:01, sky wrote: > explicit Done. https://codereview.chromium.org/2675743002/diff/320001/services/ui/public/int... File services/ui/public/interfaces/display/output_protection.mojom (right): https://codereview.chromium.org/2675743002/diff/320001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:20: // Returns true when the protection request has been made. On 2017/02/13 17:05:01, sky wrote: > Returns true on success? Also, document why it might fail. Done.
On 2017/02/13 17:05:01, sky wrote: > I only looked at services ui, that looks good. One question holding back > approval, I don't see any updates to manifest files. How come? Oops! I found the browser request the display::mojom::OutputProtection interface failed. But the netflix still works. Very weird. I will investigate it. > > https://codereview.chromium.org/2675743002/diff/320001/services/ui/display/ou... > File services/ui/display/output_protection.h (right): > > https://codereview.chromium.org/2675743002/diff/320001/services/ui/display/ou... > services/ui/display/output_protection.h:23: > OutputProtection(DisplayConfigurator* display_configurator); > explicit > > https://codereview.chromium.org/2675743002/diff/320001/services/ui/public/int... > File services/ui/public/interfaces/display/output_protection.mojom (right): > > https://codereview.chromium.org/2675743002/diff/320001/services/ui/public/int... > services/ui/public/interfaces/display/output_protection.mojom:20: // Returns > true when the protection request has been made. > Returns true on success? Also, document why it might fail.
On 2017/02/13 21:23:11, Peng wrote: > On 2017/02/13 17:05:01, sky wrote: > > I only looked at services ui, that looks good. One question holding back > > approval, I don't see any updates to manifest files. How come? > > Oops! I found the browser request the display::mojom::OutputProtection interface > failed. But the netflix still works. Very weird. I will investigate it. I found if the interface is not bound correctly, the callbacks of calling the method of the interface will never be called. And the netflix will assume all calls are successful, and start playing video just after making those calls. If the method callback is called with failed result late, the netflix will stop the video. > > > > > > https://codereview.chromium.org/2675743002/diff/320001/services/ui/display/ou... > > File services/ui/display/output_protection.h (right): > > > > > https://codereview.chromium.org/2675743002/diff/320001/services/ui/display/ou... > > services/ui/display/output_protection.h:23: > > OutputProtection(DisplayConfigurator* display_configurator); > > explicit > > > > > https://codereview.chromium.org/2675743002/diff/320001/services/ui/public/int... > > File services/ui/public/interfaces/display/output_protection.mojom (right): > > > > > https://codereview.chromium.org/2675743002/diff/320001/services/ui/public/int... > > services/ui/public/interfaces/display/output_protection.mojom:20: // Returns > > true when the protection request has been made. > > Returns true on success? Also, document why it might fail.
https://codereview.chromium.org/2675743002/diff/360001/services/ui/manifest.json File services/ui/manifest.json (right): https://codereview.chromium.org/2675743002/diff/360001/services/ui/manifest.j... services/ui/manifest.json:13: "display::mojom::OutputProtection", This interface seems different from the rest in that we likely don't want every app using it. Is that right? I'm inclined to make a new class for it.
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
https://codereview.chromium.org/2675743002/diff/360001/services/ui/manifest.json File services/ui/manifest.json (right): https://codereview.chromium.org/2675743002/diff/360001/services/ui/manifest.j... services/ui/manifest.json:13: "display::mojom::OutputProtection", On 2017/02/13 23:26:00, sky wrote: > This interface seems different from the rest in that we likely don't want every > app using it. Is that right? I'm inclined to make a new class for it. 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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by penghuang@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: This issue passed the CQ dry run.
LGTM
penghuang@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng@ for mojom and manifest.json files Hi Daniel, PTAL. Thanks.
lgtm https://codereview.chromium.org/2675743002/diff/400001/services/ui/public/int... File services/ui/public/interfaces/display/output_protection.mojom (right): https://codereview.chromium.org/2675743002/diff/400001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:9: // the same time, and the display will be protection until, all clients fix before landing: "will be protected"?
https://codereview.chromium.org/2675743002/diff/400001/services/ui/public/int... File services/ui/public/interfaces/display/output_protection.mojom (right): https://codereview.chromium.org/2675743002/diff/400001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:9: // the same time, and the display will be protection until, all clients On 2017/02/14 19:24:21, rjkroege wrote: > fix before landing: "will be protected"? Done.
LGTM with nits addressed. https://codereview.chromium.org/2675743002/diff/420001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_controller_ash.h (right): https://codereview.chromium.org/2675743002/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_controller_ash.h:32: const uint64_t client_id_; Should we make this type ContentProtectionClientId to match the return value of RegisterContentProtectionClient()? https://codereview.chromium.org/2675743002/diff/420001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_delegate.cc (right): https://codereview.chromium.org/2675743002/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_delegate.cc:97: LOG(WARNING) << "RenderFrameHost is not alive."; Nit: DLOG? https://codereview.chromium.org/2675743002/diff/420001/services/ui/display/ou... File services/ui/display/output_protection.h (right): https://codereview.chromium.org/2675743002/diff/420001/services/ui/display/ou... services/ui/display/output_protection.h:37: const uint64_t client_id_; Nit: same comment about typing https://codereview.chromium.org/2675743002/diff/420001/services/ui/public/int... File services/ui/public/interfaces/display/output_protection.mojom (right): https://codereview.chromium.org/2675743002/diff/420001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:14: uint32 link_mask, Nit: document link_mask too https://codereview.chromium.org/2675743002/diff/420001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:20: // the content protection mask will be reverted. Will these/can we declare these constants in this mojom as well? https://codereview.chromium.org/2675743002/diff/420001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:21: // Return true on success. Return false, if it failed to make the protection Nit: no comma after false
https://codereview.chromium.org/2675743002/diff/420001/services/ui/public/int... File services/ui/public/interfaces/display/output_protection.mojom (right): https://codereview.chromium.org/2675743002/diff/420001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:20: // the content protection mask will be reverted. On 2017/02/15 06:26:52, dcheng wrote: > Will these/can we declare these constants in this mojom as well? (Or have a pointer to where they're defined =)
https://codereview.chromium.org/2675743002/diff/420001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_controller_ash.h (right): https://codereview.chromium.org/2675743002/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_controller_ash.h:32: const uint64_t client_id_; On 2017/02/15 06:26:51, dcheng wrote: > Should we make this type ContentProtectionClientId to match the return value of > RegisterContentProtectionClient()? The type ContentProtectionClientId has been removed by this CL with uint64_t. https://codereview.chromium.org/2675743002/diff/420001/chrome/browser/chromeo... File chrome/browser/chromeos/display/output_protection_delegate.cc (right): https://codereview.chromium.org/2675743002/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/display/output_protection_delegate.cc:97: LOG(WARNING) << "RenderFrameHost is not alive."; On 2017/02/15 06:26:51, dcheng wrote: > Nit: DLOG? Done. https://codereview.chromium.org/2675743002/diff/420001/services/ui/display/ou... File services/ui/display/output_protection.h (right): https://codereview.chromium.org/2675743002/diff/420001/services/ui/display/ou... services/ui/display/output_protection.h:37: const uint64_t client_id_; On 2017/02/15 06:26:52, dcheng wrote: > Nit: same comment about typing The type ContentProtectionClientId has been removed by this CL with uint64_t. https://codereview.chromium.org/2675743002/diff/420001/services/ui/public/int... File services/ui/public/interfaces/display/output_protection.mojom (right): https://codereview.chromium.org/2675743002/diff/420001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:14: uint32 link_mask, On 2017/02/15 06:26:52, dcheng wrote: > Nit: document link_mask too Done. https://codereview.chromium.org/2675743002/diff/420001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:20: // the content protection mask will be reverted. On 2017/02/15 06:27:20, dcheng wrote: > On 2017/02/15 06:26:52, dcheng wrote: > > Will these/can we declare these constants in this mojom as well? > > (Or have a pointer to where they're defined =) Done. https://codereview.chromium.org/2675743002/diff/420001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:20: // the content protection mask will be reverted. On 2017/02/15 06:26:52, dcheng wrote: > Will these/can we declare these constants in this mojom as well? Probably it is not necessary, we don't have use case for using the bitmask values from non c/c++ code for now. https://codereview.chromium.org/2675743002/diff/420001/services/ui/public/int... services/ui/public/interfaces/display/output_protection.mojom:21: // Return true on success. Return false, if it failed to make the protection On 2017/02/15 06:26:52, dcheng wrote: > Nit: no comma after false Done.
The CQ bit was checked by penghuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kcwu@chromium.org, rjkroege@chromium.org, sky@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2675743002/#ps440001 (title: "Addressed review issues")
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by penghuang@chromium.org
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": 440001, "attempt_start_ts": 1487189870928910,
"parent_rev": "32e074e42207bf161510aba4c4b5539d0818d2f1", "commit_rev":
"ddb82c8b9e5f20ed1743f3b4202f8a3b0dd1f515"}
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1487189870928910,
"parent_rev": "32e074e42207bf161510aba4c4b5539d0818d2f1", "commit_rev":
"ddb82c8b9e5f20ed1743f3b4202f8a3b0dd1f515"}
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1487189870928910,
"parent_rev": "32e074e42207bf161510aba4c4b5539d0818d2f1", "commit_rev":
"ddb82c8b9e5f20ed1743f3b4202f8a3b0dd1f515"}
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
The CQ bit was checked by penghuang@chromium.org
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_TIMED_OUT, no build URL)
The CQ bit was checked by penghuang@chromium.org
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": 440001, "attempt_start_ts": 1487213824335930,
"parent_rev": "4672b3b6953050cf8a9fd4ba54bb511c24f16d8c", "commit_rev":
"ae7972cadaacc1220283a8536061350046e82a3a"}
Message was sent while issue was closed.
Description was changed from ========== PPAPI: Make output protection API work with mus+ash With mustash, mus process is responsible for controlling the display hardware instead of browser process. So the pepper output protection impl in browser process can not access DisplayConfigurator directly anymore. This CL adds several content protection related mojo IPC. The output protection impl will use those mojo IPC to access the DisplayConfigurator in mus process. PS. Netflix uses the pepper output protection API. BUG=684003 ========== to ========== PPAPI: Make output protection API work with mus+ash With mustash, mus process is responsible for controlling the display hardware instead of browser process. So the pepper output protection impl in browser process can not access DisplayConfigurator directly anymore. This CL adds several content protection related mojo IPC. The output protection impl will use those mojo IPC to access the DisplayConfigurator in mus process. PS. Netflix uses the pepper output protection API. BUG=684003 Review-Url: https://codereview.chromium.org/2675743002 Cr-Commit-Position: refs/heads/master@{#450858} Committed: https://chromium.googlesource.com/chromium/src/+/ae7972cadaacc1220283a8536061... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:440001) as https://chromium.googlesource.com/chromium/src/+/ae7972cadaacc1220283a8536061... |
