EME: Fail requestMediaKeySystemAccess if no capabilities specified

EME: Fail requestMediaKeySystemAccess if no capabilities specified

The EME spec requires that at least one of 'audioCapabilities' or
'videoCapabilities' must be specified. There has been a deprecation
message for several releases, so enforce this now.

BUG=616233
TEST=EME tests pass

Review-Url: https://codereview.chromium.org/2652373002
Cr-Commit-Position: refs/heads/master@{#448438}
Committed: https://chromium.googlesource.com/chromium/src/+/93830e41885b6183b52a7cc3e0fe2a4ebb37c910

[jrummell, 2017-01-25 22:41:40]

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

[commit-bot: I haz the power, 2017-01-25 22:42:43]

Dry run: 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, 2017-01-25 23:11:41]

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

[commit-bot: I haz the power, 2017-01-25 23:11:42]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[jrummell, 2017-01-27 02:54:20]

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

[commit-bot: I haz the power, 2017-01-27 02:55:14]

Dry run: 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, 2017-01-27 04:09:22]

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

[commit-bot: I haz the power, 2017-01-27 04:09:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jrummell, 2017-01-28 01:07:51]

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

[commit-bot: I haz the power, 2017-01-28 01:08:21]

Dry run: 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, 2017-01-28 02:30:01]

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

[commit-bot: I haz the power, 2017-01-28 02:30:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jrummell, 2017-01-31 19:29:16]

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

[commit-bot: I haz the power, 2017-01-31 19:30:03]

Dry run: 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, 2017-01-31 21:33:37]

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

[commit-bot: I haz the power, 2017-01-31 21:33:38]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jrummell, 2017-01-31 21:55:54]

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

[commit-bot: I haz the power, 2017-01-31 21:56:37]

Dry run: 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, 2017-01-31 23:50:06]

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

[commit-bot: I haz the power, 2017-01-31 23:50:06]

Dry run: 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_...)

[jrummell, 2017-02-01 20:51:12]

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

[commit-bot: I haz the power, 2017-02-01 20:51:37]

Dry run: 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, 2017-02-01 22:54:53]

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

[commit-bot: I haz the power, 2017-02-01 22:54:54]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jrummell, 2017-02-02 02:23:50]

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

[commit-bot: I haz the power, 2017-02-02 02:24:04]

Dry run: 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, 2017-02-02 06:51:59]

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

[commit-bot: I haz the power, 2017-02-02 06:52:00]

Dry run: This issue passed the CQ dry run.

[jrummell, 2017-02-02 18:18:23]

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

[jrummell, 2017-02-02 18:18:25]

Finally fixed all the tests. PTAL.

[xhwang, 2017-02-02 18:31:20]

Great! Just a few comments, mostly on tests.

https://codereview.chromium.org/2652373002/diff/120001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/KeySystemTest.java
(right):

https://codereview.chromium.org/2652373002/diff/120001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/KeySystemTest.java:73:
+ "  .then(success, failure);"
Please add a comment why we need two here. I'll be very hard for figure it out
again later.

https://codereview.chromium.org/2652373002/diff/120001/media/blink/key_system...
File media/blink/key_system_config_selector_unittest.cc (right):

https://codereview.chromium.org/2652373002/diff/120001/media/blink/key_system...
media/blink/key_system_config_selector_unittest.cc:47: // dictionary was passed
in.
Is this still accurate with the new changes?

https://codereview.chromium.org/2652373002/diff/120001/media/blink/key_system...
media/blink/key_system_config_selector_unittest.cc:327: EXPECT_EQ(1u,
config_.audioCapabilities.size());
The test is called EmptyConfig. Is it still accurate?

Also, do we have a test that both audio and video capabilities are empty?

https://codereview.chromium.org/2652373002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/media/encrypted-media/encrypted-media-encrypted-event-different-origin.html
(right):

https://codereview.chromium.org/2652373002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/media/encrypted-media/encrypted-media-encrypted-event-different-origin.html:49:
[{audioCapabilities: [{contentType: 'audio/webm; codecs=vorbis'}]}])
here and everywhere else, shall we specify two audioCapability in case we want
to run this on Android?

[jrummell, 2017-02-02 18:58:44]

Description was changed from

==========
EME: Fail RequestMediaKeySystemAccess if no capabilities specified

The EME spec requires that at least one of 'audioCapabilities' or
'videoCapabilities' must be specified. There has been a deprecation
message for several releases, so enforce this now.

BUG=616233
TEST=EME tests pass
==========

to

==========
EME: Fail requestMediaKeySystemAccess if no capabilities specified

The EME spec requires that at least one of 'audioCapabilities' or
'videoCapabilities' must be specified. There has been a deprecation
message for several releases, so enforce this now.

BUG=616233
TEST=EME tests pass
==========

[jrummell, 2017-02-02 20:03:24]

Updated.

https://codereview.chromium.org/2652373002/diff/120001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/KeySystemTest.java
(right):

https://codereview.chromium.org/2652373002/diff/120001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/KeySystemTest.java:73:
+ "  .then(success, failure);"
On 2017/02/02 18:31:20, xhwang_slow wrote:
> Please add a comment why we need two here. I'll be very hard for figure it out
> again later.

Done.

https://codereview.chromium.org/2652373002/diff/120001/media/blink/key_system...
File media/blink/key_system_config_selector_unittest.cc (right):

https://codereview.chromium.org/2652373002/diff/120001/media/blink/key_system...
media/blink/key_system_config_selector_unittest.cc:47: // dictionary was passed
in.
On 2017/02/02 18:31:20, xhwang_slow wrote:
> Is this still accurate with the new changes?

Nope. Added UsableConfiguration().

https://codereview.chromium.org/2652373002/diff/120001/media/blink/key_system...
media/blink/key_system_config_selector_unittest.cc:327: EXPECT_EQ(1u,
config_.audioCapabilities.size());
On 2017/02/02 18:31:20, xhwang_slow wrote:
> The test is called EmptyConfig. Is it still accurate?
> 
> Also, do we have a test that both audio and video capabilities are empty?

Updated the test to make sure EmptyConfig fails, and added new test to verify
the DefaultConfig.

https://codereview.chromium.org/2652373002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/media/encrypted-media/encrypted-media-encrypted-event-different-origin.html
(right):

https://codereview.chromium.org/2652373002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/media/encrypted-media/encrypted-media-encrypted-event-different-origin.html:49:
[{audioCapabilities: [{contentType: 'audio/webm; codecs=vorbis'}]}])
On 2017/02/02 18:31:20, xhwang_slow wrote:
> here and everywhere else, shall we specify two audioCapability in case we want
> to run this on Android?

Below the test plays a webm file, so it probably doesn't make sense to add a
second mp4 capability. The test(s) could be changed to look at the returned
config and decide which type of file to use, but that's a bigger change.

[xhwang, 2017-02-02 23:12:49]

https://codereview.chromium.org/2652373002/diff/140001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/KeySystemTest.java
(right):

https://codereview.chromium.org/2652373002/diff/140001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/KeySystemTest.java:59:
// as encrypted webm is only supported on Lollipop+.
Thanks! Could you also provide info on why we can't just provide the avc1 one?

https://codereview.chromium.org/2652373002/diff/140001/media/blink/key_system...
File media/blink/key_system_config_selector_unittest.cc (right):

https://codereview.chromium.org/2652373002/diff/140001/media/blink/key_system...
media/blink/key_system_config_selector_unittest.cc:66: // blink code parses the
contentType into mimeType and codecs, so mimic
nit: Blink

https://codereview.chromium.org/2652373002/diff/140001/media/blink/key_system...
media/blink/key_system_config_selector_unittest.cc:329:
configs_.push_back(DefaultConfiguration());
Looking at this, maybe EmptyConfiguration() would make more sense. Then you
don't need the ", so the..." part.

[jrummell, 2017-02-03 22:30:24]

Updated.

https://codereview.chromium.org/2652373002/diff/140001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/KeySystemTest.java
(right):

https://codereview.chromium.org/2652373002/diff/140001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/KeySystemTest.java:59:
// as encrypted webm is only supported on Lollipop+.
On 2017/02/02 23:12:49, xhwang_slow wrote:
> Thanks! Could you also provide info on why we can't just provide the avc1 one?

Done.

https://codereview.chromium.org/2652373002/diff/140001/media/blink/key_system...
File media/blink/key_system_config_selector_unittest.cc (right):

https://codereview.chromium.org/2652373002/diff/140001/media/blink/key_system...
media/blink/key_system_config_selector_unittest.cc:66: // blink code parses the
contentType into mimeType and codecs, so mimic
On 2017/02/02 23:12:49, xhwang_slow wrote:
> nit: Blink

Done.

https://codereview.chromium.org/2652373002/diff/140001/media/blink/key_system...
media/blink/key_system_config_selector_unittest.cc:329:
configs_.push_back(DefaultConfiguration());
On 2017/02/02 23:12:49, xhwang_slow wrote:
> Looking at this, maybe EmptyConfiguration() would make more sense. Then you
> don't need the ", so the..." part.

Done.

[xhwang, 2017-02-03 22:47:44]

lgtm

[jrummell, 2017-02-04 00:33:27]

jrummell@chromium.org changed reviewers:
+ foolip@chromium.org, torne@chromium.org

[jrummell, 2017-02-04 00:33:28]

+torne@ for OWNERS review of android_webview/ test
+foolip@ for OWNERS review of third_party/WebKit/Source/core code (remove use
counters and deprecation message)

[foolip, 2017-02-05 22:58:32]

third_party/WebKit/ lgtm

[Torne, 2017-02-06 12:13:21]

android_webview LGTM

[jrummell, 2017-02-06 20:51:28]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2017-02-06 20:52:03]

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, 2017-02-06 23:17:32]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1486414288987400,
"parent_rev": "33a654bafa85eef00472ae622273b2adf58ec2ef", "commit_rev":
"93830e41885b6183b52a7cc3e0fe2a4ebb37c910"}

[commit-bot: I haz the power, 2017-02-06 23:19:11]

Description was changed from

==========
EME: Fail requestMediaKeySystemAccess if no capabilities specified

The EME spec requires that at least one of 'audioCapabilities' or
'videoCapabilities' must be specified. There has been a deprecation
message for several releases, so enforce this now.

BUG=616233
TEST=EME tests pass
==========

to

==========
EME: Fail requestMediaKeySystemAccess if no capabilities specified

The EME spec requires that at least one of 'audioCapabilities' or
'videoCapabilities' must be specified. There has been a deprecation
message for several releases, so enforce this now.

BUG=616233
TEST=EME tests pass

Review-Url: https://codereview.chromium.org/2652373002
Cr-Commit-Position: refs/heads/master@{#448438}
Committed:
https://chromium.googlesource.com/chromium/src/+/93830e41885b6183b52a7cc3e0fe...
==========

[commit-bot: I haz the power, 2017-02-06 23:19:12]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/93830e41885b6183b52a7cc3e0fe...