[webkit-reviews] review denied: [Bug 124281] [CSS Regions] Redundant computation of contentBoxRect().location in repaint : [Attachment 217532] Patch

[bugzilla-daemon at webkit.org]

Mihnea Ovidenie <mihnea at adobe.com> has denied Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 124281: [CSS Regions] Redundant computation of contentBoxRect().location in
repaint
https://bugs.webkit.org/show_bug.cgi?id=124281

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

------- Additional Comments from Mihnea Ovidenie <mihnea at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=217532&action=review


r-
I am not convinced that this patch provides the claimed performance
improvement. I ran several times on my machine the performance test you
mention, i even changed the number of regions to be 1000 instead of 100. The
difference between result - with and without the patch - were in the range of
1% which in this case is inside the deviation.
I think you should come up with a different test that would show an
improvement.

> Source/WebCore/ChangeLog:13
> +	   It improves performance of selection in CSS Regions.

Please provide numbers and the performance tests that were run in this regard.

> Source/WebCore/ChangeLog:21
> +	   * rendering/RenderRegion.h: Add new member m_regionLocation.

Should we add an accessor function to be used in other places in code where
contentBoxRect().location() is used for a region?