Closed Bug 869657 Opened 8 years ago Closed 8 years ago
Fix orphaned Is
Event Name impls in SVG
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.
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.
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.
(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 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!
(...[and there aren't any] _nsSVGElement_ overrides, that is. IsEventAttributeName is now an override/implementation for a method on _nsIContent_, not nsSVGElement.)
Here is a new patch. Sorry about that.
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!
Attachment #747478 - Flags: review?(dholbert) → review+
Here it is. I should have paid a little bit more attention to comments sections. Sorry about that
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.
Attachment #747505 - Flags: review?(dholbert) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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.
(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)
You need to log in before you can comment on or make changes to this bug.