Bug 165961 - [Bindings] Remove use of Dictionary/ArrayValue in CDMSessionClearKey
Summary: [Bindings] Remove use of Dictionary/ArrayValue in CDMSessionClearKey
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
Depends on:
Reported: 2016-12-16 10:58 PST by Sam Weinig
Modified: 2016-12-16 14:29 PST (History)
0 users

See Also:

Patch (19.10 KB, patch)
2016-12-16 11:00 PST, Sam Weinig
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2016-12-16 10:58:20 PST
[Bindings] Remove use of Dictionary/ArrayValue in CDMSessionClearKey
Comment 1 Sam Weinig 2016-12-16 11:00:53 PST
Created attachment 297328 [details]
Comment 2 Darin Adler 2016-12-16 13:36:23 PST
Comment on attachment 297328 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=297328&action=review

> Source/WebCore/Modules/encryptedmedia/CDMSessionClearKey.cpp:120
> +        auto keysDataObject = asObject(keysDataValue);
> +
> +        auto keysArrayValue = keysDataObject->get(&state, Identifier::fromString(&state, "keys"));

I think i’d omit the keysDataObject local variable since it’s used only this once. But not sure everyone would agree.

> Source/WebCore/Modules/encryptedmedia/CDMSessionClearKey.cpp:127
> +        size_t length = keysArray->length();

Shouldn’t this be unsigned rather than size_t? I don’t think getIndex takes a 64-bit value.

> Source/WebCore/Modules/encryptedmedia/CDMSessionClearKey.cpp:134
>          for (size_t i = 0; i < length; ++i) {

Shouldn’t this be unsigned rather than size_t?

> Source/WebCore/Modules/encryptedmedia/CDMSessionClearKey.cpp:137
> +            if (scope.exception() || !keyValue || !keyValue.isObject()) {

I don’t think the !keyValue check is needed here. Doesn’t isObject safely return false in that case?

> Source/WebCore/Modules/encryptedmedia/CDMSessionClearKey.cpp:144
> +            auto getStringProperty = [&](ExecState& state, JSObject* object, const char* name) -> String {

Not sure we need to capture at all here. Would be nice to be explicit that we are not capturing.

I’d probably also have just written this function in the more conventional way at the top level of the namespace rather than writing it this way, here.

> Source/WebCore/Modules/encryptedmedia/CDMSessionClearKey.cpp:146
> +                if (scope.exception() || !value || !value.isString())

Not sure the !value check is needed here. Same question as above.

> Source/WebCore/Modules/encryptedmedia/CDMSessionClearKey.cpp:149
> +                auto string = value.toWTFString(&state);

This should be asString(value)->value(&state); we already checked isString, so we don’t need the rest of what toWTFString does.
Comment 3 Sam Weinig 2016-12-16 14:29:56 PST
Committed r209939: <http://trac.webkit.org/changeset/209939>