Implement navigator.requestMediaKeySystemAccess()

Implement navigator.requestMediaKeySystemAccess()

This replace MediaKeys.create().

Currently the checking of supportedConfigurations is limited to only checking initDataType (what isTypeSupported() does). This will be enhanced later.

BUG=425186
TEST=Updated EME tests pass

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184599

[jrummell, 2014-10-21 01:16:35]

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

[jrummell, 2014-10-21 01:16:35]

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

[jrummell, 2014-10-21 01:16:36]

PTAL.

[sandersd (OOO until July 31), 2014-10-21 20:42:43]

lgtm

https://codereview.chromium.org/665213002/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
(right):

https://codereview.chromium.org/665213002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:216:
scriptState, DOMException::create(NotSupportedError, "The key system '" +
keySystem + "' is not supported."));
Better to only use |keySystem| after it has been validated to be on our list;
remove it from this message.

[jrummell, 2014-10-22 00:18:56]

Updated, now supports sequence<>

https://codereview.chromium.org/665213002/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
(right):

https://codereview.chromium.org/665213002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:216:
scriptState, DOMException::create(NotSupportedError, "The key system '" +
keySystem + "' is not supported."));
On 2014/10/21 20:42:42, sandersd wrote:
> Better to only use |keySystem| after it has been validated to be on our list;
> remove it from this message.

Done.

[ddorwin, 2014-10-22 22:18:55]

Thanks. Reviewed C++ code only.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySystemAccess.cpp (right):

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:67: Document* document =
toDocument(executionContext());
OOC, should we be saving the execution context in the synchronous part?

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:69:
OwnPtr<WebContentDecryptionModule> cdm =
controller->createContentDecryptionModule(executionContext(), m_keySystem);
Maybe we should modify the creation function to return a cause so we can provide
something other than UnknownError.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:74: String message("A
content decryption module could not be loaded for the '" + m_keySystem + "' key
system.");
I think it's more accurate to say "A CDM instance could not be created for..."

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:91: :
m_keySystem(keySystem)
I think it would be better if the request method created an Access object that
had some Chromium-side equivalent that contained any information, including the
key system, from the results of the request call. That is more consistent with
the spec design. It would also give us a place to put the createCDM() call.
Maybe MediaController should be a member?

This is fine for now, but we should look into this - maybe a FIXME?

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeys.h (right):

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeys.h:53: const String& keySystem() const {
return m_keySystem; }
Do we need this anymore?

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeys.h:60: static bool isTypeSupported(const
String& keySystem, const String& contentType);
FIXME: Remove this.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
File Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
(right):

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:28: //
This class allows a MediaKeySystemAccess object to be created asynchronously.
allows capabilities to be checked and... ?

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:29: class
MediaKeySystemAccessInitializer : public ScriptPromiseResolver {
We should consider somehow avoiding the boilerplate of this Initializer pattern.
Maybe a template. Something to think about.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:76: //   
 DOMException whose name is NotSupportedError.
(Handled by Chromium.)

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:159:
Vector<MediaKeySystemOptions> supportedConfigurations;
nit: Can you do this inline below?

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:206: //
5. Asynchronously create and initialize the MediaKeySystemAccess object.
It's really determine support and ....

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
File Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h
(right):

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:25: static
ScriptPromise requestMediaKeySystemAccess(ScriptState*, Navigator&, const
String& keySystem);
nit: For consistency with the override below, please put the parameters on
separate lines.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:31:
ScriptPromise requestMediaKeySystemAccess(
I think we should probably have a different name rather than overloading with
both static and non-static. This can probably be private too.

nit: Add an empty line after static methods.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:43: // For
the generated code for Navigator to make a Vector<MediaKeySystemOptions>
nit: This is a bit hard to read. At least add a comma at the end of this line.
Also, consider restructuring to begin with "This is needed to...."

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:45: //
MediaKeySystemOptions. MediaKeySystemOptions is also generated code, so
nit: Since MediaKeySystemOptions is also generated code, this is defined here so
that it will be included by the generated code.

Or something like that. Assuming this is correct.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:46: //
including the code here. This needs to change if MediaKeySystemOptions.idl
The need to change this should be called out - probably on its own line and
definitely in the IDL file.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:66:
result.setUniqueidentifier(itemValue);
Web IDL question: What is supposed to happen if an invalid value is used for an
enum? Does it cause an error (InvalidAccessError) or is it ignored?

Implementation question: Where do we check for valid values?

[jrummell, 2014-10-23 01:20:03]

Updated.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySystemAccess.cpp (right):

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:67: Document* document =
toDocument(executionContext());
On 2014/10/22 22:18:54, ddorwin wrote:
> OOC, should we be saving the execution context in the synchronous part?

The execution context is part of ScriptState.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:69:
OwnPtr<WebContentDecryptionModule> cdm =
controller->createContentDecryptionModule(executionContext(), m_keySystem);
On 2014/10/22 22:18:54, ddorwin wrote:
> Maybe we should modify the creation function to return a cause so we can
provide
> something other than UnknownError.

Added FIXME. Also should it be async?

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:74: String message("A
content decryption module could not be loaded for the '" + m_keySystem + "' key
system.");
On 2014/10/22 22:18:54, ddorwin wrote:
> I think it's more accurate to say "A CDM instance could not be created for..."

Done.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:91: :
m_keySystem(keySystem)
On 2014/10/22 22:18:54, ddorwin wrote:
> I think it would be better if the request method created an Access object that
> had some Chromium-side equivalent that contained any information, including
the
> key system, from the results of the request call. That is more consistent with
> the spec design. It would also give us a place to put the createCDM() call.
> Maybe MediaController should be a member?
> 
> This is fine for now, but we should look into this - maybe a FIXME?

Done.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeys.h (right):

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeys.h:53: const String& keySystem() const {
return m_keySystem; }
On 2014/10/22 22:18:54, ddorwin wrote:
> Do we need this anymore?

Yes. MediaKeySession.generateRequest() needs it in step 6 ("If the CDM
corresponding to media keys's keySystem attribute does not support initDataType
..."). Maybe this will go away with the isTypeSupported changes?

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeys.h:60: static bool isTypeSupported(const
String& keySystem, const String& contentType);
On 2014/10/22 22:18:54, ddorwin wrote:
> FIXME: Remove this.

Done.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
File Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
(right):

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:28: //
This class allows a MediaKeySystemAccess object to be created asynchronously.
On 2014/10/22 22:18:54, ddorwin wrote:
> allows capabilities to be checked and... ?

Done.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:29: class
MediaKeySystemAccessInitializer : public ScriptPromiseResolver {
On 2014/10/22 22:18:54, ddorwin wrote:
> We should consider somehow avoiding the boilerplate of this Initializer
pattern.
> Maybe a template. Something to think about.

Acknowledged.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:76: //   
 DOMException whose name is NotSupportedError.
On 2014/10/22 22:18:54, ddorwin wrote:
> (Handled by Chromium.)

Done.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:159:
Vector<MediaKeySystemOptions> supportedConfigurations;
On 2014/10/22 22:18:54, ddorwin wrote:
> nit: Can you do this inline below?

Done.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:206: //
5. Asynchronously create and initialize the MediaKeySystemAccess object.
On 2014/10/22 22:18:54, ddorwin wrote:
> It's really determine support and ....

Done.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
File Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h
(right):

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:25: static
ScriptPromise requestMediaKeySystemAccess(ScriptState*, Navigator&, const
String& keySystem);
On 2014/10/22 22:18:55, ddorwin wrote:
> nit: For consistency with the override below, please put the parameters on
> separate lines.

Done.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:31:
ScriptPromise requestMediaKeySystemAccess(
On 2014/10/22 22:18:54, ddorwin wrote:
> I think we should probably have a different name rather than overloading with
> both static and non-static. This can probably be private too.
> 
> nit: Add an empty line after static methods.

This appears to be a standard pattern for Navigator overrides -- see
modules/battery/NavigatorBattery.h, modules/quota/NavigatorStorageQuota.h, and
others. Added empty line.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:43: // For
the generated code for Navigator to make a Vector<MediaKeySystemOptions>
On 2014/10/22 22:18:55, ddorwin wrote:
> nit: This is a bit hard to read. At least add a comma at the end of this line.
> Also, consider restructuring to begin with "This is needed to...."

Done.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:45: //
MediaKeySystemOptions. MediaKeySystemOptions is also generated code, so
On 2014/10/22 22:18:54, ddorwin wrote:
> nit: Since MediaKeySystemOptions is also generated code, this is defined here
so
> that it will be included by the generated code.
> 
> Or something like that. Assuming this is correct.

Done.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:46: //
including the code here. This needs to change if MediaKeySystemOptions.idl
On 2014/10/22 22:18:55, ddorwin wrote:
> The need to change this should be called out - probably on its own line and
> definitely in the IDL file.

Done.

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:66:
result.setUniqueidentifier(itemValue);
On 2014/10/22 22:18:54, ddorwin wrote:
> Web IDL question: What is supposed to happen if an invalid value is used for
an
> enum? Does it cause an error (InvalidAccessError) or is it ignored?
> 
> Implementation question: Where do we check for valid values?

Generated code for MediaKeySystemOptions has all fields as String with no
validation (although constructor does set them to the default value). Tried
passing in { stateful: 'nope' } from JS console, and no errors.

Added trivial validation.

[ddorwin, 2014-10-23 18:02:31]

LGTM with comments. Thanks!

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
File Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h
(right):

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:25: static
ScriptPromise requestMediaKeySystemAccess(ScriptState*, Navigator&, const
String& keySystem);
On 2014/10/23 01:20:03, jrummell wrote:
> On 2014/10/22 22:18:55, ddorwin wrote:
> > nit: For consistency with the override below, please put the parameters on
> > separate lines.
> 
> Done.

s/override/overload/, but you got the idea. :)

https://codereview.chromium.org/665213002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:31:
ScriptPromise requestMediaKeySystemAccess(
On 2014/10/23 01:20:03, jrummell wrote:
> On 2014/10/22 22:18:54, ddorwin wrote:
> > I think we should probably have a different name rather than overloading
with
> > both static and non-static. This can probably be private too.
> > 
> > nit: Add an empty line after static methods.
> 
> This appears to be a standard pattern for Navigator overrides -- see
> modules/battery/NavigatorBattery.h, modules/quota/NavigatorStorageQuota.h, and
> others. Added empty line.

Acknowledged.

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySystemOptions.idl (right):

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemOptions.idl:7: // Note that the code
for NativeValueTraits<MediaKeySystemOptions> in
nit: This could be a little more concise. For example,
XXX in YYY must also be updated when modifying this definition.

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemOptions.idl:8: //
NavigatorRequestMediaKeySystemAccess.h needs to be updated if this
I think this needs to be updated to say
ConvertDictionaryIntoMediaKeySystemOptions() in the .cpp file.

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeys.h (right):

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeys.h:53: const String& keySystem() const {
return m_keySystem; }
FIXME: This should be removed after crbug.com/425186 is fully implemented.
WDYT?

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
File Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
(right):

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:29: const
char* mediaKeysRequirementValidValues[] = { "required", "optional", "disallowed"
};
Okay for now, I guess. We'll probably need to convert these to an enum.

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:31:
static bool ValidMediaKeysRequirement(const String& itemValue)
IsV...

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:254: //
5. Asynchronously determine support and if allowed create and
nit: missing commas

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
File Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h
(right):

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:46: // This
is needed to convert a JavaScript object into a MediaKeySystemOptions
nit: How about this?
... needed by the generated code for Navigator to convert...

[jrummell, 2014-10-23 22:13:15]

jrummell@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[jrummell, 2014-10-23 22:13:16]

Updated.

+oilpan-reviews for Oilpan check.

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySystemOptions.idl (right):

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemOptions.idl:7: // Note that the code
for NativeValueTraits<MediaKeySystemOptions> in
On 2014/10/23 18:02:30, ddorwin wrote:
> nit: This could be a little more concise. For example,
> XXX in YYY must also be updated when modifying this definition.

Done.

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemOptions.idl:8: //
NavigatorRequestMediaKeySystemAccess.h needs to be updated if this
On 2014/10/23 18:02:30, ddorwin wrote:
> I think this needs to be updated to say
> ConvertDictionaryIntoMediaKeySystemOptions() in the .cpp file.

Done.

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeys.h (right):

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeys.h:53: const String& keySystem() const {
return m_keySystem; }
On 2014/10/23 18:02:31, ddorwin wrote:
> FIXME: This should be removed after crbug.com/425186 is fully implemented.
> WDYT?

Done.

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
File Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
(right):

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:29: const
char* mediaKeysRequirementValidValues[] = { "required", "optional", "disallowed"
};
On 2014/10/23 18:02:31, ddorwin wrote:
> Okay for now, I guess. We'll probably need to convert these to an enum.

Acknowledged.

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:31:
static bool ValidMediaKeysRequirement(const String& itemValue)
On 2014/10/23 18:02:31, ddorwin wrote:
> IsV...

Done.

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:254: //
5. Asynchronously determine support and if allowed create and
On 2014/10/23 18:02:31, ddorwin wrote:
> nit: missing commas

Done.

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
File Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h
(right):

https://codereview.chromium.org/665213002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:46: // This
is needed to convert a JavaScript object into a MediaKeySystemOptions
On 2014/10/23 18:02:31, ddorwin wrote:
> nit: How about this?
> ... needed by the generated code for Navigator to convert...

Done.

[ddorwin, 2014-10-23 23:32:01]

I skimmed the tests. LGTM with:
a) We may need to introduce some checks. WDYT?
b) Please add the tests we need to add to the BUG.

https://codereview.chromium.org/665213002/diff/40001/LayoutTests/media/encryp...
File
LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys-with-session.html
(right):

https://codereview.chromium.org/665213002/diff/40001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys-with-session.html:58:
// Ensure that MediaKeySession (but not PromiseResolver) is
Was the removed line necessary to test validity? If so, maybe we should replace
it with a typeof == function. WDYT? The same probably applies elsewhere.

https://codereview.chromium.org/665213002/diff/40001/LayoutTests/media/encryp...
File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html (right):

https://codereview.chromium.org/665213002/diff/40001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:132: // Test
creation of a second MediaKeys.
We should probably add tests to test the creation of multiple access objects.
Maybe add this to the BUG.

We'll also need more request() and Access object tests, including lifetime.

[haraken, 2014-10-24 01:06:41]

haraken@chromium.org changed reviewers:
+ bashi@chromium.org, haraken@chromium.org, yhirano@chromium.org

[haraken, 2014-10-24 01:06:42]

Oilpan part LGTM

yhirano: Would you take a look at Promise changes?

bashi: FYI for Dictionary uses.

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySystemAccess.cpp (right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:22: class
MediaKeysInitializer : public ScriptPromiseResolver {

Add final.

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySystemAccess.h (right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.h:14: class
MediaKeySystemAccess : public GarbageCollectedFinalized<MediaKeySystemAccess>,
public ScriptWrappable {

Add final.

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.h:18:
MediaKeySystemAccess(const String& keySystem);

Add explicit.

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
(right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:78: class
MediaKeySystemAccessInitializer : public ScriptPromiseResolver {

Add final.

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h
(right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:18: class
NavigatorRequestMediaKeySystemAccess : public
NoBaseWillBeGarbageCollected<NavigatorRequestMediaKeySystemAccess>, public
WillBeHeapSupplement<Navigator> {

Add final.

[bashi, 2014-10-24 01:15:15]

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h
(right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:54:
Dictionary dictionary(value, isolate);
You can remove ConvertDictionaryIntoMediaKeySystemOptions.

MediaKeySystemOptions impl;
V8MediaKeySystemOptions::toImpl(isolate, value, impl, exceptionState);
return impl;

You will need to include "bindings/modules/v8/V8MediaKeySystemOptions.h".

[yhirano, 2014-10-24 09:22:33]

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySystemAccess.cpp (right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:21: // This class allows
a MediaKeys object to be created asynchronously.
Can you place this class in an unnamed namespace?

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:51: // Start the timer so
that MediaKeys can be created asynchronously.
I'm sorry if this is the second time, but can you tell me why you want to create
MediaKeys asynchronously? Even if you create it synchronously (in terms of blink
C++), fulfill / reject handlers will be called asynchronously (in the JS world).

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeys.h (right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeys.h:53: // FIXME: This should be removed
after crbug.com/425186 is fully implemented.
Please wrap the comment in 80 columns.

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
(right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:78: class
MediaKeySystemAccessInitializer : public ScriptPromiseResolver {
Please place this class in an unnamed namespace.

[jrummell, 2014-10-24 22:43:23]

PS#5 is the changes, PS#6 just removes the reference on updating
ConvertDictionaryIntoMediaKeySystemOptions() which no longer exists.

https://codereview.chromium.org/665213002/diff/40001/LayoutTests/media/encryp...
File
LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys-with-session.html
(right):

https://codereview.chromium.org/665213002/diff/40001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys-with-session.html:58:
// Ensure that MediaKeySession (but not PromiseResolver) is
On 2014/10/23 23:32:01, ddorwin wrote:
> Was the removed line necessary to test validity? If so, maybe we should
replace
> it with a typeof == function. WDYT? The same probably applies elsewhere.

Done.

https://codereview.chromium.org/665213002/diff/40001/LayoutTests/media/encryp...
File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html (right):

https://codereview.chromium.org/665213002/diff/40001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:132: // Test
creation of a second MediaKeys.
On 2014/10/23 23:32:01, ddorwin wrote:
> We should probably add tests to test the creation of multiple access objects.
> Maybe add this to the BUG.
> 
> We'll also need more request() and Access object tests, including lifetime.

Added to bug.

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySystemAccess.cpp (right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:21: // This class allows
a MediaKeys object to be created asynchronously.
On 2014/10/24 09:22:32, yhirano wrote:
> Can you place this class in an unnamed namespace?

Only by removing the logging. Done.

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:22: class
MediaKeysInitializer : public ScriptPromiseResolver {
On 2014/10/24 01:06:42, haraken wrote:
> 
> Add final.

Done.

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:51: // Start the timer so
that MediaKeys can be created asynchronously.
On 2014/10/24 09:22:32, yhirano wrote:
> I'm sorry if this is the second time, but can you tell me why you want to
create
> MediaKeys asynchronously? Even if you create it synchronously (in terms of
blink
> C++), fulfill / reject handlers will be called asynchronously (in the JS
world).

createContentDecryptionModule() may require the CDM to be loaded, which is why
we don't want to run synchronously.

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySystemAccess.h (right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.h:14: class
MediaKeySystemAccess : public GarbageCollectedFinalized<MediaKeySystemAccess>,
public ScriptWrappable {
On 2014/10/24 01:06:42, haraken wrote:
> 
> Add final.

Done.

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.h:18:
MediaKeySystemAccess(const String& keySystem);
On 2014/10/24 01:06:42, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeys.h (right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeys.h:53: // FIXME: This should be removed
after crbug.com/425186 is fully implemented.
On 2014/10/24 09:22:32, yhirano wrote:
> Please wrap the comment in 80 columns.

Done.

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp
(right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:78: class
MediaKeySystemAccessInitializer : public ScriptPromiseResolver {
On 2014/10/24 01:06:42, haraken wrote:
> 
> Add final.

Done.

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:78: class
MediaKeySystemAccessInitializer : public ScriptPromiseResolver {
On 2014/10/24 09:22:32, yhirano wrote:
> Please place this class in an unnamed namespace.

Done.

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h
(right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:18: class
NavigatorRequestMediaKeySystemAccess : public
NoBaseWillBeGarbageCollected<NavigatorRequestMediaKeySystemAccess>, public
WillBeHeapSupplement<Navigator> {
On 2014/10/24 01:06:42, haraken wrote:
> 
> Add final.

Done.

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.h:54:
Dictionary dictionary(value, isolate);
On 2014/10/24 01:15:15, bashi1 wrote:
> You can remove ConvertDictionaryIntoMediaKeySystemOptions.
> 
> MediaKeySystemOptions impl;
> V8MediaKeySystemOptions::toImpl(isolate, value, impl, exceptionState);
> return impl;
> 
> You will need to include "bindings/modules/v8/V8MediaKeySystemOptions.h".

Great. It even does the enum value checking.

[ddorwin, 2014-10-24 23:08:14]

A few comments in the tests. Otherwise, LGTM.

https://codereview.chromium.org/665213002/diff/60001/LayoutTests/media/encryp...
File
LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys-with-session.html
(right):

https://codereview.chromium.org/665213002/diff/60001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys-with-session.html:58:
// Ensure that MediaKeySession (but not PromiseResolver) is
assert_equals(typeof result.createSession, 'function');
?

https://codereview.chromium.org/665213002/diff/60001/LayoutTests/media/encryp...
File
LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html
(right):

https://codereview.chromium.org/665213002/diff/60001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html:39:

ditto

https://codereview.chromium.org/665213002/diff/60001/LayoutTests/media/encryp...
File
LayoutTests/media/encrypted-media/encrypted-media-lifetime-multiple-mediakeys.html
(right):

https://codereview.chromium.org/665213002/diff/60001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-lifetime-multiple-mediakeys.html:110:

ditto

https://codereview.chromium.org/665213002/diff/60001/LayoutTests/media/encryp...
File LayoutTests/media/encrypted-media/encrypted-media-setmediakeys.html
(right):

https://codereview.chromium.org/665213002/diff/60001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-setmediakeys.html:40:
assert_equals(access.keySystem, 'org.w3.clearkey');
nit: Since we're not really testing request() here, we probably don't need
these. WDYT?

Also, line 59

https://codereview.chromium.org/665213002/diff/60001/LayoutTests/media/encryp...
File
LayoutTests/media/encrypted-media/encrypted-media-unprefixed-after-prefixed.html
(right):

https://codereview.chromium.org/665213002/diff/60001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-unprefixed-after-prefixed.html:23:
assert_equals(access.keySystem, 'org.w3.clearkey');
ditto

[ddorwin, 2014-10-24 23:32:12]

On 2014/10/24 23:08:14, ddorwin wrote:
> A few comments in the tests. Otherwise, LGTM.
Oops, I accidentally looked at a mix of PS4 and PS6. PS6 LGTM.

[jrummell, 2014-10-25 00:39:02]

Adding LayoutTests/webexposed/global-constructors-listing-expected.txt since the
trybots failed on this test.

[yhirano, 2014-10-27 02:26:28]

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySystemAccess.cpp (right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:51: // Start the timer so
that MediaKeys can be created asynchronously.
On 2014/10/24 22:43:22, jrummell wrote:
> On 2014/10/24 09:22:32, yhirano wrote:
> > I'm sorry if this is the second time, but can you tell me why you want to
> create
> > MediaKeys asynchronously? Even if you create it synchronously (in terms of
> blink
> > C++), fulfill / reject handlers will be called asynchronously (in the JS
> world).
> 
> createContentDecryptionModule() may require the CDM to be loaded, which is why
> we don't want to run synchronously.

Sorry I still don't understand...
1) What do you mean by "may"? I checked layout tests passed even when I remove
this timer.
2) If you are waiting for an event ("CDM gets loaded"?) here, is it guaranteed
that it's done when the timer fires? How?

[jrummell, 2014-10-27 21:23:28]

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySystemAccess.cpp (right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:51: // Start the timer so
that MediaKeys can be created asynchronously.
On 2014/10/27 02:26:28, yhirano wrote:
> On 2014/10/24 22:43:22, jrummell wrote:
> > On 2014/10/24 09:22:32, yhirano wrote:
> > > I'm sorry if this is the second time, but can you tell me why you want to
> > create
> > > MediaKeys asynchronously? Even if you create it synchronously (in terms of
> > blink
> > > C++), fulfill / reject handlers will be called asynchronously (in the JS
> > world).
> > 
> > createContentDecryptionModule() may require the CDM to be loaded, which is
why
> > we don't want to run synchronously.
> 
> Sorry I still don't understand...
> 1) What do you mean by "may"? I checked layout tests passed even when I remove
> this timer.
> 2) If you are waiting for an event ("CDM gets loaded"?) here, is it guaranteed
> that it's done when the timer fires? How?
> 

The layout tests all use ClearKey CDM, which is built in to Chrome (thus nothing
to load). Most applications (e.g YouTube encrypted content) will use WideVine
CDM, which is loaded like a plug-in.

createContentDecryptionModule() currently runs synchronously, so there is no
"CDM gets loaded" event.

Right now, I'm just trying to follow the current spec
(https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-...).

[yhirano, 2014-10-28 02:02:01]

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySystemAccess.cpp (right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:51: // Start the timer so
that MediaKeys can be created asynchronously.
On 2014/10/27 21:23:28, jrummell wrote:
> On 2014/10/27 02:26:28, yhirano wrote:
> > On 2014/10/24 22:43:22, jrummell wrote:
> > > On 2014/10/24 09:22:32, yhirano wrote:
> > > > I'm sorry if this is the second time, but can you tell me why you want
to
> > > create
> > > > MediaKeys asynchronously? Even if you create it synchronously (in terms
of
> > > blink
> > > > C++), fulfill / reject handlers will be called asynchronously (in the JS
> > > world).
> > > 
> > > createContentDecryptionModule() may require the CDM to be loaded, which is
> why
> > > we don't want to run synchronously.
> > 
> > Sorry I still don't understand...
> > 1) What do you mean by "may"? I checked layout tests passed even when I
remove
> > this timer.
> > 2) If you are waiting for an event ("CDM gets loaded"?) here, is it
guaranteed
> > that it's done when the timer fires? How?
> > 
> 
> The layout tests all use ClearKey CDM, which is built in to Chrome (thus
nothing
> to load). Most applications (e.g YouTube encrypted content) will use WideVine
> CDM, which is loaded like a plug-in.
> 
> createContentDecryptionModule() currently runs synchronously, so there is no
> "CDM gets loaded" event.
> 
> Right now, I'm just trying to follow the current spec
>
(https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-...).
> 

OK, then please write a comment describing that here (and in
NavigatorRequestMediaKeySystemAccess.cpp).
When createContentDecryptionModule gets asynchronous, you will have to wait for
the event, not a timer event, right? If so, please write so as well.

[jrummell, 2014-10-28 17:45:44]

Added FIXME.

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySystemAccess.cpp (right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:51: // Start the timer so
that MediaKeys can be created asynchronously.
On 2014/10/28 02:02:01, yhirano wrote:
> On 2014/10/27 21:23:28, jrummell wrote:
> > On 2014/10/27 02:26:28, yhirano wrote:
> > > On 2014/10/24 22:43:22, jrummell wrote:
> > > > On 2014/10/24 09:22:32, yhirano wrote:
> > > > > I'm sorry if this is the second time, but can you tell me why you want
> to
> > > > create
> > > > > MediaKeys asynchronously? Even if you create it synchronously (in
terms
> of
> > > > blink
> > > > > C++), fulfill / reject handlers will be called asynchronously (in the
JS
> > > > world).
> > > > 
> > > > createContentDecryptionModule() may require the CDM to be loaded, which
is
> > why
> > > > we don't want to run synchronously.
> > > 
> > > Sorry I still don't understand...
> > > 1) What do you mean by "may"? I checked layout tests passed even when I
> remove
> > > this timer.
> > > 2) If you are waiting for an event ("CDM gets loaded"?) here, is it
> guaranteed
> > > that it's done when the timer fires? How?
> > > 
> > 
> > The layout tests all use ClearKey CDM, which is built in to Chrome (thus
> nothing
> > to load). Most applications (e.g YouTube encrypted content) will use
WideVine
> > CDM, which is loaded like a plug-in.
> > 
> > createContentDecryptionModule() currently runs synchronously, so there is no
> > "CDM gets loaded" event.
> > 
> > Right now, I'm just trying to follow the current spec
> >
>
(https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-...).
> > 
> 
> OK, then please write a comment describing that here (and in
> NavigatorRequestMediaKeySystemAccess.cpp).
> When createContentDecryptionModule gets asynchronous, you will have to wait
for
> the event, not a timer event, right? If so, please write so as well.

Done.

[yhirano, 2014-10-29 01:44:26]

Thanks, lgtm

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySystemAccess.cpp (right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:51: // Start the timer so
that MediaKeys can be created asynchronously.
On 2014/10/28 17:45:44, jrummell wrote:
> On 2014/10/28 02:02:01, yhirano wrote:
> > On 2014/10/27 21:23:28, jrummell wrote:
> > > On 2014/10/27 02:26:28, yhirano wrote:
> > > > On 2014/10/24 22:43:22, jrummell wrote:
> > > > > On 2014/10/24 09:22:32, yhirano wrote:
> > > > > > I'm sorry if this is the second time, but can you tell me why you
want
> > to
> > > > > create
> > > > > > MediaKeys asynchronously? Even if you create it synchronously (in
> terms
> > of
> > > > > blink
> > > > > > C++), fulfill / reject handlers will be called asynchronously (in
the
> JS
> > > > > world).
> > > > > 
> > > > > createContentDecryptionModule() may require the CDM to be loaded,
which
> is
> > > why
> > > > > we don't want to run synchronously.
> > > > 
> > > > Sorry I still don't understand...
> > > > 1) What do you mean by "may"? I checked layout tests passed even when I
> > remove
> > > > this timer.
> > > > 2) If you are waiting for an event ("CDM gets loaded"?) here, is it
> > guaranteed
> > > > that it's done when the timer fires? How?
> > > > 
> > > 
> > > The layout tests all use ClearKey CDM, which is built in to Chrome (thus
> > nothing
> > > to load). Most applications (e.g YouTube encrypted content) will use
> WideVine
> > > CDM, which is loaded like a plug-in.
> > > 
> > > createContentDecryptionModule() currently runs synchronously, so there is
no
> > > "CDM gets loaded" event.
> > > 
> > > Right now, I'm just trying to follow the current spec
> > >
> >
>
(https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-...).
> > > 
> > 
> > OK, then please write a comment describing that here (and in
> > NavigatorRequestMediaKeySystemAccess.cpp).
> > When createContentDecryptionModule gets asynchronous, you will have to wait
> for
> > the event, not a timer event, right? If so, please write so as well.
> 
> Done.

As wrote above, please write in NavigatorRequestMediaKeySystemAccess.cpp, too.

[jrummell, 2014-10-29 18:24:43]

Thanks for the reviews.

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySystemAccess.cpp (right):

https://codereview.chromium.org/665213002/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySystemAccess.cpp:51: // Start the timer so
that MediaKeys can be created asynchronously.
On 2014/10/29 01:44:26, yhirano wrote:
> On 2014/10/28 17:45:44, jrummell wrote:
> > On 2014/10/28 02:02:01, yhirano wrote:
> > > On 2014/10/27 21:23:28, jrummell wrote:
> > > > On 2014/10/27 02:26:28, yhirano wrote:
> > > > > On 2014/10/24 22:43:22, jrummell wrote:
> > > > > > On 2014/10/24 09:22:32, yhirano wrote:
> > > > > > > I'm sorry if this is the second time, but can you tell me why you
> want
> > > to
> > > > > > create
> > > > > > > MediaKeys asynchronously? Even if you create it synchronously (in
> > terms
> > > of
> > > > > > blink
> > > > > > > C++), fulfill / reject handlers will be called asynchronously (in
> the
> > JS
> > > > > > world).
> > > > > > 
> > > > > > createContentDecryptionModule() may require the CDM to be loaded,
> which
> > is
> > > > why
> > > > > > we don't want to run synchronously.
> > > > > 
> > > > > Sorry I still don't understand...
> > > > > 1) What do you mean by "may"? I checked layout tests passed even when
I
> > > remove
> > > > > this timer.
> > > > > 2) If you are waiting for an event ("CDM gets loaded"?) here, is it
> > > guaranteed
> > > > > that it's done when the timer fires? How?
> > > > > 
> > > > 
> > > > The layout tests all use ClearKey CDM, which is built in to Chrome (thus
> > > nothing
> > > > to load). Most applications (e.g YouTube encrypted content) will use
> > WideVine
> > > > CDM, which is loaded like a plug-in.
> > > > 
> > > > createContentDecryptionModule() currently runs synchronously, so there
is
> no
> > > > "CDM gets loaded" event.
> > > > 
> > > > Right now, I'm just trying to follow the current spec
> > > >
> > >
> >
>
(https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-...).
> > > > 
> > > 
> > > OK, then please write a comment describing that here (and in
> > > NavigatorRequestMediaKeySystemAccess.cpp).
> > > When createContentDecryptionModule gets asynchronous, you will have to
wait
> > for
> > > the event, not a timer event, right? If so, please write so as well.
> > 
> > Done.
> 
> As wrote above, please write in NavigatorRequestMediaKeySystemAccess.cpp, too.

Done.

[jrummell, 2014-10-29 18:24:50]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-10-29 18:25:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/665213002/160001

[commit-bot: I haz the power, 2014-10-29 19:27:17]

Committed patchset #9 (id:160001) as 184599