Convert MediaKeyStatusMap to iterable<BufferSource,MediaKeyStatus>

Convert MediaKeyStatusMap to iterable<BufferSource,MediaKeyStatus>

EME spec has changed MediaKeyStatusMap from maplike to iterable, so
update the code to match.

BUG=507753, 587916
TEST=EME layout tests pass

Committed: https://crrev.com/8cdb012b5e4b8e6b9820da0e8d25fcf0dfd02631
Cr-Commit-Position: refs/heads/master@{#400041}

[jrummell, 2015-07-16 03:11:26]

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

[jrummell, 2015-07-16 03:11:27]

PTAL. Although the EME spec has an entry for forEach(), it is provided as part
of the implementation of iterable<>.

[sandersd (OOO until July 31), 2015-08-07 23:11:36]

lgtm

https://codereview.chromium.org/1237343004/diff/1/Source/modules/encryptedmed...
File Source/modules/encryptedmedia/MediaKeyStatusMap.idl (right):

https://codereview.chromium.org/1237343004/diff/1/Source/modules/encryptedmed...
Source/modules/encryptedmedia/MediaKeyStatusMap.idl:21: boolean has
(BufferSource keyId);
Remove spaces before method parameters?

[jrummell, 2015-08-13 17:49:11]

Patchset #2 (id:20001) has been deleted

[jrummell, 2015-08-13 17:59:00]

rebased now that the key ordering was checked in separately.

https://codereview.chromium.org/1237343004/diff/1/Source/modules/encryptedmed...
File Source/modules/encryptedmedia/MediaKeyStatusMap.idl (right):

https://codereview.chromium.org/1237343004/diff/1/Source/modules/encryptedmed...
Source/modules/encryptedmedia/MediaKeyStatusMap.idl:21: boolean has
(BufferSource keyId);
On 2015/08/07 23:11:35, sandersd wrote:
> Remove spaces before method parameters?

Done.

[jrummell, 2016-06-03 17:32:02]

Description was changed from

==========
Convert MediaKeyStatusMap to iterable<BufferSource,MediaKeyStatus>

EME spec has changed MediaKeyStatusMap from maplike to iterable, so
update the code to match.

BUG=507753,507789
TEST=EME layout tests pass
==========

to

==========
Convert MediaKeyStatusMap to iterable<BufferSource,MediaKeyStatus>

EME spec has changed MediaKeyStatusMap from maplike to iterable, so
update the code to match.

BUG=507753,587916
TEST=EME layout tests pass
==========

[jrummell, 2016-06-06 19:15:26]

Description was changed from

==========
Convert MediaKeyStatusMap to iterable<BufferSource,MediaKeyStatus>

EME spec has changed MediaKeyStatusMap from maplike to iterable, so
update the code to match.

BUG=507753,587916
TEST=EME layout tests pass
==========

to

==========
Convert MediaKeyStatusMap to iterable<BufferSource,MediaKeyStatus>

EME spec has changed MediaKeyStatusMap from maplike to iterable, so
update the code to match.

There is a minor discrepancy. According to the spec(), get() should
return undefined if the keyId is not found. It currently returns
null.

BUG=507753,587916
TEST=EME layout tests pass
==========

[jrummell, 2016-06-06 19:17:03]

Updated to bring up-to-date, and get it working with the current idl. However,
get() for a keyId that doesn't exist returns null rather than undefined, so
continuing work to see if the spec or this code should change.

[jrummell, 2016-06-08 23:20:19]

Description was changed from

==========
Convert MediaKeyStatusMap to iterable<BufferSource,MediaKeyStatus>

EME spec has changed MediaKeyStatusMap from maplike to iterable, so
update the code to match.

There is a minor discrepancy. According to the spec(), get() should
return undefined if the keyId is not found. It currently returns
null.

BUG=507753,587916
TEST=EME layout tests pass
==========

to

==========
Convert MediaKeyStatusMap to iterable<BufferSource,MediaKeyStatus>

EME spec has changed MediaKeyStatusMap from maplike to iterable, so
update the code to match.

BUG=507753,587916
TEST=EME layout tests pass
==========

[jrummell, 2016-06-08 23:23:21]

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

[jrummell, 2016-06-08 23:23:21]

Updated to return undefined if get() doesn't find a matching entry. This
requires a custom CallEpilogue on get(), so adding haraken@ for comments and
OWNERS review of the v8 custom code.

[haraken, 2016-06-09 01:52:35]

On 2016/06/08 23:23:21, jrummell wrote:
> Updated to return undefined if get() doesn't find a matching entry. This
> requires a custom CallEpilogue on get(), so adding haraken@ for comments and
> OWNERS review of the v8 custom code.

In terms of implementation, looks good.

But I believe this is a spec bug. Ideally we should fix the spec so that we
don't need to land this CL...

[jrummell, 2016-06-14 20:14:51]

The spec has been updated to return "any", so updating this CL to use that.

[ddorwin, 2016-06-14 20:56:05]

Thanks for working through this! Just some minor stuff.

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-keystatuses.html
(right):

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-keystatuses.html:93:
assert_true(mediaKeySession.keyStatuses.has(key1));
It would be nice to have separate tests that check for key IDs that are close
but not present. For example, first/last byte different, one byte shorter (on
each end), and one byte longer (on each end).

This can be a separate CL.

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-utils.js
(right):

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-utils.js:147:
consoleWrite(arrayBufferAsString(keyId) + ", " + status);
nit: A ':' will probably make more sense to a reader

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.cpp
(right):

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.cpp:143: //
Not found, so return an index outside the valid range.
The caller must ensure this value is not exposed outside this class.

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.cpp:144:
return m_entries.size();
Unrelated to this CL:
Is this a common pattern? It seems we should return size_t's max() instead.

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.idl
(right):

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.idl:18:
iterable <BufferSource, MediaKeyStatus>;
Is this not readonly like the maplike was? The spec doesn't have this, but maybe
it should.

[jrummell, 2016-06-14 22:24:21]

Updated.

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-keystatuses.html
(right):

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-keystatuses.html:93:
assert_true(mediaKeySession.keyStatuses.has(key1));
On 2016/06/14 20:56:05, ddorwin wrote:
> It would be nice to have separate tests that check for key IDs that are close
> but not present. For example, first/last byte different, one byte shorter (on
> each end), and one byte longer (on each end).
> 
> This can be a separate CL.

Acknowledged.

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-utils.js
(right):

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-utils.js:147:
consoleWrite(arrayBufferAsString(keyId) + ", " + status);
On 2016/06/14 20:56:05, ddorwin wrote:
> nit: A ':' will probably make more sense to a reader

Done.

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.cpp
(right):

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.cpp:143: //
Not found, so return an index outside the valid range.
On 2016/06/14 20:56:05, ddorwin wrote:
> The caller must ensure this value is not exposed outside this class.

Done.

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.cpp:144:
return m_entries.size();
On 2016/06/14 20:56:05, ddorwin wrote:
> Unrelated to this CL:
> Is this a common pattern? It seems we should return size_t's max() instead.

Done. There is also WTF::kNotFound.

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.idl
(right):

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.idl:18:
iterable <BufferSource, MediaKeyStatus>;
On 2016/06/14 20:56:05, ddorwin wrote:
> Is this not readonly like the maplike was? The spec doesn't have this, but
maybe
> it should.

According to https://heycam.github.io/webidl/#idl-iterable, readonly is not
allowed with iterable<>. And none of the existing IDL files specify readonly
with iterable<>.

[ddorwin, 2016-06-14 22:56:40]

lgtm

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.idl
(right):

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.idl:18:
iterable <BufferSource, MediaKeyStatus>;
On 2016/06/14 22:24:21, jrummell wrote:
> On 2016/06/14 20:56:05, ddorwin wrote:
> > Is this not readonly like the maplike was? The spec doesn't have this, but
> maybe
> > it should.
> 
> According to https://heycam.github.io/webidl/#idl-iterable, readonly is not
> allowed with iterable<>. And none of the existing IDL files specify readonly
> with iterable<>.

Acknowledged. Thanks for checking.

[jrummell, 2016-06-15 21:17:54]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2016-06-15 21:17:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/1237343004/#ps120001
(title: "changes")

[commit-bot: I haz the power, 2016-06-15 21:18:27]

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

[commit-bot: I haz the power, 2016-06-15 23:27:56]

Description was changed from

==========
Convert MediaKeyStatusMap to iterable<BufferSource,MediaKeyStatus>

EME spec has changed MediaKeyStatusMap from maplike to iterable, so
update the code to match.

BUG=507753,587916
TEST=EME layout tests pass
==========

to

==========
Convert MediaKeyStatusMap to iterable<BufferSource,MediaKeyStatus>

EME spec has changed MediaKeyStatusMap from maplike to iterable, so
update the code to match.

BUG=507753,587916
TEST=EME layout tests pass
==========

[commit-bot: I haz the power, 2016-06-15 23:27:58]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-06-15 23:28:14]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-15 23:29:06]

Description was changed from

==========
Convert MediaKeyStatusMap to iterable<BufferSource,MediaKeyStatus>

EME spec has changed MediaKeyStatusMap from maplike to iterable, so
update the code to match.

BUG=507753,587916
TEST=EME layout tests pass
==========

to

==========
Convert MediaKeyStatusMap to iterable<BufferSource,MediaKeyStatus>

EME spec has changed MediaKeyStatusMap from maplike to iterable, so
update the code to match.

BUG=507753,587916
TEST=EME layout tests pass

Committed: https://crrev.com/8cdb012b5e4b8e6b9820da0e8d25fcf0dfd02631
Cr-Commit-Position: refs/heads/master@{#400041}
==========

[commit-bot: I haz the power, 2016-06-15 23:29:07]

Patchset 6 (id:??) landed as
https://crrev.com/8cdb012b5e4b8e6b9820da0e8d25fcf0dfd02631
Cr-Commit-Position: refs/heads/master@{#400041}