Closed Bug 691194 Opened 8 years ago Closed 8 years ago

Move filter element attribute processing to the frame class

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: longsonr, Assigned: longsonr)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #564080 - Attachment is patch: true
Attachment #564080 - Flags: review?(dholbert)
Assignee: nobody → longsonr
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?)
(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.
Attached patch updatedSplinter Review
Attachment #564080 - Attachment is obsolete: true
Attachment #564080 - Flags: review?(dholbert)
Attachment #564472 - Flags: review?(dholbert)
Attachment #564472 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/2a151eaf035b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.