Last Comment Bug 691194 - Move filter element attribute processing to the frame class
: Move filter element attribute processing to the frame class
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Robert Longson
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-02 13:49 PDT by Robert Longson
Modified: 2011-10-10 11:11 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.36 KB, patch)
2011-10-02 13:49 PDT, Robert Longson
no flags Details | Diff | Splinter Review
updated (5.18 KB, patch)
2011-10-04 00:10 PDT, Robert Longson
dholbert: review+
Details | Diff | Splinter Review

Description Robert Longson 2011-10-02 13:49:51 PDT
Created attachment 564080 [details] [diff] [review]
patch
Comment 1 Daniel Holbert [:dholbert] 2011-10-03 22:48:05 PDT
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 2011-10-04 00:00:24 PDT
(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 2011-10-04 00:10:47 PDT
Created attachment 564472 [details] [diff] [review]
updated
Comment 5 Matt Brubeck (:mbrubeck) 2011-10-10 11:11:03 PDT
https://hg.mozilla.org/mozilla-central/rev/2a151eaf035b

Note You need to log in before you can comment on or make changes to this bug.