[webkit-reviews] review granted: [Bug 73360] CSS 2.1 failure: numerous counter-increment-* tests fail : [Attachment 118631] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Robert Hogan
<robert at webkit.org>'s request for review:
Bug 73360: CSS 2.1 failure: numerous counter-increment-* tests fail
https://bugs.webkit.org/show_bug.cgi?id=73360

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=118631&action=review


> Source/WebCore/css/CSSStyleApplyProperty.cpp:742
> +		       float newIncrementValue =
static_cast<float>(directives.m_incrementValue) +
static_cast<float>(it->second.m_incrementValue);
> +		       directives.m_incrementValue =
clampToInteger(newIncrementValue);

No double static_cast'ing, please. The following works for me and solves the
wrapping issues:

float incrementValue = directives.m_incrementValue;
directives.m_incrementValue = clampToInteger(incrementValue +
it->second.m_incrementValue);

> Source/WebCore/css/CSSStyleApplyProperty.cpp:786
> +		       directives.m_incrementValue =
clampToInteger(newIncrementValue);

Ditto.

> LayoutTests/fast/css/counters/counter-increment-inherit.htm:15
> +/*<!--	 Some extra WebKit testing for counter-increment inherit-->*/

You are mixing up 2 languages here, you're in JavaScript so it should be C++
style comment (not C style!):

// Some extra WebKit testing ...

> LayoutTests/fast/css/counters/counter-increment-inherit.htm:92
> +
> +	   <p>Test passes if the numbers '5 10 15' appear below in the same
order.</p> <!--test 90-->
> +	   <div id="wrapper90"> 
> +	       <div id="test90">
> +		   <div id="test90a"></div>
> +	       </div>
> +	   </div>
> +	   <p>Test passes if the numbers '10 20 30' appear below in the same
order.</p> <!--test 91-->
> +	   <div id="wrapper91"> 
> +	       <div id="test91">
> +		   <div id="test91a"></div>
> +	       </div>

I think I understand why you get your results at the beginning of the file
(sorry for not catching that earlier). The JS testing framework uses a <div
id="console"> which you don't provide here. If not provided, it will add one at
the beginning of the body.

If you add this element, you can remove the above note as now everything should
work as expected.

> LayoutTests/fast/css/counters/counter-increment-tests.htm:658
> +

Same comment here about the missing <div id="console">.