[webkit-reviews] review denied: [Bug 104135] JSObjectCopyPropertyNames returns non-enumerable properties. : [Attachment 183003] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Michael Brüning
<michael.bruning at digia.com>'s request for review:
Bug 104135: JSObjectCopyPropertyNames returns non-enumerable properties.
https://bugs.webkit.org/show_bug.cgi?id=104135

Attachment 183003: Patch
https://bugs.webkit.org/attachment.cgi?id=183003&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=183003&action=review


Is there any way to have test code to cover this? Is this code path only used
in JSObjectCopyPropertyNames? Isn’t it also used in JavaScript execution for
the "for in" statement? We need test coverage.

> Source/WebCore/ChangeLog:11
> +	   Also adds and additional filter in the Qt JS bridge that was there
before.   

Typo: and

> Source/WebCore/bindings/js/JSDOMStringMapCustom.cpp:56
> +	   Identifier propertyIdentifier = Identifier(exec, names[i]);

This should just use normal constructor syntax:

    Identifier propertyName(exec, names[i]);

> Source/WebCore/bindings/js/JSStorageCustom.cpp:93
> +	   Identifier propertyIdentifier = Identifier(exec,
thisObject->m_impl->key(i, ec));

This should just use normal constructor syntax:

    Identifier propertyName(exec, thisObject->m_impl->key(i, ec));

> Source/WebCore/bindings/js/JSStorageCustom.cpp:98
> +	       setDOMException(exec, ec);
> +	       if (exec->hadException())
> +		   return;

These lines of code need to be outside the if statement; if we execute the code
to get a key and get an exception, we need to detect that and propagate the
exception even if the property is non-enumerable.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2222
> +	       push(@implContent, "	   Identifier propertyIdentifier =
Identifier::from(exec, i);\n");

I think the local should be named propertyName, not propertyIdentifier.