[webkit-reviews] review granted: [Bug 132906] [CSS Regions] Add ASSERT to make sure using the flowThread cache does not return incorrect results : [Attachment 231499] Patch which applies only to regions

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Radu Stavila
<stavila at adobe.com>'s request for review:
Bug 132906: [CSS Regions] Add ASSERT to make sure using the flowThread cache
does not return incorrect results
https://bugs.webkit.org/show_bug.cgi?id=132906

Attachment 231499: Patch which applies only to regions
https://bugs.webkit.org/attachment.cgi?id=231499&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=231499&action=review


> Source/WebCore/rendering/RenderObject.cpp:550
> +	       // Failing tests when enabling this for multicol:
> +	       //
http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r1688
37%20(4654)/results.html

I don't think it's useful to put this URL in a comment. Eventually the test
results will be purged from the server. This kind of info should go in the bug.


> Source/WebCore/rendering/RenderObject.cpp:552
> +	       ASSERT(flowThread == locateFlowThreadContainingBlockNoCache());

This if() block only contains an assertion. It would be better to make the "if"
test part of the assertion so it's compiled out in release.

> Source/WebCore/rendering/RenderObject.h:893
> +    // This will walk the tree to find the flow thread.

Comment seems to be inaccurate if you're in layout.