691194 - Move filter element attribute processing to the frame class

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: patch

Comment 1

[Daniel Holbert [:dholbert]]

Comment on attachment 564080 [details] [diff] [review]
patch

This patch seems mostly straightforwardly behavior-preserving, except for this part:

>+nsSVGFilterFrame::AttributeChanged(PRInt32  aNameSpaceID,
>+                                   nsIAtom* aAttribute,
>+                                   PRInt32  aModType)
>+{
>+  } else if (aNameSpaceID == kNameSpaceID_XLink &&
>+             aAttribute == nsGkAtoms::href) {
>+    // Blow away our reference, if any
>+    Properties().Delete(nsSVGEffects::HrefProperty());

This property-delete doesn't seem to be something we were doing before.  A quick grep for nsSVGEffects::HrefProperty has hits in 3 files, with 3 hits per file, all following the same pattern (each file has 1 hit each for Delete(), Get(), and GetXXXProperty()).
http://mxr.mozilla.org/mozilla-central/search?string=nsSVGEffects%3A%3AHrefProperty

None of those hits are in nsSVGFilterFrame, or any of its parent / related classes AFAICT.  So, I don't immediately see how it'd get set for a nsSVGFilterFrame (and need deleting).  Maybe I'm missing something, though?

So, perhaps we don't need this line?  (Or, if we do, could you add a regression test for it?)

Comment 2

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> None of those hits are in nsSVGFilterFrame, or any of its parent / related
> classes AFAICT.  So, I don't immediately see how it'd get set for a
> nsSVGFilterFrame (and need deleting).  Maybe I'm missing something, though?

No, it's just wrong, a remnant of the copy paste from the gradientFrame code I used to create this.

> 
> So, perhaps we don't need this line?  (Or, if we do, could you add a
> regression test for it?)

I'll remove it.

Comment 3

[Robert Longson [:longsonr]]

Attachment: updated

Comment 4

[Robert Longson [:longsonr]]

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

Comment 5

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/2a151eaf035b