[Webkit-unassigned] [Bug 34185] REGRESSION: Mask not invalidating

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=34185





--- Comment #11 from Darin Adler <darin at apple.com>  2010-02-10 16:27:50 PST ---
(From update of attachment 48529)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.