Implement MediaKeys.setServerCertificate()

Implement MediaKeys.setServerCertificate()

Add code and tests for setServerCertificate().

BUG=358271
TEST=new EME tests pass

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

[jrummell, 2014-09-11 22:56:26]

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

[jrummell, 2014-09-11 22:56:26]

PTAL. Requires https://codereview.chromium.org/555223004/ to be in first.

[ddorwin, 2014-09-11 23:01:45]

ddorwin@chromium.org changed reviewers:
- xhwang@chromium.org

[ddorwin, 2014-09-11 23:02:21]

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

[ddorwin, 2014-09-11 23:02:22]

Dan, please take the first look.

[sandersd (OOO until July 31), 2014-09-11 23:37:14]

LGTM, but could likely be simplified.

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

https://codereview.chromium.org/563083002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:294:
m_pendingActions.append(PendingAction::CreatePendingSetServerCertificate(result,
serverCertificate));
If the Chromium promises implementation is correct, this shouldn't be different
from just calling cdm->setCertificate(), which would be simpler.

[jrummell, 2014-09-12 20:49:03]

Comments only.

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

https://codereview.chromium.org/563083002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:294:
m_pendingActions.append(PendingAction::CreatePendingSetServerCertificate(result,
serverCertificate));
On 2014/09/11 23:37:13, sandersd wrote:
> If the Chromium promises implementation is correct, this shouldn't be
different
> from just calling cdm->setCertificate(), which would be simpler.

The problem is that ClearKey will resolve the promise synchronously, so we
technically violate the spec. Any calls that go through Pepper will be async
though. ddorwin@, WDYT? Should we make the Chromium side do the async work?

[ddorwin, 2014-09-15 23:10:56]

LG % comments. Deferring to sandersd on details.

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

https://codereview.chromium.org/563083002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:294:
m_pendingActions.append(PendingAction::CreatePendingSetServerCertificate(result,
serverCertificate));
On 2014/09/12 20:49:03, jrummell wrote:
> On 2014/09/11 23:37:13, sandersd wrote:
> > If the Chromium promises implementation is correct, this shouldn't be
> different
> > from just calling cdm->setCertificate(), which would be simpler.
> 
> The problem is that ClearKey will resolve the promise synchronously, so we
> technically violate the spec. Any calls that go through Pepper will be async
> though. ddorwin@, WDYT? Should we make the Chromium side do the async work?

As discussed, let's keep it simple, consistent with the spec text, and
consistent with other methods.

https://codereview.chromium.org/563083002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:328: // Resolving promises now run
synchronously and may result in additional
"now"? I assume this was inspired by the change in behavior. We should probably
remove such text since it is already stale.

https://codereview.chromium.org/563083002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:329: // actions getting added to the
queue. As a result, swap the queue to
nit: s/As a result,/Therefore,/

https://codereview.chromium.org/563083002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:346: // These are handled by
Chromium and the CDM.
Add () as elsewhere?

[jrummell, 2014-09-17 23:39:25]

Updated.

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

https://codereview.chromium.org/563083002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:294:
m_pendingActions.append(PendingAction::CreatePendingSetServerCertificate(result,
serverCertificate));
On 2014/09/15 23:10:56, ddorwin wrote:
> On 2014/09/12 20:49:03, jrummell wrote:
> > On 2014/09/11 23:37:13, sandersd wrote:
> > > If the Chromium promises implementation is correct, this shouldn't be
> > different
> > > from just calling cdm->setCertificate(), which would be simpler.
> > 
> > The problem is that ClearKey will resolve the promise synchronously, so we
> > technically violate the spec. Any calls that go through Pepper will be async
> > though. ddorwin@, WDYT? Should we make the Chromium side do the async work?
> 
> As discussed, let's keep it simple, consistent with the spec text, and
> consistent with other methods.

Acknowledged.

https://codereview.chromium.org/563083002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:328: // Resolving promises now run
synchronously and may result in additional
On 2014/09/15 23:10:56, ddorwin wrote:
> "now"? I assume this was inspired by the change in behavior. We should
probably
> remove such text since it is already stale.

Comment removed.

https://codereview.chromium.org/563083002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:329: // actions getting added to the
queue. As a result, swap the queue to
On 2014/09/15 23:10:56, ddorwin wrote:
> nit: s/As a result,/Therefore,/

Changed.

https://codereview.chromium.org/563083002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:346: // These are handled by
Chromium and the CDM.
On 2014/09/15 23:10:56, ddorwin wrote:
> Add () as elsewhere?

Done.

[ddorwin, 2014-09-17 23:44:04]

LGTM. Thanks.

[jrummell, 2014-09-18 00:15:30]

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

[jrummell, 2014-09-29 23:43:50]

Updated. Note that the successful test of setServerCertificate() has been
removed as ClearKey doesn't support the call (so previously successful tests
have been moved into the failure test).

[wolenetz, 2014-10-02 00:35:17]

On 2014/09/29 23:43:50, jrummell wrote:
> Updated. Note that the successful test of setServerCertificate() has been
> removed as ClearKey doesn't support the call (so previously successful tests
> have been moved into the failure test).

I'll do a first round of a noob-to-eme CR of this tomorrow.

[jrummell, 2014-10-14 17:19:23]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-10-14 17:20:30]

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

[commit-bot: I haz the power, 2014-10-14 18:46:17]

Committed patchset #4 (id:60001) as 183692