Implement MediaKeySession.load()

Implement MediaKeySession.load()

BUG=338830
TEST=updated EME tests pass

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

[jrummell, 2014-10-08 00:34:38]

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

[jrummell, 2014-10-08 00:34:39]

PTAL.

[sandersd (OOO until July 31), 2014-10-08 18:18:22]

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

https://codereview.chromium.org/641433002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:56: // TODO Remove once
close/remove CL landed.
More specifically? (What is that CL doing that relates to this?)

https://codereview.chromium.org/641433002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:88: static bool
ValidSessionId(const String& sessionId)
isValidSessionId

https://codereview.chromium.org/641433002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:668: // TODO once expiration
change merged in.
I don't recall an expiration CL, perhaps this should just say "TODO: Implement
expiration."

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

https://codereview.chromium.org/641433002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:121: void finishLoadSession();
finishLoad() to be consistent?

https://codereview.chromium.org/641433002/diff/1/public/platform/WebContentDe...
File public/platform/WebContentDecryptionModuleSession.h (right):

https://codereview.chromium.org/641433002/diff/1/public/platform/WebContentDe...
public/platform/WebContentDecryptionModuleSession.h:74: virtual void
loadSession(const WebString& sessionId, WebContentDecryptionModuleResult);
load() to match update/release?

[jrummell, 2014-10-08 21:13:21]

Updated.

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

https://codereview.chromium.org/641433002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:56: // TODO Remove once
close/remove CL landed.
On 2014/10/08 18:18:22, sandersd wrote:
> More specifically? (What is that CL doing that relates to this?)

Removed TODO. Will get merge conflicts, this was just to remind me to keep the
other one.

https://codereview.chromium.org/641433002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:88: static bool
ValidSessionId(const String& sessionId)
On 2014/10/08 18:18:22, sandersd wrote:
> isValidSessionId

Done.

https://codereview.chromium.org/641433002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:668: // TODO once expiration
change merged in.
On 2014/10/08 18:18:22, sandersd wrote:
> I don't recall an expiration CL, perhaps this should just say "TODO: Implement
> expiration."

The other CL is https://codereview.chromium.org/621903003/. However, made more
generic (and a FIXME since that is what blink uses).

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

https://codereview.chromium.org/641433002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:121: void finishLoadSession();
On 2014/10/08 18:18:22, sandersd wrote:
> finishLoad() to be consistent?

Done.

https://codereview.chromium.org/641433002/diff/1/public/platform/WebContentDe...
File public/platform/WebContentDecryptionModuleSession.h (right):

https://codereview.chromium.org/641433002/diff/1/public/platform/WebContentDe...
public/platform/WebContentDecryptionModuleSession.h:74: virtual void
loadSession(const WebString& sessionId, WebContentDecryptionModuleResult);
On 2014/10/08 18:18:22, sandersd wrote:
> load() to match update/release?

Done.

[sandersd (OOO until July 31), 2014-10-08 21:38:44]

lgtm

[ddorwin, 2014-10-08 21:48:27]

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

https://codereview.chromium.org/641433002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:56: const char* kPersistent =
"persistent";
Do we have this in two locations now?
Should we just add an enum to MKS.h.

https://codereview.chromium.org/641433002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:85: // Checks that |sessionId|
looks correct. Returns true if all tests pass,
Returns whether...

https://codereview.chromium.org/641433002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:136: return m_initDataType;
C&P error

https://codereview.chromium.org/641433002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:281: bool result;
Uninitialized, though I guess that was allowed (and even preferred) in WebKit.

https://codereview.chromium.org/641433002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:508: scriptState,
DOMException::create(InvalidAccessError, "The session type is not
\"persistent\"."));
You could just use single quotes too.

https://codereview.chromium.org/641433002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:765: //      (Done by the
CDM.)
Does our code ensure that the CDM does not do these things until after this
function returns?

[jrummell, 2014-10-08 22:57:01]

Updated.

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

https://codereview.chromium.org/641433002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:56: const char* kPersistent =
"persistent";
On 2014/10/08 21:48:26, ddorwin wrote:
> Do we have this in two locations now?
> Should we just add an enum to MKS.h.

The string is currently passed to Chromium (so we would still need to do 2
conversions -- one in MK when creating the session, a second down below when
calling Chromium). Changed MK to use a static method to verify the session type,
so the strings are only declared here.

https://codereview.chromium.org/641433002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:85: // Checks that |sessionId|
looks correct. Returns true if all tests pass,
On 2014/10/08 21:48:26, ddorwin wrote:
> Returns whether...

Done.

https://codereview.chromium.org/641433002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:136: return m_initDataType;
On 2014/10/08 21:48:26, ddorwin wrote:
> C&P error

Not really. m_initDataType used for both values. Renamed to make it clearer.

https://codereview.chromium.org/641433002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:281: bool result;
On 2014/10/08 21:48:26, ddorwin wrote:
> Uninitialized, though I guess that was allowed (and even preferred) in WebKit.

Acknowledged.

https://codereview.chromium.org/641433002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:508: scriptState,
DOMException::create(InvalidAccessError, "The session type is not
\"persistent\"."));
On 2014/10/08 21:48:27, ddorwin wrote:
> You could just use single quotes too.

Done.

https://codereview.chromium.org/641433002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:765: //      (Done by the
CDM.)
On 2014/10/08 21:48:26, ddorwin wrote:
> Does our code ensure that the CDM does not do these things until after this
> function returns?

Yes (assuming resolving this promise and generating the events happen on the
same thread). If the CDM calls these events before resolving the promise,
cdm_adapter won't have the session registered so the event will get dropped. Saw
this in the ECK test, as the KeysChanged event was being generated before this
promise was resolved and thus got dropped.

[ddorwin, 2014-10-08 23:08:49]

LGTM % comments.

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

https://codereview.chromium.org/641433002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:56: const char* kPersistent =
"persistent";
On 2014/10/08 22:57:00, jrummell wrote:
> On 2014/10/08 21:48:26, ddorwin wrote:
> > Do we have this in two locations now?
> > Should we just add an enum to MKS.h.
> 
> The string is currently passed to Chromium (so we would still need to do 2
> conversions -- one in MK when creating the session, a second down below when
> calling Chromium). Changed MK to use a static method to verify the session
type,
> so the strings are only declared here.

Okay. Even more reason to change to enums! :)

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

https://codereview.chromium.org/641433002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:87: // Returns whether
|sessionId| looks correct. Returns true if all tests pass,
Checks that |sessionId| looks correct and returns whether all checks pass.

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

https://codereview.chromium.org/641433002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.h:71: static bool
isValidSessionType(const String& sessionType);
Move up with the other static method.

[jrummell, 2014-10-08 23:52:55]

Updated.

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

https://codereview.chromium.org/641433002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:87: // Returns whether
|sessionId| looks correct. Returns true if all tests pass,
On 2014/10/08 23:08:49, ddorwin wrote:
> Checks that |sessionId| looks correct and returns whether all checks pass.

Done.

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

https://codereview.chromium.org/641433002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.h:71: static bool
isValidSessionType(const String& sessionType);
On 2014/10/08 23:08:49, ddorwin wrote:
> Move up with the other static method.

Done.

[jrummell, 2014-10-15 18:50:04]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-10-15 18:50:18]

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

[commit-bot: I haz the power, 2014-10-15 19:14:45]

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

[commit-bot: I haz the power, 2014-10-15 19:14:46]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[jrummell, 2014-10-15 21:20:18]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-10-15 21:21:19]

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

[commit-bot: I haz the power, 2014-10-15 21:25:37]

Committed patchset #6 (id:100001) as 183765