Summary: | Animation <use> fixes | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | eric, webkit | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Antti Koivisto
2008-04-14 00:18:59 PDT
Created attachment 20522 [details]
<use> fixes
I guess setInstanceUpdatesBlocked isn't used? 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. 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)
Created attachment 20537 [details]
added comment and assert to removeDisallowedElementsFromSubtree()
no code changes
Created attachment 20538 [details]
...and rename the requested variable
(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. Comment on attachment 20538 [details]
...and rename the requested variable
r=me
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. |