[webkit-reviews] review granted: [Bug 23946] WebKit does not enumerate over CSS properties in HTMLElement.style : [Attachment 118549] [PATCH] Update JSC exports to fix compilability on Win and Mac

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 118549: [PATCH] Update JSC exports to fix compilability on Win and
Mac
https://bugs.webkit.org/attachment.cgi?id=118549&action=review

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


Seems OK to land like this. Still room for improvement either before or after
landing.

> Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:206
> +    static Identifier* jsPropertyIdentifiers = 0;

I don’t think you need the "js" prefix on this variable name.

> Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:211
> +	   std::sort(jsPropertyNames.begin(), jsPropertyNames.end(),
stringCompare);

Our usual style in WebKit code is to put "using namespace std" at the top of
the file rather than qualifying function names at the call site.

> Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:217
> +	   for (int i = 0; i < numCSSProperties; ++i) {
> +	       String jsPropertyName = jsPropertyNames.at(i);
> +	       jsPropertyIdentifiers[i] = Identifier(&exec->globalData(),
jsPropertyName.characters(), jsPropertyName.length());
> +	   }

The idiom here is slightly peculiar and unnecessarily inefficient. I would have
written this:

    for (int i = 0; i < numCSSProperties; ++i)
	jsPropertyIdentifiers[i] = Identifier(exec, jsPropertyNames[i].impl());


This would use the buffers already allocated for the strings rather than
reallocating each one. Since this is one-time code, it’s not critical, but I
think my version of the code is already more readable because it’s more brief.

> Source/WebCore/css/makeprop.pl:119
> +String getJSPropertyName(const char* cssPropertyName)

We normally try to avoid abbreviations and use words for variable names. This
function uses the names p, isFirstChar, ch, and nextCh, all of which are
abbreviations.

> Source/WebCore/css/makeprop.pl:121
> +    StringBuilder result;

Since the maximum length of any property name is known at compile time, this
could just use a local char array and construct the String from that. It need
not use StringBuilder at all.

> Source/WebCore/css/makeprop.pl:123
> +    bool isFirstChar = true;

There is no need for the isFirstChar boolean. You can do the same check by
comparing p - 2 with cssPropertyName inside the loop.

> Source/WebCore/css/makeprop.pl:124
> +    while (char ch = *(p++)) {

Since *p++ is idiomatic, I think it’s better to leave off the parentheses.

> Source/WebCore/css/makeprop.pl:129
> +	       ch = !isFirstChar ? (nextCh - 0x20) : nextCh;

Just doing the - 0x20 here is not really all that great. I would suggest using
toASCIILower instead for clarity even though it’s slightly less efficient.

> Source/WebCore/css/makeprop.pl:149
> +#include <wtf/text/WTFString.h>

If you move the inline function out of the header file then you can just
forward-declare instead of including this header.

> Source/WebCore/css/makeprop.pl:185
> +String getJSPropertyName(const char*);

The use of get in this function name is consistent with the function above, but
not normal WebKit naming.

It’s strange to have this take a const char*. I would be more logical to have
it take a CSSPropertyID to match the function above it. If it’s only going to
take const char* then there is no good reason to put the code here
auto-generated, and it can instead go into a normal source file.

> Source/WebCore/css/makeprop.pl:186
> +static inline bool stringCompare(const String& first, const String& second)
{ return codePointCompare(first, second) < 0; }

This function does not belong here. A basic string comparison function like
this should be in WTFString.h. Also, it should have a name making clear how it
differs from other functions. I suggest including the words “less than” in the
function name.

A function in a header file, even an inline one, should not be marked “static”.
That should be left off so it will get external linkage.