[webkit-reviews] review denied: [Bug 23946] WebKit does not enumerate over CSS properties in HTMLElement.style : [Attachment 117354] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 23946: WebKit does not enumerate over CSS properties in HTMLElement.style
https://bugs.webkit.org/show_bug.cgi?id=23946

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

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


Looks good. I’d still like to see a little more refinement.

> Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:211
> +    DEFINE_STATIC_LOCAL(Vector<UString>, jsPropertyNames, ());
> +    if (jsPropertyNames.isEmpty()) {
> +	   jsPropertyNames.reserveInitialCapacity(numCSSProperties);
> +	   for (int i = 0; i < numCSSProperties; ++i)
> +	       jsPropertyNames.append(UString(jsPropertyNameStrings[i]));
> +    }

This should be Identifier[numCSSProperties], not Vector<UString>. The
Identifier constructor takes a JSGlobalData*, not an ExecState*. The ExecState*
constructor is just a convenience cover that uses the ExecState to find the
JSGlobalData. All WebCore use of JavaScript shares a single JSGlobalData; to
oversimplify, it’s basically a per-thread data structure. So it’s safe to
construct this array only once with whatever ExecState you have that one time.
And there is no reason to use a Vector for something we are not going to
resize.

Also, since we are doing this only one time, we could consider writing the code
to convert the CSS style of property name string into the JavaScript binding
style and avoid having a separate table.