Last Comment Bug 693145 - simplify class animation processing
: simplify class animation processing
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Robert Longson
:
: Jet Villegas (:jet)
Mentors:
Depends on: 698534
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-09 04:45 PDT by Robert Longson
Modified: 2011-10-31 12:12 PDT (History)
3 users (show)
longsonr: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (16.77 KB, patch)
2011-10-09 04:46 PDT, Robert Longson
no flags Details | Diff | Splinter Review
make GetBaseValue argument const (17.78 KB, patch)
2011-10-09 05:33 PDT, Robert Longson
no flags Details | Diff | Splinter Review
unbitrotted (17.78 KB, patch)
2011-10-10 12:52 PDT, Robert Longson
no flags Details | Diff | Splinter Review
address review comments (17.93 KB, patch)
2011-10-15 13:13 PDT, Robert Longson
dholbert: review+
Details | Diff | Splinter Review

Description Robert Longson 2011-10-09 04:45:18 PDT
If we move all processing to nsSVGStylableElement then we don't need virtual methods in nsSVGElement to animate class attributes.
Comment 1 Robert Longson 2011-10-09 04:46:01 PDT
Created attachment 565794 [details] [diff] [review]
patch
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2011-10-09 05:03:59 PDT
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
Comment 3 Robert Longson 2011-10-09 05:29:34 PDT
(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 :Ms2ger (⌚ UTC+1/+2) 2011-10-09 05:32:06 PDT
(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.
Comment 5 Robert Longson 2011-10-09 05:33:33 PDT
Created attachment 565796 [details] [diff] [review]
make GetBaseValue argument const
Comment 6 Robert Longson 2011-10-09 05:40:28 PDT
(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.
Comment 7 Robert Longson 2011-10-09 05:41:39 PDT
When I say each svg tag I mean each tag in the svg namespace not just the <svg> tag itself.
Comment 8 Daniel Holbert [:dholbert] 2011-10-10 12:43:20 PDT
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?
Comment 9 Robert Longson 2011-10-10 12:52:17 PDT
Created attachment 565999 [details] [diff] [review]
unbitrotted
Comment 10 Robert Longson 2011-10-10 12:56:01 PDT
oh and it only saves 8 byes per class, not per instance.
Comment 11 Daniel Holbert [:dholbert] 2011-10-10 13:54:46 PDT
(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 Daniel Holbert [:dholbert] 2011-10-10 15:16:24 PDT
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?
Comment 13 Robert Longson 2011-10-15 13:13:15 PDT
Created attachment 567297 [details] [diff] [review]
address review comments
Comment 14 Robert Longson 2011-10-15 13:14:35 PDT
(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 Daniel Holbert [:dholbert] 2011-10-17 17:02:32 PDT
Comment on attachment 567297 [details] [diff] [review]
address review comments

ok, thanks! r=me
Comment 17 Marco Bonardo [::mak] 2011-10-19 03:04:37 PDT
https://hg.mozilla.org/mozilla-central/rev/d7295730c0ef

Note You need to log in before you can comment on or make changes to this bug.