media: Add OutputProtectionProxy

media: Add OutputProtectionProxy

This class handles output protection query and request in the browser
process on all platforms. On ChromeOS, it delegates the operations on
the hardware displays to OutputProtectionDelegate. On other platforms,
it does not check hardware displays. On all platforms, it checks
whether there is any network link and adds it to the existing link mask.

This class will also be used to implement mojo OutputProtection service
on all platforms.

This CL also updated PPAPI cdm adapter and PPAPI code to enable output
protection calls on all platforms. A unit test is added via external
clear key tests.

BUG=592122, 589618
TEST=Added new tests.

Committed: https://crrev.com/22f8a43cae802c102b5e31211ca19466294a0157
Cr-Commit-Position: refs/heads/master@{#401896}

[xhwang, 2016-06-21 20:03:11]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org, jrummell@chromium.org

[xhwang, 2016-06-21 20:03:11]

xhwang@chromium.org changed required reviewers:
+ ddorwin@chromium.org

[xhwang, 2016-06-21 20:07:18]

ddorwin / jrummell: PTAL!

I tried to do everything in OutputProtectionDelegate but it turns out to be
really messy as we have too many ifdefs there. So I added the new class
OutputProtectionProxy. As always, naming is hard and I pretty much just picked
this name randomly. I am open to any naming suggestions.

https://chromiumcodereview.appspot.com/2085063002/diff/1/chrome/browser/media...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://chromiumcodereview.appspot.com/2085063002/diff/1/chrome/browser/media...
chrome/browser/media/encrypted_media_browsertest.cc:634: // network link
detection logic in OutputProtectionProxy.
+miu for ideas on this

https://chromiumcodereview.appspot.com/2085063002/diff/1/media/cdm/ppapi/exte...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://chromiumcodereview.appspot.com/2085063002/diff/1/media/cdm/ppapi/exte...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:717: // TODO(xhwang): Check
how to make status query work on ChromeOS in tests.
+kcwu: Any idea how to get this working in a test environment? Currently
QueryStatus() fails on a linux ChromeOS build.

[xhwang, 2016-06-21 20:07:58]

Description was changed from

==========
media: Add OutputProtectionProxy

This class handles output protection query and requrest in the browser
process on all platforms. On ChromeOS, it delegates the operations on
the hardware displays to OutputProtectionDelegate. On other platforms,
it doesn't not check hardware displays. On all platforms, it checks
whether there is any network link and adds it to the existing link mask.

This class will also be used to implement mojo OutputProtection service
on all platforms.

This CL also updated PPAPI cdm adapter and PPAPI code to enable output
protection calls on all platforms. A unit test is added via external
clear key tests.

BUG=592122
TEST=Added new tests.
==========

to

==========
media: Add OutputProtectionProxy

This class handles output protection query and requrest in the browser
process on all platforms. On ChromeOS, it delegates the operations on
the hardware displays to OutputProtectionDelegate. On other platforms,
it doesn't not check hardware displays. On all platforms, it checks
whether there is any network link and adds it to the existing link mask.

This class will also be used to implement mojo OutputProtection service
on all platforms.

This CL also updated PPAPI cdm adapter and PPAPI code to enable output
protection calls on all platforms. A unit test is added via external
clear key tests.

BUG=592122,589618
TEST=Added new tests.
==========

[ddorwin, 2016-06-23 06:32:05]

Description was changed from

==========
media: Add OutputProtectionProxy

This class handles output protection query and requrest in the browser
process on all platforms. On ChromeOS, it delegates the operations on
the hardware displays to OutputProtectionDelegate. On other platforms,
it doesn't not check hardware displays. On all platforms, it checks
whether there is any network link and adds it to the existing link mask.

This class will also be used to implement mojo OutputProtection service
on all platforms.

This CL also updated PPAPI cdm adapter and PPAPI code to enable output
protection calls on all platforms. A unit test is added via external
clear key tests.

BUG=592122,589618
TEST=Added new tests.
==========

to

==========
media: Add OutputProtectionProxy

This class handles output protection query and request in the browser
process on all platforms. On ChromeOS, it delegates the operations on
the hardware displays to OutputProtectionDelegate. On other platforms,
it does not check hardware displays. On all platforms, it checks
whether there is any network link and adds it to the existing link mask.

This class will also be used to implement mojo OutputProtection service
on all platforms.

This CL also updated PPAPI cdm adapter and PPAPI code to enable output
protection calls on all platforms. A unit test is added via external
clear key tests.

BUG=592122,589618
TEST=Added new tests.
==========

[kcwu, 2016-06-23 09:07:39]

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

[kcwu, 2016-06-23 09:07:40]

https://chromiumcodereview.appspot.com/2085063002/diff/1/media/cdm/ppapi/exte...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://chromiumcodereview.appspot.com/2085063002/diff/1/media/cdm/ppapi/exte...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:717: // TODO(xhwang): Check
how to make status query work on ChromeOS in tests.
On 2016/06/21 20:07:18, xhwang wrote:
> +kcwu: Any idea how to get this working in a test environment? Currently
> QueryStatus() fails on a linux ChromeOS build.

Sorry for late reply. 
How about using base::SysInfo::IsRunningOnChromeOS()?

[jrummell, 2016-06-23 18:07:57]

lgtm

https://codereview.chromium.org/2085063002/diff/40001/chrome/browser/media/ou...
File chrome/browser/media/output_protection_proxy.cc (right):

https://codereview.chromium.org/2085063002/diff/40001/chrome/browser/media/ou...
chrome/browser/media/output_protection_proxy.cc:36:
content::RenderFrameHost::FromID(render_process_id_, render_frame_id_);
It appears you just copied the code, but why check if RenderFrameHost is
available? It's not used (here or below). If it's needed to avoid errors in
output_protection_delegate_, move it inside the #if block. Maybe in the code
below you need to check before calling MediaCaptureDevicesDispatcher, but then
the check could be done inside the if (success).

[ddorwin, 2016-06-23 19:24:29]

Minor stuff.

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
chrome/browser/media/encrypted_media_browsertest.cc:38: // Variants of external
clear key key system to test different scenarios.
nit: E... C... K...

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
File chrome/browser/media/output_protection_proxy.cc (right):

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
chrome/browser/media/output_protection_proxy.cc:61: callback.Run(false);
NOTIMPLEMENTED()?

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
chrome/browser/media/output_protection_proxy.cc:75: // TODO(xjz): Investigate
whether this check (and the other one above) should
It definitely seems weird that we have to protect other code that doesn't
actually take this object.

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
chrome/browser/media/output_protection_proxy.cc:86: const bool
insecure_capture_detected =
nit: is_...

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
File chrome/browser/media/output_protection_proxy.h (right):

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
chrome/browser/media/output_protection_proxy.h:20: // On ChromeOS, operations on
the physical displays are delegated to
nit: space before OS.

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
chrome/browser/media/output_protection_proxy.h:45: void
QueryStatusComplete(const QueryStatusCallback& callback,
On... ? Or something like ProcessQueryStatusResult, since it does modify the
values.

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/r...
File
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.h
(right):

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/r...
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.h:60:
// TODO(xhwang): Use unique_ptr to avoid the confusion on ownership.
Is there a reason this can't be done now?

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/renderer/...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/renderer/...
chrome/renderer/media/chrome_key_systems.cc:163: // A key system that triggers
output protection test in ClearKeyCdm.
nit: triggers *the*...
Same at 160.

https://chromiumcodereview.appspot.com/2085063002/diff/40001/media/cdm/ppapi/...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://chromiumcodereview.appspot.com/2085063002/diff/40001/media/cdm/ppapi/...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:62: // Variants of external
clear key key system to test different scenarios.
nit: E... C... K...

https://chromiumcodereview.appspot.com/2085063002/diff/40001/media/cdm/ppapi/...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:715: // On ChromeOS, status
query will fail on Linux ChromeOS build. So we ignore
nit: space before "OS" x 3

https://chromiumcodereview.appspot.com/2085063002/diff/40001/media/cdm/ppapi/...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:724:
OnUnitTestComplete(true);
Are there any checks we can make on the values? For example, links should be
non-zero but also not >= than the max flag << 1.

[xhwang, 2016-06-23 20:45:53]

On 2016/06/23 09:07:40, kcwu wrote:
>
https://chromiumcodereview.appspot.com/2085063002/diff/1/media/cdm/ppapi/exte...
> File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):
> 
>
https://chromiumcodereview.appspot.com/2085063002/diff/1/media/cdm/ppapi/exte...
> media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:717: // TODO(xhwang):
Check
> how to make status query work on ChromeOS in tests.
> On 2016/06/21 20:07:18, xhwang wrote:
> > +kcwu: Any idea how to get this working in a test environment? Currently
> > QueryStatus() fails on a linux ChromeOS build.
> 
> Sorry for late reply. 
> How about using base::SysInfo::IsRunningOnChromeOS()?

Added TODO to try this. Thanks!

[xhwang, 2016-06-23 20:53:36]

comments addressed

[xhwang, 2016-06-23 20:53:49]

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
chrome/browser/media/encrypted_media_browsertest.cc:38: // Variants of external
clear key key system to test different scenarios.
On 2016/06/23 19:24:28, ddorwin wrote:
> nit: E... C... K...

Done.

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
File chrome/browser/media/output_protection_proxy.cc (right):

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
chrome/browser/media/output_protection_proxy.cc:36:
content::RenderFrameHost::FromID(render_process_id_, render_frame_id_);
On 2016/06/23 18:07:57, jrummell wrote:
> It appears you just copied the code, but why check if RenderFrameHost is
> available? It's not used (here or below). If it's needed to avoid errors in
> output_protection_delegate_, move it inside the #if block. Maybe in the code
> below you need to check before calling MediaCaptureDevicesDispatcher, but then
> the check could be done inside the if (success).

Makes sense. Done.

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
chrome/browser/media/output_protection_proxy.cc:61: callback.Run(false);
On 2016/06/23 19:24:28, ddorwin wrote:
> NOTIMPLEMENTED()?

Done.

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
chrome/browser/media/output_protection_proxy.cc:75: // TODO(xjz): Investigate
whether this check (and the other one above) should
On 2016/06/23 19:24:28, ddorwin wrote:
> It definitely seems weird that we have to protect other code that doesn't
> actually take this object.

Agreed. But I'd like to keep this TODO here and address it separately.

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
chrome/browser/media/output_protection_proxy.cc:86: const bool
insecure_capture_detected =
On 2016/06/23 19:24:28, ddorwin wrote:
> nit: is_...

Done.

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
File chrome/browser/media/output_protection_proxy.h (right):

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
chrome/browser/media/output_protection_proxy.h:20: // On ChromeOS, operations on
the physical displays are delegated to
On 2016/06/23 19:24:28, ddorwin wrote:
> nit: space before OS.

Done.

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/m...
chrome/browser/media/output_protection_proxy.h:45: void
QueryStatusComplete(const QueryStatusCallback& callback,
On 2016/06/23 19:24:28, ddorwin wrote:
> On... ? Or something like ProcessQueryStatusResult, since it does modify the
> values.

Done.

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/r...
File
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.h
(right):

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/browser/r...
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.h:60:
// TODO(xhwang): Use unique_ptr to avoid the confusion on ownership.
On 2016/06/23 19:24:28, ddorwin wrote:
> Is there a reason this can't be done now?

I wasn't super sure how to do this with unique_ptr. Now I know :) Done.

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/renderer/...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://chromiumcodereview.appspot.com/2085063002/diff/40001/chrome/renderer/...
chrome/renderer/media/chrome_key_systems.cc:163: // A key system that triggers
output protection test in ClearKeyCdm.
On 2016/06/23 19:24:28, ddorwin wrote:
> nit: triggers *the*...
> Same at 160.

Done.

https://chromiumcodereview.appspot.com/2085063002/diff/40001/media/cdm/ppapi/...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://chromiumcodereview.appspot.com/2085063002/diff/40001/media/cdm/ppapi/...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:62: // Variants of external
clear key key system to test different scenarios.
On 2016/06/23 19:24:28, ddorwin wrote:
> nit: E... C... K...

Done.

https://chromiumcodereview.appspot.com/2085063002/diff/40001/media/cdm/ppapi/...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:715: // On ChromeOS, status
query will fail on Linux ChromeOS build. So we ignore
On 2016/06/23 19:24:28, ddorwin wrote:
> nit: space before "OS" x 3

Done.

https://chromiumcodereview.appspot.com/2085063002/diff/40001/media/cdm/ppapi/...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:724:
OnUnitTestComplete(true);
On 2016/06/23 19:24:28, ddorwin wrote:
> Are there any checks we can make on the values? For example, links should be
> non-zero but also not >= than the max flag << 1.

On non-Chrome-OS, the link will actually be zero since we don't check the
hardware displays, and we don't fake any network link yet. Added this check.

[xhwang, 2016-06-23 20:55:53]

xhwang@chromium.org changed reviewers:
+ bbudge@chromium.org, oshima@chromium.org

[xhwang, 2016-06-23 20:55:54]

oshima@chromium.org: Please OWNERS review
chrome/browser/chromeos/display/output_protection_delegate.cc.

bbudge@chromium.org: Please OWNERS review
chrome/browser/renderer_host/pepper/*

[ddorwin, 2016-06-23 21:36:35]

LGTM % comment.

https://chromiumcodereview.appspot.com/2085063002/diff/60001/chrome/browser/m...
File chrome/browser/media/output_protection_proxy.cc (right):

https://chromiumcodereview.appspot.com/2085063002/diff/60001/chrome/browser/m...
chrome/browser/media/output_protection_proxy.cc:67: // TODO(xjz): Investigate
whether this check (and the other one above) should
You removed the one above per jrummell's comment. Are you sure that is okay? If
so, update this comment.

[xhwang, 2016-06-23 21:52:35]

https://chromiumcodereview.appspot.com/2085063002/diff/60001/chrome/browser/m...
File chrome/browser/media/output_protection_proxy.cc (right):

https://chromiumcodereview.appspot.com/2085063002/diff/60001/chrome/browser/m...
chrome/browser/media/output_protection_proxy.cc:67: // TODO(xjz): Investigate
whether this check (and the other one above) should
On 2016/06/23 21:36:35, ddorwin wrote:
> You removed the one above per jrummell's comment. Are you sure that is okay?
If
> so, update this comment.

Yeah, this class is just a proxy, output_protection_delegate_ will still check
this.

Updated comment.

[xhwang, 2016-06-23 21:53:14]

comments addressed

[xhwang, 2016-06-24 16:20:11]

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

[commit-bot: I haz the power, 2016-06-24 16:21:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oshima, 2016-06-24 16:22:39]

chrome/browser/chromeos/display lgtm

[bbudge, 2016-06-24 16:33:13]

chrome/browser/renderer_host/pepper LGTM (rubber stamp)

[xhwang, 2016-06-24 16:37:38]

The CQ bit was unchecked by xhwang@chromium.org

[xhwang, 2016-06-24 16:37:39]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2016-06-24 16:37:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
jrummell@chromium.org, ddorwin@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/2085063002/#ps80001
(title: "comments addressed")

[commit-bot: I haz the power, 2016-06-24 16:37:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-24 17:40:19]

Description was changed from

==========
media: Add OutputProtectionProxy

This class handles output protection query and request in the browser
process on all platforms. On ChromeOS, it delegates the operations on
the hardware displays to OutputProtectionDelegate. On other platforms,
it does not check hardware displays. On all platforms, it checks
whether there is any network link and adds it to the existing link mask.

This class will also be used to implement mojo OutputProtection service
on all platforms.

This CL also updated PPAPI cdm adapter and PPAPI code to enable output
protection calls on all platforms. A unit test is added via external
clear key tests.

BUG=592122,589618
TEST=Added new tests.
==========

to

==========
media: Add OutputProtectionProxy

This class handles output protection query and request in the browser
process on all platforms. On ChromeOS, it delegates the operations on
the hardware displays to OutputProtectionDelegate. On other platforms,
it does not check hardware displays. On all platforms, it checks
whether there is any network link and adds it to the existing link mask.

This class will also be used to implement mojo OutputProtection service
on all platforms.

This CL also updated PPAPI cdm adapter and PPAPI code to enable output
protection calls on all platforms. A unit test is added via external
clear key tests.

BUG=592122,589618
TEST=Added new tests.
==========

[commit-bot: I haz the power, 2016-06-24 17:40:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-24 17:41:49]

Description was changed from

==========
media: Add OutputProtectionProxy

This class handles output protection query and request in the browser
process on all platforms. On ChromeOS, it delegates the operations on
the hardware displays to OutputProtectionDelegate. On other platforms,
it does not check hardware displays. On all platforms, it checks
whether there is any network link and adds it to the existing link mask.

This class will also be used to implement mojo OutputProtection service
on all platforms.

This CL also updated PPAPI cdm adapter and PPAPI code to enable output
protection calls on all platforms. A unit test is added via external
clear key tests.

BUG=592122,589618
TEST=Added new tests.
==========

to

==========
media: Add OutputProtectionProxy

This class handles output protection query and request in the browser
process on all platforms. On ChromeOS, it delegates the operations on
the hardware displays to OutputProtectionDelegate. On other platforms,
it does not check hardware displays. On all platforms, it checks
whether there is any network link and adds it to the existing link mask.

This class will also be used to implement mojo OutputProtection service
on all platforms.

This CL also updated PPAPI cdm adapter and PPAPI code to enable output
protection calls on all platforms. A unit test is added via external
clear key tests.

BUG=592122,589618
TEST=Added new tests.

Committed: https://crrev.com/22f8a43cae802c102b5e31211ca19466294a0157
Cr-Commit-Position: refs/heads/master@{#401896}
==========

[commit-bot: I haz the power, 2016-06-24 17:41:50]

Patchset 5 (id:??) landed as
https://crrev.com/22f8a43cae802c102b5e31211ca19466294a0157
Cr-Commit-Position: refs/heads/master@{#401896}

[xhwang, 2016-06-24 22:03:32]

A revert of this CL (patchset #5 id:80001) has been created in
https://chromiumcodereview.appspot.com/2095963004/ by xhwang@chromium.org.

The reason for reverting is: This is breaking bots using --isolation-extensions

https://build.chromium.org/p/chromium.fyi/builders/Site%20Isolation%20Win/bui...

Also see issue 623214..