Bug 34185 – REGRESSION: Mask not invalidating

• Component: SVG
• Status: RESOLVED FIXED

[Beth Dakin]

Created attachment 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>

[Dirk Schulze]

(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?

[Beth Dakin]

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.

[Dirk Schulze]

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

[Beth Dakin]

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

[Dirk Schulze]

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 :-)

[Beth Dakin]

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

[Dirk Schulze]

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. :-/

[Beth Dakin]

I have a patch for this. Will post shortly.

[Beth Dakin]

Created attachment 48529 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 48529 [details]
Patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 54627)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,25 @@
> [phone]  Beth Dakin  <[e-mail]>
> +
> +        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

[Darin Adler]

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

[Beth Dakin]

(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.

[Beth Dakin]

Fixed with r54638.

[Dirk Schulze]

(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.

[Dirk Schulze]

(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 :-)