Bug 34185

(In reply to comment #0) > Created an attachment (id=47450) [details] > Reduction > > This regressed with: http://trac.webkit.org/changeset/52449 > > Open the attached test case and click "Move object." The masked image should > shift to the right keeping everything intact. But in reality the mask is left > behind and is clipped to the left. > > <rdar://problem/7577782> Hi Beth, do you have a more simple test case for reduction? Not sure what causes this problem at the moment. Is the problem related to the patterns?

Unfortunately, I don't have a more reduced test case right now :-(. Someone else filed the bug in Radar, and I just moved it here. I have been looking at it as well, and I will let you know if I come up with a simpler test or an idea of what in the patch caused this.

Btw, I can't reproduce it with a one week old build. Checking trunk now.

You often have to click "Move object" and "Restore object" several times to see the bug.

Still can't reproduce it with trunk. I pressed the 2 buttons about 50 times. I don't see any problems. The apple image moves from left to right and back, no clipping problems on WebKitGtk. Just much faster than in the past :-)

It reproduces within a maximum of 5 clicks on the Mac. Odd that is is platform-dependent.

Created attachment 47506 [details] shrinked test This is the basic version of the test above. The green rect should just move but not resize on clicking the text. The author deletes and recreates the element that uses the mask on every click. Saddly this doesn't cause a invalidation of the mask. We make the assignment of an element to it's resource (mask here) with the elements RenderObject. I'm not sure why, but mask can't differentiate between the RenderObject of the old element and the one of the new element here. Note: if the author would just modify the attributes instead of recreating the element, the mask would invalidate and we don't see the clipping. :-/

I have a patch for this. Will post shortly.

Created attachment 48529 [details] Patch

Comment on attachment 48529 [details] Patch > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 54627) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,25 @@ > +2010-02-10 Beth Dakin <bdakin@apple.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Fix for https://bugs.webkit.org/show_bug.cgi?id=34185 REGRESSION: > + Mask not updated according to transform > + > + SVGMaskElement is the only class that keeps a HashMap of canvas > + resources rather than just a since pointer to a resource. This mis-typing here: "since pointer". > Index: WebCore/svg/SVGMaskElement.cpp > =================================================================== > --- WebCore/svg/SVGMaskElement.cpp (revision 54613) > +++ WebCore/svg/SVGMaskElement.cpp (working copy) > @@ -105,9 +105,6 @@ void SVGMaskElement::svgAttributeChanged > { > SVGStyledElement::svgAttributeChanged(attrName); > > - if (m_masker.isEmpty()) > - return; I'm not sure why this early return can go away, and the changelog doesn't say? > +void SVGMaskElement::invalidateCanvasResources() > +{ > + if (m_masker.isEmpty()) > + return; > + > + for (HashMap<const RenderObject*, RefPtr<SVGResourceMasker> >::iterator it = m_masker.begin(); it != m_masker.end(); ++it) > + it->second->invalidate(); Can that be a const_iterator? You may have to make a local variable to store m_masker.end(). > Index: WebCore/svg/SVGStyledElement.h > =================================================================== > --- WebCore/svg/SVGStyledElement.h (revision 54613) > +++ WebCore/svg/SVGStyledElement.h (working copy) > @@ -62,6 +62,7 @@ namespace WebCore { > > void invalidateResourcesInAncestorChain() const; > void invalidateResources(); > + virtual void invalidateCanvasResources(); It's unfortunate to propagate the 'canvas' syntax because that is easily confused with HTML canvas. Perhaps add a comment here to explain how invalidateResources() is different from invalidateCanvasResources(). r=me

Comment on attachment 48529 [details] Patch I reviewed too. Some comments: > + if (m_masker.isEmpty()) > + return; > + > + for (HashMap<const RenderObject*, RefPtr<SVGResourceMasker> >::iterator it = m_masker.begin(); it != m_masker.end(); ++it) > + it->second->invalidate(); The early return here (you moved it but did not write it) is not needed. The loop below will efficiently handle the case where m_masker is empty without an explicit empty check. We are intentionally not calling through to the base class version of invalidateCanvasResources? Maybe a brief comment in this function explaining why that's the right thing to do would be a good addition. I suggest making the SVGStyledElement invalidateCanvasResources protected and the SVGMaskElement override of invalidateCanvasResources private. Unless you feel strongly they should be public. Making such functions private can help catch cases where we do a needless virtual function call. Like in the two places in SVGMaskElement. It's a slight shame to call a virtual function when we know it's never overridden, but probably OK to leave as-is. SVGStyledElement::svgAttributeChanged already calls both invalidateResourcesInAncestorChain and invalidateResources. I'm surprised that we still need an override in SVGMaskElement ::svgAttributeChanged to call invalidateCanvasResources. But maybe it is still needed; it does seem that invalidateResourcesInAncestorChain does not invalidate its own resources. r=me

(In reply to comment #11) > > SVGStyledElement::svgAttributeChanged already calls both > invalidateResourcesInAncestorChain and invalidateResources. I'm surprised that > we still need an override in SVGMaskElement ::svgAttributeChanged to call > invalidateCanvasResources. But maybe it is still needed; it does seem that > invalidateResourcesInAncestorChain does not invalidate its own resources. > In this particular case, the SVGMaskElement is one of the ancestors, so ::svgAttributeChanged is not called for this mask; the code that I changed in invalidateResourcesInAncestorChain() does the invalidating. In the case where the mask is not the ancestor, you are correct that the problem is that invalidateResourcesInAncestorChain() does not invalidate itself. Thanks for the reviews! I am addressing your comments now.

Fixed with r54638.

(In reply to comment #13) > Fixed with r54638. I think the changes in SVGStyledElement are not correct. The goal was to just invalidate one Resource, not all. Imagine one Mask with multiple objects calling it. One object has attribute changes. Why should we invalidate all Resources? This is a performance lost.

(In reply to comment #14) > (In reply to comment #13) > > Fixed with r54638. > > I think the changes in SVGStyledElement are not correct. The goal was to just > invalidate one Resource, not all. > Imagine one Mask with multiple objects calling it. One object has attribute > changes. Why should we invalidate all Resources? This is a performance lost. hehe sorry. Looks like I have ignored SVGStyledElement::invalidateCanvasResources :-)