Schedule session initialization in MediaKeys.

Schedule session initialization in MediaKeys.

BUG=224786
TEST=none

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

[xhwang, 2014-01-11 02:25:59]

PTAL

[acolwell GONE FROM CHROMIUM, 2014-01-13 18:35:36]

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

https://codereview.chromium.org/132823005/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:82: void
MediaKeySession::initializeNewSession(const String& mimeType, Uint8Array*
initData)
nit: Make initData const Uint8Array& here too so this code doesn't have to worry
about null pointers.

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

https://codereview.chromium.org/132823005/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:113:
m_initializeNewSessionTimer.startOneShot(0);
Shouldn't this have a if (!m_initializeNewSessionTimer.isActive()) guard?

https://codereview.chromium.org/132823005/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/MediaKeys.h (right):

https://codereview.chromium.org/132823005/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.h:31: #include
"modules/encryptedmedia/MediaKeySession.h"
Why do you need to include this now?

https://codereview.chromium.org/132823005/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.h:80:
InitializeNewSessionData(PassRefPtr<MediaKeySession> session, const String&
mimeType, Uint8Array* initData)
Use PassRefPtr<Uint8Array> for initData.

[xhwang, 2014-01-13 19:05:46]

comments addressed

[xhwang, 2014-01-13 19:06:02]

PTAL again

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

https://codereview.chromium.org/132823005/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:82: void
MediaKeySession::initializeNewSession(const String& mimeType, Uint8Array*
initData)
On 2014/01/13 18:35:37, acolwell wrote:
> nit: Make initData const Uint8Array& here too so this code doesn't have to
worry
> about null pointers.

Done.

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

https://codereview.chromium.org/132823005/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:113:
m_initializeNewSessionTimer.startOneShot(0);
On 2014/01/13 18:35:37, acolwell wrote:
> Shouldn't this have a if (!m_initializeNewSessionTimer.isActive()) guard?

Done. Good to learn! Thanks!

https://codereview.chromium.org/132823005/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/MediaKeys.h (right):

https://codereview.chromium.org/132823005/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.h:31: #include
"modules/encryptedmedia/MediaKeySession.h"
On 2014/01/13 18:35:37, acolwell wrote:
> Why do you need to include this now?

For the template parameter on l.80.

https://codereview.chromium.org/132823005/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.h:80:
InitializeNewSessionData(PassRefPtr<MediaKeySession> session, const String&
mimeType, Uint8Array* initData)
On 2014/01/13 18:35:37, acolwell wrote:
> Use PassRefPtr<Uint8Array> for initData.

Done.

[xhwang, 2014-01-13 19:24:03]

comments addressed

[acolwell GONE FROM CHROMIUM, 2014-01-13 19:37:05]

lgtm % nit

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:82: void
MediaKeySession::initializeNewSession(const String& mimeType, Uint8Array&
initData)
nit: make const&

[ddorwin, 2014-01-13 20:20:56]

Code comments in PS3. Test comments in PS4.

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:101:
m_pendingKeys.append(response);
m_pendingUpdate[Message]s?

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeySession.h (right):

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.h:67: void
initializeNewSession(const String& mimeType, Uint8Array& initData);
const &?

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.cpp (right):

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.cpp:84: PassRefPtr<MediaKeySession>
MediaKeys::createSession(ExecutionContext* context, const String& type,
Uint8Array* initData, ExceptionState& exceptionState)
type is now contentType

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.cpp:91:
exceptionState.throwDOMException(InvalidAccessError, "The type or initData
provided is empty or null.");
I wonder what is the best practice for these strings.

It seems odd that we combine two parameters in the exception message. I wonder
if we should separate them.

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.h (right):

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.h:82: , mimeType(mimeType)
contentType now

https://codereview.chromium.org/132823005/diff/290003/LayoutTests/media/encry...
File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax-expected.txt
(right):

https://codereview.chromium.org/132823005/diff/290003/LayoutTests/media/encry...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax-expected.txt:11:
TEST(mediaKeys.createSession("")) THROWS(DOMException.INVALID_ACCESS_ERR: Failed
to execute 'createSession' on 'MediaKeys': The type or initData provided is
empty or null.) OK
MediaKeys.idl needs to be updated. This should be a SYNTAX_ERR because of too
few parameters.
We should also add a test for createSession().

https://codereview.chromium.org/132823005/diff/290003/LayoutTests/media/encry...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax-expected.txt:12:
TEST(mediaKeys.createSession("", new Uint8Array(1)))
THROWS(DOMException.INVALID_ACCESS_ERR: Failed to execute 'createSession' on
'MediaKeys': The type or initData provided is empty or null.) OK
Test for null too.

https://codereview.chromium.org/132823005/diff/290003/LayoutTests/media/encry...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax-expected.txt:13:
TEST(mediaKeys.createSession("video/webm", null))
THROWS(DOMException.INVALID_ACCESS_ERR: Failed to execute 'createSession' on
'MediaKeys': The type or initData provided is empty or null.) OK
We can also check for an empty initData array.

https://codereview.chromium.org/132823005/diff/290003/LayoutTests/media/encry...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax-expected.txt:27:
TEST(mediaKeySession.update(null)) THROWS(DOMException.INVALID_ACCESS_ERR:
Failed to execute 'update' on 'MediaKeySession': The response argument provided
is invalid.) OK
Include a valid test for update()?

https://codereview.chromium.org/132823005/diff/290003/LayoutTests/media/encry...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax-expected.txt:28: END
OF TEST
test release()

[xhwang, 2014-01-14 04:04:11]

PTAL again

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:82: void
MediaKeySession::initializeNewSession(const String& mimeType, Uint8Array&
initData)
On 2014/01/13 19:37:05, acolwell wrote:
> nit: make const&

Done.

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:101:
m_pendingKeys.append(response);
On 2014/01/13 20:20:57, ddorwin wrote:
> m_pendingUpdate[Message]s?

Done.

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeySession.h (right):

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.h:67: void
initializeNewSession(const String& mimeType, Uint8Array& initData);
On 2014/01/13 20:20:57, ddorwin wrote:
> const &?

Done.

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.cpp (right):

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.cpp:84: PassRefPtr<MediaKeySession>
MediaKeys::createSession(ExecutionContext* context, const String& type,
Uint8Array* initData, ExceptionState& exceptionState)
On 2014/01/13 20:20:57, ddorwin wrote:
> type is now contentType

Done.

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.cpp:91:
exceptionState.throwDOMException(InvalidAccessError, "The type or initData
provided is empty or null.");
On 2014/01/13 20:20:57, ddorwin wrote:
> I wonder what is the best practice for these strings.
> 
> It seems odd that we combine two parameters in the exception message. I wonder
> if we should separate them.

Done.

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.h (right):

https://codereview.chromium.org/132823005/diff/260001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.h:82: , mimeType(mimeType)
On 2014/01/13 20:20:57, ddorwin wrote:
> contentType now

Done.

https://codereview.chromium.org/132823005/diff/290003/LayoutTests/media/encry...
File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax-expected.txt
(right):

https://codereview.chromium.org/132823005/diff/290003/LayoutTests/media/encry...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax-expected.txt:11:
TEST(mediaKeys.createSession("")) THROWS(DOMException.INVALID_ACCESS_ERR: Failed
to execute 'createSession' on 'MediaKeys': The type or initData provided is
empty or null.) OK
On 2014/01/13 20:20:57, ddorwin wrote:
> MediaKeys.idl needs to be updated. This should be a SYNTAX_ERR because of too
> few parameters.
> We should also add a test for createSession().

Done.

https://codereview.chromium.org/132823005/diff/290003/LayoutTests/media/encry...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax-expected.txt:12:
TEST(mediaKeys.createSession("", new Uint8Array(1)))
THROWS(DOMException.INVALID_ACCESS_ERR: Failed to execute 'createSession' on
'MediaKeys': The type or initData provided is empty or null.) OK
On 2014/01/13 20:20:57, ddorwin wrote:
> Test for null too.

Done.

https://codereview.chromium.org/132823005/diff/290003/LayoutTests/media/encry...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax-expected.txt:13:
TEST(mediaKeys.createSession("video/webm", null))
THROWS(DOMException.INVALID_ACCESS_ERR: Failed to execute 'createSession' on
'MediaKeys': The type or initData provided is empty or null.) OK
On 2014/01/13 20:20:57, ddorwin wrote:
> We can also check for an empty initData array.

Done.

https://codereview.chromium.org/132823005/diff/290003/LayoutTests/media/encry...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax-expected.txt:27:
TEST(mediaKeySession.update(null)) THROWS(DOMException.INVALID_ACCESS_ERR:
Failed to execute 'update' on 'MediaKeySession': The response argument provided
is invalid.) OK
On 2014/01/13 20:20:57, ddorwin wrote:
> Include a valid test for update()?

Done.

https://codereview.chromium.org/132823005/diff/290003/LayoutTests/media/encry...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax-expected.txt:28: END
OF TEST
On 2014/01/13 20:20:57, ddorwin wrote:
> test release()

Added FIXME.

[xhwang, 2014-01-15 01:33:07]

https://codereview.chromium.org/132823005/diff/350001/LayoutTests/media/encry...
File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html (right):

https://codereview.chromium.org/132823005/diff/350001/LayoutTests/media/encry...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:55:
testExpected('mediaKeySession.error', null);
Actually this doesn't work. We need to wait for the error event to check the
error attribute. I'll probably just drop this check here.

[ddorwin, 2014-01-15 01:41:01]

lgtm % comments

https://codereview.chromium.org/132823005/diff/350001/LayoutTests/media/encry...
File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html (right):

https://codereview.chromium.org/132823005/diff/350001/LayoutTests/media/encry...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:55:
testExpected('mediaKeySession.error', null);
On 2014/01/15 01:33:08, xhwang wrote:
> Actually this doesn't work. We need to wait for the error event to check the
> error attribute. I'll probably just drop this check here.

Should it really be null or is that just the current state? Since this is just
syntax, we can probably drop the error checking.

https://codereview.chromium.org/132823005/diff/350001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.idl (right):

https://codereview.chromium.org/132823005/diff/350001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.idl:31: [CallWith=ExecutionContext,
RaisesException] MediaKeySession createSession([Default=Undefined] DOMString
type, [Default=Undefined] Uint8Array initData);
Do we we need the Default part now that it's not optional?

[xhwang, 2014-01-15 02:14:48]

https://codereview.chromium.org/132823005/diff/530001/LayoutTests/media/encry...
File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html (right):

https://codereview.chromium.org/132823005/diff/530001/LayoutTests/media/encry...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:61:
waitForEvent('webkitkeyerror', checkError, true, true, mediaKeySession);
FYI, this works. But as you mentioned, this should be in another test. So I'll
remove them in the next patch set.

[xhwang, 2014-01-15 02:22:21]

https://codereview.chromium.org/132823005/diff/350001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.idl (right):

https://codereview.chromium.org/132823005/diff/350001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.idl:31: [CallWith=ExecutionContext,
RaisesException] MediaKeySession createSession([Default=Undefined] DOMString
type, [Default=Undefined] Uint8Array initData);
On 2014/01/15 01:41:01, ddorwin wrote:
> Do we we need the Default part now that it's not optional?

Done.

[commit-bot: I haz the power, 2014-01-15 02:28:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/132823005/500008

[ddorwin, 2014-01-15 02:30:30]

LGTM. One nit renaming.

https://codereview.chromium.org/132823005/diff/500008/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.idl (right):

https://codereview.chromium.org/132823005/diff/500008/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.idl:31: [CallWith=ExecutionContext,
RaisesException] MediaKeySession createSession(DOMString type, Uint8Array
initData);
It probably doesn't matter, but we should probably update |type| to
|contentType| too. That's the name for now, at least, and that makes consistent
with the actual code.

[commit-bot: I haz the power, 2014-01-15 15:43:34]

Change committed as 165137