Move filter element attribute processing to the frame class

RESOLVED FIXED in mozilla10

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Robert Longson, Assigned: Robert Longson)

Tracking

Trunk
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

5.18 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
Created attachment 564080 [details] [diff] [review]
patch
(Assignee)

Updated

6 years ago
Attachment #564080 - Attachment is patch: true
Attachment #564080 - Flags: review?(dholbert)
(Assignee)

Updated

6 years ago
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?)
(Assignee)

Comment 2

6 years ago
(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.
(Assignee)

Comment 3

6 years ago
Created attachment 564472 [details] [diff] [review]
updated
Attachment #564080 - Attachment is obsolete: true
Attachment #564080 - Flags: review?(dholbert)
Attachment #564472 - Flags: review?(dholbert)
Attachment #564472 - Flags: review?(dholbert) → review+
(Assignee)

Comment 4

6 years ago
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/2a151eaf035b
https://hg.mozilla.org/mozilla-central/rev/2a151eaf035b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.