Re: Convert MediaKeyStatusMap to iterable<BufferSource,MediaKeyStatus> (issue 1237343004 by jrummell@chromium.org)

1 view
Skip to first unread message

jrum...@chromium.org

unread,
Jun 6, 2016, 3:17:03 PM6/6/16
to ddorwin+...@chromium.org, sand...@chromium.org, blink-...@chromium.org, mlamouri+w...@chromium.org, feature-me...@chromium.org, phili...@opera.com, eric.c...@apple.com
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.

https://codereview.chromium.org/1237343004/

jrum...@chromium.org

unread,
Jun 8, 2016, 7:23:22 PM6/8/16
to ddorwin+...@chromium.org, sand...@chromium.org, har...@chromium.org, blink-...@chromium.org, mlamouri+w...@chromium.org, feature-me...@chromium.org, phili...@opera.com, eric.c...@apple.com
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.



https://codereview.chromium.org/1237343004/

har...@chromium.org

unread,
Jun 8, 2016, 9:52:35 PM6/8/16
to jrum...@chromium.org, ddorwin+...@chromium.org, sand...@chromium.org, blink-...@chromium.org, mlamouri+w...@chromium.org, feature-me...@chromium.org, phili...@opera.com, eric.c...@apple.com
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...



https://codereview.chromium.org/1237343004/

jrum...@chromium.org

unread,
Jun 14, 2016, 4:14:52 PM6/14/16
to ddorwin+...@chromium.org, sand...@chromium.org, har...@chromium.org, blink-...@chromium.org, mlamouri+w...@chromium.org, feature-me...@chromium.org, phili...@opera.com, eric.c...@apple.com
The spec has been updated to return "any", so updating this CL to use that.

https://codereview.chromium.org/1237343004/

ddo...@chromium.org

unread,
Jun 14, 2016, 4:56:06 PM6/14/16
to jrum...@chromium.org, sand...@chromium.org, har...@chromium.org, blink-...@chromium.org, mlamouri+w...@chromium.org, feature-me...@chromium.org, phili...@opera.com, eric.c...@apple.com
Thanks for working through this! Just some minor stuff.


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

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-keystatuses.html#newcode93
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/LayoutTests/media/encrypted-media/encrypted-media-utils.js
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-utils.js
(right):

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-utils.js#newcode147
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/Source/modules/encryptedmedia/MediaKeyStatusMap.cpp
File
third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.cpp
(right):

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.cpp#newcode143
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/Source/modules/encryptedmedia/MediaKeyStatusMap.cpp#newcode144
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/Source/modules/encryptedmedia/MediaKeyStatusMap.idl
File
third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.idl
(right):

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.idl#newcode18
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.

https://codereview.chromium.org/1237343004/

jrum...@chromium.org

unread,
Jun 14, 2016, 6:24:21 PM6/14/16
to ddorwin+...@chromium.org, sand...@chromium.org, har...@chromium.org, blink-...@chromium.org, mlamouri+w...@chromium.org, feature-me...@chromium.org, phili...@opera.com, eric.c...@apple.com
Updated.



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

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-keystatuses.html#newcode93
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/LayoutTests/media/encrypted-media/encrypted-media-utils.js
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-utils.js
(right):

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-utils.js#newcode147
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/Source/modules/encryptedmedia/MediaKeyStatusMap.cpp
File
third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.cpp
(right):

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.cpp#newcode143
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/Source/modules/encryptedmedia/MediaKeyStatusMap.cpp#newcode144
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/Source/modules/encryptedmedia/MediaKeyStatusMap.idl
File
third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.idl
(right):

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.idl#newcode18
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<>.

https://codereview.chromium.org/1237343004/

ddo...@chromium.org

unread,
Jun 14, 2016, 6:56:40 PM6/14/16
to jrum...@chromium.org, sand...@chromium.org, har...@chromium.org, blink-...@chromium.org, mlamouri+w...@chromium.org, feature-me...@chromium.org, phili...@opera.com, eric.c...@apple.com
lgtm





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

https://codereview.chromium.org/1237343004/diff/100001/third_party/WebKit/Source/modules/encryptedmedia/MediaKeyStatusMap.idl#newcode18
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.

https://codereview.chromium.org/1237343004/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jun 15, 2016, 5:18:27 PM6/15/16
to jrum...@chromium.org, ddorwin+...@chromium.org, sand...@chromium.org, har...@chromium.org, commi...@chromium.org, blink-...@chromium.org, mlamouri+w...@chromium.org, feature-me...@chromium.org, phili...@opera.com, eric.c...@apple.com

commit-bot@chromium.org via codereview.chromium.org

unread,
Jun 15, 2016, 7:27:59 PM6/15/16
to jrum...@chromium.org, ddorwin+...@chromium.org, sand...@chromium.org, har...@chromium.org, commi...@chromium.org, blink-...@chromium.org, mlamouri+w...@chromium.org, feature-me...@chromium.org, phili...@opera.com, eric.c...@apple.com
Committed patchset #6 (id:120001)

https://codereview.chromium.org/1237343004/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jun 15, 2016, 7:28:14 PM6/15/16
to jrum...@chromium.org, ddorwin+...@chromium.org, sand...@chromium.org, har...@chromium.org, commi...@chromium.org, blink-...@chromium.org, mlamouri+w...@chromium.org, feature-me...@chromium.org, phili...@opera.com, eric.c...@apple.com

commit-bot@chromium.org via codereview.chromium.org

unread,
Jun 15, 2016, 7:29:08 PM6/15/16
to jrum...@chromium.org, ddorwin+...@chromium.org, sand...@chromium.org, har...@chromium.org, commi...@chromium.org, blink-...@chromium.org, mlamouri+w...@chromium.org, feature-me...@chromium.org, phili...@opera.com, eric.c...@apple.com
Patchset 6 (id:??) landed as
https://crrev.com/8cdb012b5e4b8e6b9820da0e8d25fcf0dfd02631
Cr-Commit-Position: refs/heads/master@{#400041}

https://codereview.chromium.org/1237343004/
Reply all
Reply to author
Forward
0 new messages