Closed
Bug 693145
Opened 13 years ago
Closed 13 years ago
simplify class animation processing
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: longsonr, Assigned: longsonr)
References
Details
Attachments
(1 file, 3 obsolete files)
17.93 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
If we move all processing to nsSVGStylableElement then we don't need virtual methods in nsSVGElement to animate class attributes.
Assignee | ||
Comment 1•13 years ago
|
||
Assignee: nobody → longsonr
Attachment #565794 -
Flags: review?(dholbert)
Comment 2•13 years ago
|
||
Comment on attachment 565794 [details] [diff] [review] patch Drive-by nits: >--- a/content/svg/content/src/nsSVGClass.cpp >+++ b/content/svg/content/src/nsSVGClass.cpp >+#include "nsSVGStylableElement.h" > #include "nsSVGClass.h" Foo.h should be included first in Foo.cpp, to ensure Foo.h compiles on it own. >+nsSVGClass::GetBaseValue(nsAString& aValue, nsSVGStylableElement *aSVGElement) const >+{ >+ aSVGElement->GetAttr(kNameSpaceID_None, nsGkAtoms::_class, aValue); aSVGElement can be const. >--- a/content/svg/content/src/nsSVGStylableElement.cpp >+++ b/content/svg/content/src/nsSVGStylableElement.cpp >+nsSVGStylableElement::ParseAttribute(PRInt32 aNamespaceID, >+ nsIAtom* aAttribute, >+ const nsAString& aValue, >+ nsAttrValue& aResult) >+{ >+ if (aAttribute == nsGkAtoms::_class) { >+ mClassAttribute.SetBaseValue(aValue, this, PR_FALSE); >+ aResult.ParseAtomArray(aValue); >+ return PR_TRUE; false/true
Assignee | ||
Comment 3•13 years ago
|
||
(In reply to Ms2ger from comment #2) > Comment on attachment 565794 [details] [diff] [review] [diff] [details] [review] > patch > > Drive-by nits: > > >--- a/content/svg/content/src/nsSVGClass.cpp > >+++ b/content/svg/content/src/nsSVGClass.cpp > > >+#include "nsSVGStylableElement.h" > > #include "nsSVGClass.h" > > Foo.h should be included first in Foo.cpp, to ensure Foo.h compiles on it > own. Both classes reference each other. Change of order results in non-compilation. > >+{ > >+ if (aAttribute == nsGkAtoms::_class) { > >+ mClassAttribute.SetBaseValue(aValue, this, PR_FALSE); > >+ aResult.ParseAtomArray(aValue); > >+ return PR_TRUE; > > false/true bug 690892 will do that.
Comment 4•13 years ago
|
||
(In reply to Robert Longson from comment #3) > (In reply to Ms2ger from comment #2) > > Comment on attachment 565794 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review] > > patch > > > > Drive-by nits: > > > > >--- a/content/svg/content/src/nsSVGClass.cpp > > >+++ b/content/svg/content/src/nsSVGClass.cpp > > > > >+#include "nsSVGStylableElement.h" > > > #include "nsSVGClass.h" > > > > Foo.h should be included first in Foo.cpp, to ensure Foo.h compiles on it > > own. > > Both classes reference each other. Change of order results in > non-compilation. I see, but that's a bug by itself.
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #565794 -
Attachment is obsolete: true
Attachment #565794 -
Flags: review?(dholbert)
Attachment #565796 -
Flags: review?(dholbert)
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to Ms2ger from comment #4) > I see, but that's a bug by itself. nsSVGStyleable element contains nsSVGClass as a member so you need a concrete nsSVGClass to compile it. nsSVGClass contains nsSVGStyleable as a pointer so you can get by with just a class nsSVGClass in the header but you need a concrete class in the cpp and that needs to come first to compile. This change saves 8 bytes for each svg tag as there are 2 fewer virtual functions. It's also faster as we're refreshing once instead of twice when the class attribute changes now.
Assignee | ||
Comment 7•13 years ago
|
||
When I say each svg tag I mean each tag in the svg namespace not just the <svg> tag itself.
Comment 8•13 years ago
|
||
This patch doesn't seem to apply cleanly on mozilla-central (even with -F10) -- does it layer on top of something else? Or perhaps something landed that bitrotted it?
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #565796 -
Attachment is obsolete: true
Attachment #565796 -
Flags: review?(dholbert)
Attachment #565999 -
Flags: review?(dholbert)
Assignee | ||
Comment 10•13 years ago
|
||
oh and it only saves 8 byes per class, not per instance.
Comment 11•13 years ago
|
||
(In reply to Ms2ger from comment #2) > >--- a/content/svg/content/src/nsSVGClass.cpp > >+++ b/content/svg/content/src/nsSVGClass.cpp > > >+#include "nsSVGStylableElement.h" > > #include "nsSVGClass.h" > > Foo.h should be included first in Foo.cpp, to ensure Foo.h compiles on it > own. [...] (In reply to Robert Longson from comment #6) > nsSVGClass contains nsSVGStyleable as a pointer so you can get by with just > a class nsSVGClass in the header but you need a concrete class in the cpp > and that needs to come first to compile. No, it [nsSVGStyleable] only needs to come first to compile because nsSVGClass is missing some #includes. :) (which switching the ordering reveals) Specifically, nsSVGClass.h needs #includes for "nsAutoPtr.h" and "nsCycleCollectionParticipant.h". With those, you can have nsSVGClass.h as the first include in nsSVGClass.cpp, and everything works fine. (also, there's still some bitrot from bug 550047's s/return rv/return/ tweaks to UnsetAttr/UnsetAttrInternal/) (Still reviewing, that's just what I've got so far. :))
Comment 12•13 years ago
|
||
Comment on attachment 565999 [details] [diff] [review] unbitrotted >diff --git a/content/svg/content/src/nsSVGClass.h b/content/svg/content/src/nsSVGClass.h > void SetBaseValue(const nsAString& aValue, >- nsSVGElement *aSVGElement, >+ nsSVGStylableElement *aSVGStylableElement, Revert the argument-name-change here. There are a lot of lines in nsSVGClass.* with "aSVGElement" and "mSVGElement", and they probably all want to be renamed after this patch lands, but this is the only line of your path with such a rename. (and note that the definition of this function in the .cpp file still uses the old name "aSVGElement") I agree that we should rename these variables, but I think it'd be best to do that in a followup patch (or followup bug), since it's more of a mechanical change which would make the exact same change to a whole bunch of lines. (Also, "aSVGStylableElement" might be an overly long name -- I think "aElement" would probably be fine here -- but it doesn't matter much to me.) >+bool >+nsSVGStylableElement::ParseAttribute(PRInt32 aNamespaceID, >+ nsIAtom* aAttribute, [...] >+{ >+ if (aAttribute == nsGkAtoms::_class) { >+ mClassAttribute.SetBaseValue(aValue, this, PR_FALSE); This looks like it needs a check for "aNamespaceID == kNameSpaceID_None". >+nsresult >+nsSVGStylableElement::UnsetAttr(PRInt32 aNamespaceID, nsIAtom* aName, >+ bool aNotify) >+{ >+ if (aName == nsGkAtoms::_class) { >+ mClassAttribute.Init(); >+ return nsSVGElementBase::UnsetAttr(aNamespaceID, aName, aNotify); >+ } >+ return nsSVGStylableElementBase::UnsetAttr(aNamespaceID, aName, aNotify); This too. Also: I'm not sold on the "nsSVGElementBase::UnsetAttr" early-return in the above chunk. It looks like you're doing that as a shortcut, because you know that nsSVGStylableElementBase (aka nsSVGElement) won't do anything special for "class", so you're jumping straight *its* final line where it passes off to its base class. It seems fragile to hardcode that assumption (and the implicit assumption about class-ancestry) here, though. I'd rather we get rid of this early-return, and just unconditionally hit the "nsSVGStylableElementBase::UnsetAttr()" call. (If we were to keep the nsSVGElementBase::UnsetAttr early-return, though, we should definitely have a comment explaining why it's there.) >+++ b/content/svg/content/src/nsSVGStylableElement.h >+ virtual bool ParseAttribute(PRInt32 aNamespaceID, nsIAtom* aAttribute, >+ const nsAString& aValue, nsAttrValue& aResult); Fix up indentation before "const nsAString& aValue" there. >+++ b/content/svg/content/src/nsSVGElement.cpp >-void >-nsSVGElement::DidAnimateClass() >-{ >- nsIFrame* frame = GetPrimaryFrame(); >- >- if (frame) { >- frame->AttributeChanged(kNameSpaceID_None, nsGkAtoms::_class, >- nsIDOMMutationEvent::MODIFICATION); >- } So, the patch removes this function -- but the corresponding function in nsSVGStylableElement doesn't notify the frame, so it looks like we'll no longer be sending frame->AttributeChanged() notifications when "class" changes. Is that a problem?
Assignee | ||
Comment 13•13 years ago
|
||
Attachment #565999 -
Attachment is obsolete: true
Attachment #565999 -
Flags: review?(dholbert)
Attachment #567297 -
Flags: review?(dholbert)
Assignee | ||
Comment 14•13 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #12) > > So, the patch removes this function -- but the corresponding function in > nsSVGStylableElement doesn't notify the frame, so it looks like we'll no > longer be sending frame->AttributeChanged() notifications when "class" > changes. Is that a problem? It appears not to be, class animations refresh normally and the reftests that check that still pass.
Comment 15•13 years ago
|
||
Comment on attachment 567297 [details] [diff] [review] address review comments ok, thanks! r=me
Attachment #567297 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 16•13 years ago
|
||
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/d7295730c0ef
Assignee | ||
Updated•13 years ago
|
Flags: in-testsuite-
Comment 17•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d7295730c0ef
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in
before you can comment on or make changes to this bug.
Description
•