693145 - simplify class animation processing

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

If we move all processing to nsSVGStylableElement then we don't need virtual methods in nsSVGElement to animate class attributes.

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 [:longsonr]]

(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 (he/him; ⌚ UTC+1/+2)]

(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 [:longsonr]]

Attachment: make GetBaseValue argument const

Comment 6

[Robert Longson [:longsonr]]

(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 [:longsonr]]

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]]

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 [:longsonr]]

Attachment: unbitrotted

Comment 10

[Robert Longson [:longsonr]]

oh and it only saves 8 byes per class, not per instance.

Comment 11

[Daniel Holbert [:dholbert]]

(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]]

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 [:longsonr]]

Attachment: address review comments

Comment 14

[Robert Longson [:longsonr]]

(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]]

Comment on attachment 567297 [details] [diff] [review]
address review comments

ok, thanks! r=me

Comment 16

[Robert Longson [:longsonr]]

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/d7295730c0ef

Comment 17

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/d7295730c0ef