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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Dec 7 09:29:11 PST 2011

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

Attachment 117354: Patch

------- 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

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.

More information about the webkit-reviews mailing list