Bug 18473 – Animation <use> fixes

• Component: SVG
• Status: RESOLVED FIXED

[Antti Koivisto]

Animations should not be cloned to instance trees etc.

[Antti Koivisto]

Created attachment 20522 [details]
<use> fixes

[Eric Seidel (no email)]

I guess setInstanceUpdatesBlocked isn't used?

[Antti Koivisto]

Not yet. I need it for the next patch that does additive animations and <use> properly. I added it here that keep that one bit smaller.

[Eric Seidel (no email)]

Comment on attachment 20522 [details]
<use> fixes

I prefer arguments to have more informative names than "b":
setInstanceUpdatesBlocked
"blockUpdates" perhaps?

Is gElementsWithInstanceUpdatesBlocked the official pattern now? I thought s_ was?

 void SVGUseElement::removeDisallowedElementsFromSubtree(Node* element)

needs a comment as to why RefPtr<Node> is not required to protect against MutationEvent listeners doing bad things.

+bool SVGSMILElement::isSMILElement(Node* node)
I'm surprised that's even needed.  I guess I would have figured there would have been a virtual bool isSVGSMILElement() const { return false; } on SVGElement.

r- due to the possible crasher due to mutation events (or lack of comment)

[Antti Koivisto]

Created attachment 20537 [details]
added comment and assert to removeDisallowedElementsFromSubtree()

no code changes

[Antti Koivisto]

Created attachment 20538 [details]
...and rename the requested variable

[Antti Koivisto]

(In reply to comment #4)

> Is gElementsWithInstanceUpdatesBlocked the official pattern now? I thought s_
> was?

s_ is for static member variables only.

> needs a comment as to why RefPtr<Node> is not required to protect against
> MutationEvent listeners doing bad things.

The subtree is not in document so mutation events won't happen. Added assert and comment.

Generally using RefPtr<> in this kind of places is bad idea since it is a) slow, b) hides bugs.

> 
> +bool SVGSMILElement::isSMILElement(Node* node)
> I'm surprised that's even needed.  I guess I would have figured there would
> have been a virtual bool isSVGSMILElement() const { return false; } on
> SVGElement.

That might be good to add as well, but this kind of interface has advantage of being usable without casting for Node*'s.

[Eric Seidel (no email)]

Comment on attachment 20538 [details]
...and rename the requested variable

r=me

[Antti Koivisto]

Sending        WebCore/ChangeLog
Sending        WebCore/svg/SVGAElement.cpp
Sending        WebCore/svg/SVGElementInstance.cpp
Sending        WebCore/svg/SVGStyledElement.cpp
Sending        WebCore/svg/SVGStyledElement.h
Sending        WebCore/svg/SVGUseElement.cpp
Sending        WebCore/svg/SVGUseElement.h
Sending        WebCore/svg/animation/SVGSMILElement.cpp
Sending        WebCore/svg/animation/SVGSMILElement.h
Transmitting file data .........
Committed revision 31891.