Bug 73360

Created attachment 117278 [details] Patch

Comment on attachment 117278 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117278&action=review > Source/WebCore/ChangeLog:12 > + (WebCore::ApplyPropertyCounter::clamp): clamp the new value after increment to INT_MAX/INT_MIN What's wrong with the existing clamping methods like clampTo<int>()? Your function is confusing in that it doesn't return and modify the second argument. > Source/WebCore/css/CSSStyleApplyProperty.cpp:640 > + if (newValue > 0 && currentValue > (INT_MAX - newValue)) IIRC we prefer std::numeric_limits<int>::min() / max() here instead of INT_MAX / INT_MIN. > Source/WebCore/css/CSSStyleApplyProperty.cpp:655 > + if (counterBehavior) { It would be more readable here if you actually put the existing behavior value to compare against. > Source/WebCore/css/CSSStyleApplyProperty.cpp:712 > } Could you see that the update the current CounterDirective logic is shared between the 2 functions? It really looks like some copy and paste. > LayoutTests/ChangeLog:6 > + WebKit versions of the CSS test suite's counter-increment-001 and 002 are already present in If they are part of the official CSS test suite then they should be in LayoutTests/css/ not in fast/css/ unless there is something more here. > LayoutTests/fast/css/counters/counter-increment-tests-expected.txt:59 > + Hum, the output makes it look like we failed all the tests here. Could we rearrange it to be less confusing? > LayoutTests/fast/css/counters/counter-increment-tests.htm:513 > + } It looks like this is the only test for inherit. Is it also undertested in the official test suite? You could actually move the 'inherit' part to another patch if it's too painful to land all the normal cases. Don't feel compelled to do so as the patch has a decent size.

Comment on attachment 117278 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117278&action=review > Source/WebCore/css/CSSStyleApplyProperty.cpp:637 > + static void clamp(int currentValue, int& newValue) I'm finding this function very non-obvious. I think you need to do one or more of improve the naming, rewrite as a function that takes one or two ints and returns an int (no pass by ref), or use the clampTo<int> function in MathExtras.h - Could you at explain what you think is the expected behavior here? >> Source/WebCore/css/CSSStyleApplyProperty.cpp:655 > > It would be more readable here if you actually put the existing behavior value to compare against. Julien is right, you are probably just conforming to the existing style in the file though, which is my fault.

Do the values in CounterDirectives need to be signed, ie. is an increment or total ever validly negative? If not, clamped increment of unsigned values is simpler and easy to grok.

Created attachment 117798 [details] Patch

(In reply to comment #2) > What's wrong with the existing clamping methods like clampTo<int>()? Your function is confusing in that it doesn't return and modify the second argument. I've made this less confusing I hope, and added a comment to explain what it does. Rather than just clamp an int, it needs to figure out if applying the increment will overflow or underflow the counter, and if so, reduce the increment so that the counter will not exceed the maximum/minimum possible value of a signed int when the it is applied. > > Could you see that the update the current CounterDirective logic is shared between the 2 functions? It really looks like some copy and paste. They look similar but are different enough to make turning the common statements into a separate function even more confusing I think. > > If they are part of the official CSS test suite then they should be in LayoutTests/css/ not in fast/css/ unless there is something more here. They're dumpAsText() versions. Adding pixel tests and all their expectations after landing is so painful I've started to avoid it where possible! > > > LayoutTests/fast/css/counters/counter-increment-tests-expected.txt:59 > > + > > Hum, the output makes it look like we failed all the tests here. Could we rearrange it to be less confusing? Added a note to the top of the output. > > > LayoutTests/fast/css/counters/counter-increment-tests.htm:513 > > + } > > It looks like this is the only test for inherit. Is it also undertested in the official test suite? Yes it is. I've added a couple more tests for the inherit case. Addressed everything else I think!

(In reply to comment #4) > Do the values in CounterDirectives need to be signed, ie. is an increment or total ever validly negative? > If not, clamped increment of unsigned values is simpler and easy to grok. I hope I've addressed your concerns and Luke's about this function with the updated patch. I know it's not any easier to grok, but I think the comment helps explain what it does.

> > If they are part of the official CSS test suite then they should be in LayoutTests/css/ not in fast/css/ unless there is something more here. > > They're dumpAsText() versions. I don't think this makes a case for moving them to fast/. If they are just slightly modified for our test harness then they totally have their place in css/. It will avoid importing them twice by mistake. See Philip's canvas test suite as an example where this was done.

(In reply to comment #8) > > > If they are part of the official CSS test suite then they should be in LayoutTests/css/ not in fast/css/ unless there is something more here. > > > > They're dumpAsText() versions. > > I don't think this makes a case for moving them to fast/. If they are just slightly modified for our test harness then they totally have their place in css/. It will avoid importing them twice by mistake. See Philip's canvas test suite as an example where this was done. My preferred approach is to land the dumpAsText test in fast/css as a placeholder against regressions. Then later, in a bug dependent on this one, add the actual counter-increment-* tests (there's 56 of them) in css2.1/20110323. Landing tests from the CSS 2.1 test suite modified to dump as text in fast/css seems to be the convention so far. See counter-increment-001.htm in fast/css for example.

(In reply to comment #9) > > My preferred approach is to land the dumpAsText test in fast/css as a placeholder against regressions. Then later, in a bug dependent on this one, add the actual counter-increment-* tests (there's 56 of them) in css2.1/20110323. Landing tests from the CSS 2.1 test suite modified to dump as text in fast/css seems to be the convention so far. See counter-increment-001.htm in fast/css for example. The reason for this preference btw is the volume of expected results I have to land for a new pixel test. In the event of a rollout this can get nasty so would prefer to let a change like this settle before landing all those pixels! Also putting the test in this patch in css2.1 wouldn't prevent anyone landing the real counter-increment-* tests. They wouldn't even notice them I'd say. I agree the downside is testing things twice. Unless I remove the fast/css version when landing the pixel tests.

Comment on attachment 117798 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117798&action=review > Source/WebCore/css/CSSStyleApplyProperty.cpp:667 > + int increment = clampIncrement(directives.m_incrementValue, it->second.m_incrementValue); I am really not a fan of your custom clampToIncrement. I would rather see: float newIncrementValue = directives.m_incrementValue + it->second.m_incrementValue; directives.m_incrementValue = clampToInteger(newIncrementValue); which is a lot more readable. > LayoutTests/fast/css/counters/counter-increment-tests.htm:716 > +<!-- Some extra WebKit testing for counter-increment inherit--> If we intent to remove this test when the official test suite lands, it would be nice to put those new tests in a separate test to avoid losing coverage.

Created attachment 118631 [details] Patch

Comment on attachment 118631 [details] Patch 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">.

Committed r102528: <http://trac.webkit.org/changeset/102528>