EME: Update MediaKeySystemConfiguration defaults; require non-empty capabilities

EME: Update MediaKeySystemConfiguration defaults; check capabilities

The current EME spec has added defaults for some fields in
MediaKeySystemConfiguration, so update the code to specify
the defaults and match the latest algorithm.

As well, at least one of 'audioCapabilities' or 'videoCapabilities'
should be specified, so add a UseCounter if none are provided to
help determine when this can be enforced in the future.

Also update some of the EME tests to provide either
'audioCapabilities' or 'videoCapabilities' (or both)
when calling requestMediaKeySystemAccess().

BUG=616233
TEST=EME tests pass

Committed: https://crrev.com/2737b99610b13c7fcaad6ec3041cce4b538b372f
Cr-Commit-Position: refs/heads/master@{#422596}

[jrummell, 2016-09-16 19:05:22]

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

[jrummell, 2016-09-16 19:05:23]

PTAL. Should match the latest spec.

[sandersd (OOO until July 31), 2016-09-16 23:02:11]

LGTM % nits.

I verified that this implements the documented algorithm, I didn't attempt to
determine if the updated algorithm makes sense. (I can do that as well if that's
requested, but it will require scheduling several hours to re-familiarize myself
with the algorithm.)

https://codereview.chromium.org/2349813002/diff/1/media/blink/key_system_conf...
File media/blink/key_system_config_selector.cc (right):

https://codereview.chromium.org/2349813002/diff/1/media/blink/key_system_conf...
media/blink/key_system_config_selector.cc:503: EmeFeatureSupport
distinctiveIdentifierSupport =
Nit: variable name does not match Chromium style.

https://codereview.chromium.org/2349813002/diff/1/media/blink/key_system_conf...
media/blink/key_system_config_selector.cc:539: EmeFeatureRequirement
persistentState = candidate.persistentState;
Nit: variable name does not match Chromium style.

https://codereview.chromium.org/2349813002/diff/1/media/blink/key_system_conf...
media/blink/key_system_config_selector.cc:544: EmeFeatureSupport
persistentStateSupport =
Nit: variable name does not match Chromium style.

[ddorwin, 2016-09-16 23:07:25]

I only skimmed very briefly.

https://codereview.chromium.org/2349813002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/Deprecation.cpp (right):

https://codereview.chromium.org/2349813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/Deprecation.cpp:350: return
String::format("EME requires that one of 'audioCapabilities' and
'videoCapabilities' must be non-empty. Non-standard support for this will be
removed in %s. Please specify at least one valid capability for
'audioCapabilities' or 'videoCapabilities' .", milestoneString(56));
extra space before period

[jrummell, 2016-09-17 00:15:04]

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

[jrummell, 2016-09-17 00:15:06]

Updated.

+foolip@ for OWNERS review of
  - third_party/WebKit/Source/core/frame/* (deprecation message)
  - third_party/WebKit/public/platform/WebMediaKeySystemConfiguration.h (enum)
+holta@ for OWNERS review of histograms.xml

https://codereview.chromium.org/2349813002/diff/1/media/blink/key_system_conf...
File media/blink/key_system_config_selector.cc (right):

https://codereview.chromium.org/2349813002/diff/1/media/blink/key_system_conf...
media/blink/key_system_config_selector.cc:503: EmeFeatureSupport
distinctiveIdentifierSupport =
On 2016/09/16 23:02:10, sandersd wrote:
> Nit: variable name does not match Chromium style.

Done.

https://codereview.chromium.org/2349813002/diff/1/media/blink/key_system_conf...
media/blink/key_system_config_selector.cc:539: EmeFeatureRequirement
persistentState = candidate.persistentState;
On 2016/09/16 23:02:10, sandersd wrote:
> Nit: variable name does not match Chromium style.

Done.

https://codereview.chromium.org/2349813002/diff/1/media/blink/key_system_conf...
media/blink/key_system_config_selector.cc:544: EmeFeatureSupport
persistentStateSupport =
On 2016/09/16 23:02:10, sandersd wrote:
> Nit: variable name does not match Chromium style.

Done.

https://codereview.chromium.org/2349813002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/Deprecation.cpp (right):

https://codereview.chromium.org/2349813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/Deprecation.cpp:350: return
String::format("EME requires that one of 'audioCapabilities' and
'videoCapabilities' must be non-empty. Non-standard support for this will be
removed in %s. Please specify at least one valid capability for
'audioCapabilities' or 'videoCapabilities' .", milestoneString(56));
On 2016/09/16 23:07:25, ddorwin wrote:
> extra space before period

Done.

[Steven Holte, 2016-09-19 18:51:00]

histograms lgtm

[jrummell, 2016-09-23 00:53:12]

ping foolip@ for OWNERS review.

[foolip, 2016-09-23 08:18:39]

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Deprecation.cpp (right):

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Deprecation.cpp:350: return
String::format("EME requires that one of 'audioCapabilities' and
'videoCapabilities' must be non-empty. Non-standard support for this will be
removed in %s. Please specify at least one valid capability for
'audioCapabilities' or 'videoCapabilities'.", milestoneString(56));
This needs an Intent to Deprecate and Remove on blink-dev. Since there's no use
counter data yet, some reason to think usage is low will be needed, to not risk
committing to removal and then having to delay/revert. If this isn't in a hurry,
then adding the use counter first would be less effort probably.

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/encryptedmedia/MediaKeySystemConfiguration.idl
(left):

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeySystemConfiguration.idl:13:
dictionary MediaKeySystemConfiguration {
The URL in this file doesn't work, can you link to
https://w3c.github.io/encrypted-media/#mediakeysystemconfiguration-dictionary ?

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
(right):

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:155:
checkCapabilitiesProvided(webConfig);
I can't find where the "If video capabilities is null, return NotSupported" bit
of the spec is implemented, does it already reject?

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:259:
Deprecation::countDeprecation(m_resolver->getExecutionContext(),
UseCounter::EncryptedMediaCapabilityNotProvided);
Optional nit: refactor so that the deprecation is exactly where the promise
would be rejected with NotSupported. It'd be more obviously like what the spec
if handled in MediaKeySystemAccess::getConfiguration, working with the generated
MediaKeySystemConfiguration instead of the WebMediaKeySystemConfiguration it
populates.

(Then the "removal" CL would be a one-liner replacing the reprecation by a
rejection.)

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMediaKeySystemConfiguration.h
(right):

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMediaKeySystemConfiguration.h:27: // As
|sessionTypes| is optional, we require a presence flag for it.
To be pendantic, all of the above are also optional, but they have defaults in
the IDL. This one also has a default, in prose: "If this member is not present
[WebIDL] when the dictionary is passed to requestMediaKeySystemAccess(), the
dictionary will be treated as if this member is set to [ "temporary" ]."

In other words, you should be able to handle that in the conversion to
WebMediaKeySystemConfiguration and drop hasSessionTypes here too.

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMediaKeySystemConfiguration.h:31: //
|label| may be not-present, but we use a null string to represent that.
It now has a default.

[jrummell, 2016-09-29 20:40:02]

Updated to remove the deprecation message for now.

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Deprecation.cpp (right):

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Deprecation.cpp:350: return
String::format("EME requires that one of 'audioCapabilities' and
'videoCapabilities' must be non-empty. Non-standard support for this will be
removed in %s. Please specify at least one valid capability for
'audioCapabilities' or 'videoCapabilities'.", milestoneString(56));
On 2016/09/23 08:18:39, foolip wrote:
> This needs an Intent to Deprecate and Remove on blink-dev. Since there's no
use
> counter data yet, some reason to think usage is low will be needed, to not
risk
> committing to removal and then having to delay/revert. If this isn't in a
hurry,
> then adding the use counter first would be less effort probably.

Removed message for now until we get some data.

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/encryptedmedia/MediaKeySystemConfiguration.idl
(left):

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeySystemConfiguration.idl:13:
dictionary MediaKeySystemConfiguration {
On 2016/09/23 08:18:39, foolip wrote:
> The URL in this file doesn't work, can you link to
> https://w3c.github.io/encrypted-media/#mediakeysystemconfiguration-dictionary
?

Done.

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
(right):

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:155:
checkCapabilitiesProvided(webConfig);
On 2016/09/23 08:18:39, foolip wrote:
> I can't find where the "If video capabilities is null, return NotSupported"
bit
> of the spec is implemented, does it already reject?

Most of the steps are done in media/blink/key_system_config_selector.cc, see
line 652 in this CL. It has a TODO to check it once we require it.

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:259:
Deprecation::countDeprecation(m_resolver->getExecutionContext(),
UseCounter::EncryptedMediaCapabilityNotProvided);
On 2016/09/23 08:18:39, foolip wrote:
> Optional nit: refactor so that the deprecation is exactly where the promise
> would be rejected with NotSupported. It'd be more obviously like what the spec
> if handled in MediaKeySystemAccess::getConfiguration, working with the
generated
> MediaKeySystemConfiguration instead of the WebMediaKeySystemConfiguration it
> populates.
> 
> (Then the "removal" CL would be a one-liner replacing the reprecation by a
> rejection.)

I wish :). Most of the checks happen outside of blink, where we know more about
what is supported by the key system.

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMediaKeySystemConfiguration.h
(right):

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMediaKeySystemConfiguration.h:27: // As
|sessionTypes| is optional, we require a presence flag for it.
On 2016/09/23 08:18:39, foolip wrote:
> To be pendantic, all of the above are also optional, but they have defaults in
> the IDL. This one also has a default, in prose: "If this member is not present
> [WebIDL] when the dictionary is passed to requestMediaKeySystemAccess(), the
> dictionary will be treated as if this member is set to [ "temporary" ]."
> 
> In other words, you should be able to handle that in the conversion to
> WebMediaKeySystemConfiguration and drop hasSessionTypes here too. 

Done.

https://codereview.chromium.org/2349813002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMediaKeySystemConfiguration.h:31: //
|label| may be not-present, but we use a null string to represent that.
On 2016/09/23 08:18:39, foolip wrote:
> It now has a default.

Done.

[foolip, 2016-09-29 21:14:31]

I think my lgtm is no longer needed, but rs lgtm if it is.

[jrummell, 2016-10-03 18:29:33]

Description was changed from

==========
EME: Update MediaKeySystemConfiguration defaults; require non-empty capabilities

The current EME spec has added defaults for some fields in
MediaKeySystemConfiguration, so update the code to specify
the defaults and match the latest algorithm.

As well, at least one of 'audioCapabilities' or 'videoCapabilities'
must now be specified, so add a deprecation message if none are
provided (and a UseCounter) to help sites see that this will be
enforced in the future.

Also update some of the EME tests to provide either
'audioCapabilities' or 'videoCapabilities' (or both)
when calling requestMediaKeySystemAccess().

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

to

==========
EME: Update MediaKeySystemConfiguration defaults; check capabilities

The current EME spec has added defaults for some fields in
MediaKeySystemConfiguration, so update the code to specify
the defaults and match the latest algorithm.

As well, at least one of 'audioCapabilities' or 'videoCapabilities'
should be specified, so add a UseCounter if none are provided to
help determine when this can be enforced in the future.

Also update some of the EME tests to provide either
'audioCapabilities' or 'videoCapabilities' (or both)
when calling requestMediaKeySystemAccess().

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

[jrummell, 2016-10-03 18:29:47]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2016-10-03 18:29:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org, holte@chromium.org, foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2349813002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-03 18:30:24]

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-10-03 18:45:03]

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

[commit-bot: I haz the power, 2016-10-03 18:45:07]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[jrummell, 2016-10-03 21:28:47]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2016-10-03 21:28:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org, holte@chromium.org, foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2349813002/#ps80001
(title: "fix Android compile")

[commit-bot: I haz the power, 2016-10-03 21:30:00]

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-10-03 23:30:39]

Description was changed from

==========
EME: Update MediaKeySystemConfiguration defaults; check capabilities

The current EME spec has added defaults for some fields in
MediaKeySystemConfiguration, so update the code to specify
the defaults and match the latest algorithm.

As well, at least one of 'audioCapabilities' or 'videoCapabilities'
should be specified, so add a UseCounter if none are provided to
help determine when this can be enforced in the future.

Also update some of the EME tests to provide either
'audioCapabilities' or 'videoCapabilities' (or both)
when calling requestMediaKeySystemAccess().

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

to

==========
EME: Update MediaKeySystemConfiguration defaults; check capabilities

The current EME spec has added defaults for some fields in
MediaKeySystemConfiguration, so update the code to specify
the defaults and match the latest algorithm.

As well, at least one of 'audioCapabilities' or 'videoCapabilities'
should be specified, so add a UseCounter if none are provided to
help determine when this can be enforced in the future.

Also update some of the EME tests to provide either
'audioCapabilities' or 'videoCapabilities' (or both)
when calling requestMediaKeySystemAccess().

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

[commit-bot: I haz the power, 2016-10-03 23:30:41]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-03 23:32:31]

Description was changed from

==========
EME: Update MediaKeySystemConfiguration defaults; check capabilities

The current EME spec has added defaults for some fields in
MediaKeySystemConfiguration, so update the code to specify
the defaults and match the latest algorithm.

As well, at least one of 'audioCapabilities' or 'videoCapabilities'
should be specified, so add a UseCounter if none are provided to
help determine when this can be enforced in the future.

Also update some of the EME tests to provide either
'audioCapabilities' or 'videoCapabilities' (or both)
when calling requestMediaKeySystemAccess().

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

to

==========
EME: Update MediaKeySystemConfiguration defaults; check capabilities

The current EME spec has added defaults for some fields in
MediaKeySystemConfiguration, so update the code to specify
the defaults and match the latest algorithm.

As well, at least one of 'audioCapabilities' or 'videoCapabilities'
should be specified, so add a UseCounter if none are provided to
help determine when this can be enforced in the future.

Also update some of the EME tests to provide either
'audioCapabilities' or 'videoCapabilities' (or both)
when calling requestMediaKeySystemAccess().

BUG=616233
TEST=EME tests pass

Committed: https://crrev.com/2737b99610b13c7fcaad6ec3041cce4b538b372f
Cr-Commit-Position: refs/heads/master@{#422596}
==========

[commit-bot: I haz the power, 2016-10-03 23:32:32]

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