[webkit-reviews] review denied: [Bug 97337] Implement counter-set property from CSS3 lists spec : [Attachment 165563] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Andrei Onea <onea at adobe.com>'s request
for review:
Bug 97337: Implement counter-set property from CSS3 lists spec
https://bugs.webkit.org/show_bug.cgi?id=97337

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

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


> Source/WebCore/ChangeLog:10
> +	   Test: fast/css/counters/counter-set.html

This seems like really light test coverage for this. We need a test that covers
a bit more, at least exercising all the code that was added.

For example, this does not test getComputedStyle behavior for the new property,
nor does it test the behavior of changes to the property rather than just an
initial value set. It does not test parsing edge cases instead having just a
single test case.

With all the code we have to write to support things like inheriting we need to
make sure our test cases cover those cases as well. Removing any line of code
should make a test fail, but I think that currently there are many lines of
code I could remove without having any effect on this one test.

> Source/WebCore/css/StyleBuilder.cpp:1002
> -enum CounterBehavior {Increment = 0, Reset};
> +enum CounterBehavior {Increment = 0, Reset, Set};

Nor new, but the formatting here is not our standard. There should be spaces
inside the braces { Increment, Reset, Set } and the = 0 should not be there
since it adds no value.

> Source/WebCore/css/StyleBuilder.cpp:1028
> -	       if (counterBehavior == Reset) {
> +	       switch (counterBehavior) {
> +	       case Reset:
>		   directives.inheritReset(it->second);
> -	       } else {
> +		   break;
> +	       case Increment:
>		   directives.inheritIncrement(it->second);
> +		   break;
> +	       case Set:
> +		   directives.inheritSet(it->second);
> +		   break;
> +	       default:
> +		   ASSERT_NOT_REACHED();
>	       }

We normally don’t include defaults in switch statements, because that turns off
the compiler warnings for unhandled enum values. I understand the value of the
ASSERT_NOT_REACHED, but the compile time warning is even more valuable.

There may also be a more elegant way to handle this. Passing in an enum and
using a bunch of switch statements seems inferior to using a traits class that
can supply the various functions needed inside the class.

> Source/WebCore/css/StyleBuilder.cpp:1055
> -	       if (counterBehavior == Reset)
> +	       switch (counterBehavior) {
> +	       case Reset:
>		   it->second.clearReset();
> -	       else
> +		   break;
> +	       case Increment:
>		   it->second.clearIncrement();
> +		   break;
> +	       case Set:
> +		   it->second.clearSet();
> +		   break;
> +	       default:
> +		   ASSERT_NOT_REACHED();
> +	       }

Same comment applies here.

> Source/WebCore/css/StyleBuilder.cpp:1082
> -	       if (counterBehavior == Reset) {
> +	       switch (counterBehavior) {
> +	       case Reset:
>		   directives.setResetValue(value);
> -	       } else {
> +		   break;
> +	       case Increment:
>		   directives.addIncrementValue(value);
> +		   break;
> +	       case Set:
> +		   directives.setSetValue(value);
> +		   break;
> +	       default:
> +		   ASSERT_NOT_REACHED();
> +		   break;

And here.

> Source/WebCore/rendering/CounterNode.h:45
> -    static PassRefPtr<CounterNode> create(RenderObject*, bool isReset, int
value);
> +    static PassRefPtr<CounterNode> create(RenderObject*, bool isSet, bool
isReset, int value);

The use of two booleans here is not good. We should replace the boolean with an
enum. Rather than having two booleans that if they were both set would be
incorrect.

> LayoutTests/fast/css/counters/counter-set.html:19
> +	   newSpanElement.innerText =
window.internals.counterValue(document.getElementById(spanList.item(i).getAttri
bute("id"))) + " ";

You can use “internals” here, no need to use “window.internals”.

But also, the test is carefully using “window.testRunner” to try to run outside
the test runner, but that’s really pointless if the test relies on
window.internals which is also present only inside the test runner.

What does this test do in the browser?