Bug 165961 – [Bindings] Remove use of Dictionary/ArrayValue in CDMSessionClearKey

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

[Bindings] Remove use of Dictionary/ArrayValue in CDMSessionClearKey

[Sam Weinig]

Created attachment 297328 [details]
Patch

[Darin Adler]

Comment on attachment 297328 [details]
Patch

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.

[Sam Weinig]

Committed r209939: <http://trac.webkit.org/changeset/209939>