Last Comment Bug 690486 - Kill nsISVGValue, nsSVGValue, nsISVGValueObserver, nsISVGValueUtils and nsSVGStringProxyValue
: Kill nsISVGValue, nsSVGValue, nsISVGValueObserver, nsISVGValueUtils and nsSVG...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
:
Mentors:
Depends on: 436276
Blocks: 604176 608495 664744 677504
  Show dependency treegraph
 
Reported: 2011-09-29 12:44 PDT by Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
Modified: 2011-12-07 15:11 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (60.41 KB, patch)
2011-09-29 14:13 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
longsonr: review+
Details | Diff | Review
patch with enum tweaked (60.63 KB, patch)
2011-09-29 16:22 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
longsonr: review+
Details | Diff | Review

Description Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-09-29 12:44:25 PDT
We should kill nsSVGValue and nsISVGValue. This shouldn't take long now, so I'll knock up a patch for this later this evening.
Comment 1 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-09-29 14:13:27 PDT
Created attachment 563550 [details] [diff] [review]
patch
Comment 2 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-09-29 14:18:49 PDT
The do_GetWeakReference calls in SVGLengthList.h (and the other list types) now depend on nsGenericElement implementing nsISupportsWeakReference, although nsGenericElement implements that interface using a tearoff (called nsNodeSupportsWeakRefTearoff). That means we'll be allocating a tearoff when animating list type attributes, but that seems okay.
Comment 3 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-09-29 14:28:51 PDT
(For the record, XUL templates still work with the new world C++ attribute class types. See the still working http://www.croczilla.com/bits_and_pieces/svg/samples/xulsvg1/xulsvg1.xul (for which you'll need https://addons.mozilla.org/en-US/firefox/addon/remote-xul-manager/ installed) for example.)
Comment 4 Robert Longson 2011-09-29 14:46:28 PDT
Is Brian going to want to put any of this back in order to implement bug 629200? I suspect the answer is no based on the final comment in that bug.

As far as the change itself is concerned all I have is this...

>     eColor =        0x07, // 0111
>     eEnum =         0x0B, // 1011  This should eventually die
>     ePercent =      0x0F, // 1111
>     // Values below here won't matter, they'll be always stored in the 'misc'
>     // struct.
>     eCSSStyleRule = 0x10,
>     eAtomArray =    0x11 
>-    ,eSVGValue =    0x12
>     ,eDoubleValue  = 0x13
>     ,eIntMarginValue = 0x14

Can you should shuffle up the remainder and while you're there make the commas nice please?
Comment 5 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-09-29 15:41:31 PDT
(In reply to Robert Longson from comment #4)
> Can you should shuffle up the remainder and while you're there make the
> commas nice please?

I don't really want to unnecessarily mess with blame in someone else's module.
Comment 6 Robert Longson 2011-09-29 15:56:27 PDT
OK, how about raising a follow up then to see if the module owner wants it done or not.
Comment 7 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-09-29 16:09:27 PDT
Sicking, regarding comment 4, do you think I should make any of the suggested changes to the enumerations? Or perhaps at least renumber eDoubleValue and eIntMarginValue so there's no gap in the numbering?
Comment 8 Brian Birtles (:birtles) 2011-09-29 16:12:57 PDT
(In reply to Robert Longson from comment #4)
> Is Brian going to want to put any of this back in order to implement bug
> 629200? I suspect the answer is no based on the final comment in that bug.

That's right. The current approach I'm using it to add a number of new types in place of eSVGValue so it can definitely disappear as far as I'm concerned. Thanks for checking though.

Bug 629200 is coming along pretty well but I'll be away for about the next month so I'll hopefully post a few wip patches today.
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2011-09-29 16:15:57 PDT
Please do shuffle things up or rearrange them as much as needed to keep the code clean and gap-less.

Don't change the values *above* the "Values below here..." comment unless you understand how they interact with the types.
Comment 10 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-09-29 16:22:48 PDT
Created attachment 563589 [details] [diff] [review]
patch with enum tweaked

Good enough compromize for you, longsonr?
Comment 11 Robert Longson 2011-09-29 23:29:40 PDT
Comment on attachment 563589 [details] [diff] [review]
patch with enum tweaked

You had an r+ last but here's another anyway :-)
Comment 12 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-09-30 04:04:35 PDT
Thanks, just checking! :-)

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/d74000e4f76a
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-30 07:38:01 PDT
https://hg.mozilla.org/mozilla-central/rev/d74000e4f76a

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