[Webkit-unassigned] [Bug 97337] Implement counter-set property from CSS3 lists spec

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=97337


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #165563|review?                     |review-
               Flag|                            |




--- Comment #10 from Darin Adler <darin at apple.com>  2012-09-25 09:48:52 PST ---
(From update of attachment 165563)
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).getAttribute("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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.