[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