Bug 77903

Created attachment 126897 [details] Proposed Patch A patch proposal. The problem occurred because repainting used the new viewbox transform for both the old and new bounding rectangles. The patch updates the transform after the old bounds have been marked as part of layout. Running the bots, then will mark for review.

Comment on attachment 126897 [details] Proposed Patch 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.

Created attachment 127037 [details] Updated Patch Updated the test to use the repaint harness. Updated the code transform invalidation to use the svg dom rather than the renderer, which avoids storing the viewbox rectangle.

Comment on attachment 127037 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127037&action=review I think this is the right approach. In general a viewBox is nothing else than a transform. r- because of some snippets. > LayoutTests/svg/repaint/inner-svg-change-viewBox-contract.svg:10 > +<!-- blue rect's lower right edges should be visible after viewbox resizing --> It is not. Is that because of the repaint logic? > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:82 > + bool needsUpdate = m_needsTransformUpdate; > + if (needsUpdate) { > + m_localToParentTransform = AffineTransform::translation(m_viewport.x(), m_viewport.y()) * viewportTransform(); > + // If this class were ever given a localTransform(), then the above would read: > + // return viewportTranslation * localTransform() * viewportTransform() > + m_needsTransformUpdate = false; > + } > + return needsUpdate; why not just if (m_needsTransformUpdate) { ... return true; } return false; Also, should it be If this class _was_ ever given? :P I am not sure about that atm :) > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:-85 > - m_localToParentTransform = AffineTransform::translation(m_viewport.x(), m_viewport.y()) * viewportTransform(); > return m_localToParentTransform; > - // If this class were ever given a localTransform(), then the above would read: > - // return viewportTranslation * localTransform() * viewportTransform() Did you run all tests to verify that we don't get a regression? > Source/WebCore/svg/SVGSVGElement.cpp:297 > + if (renderer()) Why not: if (RenderObject* object = renderer()) object->setNeedsTransformUpdate(); saves a renderer() call. Also, don't we just need to call it for view box updates? What about: if (attrName == SVGNames::viewBoxAttr) { if (RenderObject* object = renderer()) object->setNeedsTransformUpdate(); updateRelativeLengths = true; } After the current if clause? Of course it would violate the name updateRelativeLengths. What about renaming it to updateRelativeLengthsOrViewBox? Of course you have to remove the attName check for viewBox in the later if clause.

Created attachment 127085 [details] Updated Patch Updated expected .png so background is blue. You should see a blue edge on the lower right when running the test case. Simplified boolean logic in RenderSVGViewPortContainer::calculateLocalTransform (As an aside, _were_ should work, as a present subjunctive, but _was_ sounds like it might fit too. Thank goodness I'm not an English teacher) All the tests have been run. I'm going to run the EWS again before marking this for review. Adding the (RenderObject * object = renderer()) to the conditional at SVGSVGElement.cpp:297. Broke up the logic in SVGSVGElement::svgAttributeChanged. - SVGFitToViewBox::isKnownAttribute (viewBox or aspectRatio) only needs to update the transform and be marked for layout. It doesn't need to update the relative lengths information, so I'm moving it to its own conditional. - Removing the viewBox check for layout marking, as this will be covered by the updateRelativeLengthsOrViewBox == true case.

Comment on attachment 127085 [details] Updated Patch Attachment 127085 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11518527

Comment on attachment 127085 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127085&action=review Adding my 50 cent as well. Approach is much nicer this way. > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:75 > + if (m_needsTransformUpdate) { Early exit should be used here. > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:78 > + // If this class were ever given a localTransform(), then the above would read: > + // return viewportTranslation * localTransform() * viewportTransform() The comment can go away, it's aged, and we won't do this :-) > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:97 > const AffineTransform& RenderSVGViewportContainer::localToParentTransform() const > { > - m_localToParentTransform = AffineTransform::translation(m_viewport.x(), m_viewport.y()) * viewportTransform(); > return m_localToParentTransform; Please inline this function as it has no content anymore. > Source/WebCore/svg/SVGSVGElement.cpp:290 > - bool updateRelativeLengths = false; > + bool updateRelativeLengthsOrViewBox = false; This is unncessary, I'd leave out this change and just do a if (SVGFitToViewBox::isKnownAttribute(attrName)) { if (RenderObject* renderer = this->renderer()) { renderer->setNeedsTransformUpdate(); RenderSVGresources::markForLayoutAndParent...( } return; } This way you don't need to touch the rest of the code at all, except removing the attrName == SVGNames::viewBoxAttr from the if (updateRelativeLengths branch below)

(In reply to comment #7) > (From update of attachment 127085 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127085&action=review > > Adding my 50 cent as well. Approach is much nicer this way. > > > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:75 > > + if (m_needsTransformUpdate) { > > Early exit should be used here. > > > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:78 > > + // If this class were ever given a localTransform(), then the above would read: > > + // return viewportTranslation * localTransform() * viewportTransform() > > The comment can go away, it's aged, and we won't do this :-) Be careful with that. The SVG WG decided to support transform on SVGSVGelements for SVG 2.0.

(In reply to comment #7) > (From update of attachment 127085 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127085&action=review > > Adding my 50 cent as well. Approach is much nicer this way. > > > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:75 > > + if (m_needsTransformUpdate) { > > Early exit should be used here. > > > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:78 > > + // If this class were ever given a localTransform(), then the above would read: > > + // return viewportTranslation * localTransform() * viewportTransform() > > The comment can go away, it's aged, and we won't do this :-) > > > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:97 > > const AffineTransform& RenderSVGViewportContainer::localToParentTransform() const > > { > > - m_localToParentTransform = AffineTransform::translation(m_viewport.x(), m_viewport.y()) * viewportTransform(); > > return m_localToParentTransform; > > Please inline this function as it has no content anymore. > > > Source/WebCore/svg/SVGSVGElement.cpp:290 > > - bool updateRelativeLengths = false; > > + bool updateRelativeLengthsOrViewBox = false; > > This is unncessary, I'd leave out this change and just do a > if (SVGFitToViewBox::isKnownAttribute(attrName)) { > if (RenderObject* renderer = this->renderer()) { > renderer->setNeedsTransformUpdate(); > RenderSVGresources::markForLayoutAndParent...( > } > return; > } > This way you don't need to touch the rest of the code at all, except removing the attrName == SVGNames::viewBoxAttr from the if (updateRelativeLengths branch below)

Please ignore my last comment, my mouse has decided to have a field day. I'll make these adjustments in the next patch. I'll leave the comment per Dirk's concerns. > > Source/WebCore/svg/SVGSVGElement.cpp:290 > > - bool updateRelativeLengths = false; > > + bool updateRelativeLengthsOrViewBox = false; > > This is unncessary, I'd leave out this change and just do a > if (SVGFitToViewBox::isKnownAttribute(attrName)) { > if (RenderObject* renderer = this->renderer()) { > renderer->setNeedsTransformUpdate(); > RenderSVGresources::markForLayoutAndParent...( > } > return; > } > This way you don't need to touch the rest of the code at all, except removing the attrName == SVGNames::viewBoxAttr from the if (updateRelativeLengths branch below) As I understand it, this would require moving the invalidation guard as well. If we can keep the setTransformUpdate and markForLayout calls separate, we won't invalidate when we are not yet attached.

Created attachment 127220 [details] Updated Patch Per Nikolas' comments: * Changed to use early exit in RenderSVGViewportContainer.cpp::calculateLocalTransform * Inlined RenderSVGViewportContainer::localToParentTransform() Per Dirk's comments: * Leaving comment at RenderSVGViewportContainer.cpp::calculateLocalTransform intact Leaving SVGSVGElement.cpp::svgAttribute changed as-is for now The code, per Nikolas' comments, would look like: SVGElementInstance::InvalidationGuard invalidationGuard(this); if (SVGFitToViewBox::isKnownAttribute(attrName)) { if (RenderObject* object = renderer()) { object->setNeedsTransformUpdate(); RenderSVGResource::markForLayoutAndParentResourceInvalidation(renderer()); } return; } if (SVGTests::handleAttributeChange(this, attrName)) return; if (updateRelativeLengths || SVGLangSpace::isKnownAttribute(attrName) || SVGExternalResourcesRequired::isKnownAttribute(attrName) || SVGZoomAndPan::isKnownAttribute(attrName)) { if (renderer()) RenderSVGResource::markForLayoutAndParentResourceInvalidation(renderer()); return; } I can change this if it looks better. There is a chance we'd invalidate when we don't have to (SVGTests::handleAttributeChange returns true).

(In reply to comment #8) > > > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:78 > > > + // If this class were ever given a localTransform(), then the above would read: > > > + // return viewportTranslation * localTransform() * viewportTransform() > > > > The comment can go away, it's aged, and we won't do this :-) > Be careful with that. The SVG WG decided to support transform on SVGSVGelements for SVG 2.0. Heh, you misunderstood Erics comment it seems. Eric thought about passing localTransform() as _argument_ to localToParentTransform in his early work, hence the FIXME. It has nothing to do with SVGSVGElements being transformable.

(In reply to comment #11) > I can change this if it looks better. There is a chance we'd invalidate when we don't have to (SVGTests::handleAttributeChange returns true). You're right, I missed the <use> shadow tree invalidation case. Keep it as-is.

(In reply to comment #13) > (In reply to comment #11) > > I can change this if it looks better. There is a chance we'd invalidate when we don't have to (SVGTests::handleAttributeChange returns true). > You're right, I missed the <use> shadow tree invalidation case. Keep it as-is. Btw, can you look at bug 77535 as well, it seems highly related (not viewBox though, but relative length props and inner svgs).

(In reply to comment #12) > (In reply to comment #8) > > > > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:78 > > > > + // If this class were ever given a localTransform(), then the above would read: > > > > + // return viewportTranslation * localTransform() * viewportTransform() > > > > > > The comment can go away, it's aged, and we won't do this :-) > > Be careful with that. The SVG WG decided to support transform on SVGSVGelements for SVG 2.0. > Heh, you misunderstood Erics comment it seems. Eric thought about passing localTransform() as _argument_ to localToParentTransform in his early work, hence the FIXME. > It has nothing to do with SVGSVGElements being transformable. Ok yes, I definitely interpreted it differently :)

Created attachment 127270 [details] Updated Patch Removing comment from RenderSVGViewportContainer::calculateLocalTransform Kicking off the bots again

Comment on attachment 127270 [details] Updated Patch Hm seems the EWS bots are not working. Still it looks great, r=me. Please update the chromium test_expectations.txt file in LayoutTest/platform/chromium and upate the ChangeLog to include this file. It should list sth. like: // New test, needs a rebaseline BUGWK77903 : svg/repaint/innert-sg-change-viewBox-contract.svg = IMAGE IMAGE+TEXT Otherwise chromium will turn red.

Created attachment 127275 [details] Updated Patch Updated LayoutTests/chromium/test_expectations.txt and LayoutTests/ChangeLog

The commit-queue encountered the following flaky tests while processing attachment 127275 [details]: perf/array-reverse.html bug 78593 (author: ojan@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 127275 [details] Updated Patch Clearing flags on attachment: 127275 Committed r107893: <http://trac.webkit.org/changeset/107893>

All reviewed patches have been landed. Closing bug.