Bug 94587 – Add methods to CounterDirectives to clean up StyleBuilder and RenderCounter.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Elliott Sprehn]

Add methods to CounterDirectives to clean up StyleBuilder and RenderCounter.

[Elliott Sprehn]

Created attachment 159653 [details]
Patch

[Julien Chaffraix]

Comment on attachment 159653 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159653&action=review

> Source/WebCore/rendering/style/CounterDirectives.h:40
>          : m_reset(false)
>          , m_increment(false)

This naming should be improved while we are touching this code. Some better names:
* m_isResetCounter / m_hasCounterResetSet
* m_isIncrementCounter / m_hasCounterIncrementSet

> Source/WebCore/rendering/style/CounterDirectives.h:86
> +    int value() const

This value name is dangerous as you have lost the information whether you actually reset. I would have anticipated that value would return something I could just add to the current value.

Maybe valueForResetOrIncrement would better convey this idea.

[Elliott Sprehn]

This fixes 94642 because it's more careful about initializing the values.

[Elliott Sprehn]

(In reply to comment #2)
> (From update of attachment 159653 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159653&action=review
> ...
> 
> > Source/WebCore/rendering/style/CounterDirectives.h:86
> > +    int value() const
> 
> This value name is dangerous as you have lost the information whether you actually reset. I would have anticipated that value would return something I could just add to the current value.
> 
> Maybe valueForResetOrIncrement would better convey this idea.

Indeed it is something you can just add to the "current value", the catch being that on a reset CounterNode the current value is 0, while on other nodes the current value is the value of our ancestor node.

I changed this to combinedValue(). Overall it would be nice to get rid of CounterDirectives entirely, it's a confusing container for what should just be two maps and RenderStyle should have something like getCounterReset(String& identifier) and getCounterIncrement(String& identifier) that return a struct CounterValue() { bool isDefined(); int value(); } or something like that that.

[Elliott Sprehn]

Created attachment 162643 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 162643 [details]
Patch for landing

Clearing flags on attachment: 162643

Committed r127826: <http://trac.webkit.org/changeset/127826>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.