[webkit-reviews] review denied: [Bug 16090] carto.net Dock example redraws *way* too often : [Attachment 70848] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 16090: carto.net Dock example redraws *way* too often
https://bugs.webkit.org/show_bug.cgi?id=16090

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=70848&action=review

Great patch, still needs some love.

> WebCore/ChangeLog:12
> +	   Also added caching of the repaint rect to avoid mutliple
calculations.

typo: multiple.

> WebCore/ChangeLog:21
> +	   (WebCore::RenderSVGImage::repaintRectInLocalCoordinates): Give back
the stored repaint rect.

s/Give back../Return the cached repaint rect./

> WebCore/rendering/RenderSVGImage.cpp:72
> +	   m_needsBoundariesUpdate = true;

This is wrong. You don't need to recalculate the boundaries. m_localBounds
won't change.
You only have to notify the parents about the bbox change, because they will
map the "local bounds" through the "local transforms".

> WebCore/rendering/RenderSVGImage.cpp:87
> +    bool a = repainter.repaintAfterLayout();
> +    UNUSED_PARAM(a);

Just ignore the parameter.

> WebCore/rendering/RenderSVGImage.cpp:88
> +    m_needsBoundariesUpdate = false;

This should only be set in the if (m_needsBoundariesUpdate) branch, no need to
do it every time.

> WebCore/rendering/RenderSVGImage.h:76
>      FloatRect m_localBounds;

Please name it "m_objectBoundingBox" for consistency.

> LayoutTests/svg/custom/repaint-on-constant-size-change.svg:21
> +	       }, 50);

This indicates a flaw test. As images don't load immediately, you should wait
for the load event to be fired, before doing your actual testing.