[webkit-reviews] review denied: [Bug 23946] Webkit does not enumerate over CSS properties in HTMLElement.style : [Attachment 116923] 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 116923: Patch
https://bugs.webkit.org/attachment.cgi?id=116923&action=review

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


> Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:202
> +    for (unsigned i = 0, length =
static_cast<CSSStyleDeclaration*>(thisObject->impl())->length(); i < length;
++i)

The typecast is not needed here. The result of thisObject->impl() is already a
CSSStyleDeclaration. I’m not sure why the generated code included it.

The initialization of length here is not consistent with our usual style. We
normally would just do it separately outside the loop. I wonder if we should do
this more often, or frown on it.

> Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:203
> +	   propertyNames.add(Identifier::from(exec, i));

This is terribly inefficient, but since it’s the same idiom already used and is
also used for JavaScript arrays then I suppose it’s what we want for now at
least.

> Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:206
> +    for (int i = 0; i < numCSSProperties; ++i)
> +	   propertyNames.add(Identifier(exec,
stringToUString(jsPropertyNameStrings[i])));

These are const char*, not WTF strings, the call to stringToUString can and
should omitted. The use of stringToUString forces additional unneeded memory
allocation.

Exposing the names in the order they appeared in the CSSPropertyNames.in source
file does not seem like a good idea. We should expose them in some order that
is consistent and does not reflect the peculiarities of our source code. I
would suggest alphabetical order if we can’t think of anything else.

If we actually want this to be efficient we should be creating an array of
identifiers one time only and then reusing the same identifiers in the future.
Converting from const char* every time doesn’t seem helpful.

> Source/WebCore/bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp:159
> +    // We know that "filter" is present in the list but should not be
provided in the enumeration.
> +    // See a comment in the V8CSSStyleDeclaration::namedPropertyGetter()
implementation.

I don’t understand why this special rule to omit filter is in V8 but not in the
other JavaScript binding, especially considering the comment in
namedPropertyGetter implies it is there. I don’t understand why there is no
test for it. I suspect this rule about "filter" was removed long ago but was
left behind in V8 for not good reason.

> Source/WebCore/css/makeprop.pl:166
> +  $name =~ s/(\S)-(.)/$1@{[uc($2)]}/g;
> +  $name =~ s/^-//;
> +  print HEADER "\"$name\",\n";
> +}
> +print HEADER "};\n";

I think it would be better to not change the contents of this file at all.
Instead we could use the existing array of const char* and the first time it’s
needed we could build an array of JSC::Identifier one time inside the bindings
code and then reuse that each time instead of doing it over and over again.

If we do want to keep this, then I suggest we sort the array here, since we
only use it for enumeration and so the order is not important.

I think (.)-(.) would work fine; I don’t think you need the \S there.

> LayoutTests/fast/css/style-enumerate-properties-expected.txt:3
> +Test for https://bugs.webkit.org/show_bug.cgi?id=23946 Webkit does not
enumerate over CSS properties in HTMLElement.style - checks that the style
contains exactly one numeric property "0" (for "text-decoration") and named
properties "textDecoration", "border", "font", and "cssText".
> +
> +PASSED

Tests should show what they are testing, not just say PASSED.

> LayoutTests/fast/css/style-enumerate-properties.html:25
> +	     if (!(p in style)) {
> +		   console.textContent = "Property '" + p + "' not found!";
> +		   return;
> +	       }

Better style would be to use the shouldBe macros and do shouldBe for each
property so the test output would show what it’s testing.

This test does not cover the order of the properties. It should.

This test does not cover any properties with more then two words in their
names, and it should.

> LayoutTests/fast/css/style-enumerate-properties.html:34
> +	   Webkit does not enumerate over CSS properties in
HTMLElement.style</i> - checks that the style contains exactly one numeric
property "0" (for "text-decoration") and named properties "textDecoration",
"border", "font", and "cssText".

WebKit has a capital K in its name.

> LayoutTests/fast/dom/script-tests/domListEnumeration.js:176
> +// More properties can be added soon. Plus, Chromium hides the "filter"
property.

This comment is more confusing than helpful. It’s not clear how either of these
is relevant to the next line of code. I realize that you are justifying the use
of >= rather than == for the test, but only because I am reading the whole
patch. The comment alone does not make it clear.

> LayoutTests/fast/dom/script-tests/domListEnumeration.js:177
> +shouldBeGreaterThanOrEqual("resultArray.length", "376");

Where did the 376 number come from? Is it right even if SVG is turned off? I
think you should just not check the resultArray’s length at all.