[webkit-reviews] review denied: [Bug 77903] Repaint issues on changing 'viewBox' of inner SVG : [Attachment 126897] Proposed Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Feb 14 00:56:33 PST 2012


Nikolas Zimmermann <zimmermann at kde.org> has denied  review:
Bug 77903: Repaint issues on changing 'viewBox' of inner SVG
https://bugs.webkit.org/show_bug.cgi?id=77903

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

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


r- for the test, and the code changes, which are not a perfect solution yet.

> LayoutTests/svg/repaint/inner-svg-change-viewBox-contract.svg:2
> +<svg xmlns="http://www.w3.org/2000/svg"
xmlns:xlink="http://www.w3.org/1999/xlink"
> + width="300" height="300" onload="window.setTimeout(resizeViewBox, 0)">

I think it's not a good idea to use reftests here - we really want to check the
repainting of inner <svgs>, and thus the repaint.js harness is really helpful,
to spot in the pngs that repainting worked correctly. I propose to use:
<svg onload="runRepaintTest()">
<script xlink:href="../../fast/repaint/resources/repaint.js">
..

> LayoutTests/svg/repaint/inner-svg-change-viewBox-contract.svg:5
> +if (window.layoutTestController)
> +    layoutTestController.waitUntilDone();

This can be removed.

> LayoutTests/svg/repaint/inner-svg-change-viewBox-contract.svg:7
> +function resizeViewBox() { 

Rename this to repaintTest(), as it gets called by runRepaintTest for you,
after the initial painting finished.

> LayoutTests/svg/repaint/inner-svg-change-viewBox-contract.svg:10
> +    if (window.layoutTestController)
> +	   layoutTestController.notifyDone();

This can be removed as well.

> Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:68
> +    m_viewbox = svg->currentViewBoxRect();

Slightly worried that we have to store m_viewbox and m_viewport now.
I think viewBox changes should be handled from the SVG DOM side, if viewBox
attr changes. I'll investigate a bit and come back to your patch soon.


More information about the webkit-reviews mailing list