869657 - Fix orphaned IsEventName impls in SVG

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

As noted in bug 811753 comment 12, that bug was intended to rename the existing IsEventName methods in SVG classes to IsEventAttributeName, and then remove nsSVGElement::IsEventName

However: we have two remaining (orphaned) IsEventName implementations, which are incorrectly flagged as nsSVGElement overrides:
 http://mxr.mozilla.org/mozilla-central/search?string=IsEventName%28&find=%2Fcontent%2Fsvg

Links:
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGAltGlyphElement.h?rev=e98c80465618#47
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGAltGlyphElement.cpp?rev=e98c80465618#92
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGTSpanElement.cpp?rev=e98c80465618#59
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGTSpanElement.h?rev=e98c80465618#36

Both of these methods are just calls to
  nsContentUtils::IsEventAttributeName(aName, EventNameType_SVGGraphic);
and both of these methods are currently never invoked (since they're orphaned)

Not sure yet whether they need renaming or just removing.

Comment 1

[Julien Levesy]

Well. 
I can't answer, i don't have the necessary knowledge.
But I can do the job, as soon as you tell me what to do.

Comment 2

[Daniel Holbert [:dholbert]]

Thanks, Julien!

So: actually: after further investigation, it looks like bug 811753's patch did correctly convert these methods:
  AltGlyph: https://hg.mozilla.org/mozilla-central/rev/dd897bfc2469#l9.1
  TSpan: https://hg.mozilla.org/mozilla-central/rev/dd897bfc2469#l18.1

***but*** those changes were accidentally reverted (probably in a busted bitrot-fix) in the patches that converted these files to WebIDL, in bug 825732:
-AltGlyph getting reverted: https://hg.mozilla.org/mozilla-central/rev/59ae61d1d059#l2.185
-TSpan getting reverted: https://hg.mozilla.org/mozilla-central/rev/c4ba770505d9#l2.122


So, Julien: I think what we need to do here is just re-apply your fix from bug 811753 to these files -- just rename the "IsEventName" methods to "IsEventAttributeName", in SVGAltGlyphElement.* and SVGTSpanElement.*, and mark them as MOZ_OVERRIDE.

Comment 3

[Daniel Holbert [:dholbert]]

(I'm saying "re-apply your fix" loosely -- not asking you to re-apply the exact same patch, but rather to re-apply your patch strategy from that other bug, since it got accidentally reversed in these two instances.)

Comment 4

[Julien Levesy]

Attachment: Bug 869657 : renamed IsEventName to IsEventAttributeName in SVGAltGlyphElement and SVGTSpanElement.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 747313 [details] [diff] [review]
Bug 869657 : renamed IsEventName to IsEventAttributeName in SVGAltGlyphElement and SVGTSpanElement.

>+++ b/content/svg/content/src/SVGAltGlyphElement.cpp
>@@ -84,17 +84,17 @@ SVGAltGlyphElement::IsAttributeMapped(co
>   return FindAttributeDependence(name, map) ||
>     SVGAltGlyphElementBase::IsAttributeMapped(name);
> }
> 
> //----------------------------------------------------------------------
> // nsSVGElement overrides
> 
> bool
>-SVGAltGlyphElement::IsEventName(nsIAtom* aName)
>+SVGAltGlyphElement::IsEventAttributeName(nsIAtom* aName)

From skimming your patch back in bug 811753: it looks like  is no longer a "nsSVGElement override", so that comment should move down to below this method (in between it and GetStringInfo). 

>diff --git a/content/svg/content/src/SVGAltGlyphElement.h b/content/svg/content/src/SVGAltGlyphElement.h
>--- a/content/svg/content/src/SVGAltGlyphElement.h
>+++ b/content/svg/content/src/SVGAltGlyphElement.h
>@@ -39,17 +39,17 @@ public:
>   void GetFormat(nsAString & aFormat);
>   void SetFormat(const nsAString & aFormat, ErrorResult& rv);
> 
> protected:
> 
>   // nsSVGElement overrides
>   virtual StringAttributesInfo GetStringInfo() MOZ_OVERRIDE;
> 
>-  virtual bool IsEventName(nsIAtom* aName);
>+  virtual bool IsEventAttributeName(nsIAtom* aName) MOZ_OVERRIDE;

IsEventAttributeName is a public method (and isn't a nsSVGElement override) -- so -- move its line up to before "protected" in this chunk, as you did in bug 811753.

>+++ b/content/svg/content/src/SVGTSpanElement.cpp
>@@ -51,15 +51,15 @@ SVGTSpanElement::IsAttributeMapped(const
>   return FindAttributeDependence(name, map) ||
>     SVGTSpanElementBase::IsAttributeMapped(name);
> }
> 
> //----------------------------------------------------------------------
> // nsSVGElement overrides
> 
> bool
>-SVGTSpanElement::IsEventName(nsIAtom* aName)
>+SVGTSpanElement::IsEventAttributeName(nsIAtom* aName)

Just delete the "nsSVGElement overrides" comment here, since there aren't any of those overrides in this file now.
(as you did in bug 811753)

>@@ -28,15 +28,15 @@ protected:
> public:
>   // nsIContent interface
>   NS_IMETHOD_(bool) IsAttributeMapped(const nsIAtom* aAttribute) const MOZ_OVERRIDE;
> 
>   virtual nsresult Clone(nsINodeInfo *aNodeInfo, nsINode **aResult) const MOZ_OVERRIDE;
> protected:
> 
>   // nsSVGElement overrides
>-  virtual bool IsEventName(nsIAtom* aName);
>+  virtual bool IsEventAttributeName(nsIAtom* aName) MOZ_OVERRIDE;
> };

...and delete the "protected" and "nsSVGElement overrides" lines here, too. (since IsEventAttributeName is public, not protected, and there aren't any overrides)

Thanks!

Comment 6

[Daniel Holbert [:dholbert]]

(...[and there aren't any] _nsSVGElement_ overrides, that is. IsEventAttributeName is now an override/implementation for a method on _nsIContent_, not nsSVGElement.)

Comment 7

[Julien Levesy]

Attachment: Bug 869657 : renamed IsEventName to IsEventAttributeName in SVGAltGlyphElement and SVGTSpanElement. r2

Here is a new patch.
Sorry about that.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 747478 [details] [diff] [review]
Bug 869657 : renamed IsEventName to IsEventAttributeName in SVGAltGlyphElement and SVGTSpanElement. r2

>+++ b/content/svg/content/src/SVGAltGlyphElement.h
>@@ -34,23 +34,23 @@ public:
> 
>   // WebIDL
>   already_AddRefed<nsIDOMSVGAnimatedString> Href();
>   void GetGlyphRef(nsAString & aGlyphRef);
>   void SetGlyphRef(const nsAString & aGlyphRef, ErrorResult& rv);
>   void GetFormat(nsAString & aFormat);
>   void SetFormat(const nsAString & aFormat, ErrorResult& rv);
> 
>+  virtual bool IsEventAttributeName(nsIAtom* aName) MOZ_OVERRIDE;
>+


This isn't a "// WebIDL" method, either -- it's a nsIContent method implementation. So move this line up even higher, to the nsIContent section (just above "WebIDL").

[sorry for not noticing that in the last patch -- the WebIDL wasn't in the patch-context there, so I didn't realize what the chunk of methods just before the "protected" was.]

r=me with that. Thanks!

Comment 9

[Julien Levesy]

Attachment: Bug 869657 : renamed IsEventName to IsEventAttributeName in SVGAltGlyphElement and SVGTSpanElement.

Here it is.
I should have paid a little bit more attention to comments sections. Sorry about that

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 747505 [details] [diff] [review]
Bug 869657 : renamed IsEventName to IsEventAttributeName in SVGAltGlyphElement and SVGTSpanElement.

No worries. Thanks for the patch! This version looks good -- r=me.

Comment 11

[Daniel Holbert [:dholbert]]

I pushed this to TryServer as a sanity-check:
 https://tbpl.mozilla.org/?tree=Try&rev=231f383be347
and assuming it passes there, we can go ahead and land it.  needinfo?me to remind me.

Comment 12

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d43e27c6362e

Comment 13

[Ed Morley [:emorley]]

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

Comment 14

[Robert Longson [:longsonr]]

When originally written (prior to webidl) this code was all correct, however WebIDL implemented the SVG 2 hierarchy which means that SVGAltGlyphElement and SVGTSpanElement both now inherit from SVGTransformableElement which does this...

http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGTransformableElement.cpp#80

So the correct solution in both cases should have been to remove the code as it makes no functional change in the override. Can someone raise a followup for that please?

Note that we also have an existing spurious copy of IsEventAttributeName in SVGTextPathElement so that can go on the bonfire too.

Comment 15

[Daniel Holbert [:dholbert]]

(Robert is addressing comment 14 in bug 872574, so no further action needs to be taken here.)
 
Thanks for catching & fixing that, Robert! (and sorry for not noticing that myself)