[webkit-reviews] review granted: [Bug 103510] Reduce the children repaints when moved multiple times during the layout : [Attachment 176952] Updated patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 103510: Reduce the children repaints when moved multiple times during the
layout
https://bugs.webkit.org/show_bug.cgi?id=103510

Attachment 176952: Updated patch
https://bugs.webkit.org/attachment.cgi?id=176952&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=176952&action=review


> LayoutTests/css3/flexbox/repaint-column-reverse.html:23
> +    if (window.testRunner) {
> +	   testRunner.waitUntilDone();
> +	   testRunner.display();

Please add testRunner.dumpAsText(true) so we get the pixel result, but no
render tree dump which isn't important for this test.

> LayoutTests/css3/flexbox/repaint-column-reverse.html:29
> +	   document.body.appendChild(document.createTextNode(
> +	       "This test checks that for flex items that are moved multiple
times during the layout "
> +	       + "only the initial and final positions are repainted. Only the
blue flex item should be repainted "
> +	       + "after changing its position. If the other flex items are
repainted, this test fails."));
> +    }

I would always include this text. It makes it easier for gardeners.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:395
> +	   childIndex++;

Nit: ++childIndex

> Source/WebCore/rendering/RenderFlexibleBox.cpp:735
> +    iterator.first();

Ah right, previously the constructor would do this for us. Can we also remove
the call to first() in the OrderIterator constructor?