842186 - Replace all use of jsval with JS::Value and all uses of *_TO_JSVAL/JSVAL_TO_*/JSVAL_IS_* with the appropriate JS::Value methods

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Jeff Walden [:Waldo]]

jsval was the old type for values in the JSAPI.  JS::Value is a new C++ type, with built-in methods for querying value properties, extracting typed values (object, int32_t, string, etc.) from them.  The JSVAL-named methods have been supplanted by methods directly in JS::Value -- or by standalone methods, in the case of *_TO_JSVAL.  jsval remains now for backward compatibility, but it and its related methods are a crufty enough API that we're slowly working at deprecating them.  Possibly at some point (likely months to years out) we might even remove them, and thus have only one way to do it, for clarity/simplicity.

jsval is just a typedef for JS::Value, so this should be a pretty trivial change, except to the extent that indentation/whitespace preservation needs to happen.

*_TO_JSVAL just becomes *Value, e.g. StringValue, NumberValue, and so on.  There's a little trickiness for ObjectValue, which takes a JSObject& rather than a JSObject*, but it should be straightforward to handle.  Note that places that expected to take either a null or non-null JSObject* will want ObjectOrNullValue.

JSVAL_TO_* is mostly straightforward as well, as they just become JS::Value.to*.  Note that again Object is a little different in returning a JSObject& and not a JSObject*.

JSVAL_IS_* is again straightforward.  JSVAL_IS_OBJECT used to exist, and would return true for null as well as for objects, but I think we removed it, so there shouldn't be any complexity to this change.

This can be incrementalized as much or as little as desired -- no need to make all the changes at once in a single patch.  In fact I'd even strongly advise against doing so, so that reviews can be shorter and won't bog down.  Possibly doing each of these four steps in its own patch is a reasonable breakdown, or perhaps even split more finely than that.

Most places that use jsval right now probably do so because they're using some JSAPI method, and therefore they must #include "jsapi.h".  The ones that don't care about JSAPI, only about Value, might be well-served by being converted to #include "js/Value.h".  If that gets done in the patches here, all the better.  But this is purely optional.

I'm happy to review any patches that get written for this, and to answer any questions that might arise about doing this.  It should be search-and-replace for some parts, partially search-and-replace for others, and a little bit of manual determination for little bits at the edges.

Comment 1

[Jose Cortes]

Can I take this one? I want to start contributing to Mozilla, and this seems like a great place to start!

Comment 2

[Jeff Walden [:Waldo]]

Sure, go for it!  Maybe try a patch that changes a dozen or so locations somewhere and post it for feedback, to be sure you're on the right path.  Use the "Add an attachment" link, and set the review? flag to my email address here.  Then I'll take a look and see whether you're doing things right.  If you've got it right, great, I'll land it for you, and you can make another patch changing more places.  If not, I'll let you know what's wrong, and we can iterate.

Once you've gotten the hang of it, you can feel free to make bigger patches changing more at once.  But at the start, for a first time, it's probably best to start small until we're sure you're on the right track.

Sound good?

Comment 3

[Jose Cortes]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Sure, go for it!  Maybe try a patch that changes a dozen or so locations
> somewhere and post it for feedback, to be sure you're on the right path. 
> Use the "Add an attachment" link, and set the review? flag to my email
> address here.  Then I'll take a look and see whether you're doing things
> right.  If you've got it right, great, I'll land it for you, and you can
> make another patch changing more places.  If not, I'll let you know what's
> wrong, and we can iterate.
> 
> Once you've gotten the hang of it, you can feel free to make bigger patches
> changing more at once.  But at the start, for a first time, it's probably
> best to start small until we're sure you're on the right track.
> 
> Sound good?

Sound great, thanks!

Comment 4

[Jose Cortes]

Attachment: Updated a couple of references to jsval

Okay, here's a first patch as a sort of sanity check. I updated the references to jsval and some JSVAL_TO_OBJECTs to become a .toObject() call in about four methods of a class called WorkerPrivate.

Let me know if this is good, or if I'm entirely off the tracks.

Also, is there any standard on how the patches should be named, or anything of that nature that I should know about?

Thanks!

Comment 5

[Jose Cortes]

Oh, darn, I put your email in as I would to send you an actual email and it seems Bugzilla was expecting the suffix. Sorry about that.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 715838 [details] [diff] [review]
Updated a couple of references to jsval

Review of attachment 715838 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!  A couple small comments on patch format -- you'll want to mention the bug number in your patch summary.  The most common format for this would be so:

Bug 842186 - Replace a few instances of jsval with JS::Value in the WorkerPrivate class.

The summary in the patch comment should explain what was changed, first and foremost.  Your summary does this admirably.  Then, if there's some extra detail that's important, another sentence or so giving further detail is always nice.  The change here is simple, so there's no need to say any more.

As the final step of the patch you'll want to add reviewer information, in the form of "r=nickname" at the end of the summary, so people know who approved the change.  (You can use "jwalden" for me.)  So the final summary would be:

Bug 842186 - Replace a few instances of jsval with JS::Value in the WorkerPrivate class.  r=jwalden

Once the patch is reviewed and the patch has a username set on it (yours does) and the correct summary, someone can check it in.  It's getting late enough here that I'll push it in the morning.  If you want to post a new patch with those last little changes, feel free -- otherwise I'll make them myself before I land it.  (I don't mind adding the reviewer to patches I push, because my workflow involves me adjusting the summary of every patch I push, just before I push it.  Other people work differently and want the patch they're going to push to be just right in every respect.)  If you do decide to post one, don't bother requesting review on it -- that step's taken care of already, no need to request review again.

In the meantime, feel free to start on more places and post more patches!  Up to you how big or small to make them -- you could go several times bigger than this and I think I'd be fine with it.  :-)

Comment 7

[Jose Cortes]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> Comment on attachment 715838 [details] [diff] [review]
> Updated a couple of references to jsval
> 
> Review of attachment 715838 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good!  A couple small comments on patch format -- you'll want to
> mention the bug number in your patch summary.  The most common format for
> this would be so:
> 
> Bug 842186 - Replace a few instances of jsval with JS::Value in the
> WorkerPrivate class.
> 
> The summary in the patch comment should explain what was changed, first and
> foremost.  Your summary does this admirably.  Then, if there's some extra
> detail that's important, another sentence or so giving further detail is
> always nice.  The change here is simple, so there's no need to say any more.
> 
> As the final step of the patch you'll want to add reviewer information, in
> the form of "r=nickname" at the end of the summary, so people know who
> approved the change.  (You can use "jwalden" for me.)  So the final summary
> would be:
> 
> Bug 842186 - Replace a few instances of jsval with JS::Value in the
> WorkerPrivate class.  r=jwalden
> 
> Once the patch is reviewed and the patch has a username set on it (yours
> does) and the correct summary, someone can check it in.  It's getting late
> enough here that I'll push it in the morning.  If you want to post a new
> patch with those last little changes, feel free -- otherwise I'll make them
> myself before I land it.  (I don't mind adding the reviewer to patches I
> push, because my workflow involves me adjusting the summary of every patch I
> push, just before I push it.  Other people work differently and want the
> patch they're going to push to be just right in every respect.)  If you do
> decide to post one, don't bother requesting review on it -- that step's
> taken care of already, no need to request review again.
> 
> In the meantime, feel free to start on more places and post more patches! 
> Up to you how big or small to make them -- you could go several times bigger
> than this and I think I'd be fine with it.  :-)

Awesome! I'll start working on a bigger patch then and I'll probably have it ready for you soon.

Comment 8

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f00aa5f44b0

Comment 9

[Jose Cortes]

Attachment: Updated many instances of jsval in C++ code

All right, I went for a bigger patch this time and (as far as I can tell) updated all the jsvals in the .cpp and .h files (except for those in the dom and js directories; they're quite packed, so I'll do a patch for each).

I do have a few quick questions that emerged while I was doing this. I've never worked with IDL before, should I update the jsvals to JS::Value in the same fashion, or is there some special way of dealing with it?

Next, am I correct in assuming that doing a mach build will run the tests? I'm sort of worried that I'll manage to break something in a way that the code will still compile but will be erroneous.

And lastly, the file gfx/skia/include/xml/SkJS.h typedefs a different jsval, which seems to be a long. I suppose it would be messy to change any jsvals in files that use that definition, so I haven't touched the gfx directory at all. I'm unsure on how to proceed around this one, from what I see some files under gfx also pull in jsapi.h. I can probably work out which definition is in use by looking at the includes for each file, but is there any specific guidance you can offer?

Feel free to let me know if there are any issues or if you have any feedback.

Thanks again! :)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3f00aa5f44b0

Comment 11

[Jose Cortes]

Attachment: Replace use of jsval with JS::Value in .h and .cpp files in the dom directory

Comment 12

[Jeff Walden [:Waldo]]

(In reply to Jose Cortes from comment #9)
> All right, I went for a bigger patch this time and (as far as I can tell)
> updated all the jsvals in the .cpp and .h files (except for those in the dom
> and js directories; they're quite packed, so I'll do a patch for each).

Hmm, maybe too big, kind of.  Any chance you could split this up into patches changing, say, 10 files at a time or so?  It's pretty easy to split up patches using mq:

http://mercurial.selenic.com/wiki/MqTutorial#Split_a_patch_into_multiple_patches

With a big patch, you'll be stuck waiting for me to review the entire thing.  With small patches, I can review a little and push that, then a little more and push that, and so on.  It's also easier to squeeze in small-patch reviews in a lull, generally -- although this is a little special because it's a straightforward change.  I can review big patches incrementally in lulls, to be sure -- but you're still waiting for the entire thing before anything more can happen.

> I do have a few quick questions that emerged while I was doing this. I've
> never worked with IDL before, should I update the jsvals to JS::Value in the
> same fashion, or is there some special way of dealing with it?

Probably for sanity the "jsval" name in IDL files should change, eventually.  But for the purpose of being able to deprecate jsval as a typedef from the SpiderMonkey API, the name "jsval" in IDL doesn't matter.  Please ignore all IDL files.  Somewhere there's probably a header that tells the XPIDL compiler how to interpret the name "jsval", that likely uses "jsval" the typedef, but it's not important to change now.  And when all the other files that use jsval are gone, it'll stick out like a sore thumb in a code search.

> Next, am I correct in assuming that doing a mach build will run the tests?

No -- that just does a build.  There are various commands to run tests:

https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing

But...

> I'm sort of worried that I'll manage to break something in a way that the
> code will still compile but will be erroneous.

jsval and JS::Value are the exact same type.  You can't break anything substituting JS::Value for jsval, unless there's a compiler bug somehow.  :-)  Which is not entirely impossible, but I don't think you should worry about it, while you're just replacing jsval with JS::Value.

But converting *_TO_JSVAL and the other macros to the JS::Value methods *is* somewhat more fragile, and more prone to breaking something.  For those changes you probably will want to start running tests at some point.  Just starting up a browser is a good enough smoketest for now -- then with patches posted I can push them to try for you, and we can get full test results.  (At some point you'll probably want that access yourself, but it's probably a little early for that now.)

> And lastly, the file gfx/skia/include/xml/SkJS.h typedefs a different jsval,
> which seems to be a long. I suppose it would be messy to change any jsvals
> in files that use that definition, so I haven't touched the gfx directory at
> all. I'm unsure on how to proceed around this one, from what I see some
> files under gfx also pull in jsapi.h. I can probably work out which
> definition is in use by looking at the includes for each file, but is there
> any specific guidance you can offer?

Looking there, I *think* those files aren't actually part of the build, and they're just incidentals that happen to be part of skia that we just ignore.  So I don't think you need to make any changes to gfx/.

Comment 13

[Jose Cortes]

All right, roger that!

I'll get to splitting the two patches that I've posted, then (the second one being the DOM patch I posted yesterday). I forget the exact number of affected files, but I think 10 files per patch will do the trick.

Comment 14

[Jose Cortes]

Attachment: Update instances of jsval in the netwerk and toolkit directories.

Bam, new patch. It's a subset of the previous one, so I'm marking that one obsolete.

Comment 15

[Jose Cortes]

My life I'm an idiot, it seems I've managed to upload the same thing again?

In my defence, I'd never used Mercurial (I hail from git-land).

I'll fix this in a sec.

Comment 16

[Jose Cortes]

Attachment: Update instances of jsval in the netwerk and toolkit directories.

Okay fixed.

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 718214 [details] [diff] [review]
Update instances of jsval in the netwerk and toolkit directories.

Review of attachment 718214 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Will push in the morning!

Comment 18

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1deed14c3a6b

Comment 19

[Jose Cortes]

Attachment: Second patch replacing attachment 716337

All right, new patch then!

This one handles around 12 files, which picks up most of the smaller directories in a single go.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1deed14c3a6b

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 721982 [details] [diff] [review]
Second patch replacing attachment 716337 [details] [diff] [review]

Review of attachment 721982 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good again.  Feel free to post more than one of these at a time if you want, so I can continue on to the next one without waiting on you for it.  (Although you've been more than prompt in responding.  :-)  Certainly more timely than I've been!)

Comment 22

[Jose Cortes]

Attachment: Replace use of jsval in the content/events directory

Comment 23

[Jose Cortes]

Attachment: Replace use of jsval in the content/html directory

Comment 24

[Jose Cortes]

Attachment: Replace use of jsval in the content/canvas, content/xbl, content/xslt and content/xul directories

Comment 25

[Jose Cortes]

Attachment: Replace use of jsval in the content/base directory

All right, so these four should pick up everything that was left over from attachment 716337 [details] [diff] [review]

This last one is somewhat bigger (16 files), but the directory structure for the content/base directory didn't really allow me to split it up the way I've split up most of them. Let me know if it's too big, and I'll figure out some way of splitting it.

Comment 26

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77639edc57a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/124883708b7c

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/77639edc57a1
https://hg.mozilla.org/mozilla-central/rev/124883708b7c

Comment 28

[Jeff Walden [:Waldo]]

Comment on attachment 725778 [details] [diff] [review]
Replace use of jsval in the content/canvas, content/xbl, content/xslt and content/xul directories

Review of attachment 725778 [details] [diff] [review]:
-----------------------------------------------------------------

I delayed long enough on the previous patch that various parts of it bitrotted.  But it was good bitrot -- the relevant code went away entirely -- so easy to resolve.  :-)

I'll keep queueing these up to push in one go with a few other patches.

Comment 29

[Jeff Walden [:Waldo]]

Comment on attachment 725779 [details] [diff] [review]
Replace use of jsval in the content/base directory

Review of attachment 725779 [details] [diff] [review]:
-----------------------------------------------------------------

Good too.

Converting most jsval to JS::Value is fairly easy.  A bunch you could probably even do with an automated search-and-replace tool.  Do you have any interest in moving to somewhat trickier things like converting *_TO_JSVAL to StringValue, ObjectValue, NumberValue, etc. methods?

Comment 30

[Jose Cortes]

By all means! I'll get started on that this weekend and will hopefully have some patches for that soon.

In addition, there's a bunch of jsval->JS::Value transitions that I still haven't posted patches for, even though they've been sitting around my home folder for some time now. It's basically two massive patches, so should I split them up and post them up here before moving on to the *_TO_JSVALs? It can probably be done by tomorrow afternoon, and I'll also pull and merge before doing it, to prevent any further bitrot.

Comment 31

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4263d37413d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b9375365c0e
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc8488c25346

Sure, feel free to post 'em.

Comment 32

[Jeff Walden [:Waldo]]

Comment on attachment 717675 [details] [diff] [review]
Replace use of jsval with JS::Value in .h and .cpp files in the dom directory

Review of attachment 717675 [details] [diff] [review]:
-----------------------------------------------------------------

Canceling mostly to help clear out queue -- too big for it to be a good idea to review all at once.

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f4263d37413d
https://hg.mozilla.org/mozilla-central/rev/0b9375365c0e
https://hg.mozilla.org/mozilla-central/rev/fc8488c25346

Comment 34

[Jose Cortes]

Attachment: Replace use of jsval with JS::Value in h and cpp files in the dom/sms directory.

Comment 35

[Jose Cortes]

Attachment: Replace use of jsval with JS::Value in h and cpp files in the dom/bindings/ dom/plugins/ dom/src/ dom/activities/ directories.

Comment 36

[Jose Cortes]

Attachment: Replace use of jsval with JS::Value in h and cpp files in the dom/telephony dom/bluetooth directories.

Comment 37

[Jose Cortes]

Attachment: Replace use of jsval with JS::Value in h and cpp files in the dom/system/ and dom/file/ directories.

Comment 38

[Jose Cortes]

Attachment: Replace use of jsval with JS::Value in h and cpp files in the dom/network/ dom/camera/ and dom/devicestorage/ directories.

Comment 39

[Jose Cortes]

Attachment: Replace use of jsval with JS::Value in h and cpp files in the dom/base/ directory.

Okay, these six (mostly) replace that massive attachment 717675 [details] [diff] [review] patch. There's still about 43 files from that patch not included in this drop, so I'll put up patches for those tomorrow.

Additionally, I did an hg pull and hg update before, so hopefully these should be up to date. However, I'm not all that familiar with mercurial, so feel free to let me know if you run across any more out of date code, or just overall weirdness.

Comment 40

[Jose Cortes]

Attachment: Replace use of jsval with JS::Value in h and cpp files in the dom/ipc/ dom/icc/ directories.

Oh dear, I should really stop making time-related promises. I'm sorry that took longer than I said it would, but here's one of the other patches for that dom/ directory

Comment 41

[Jeff Walden [:Waldo]]

Comment on attachment 729910 [details] [diff] [review]
Replace use of jsval with JS::Value in h and cpp files in the dom/sms directory.

Review of attachment 729910 [details] [diff] [review]:
-----------------------------------------------------------------

Queueing up to push this when the tree reopens.  Thanks!

Comment 42

[Jeff Walden [:Waldo]]

Hoo boy, that had some conflicts due to file-moves.  :-)  Oh well, guess that's my problem for not reviewing fast enough.  Queued up properly now, conflicts resolved.

Comment 43

[Jeff Walden [:Waldo]]

Comment on attachment 729912 [details] [diff] [review]
Replace use of jsval with JS::Value in h and cpp files in the dom/bindings/ dom/plugins/ dom/src/ dom/activities/ directories.

Review of attachment 729912 [details] [diff] [review]:
-----------------------------------------------------------------

Also looking good.

Comment 44

[Jeff Walden [:Waldo]]

Comment on attachment 729921 [details] [diff] [review]
Replace use of jsval with JS::Value in h and cpp files in the dom/base/ directory.

Review of attachment 729921 [details] [diff] [review]:
-----------------------------------------------------------------

So, this looks fine enough, but when I apply it I get a whole screenful of merge conflicts -- probably too many for me to go through them all and fix them all myself.  Could you redo this patch, please?  Sorry for taking so long to get to it, that probably didn't help with its staying fresh.  :-(

Comment 45

[Jose Cortes]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #44)
> Comment on attachment 729921 [details] [diff] [review]
> Replace use of jsval with JS::Value in h and cpp files in the dom/base/
> directory.
> 
> Review of attachment 729921 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So, this looks fine enough, but when I apply it I get a whole screenful of
> merge conflicts -- probably too many for me to go through them all and fix
> them all myself.  Could you redo this patch, please?  Sorry for taking so
> long to get to it, that probably didn't help with its staying fresh.  :-(

No worries, that was likely my fault for not freshening it up as I went.

I'll redo it this week; it's about 15 files, so it shouldn't be too long.
Apologies for slowing down a bit, I've been somewhat busy

Comment 46

[Jeff Walden [:Waldo]]

No worries about slowdown, I'm still not fully caught up on reviews yet, so any supposed lag on your end doesn't affect me at all.  And it could have been either of us for that -- a week or so's long enough to bitrot any wide-ranging patch to some degree.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d42d278b2535
https://hg.mozilla.org/integration/mozilla-inbound/rev/4287ba54fd16
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc52fa570c12
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c762928609d
https://hg.mozilla.org/integration/mozilla-inbound/rev/d01e5b96ae71
https://hg.mozilla.org/integration/mozilla-inbound/rev/a13a219428c0

Comment 47

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d42d278b2535
https://hg.mozilla.org/mozilla-central/rev/4287ba54fd16
https://hg.mozilla.org/mozilla-central/rev/cc52fa570c12
https://hg.mozilla.org/mozilla-central/rev/1c762928609d
https://hg.mozilla.org/mozilla-central/rev/d01e5b96ae71
https://hg.mozilla.org/mozilla-central/rev/a13a219428c0

Comment 48

[Jose Cortes]

Attachment: Replace use of jsval in the dom/base directory

Okay, got the replacement patch. It tackles those files that were left over in the dom/base directory, I'll look into hunting down the ones that were moved soon.

I've switched to git, so let me know if there are any formatting issues.

Comment 49

[Jeff Walden [:Waldo]]

Comment on attachment 736559 [details] [diff] [review]
Replace use of jsval in the dom/base directory

Review of attachment 736559 [details] [diff] [review]:
-----------------------------------------------------------------

Cool.  With luck I'll have a few things to push later today, if I can finish up some debugging first, so I'll take care of pushing this then.  Thanks!

Comment 50

[Jeff Walden [:Waldo]]

I imported this on Friday and rebased my tree, and amusingly enough even after a day or so I hit some minor merge/rebase errors.  :-)  You're changing frequently-touched files.  Conflicts come with the territory.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6b56a3f243

Re git formatting, I don't *know* that I've seen people push other things with the git patch header format before, but it didn't seem to hurt to have it.  (I don't use git, so I don't think about these things, myself.  Other Mozilla hackers could talk more to the issues that arise from using git regularly.)  I *would* note that the first/summary line of your patch was a bit mangled (check out the plain-text view of it to see), and I fixed it up to the regular format from comment 6 before pushing.

Comment 51

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9a6b56a3f243

Comment 52

[Florian Bender]

Jose, is this still on your radar?

Comment 53

[Jose Cortes]

(In reply to Florian Bender from comment #52)
> Jose, is this still on your radar?

Woah, completely dropped the ball here. Really, really sorry about that. If you, or someone else, want to take it over that's alright. Otherwise, I can just pick it up again and keep going.

Again, I'm really very sorry about this.

Comment 54

[Florian Bender]

No worries, just wanted to ping you on that. Otherwise I would have freed the bug for someone else (I'm in no position to fix this myself, unfortunately – I should really learn C++ even if for relatively simple bugs like this …).

Comment 55

[Masatoshi Kimura [:emk]]

Maybe this bug was superseded by bug 952650.

Comment 56

[Florian Bender]

(In reply to Masatoshi Kimura [:emk] from comment #55)
> Maybe this bug was superseded by bug 952650.

Nicholas, can you please clarify?

Comment 57

[Nicholas Nethercote [inactive]]

It looks like this bug is focussing on converting |jsval| instances to |JS::Value|, and bug 952650 is focussing on removing the various JSVAL_* functions. So they're complementary.

However, given that this bug hasn't seen action in 9 months plus patches landed, I'm going to mark it fixed, and further action can move into bug 952650. I'm marking the target milestone as "mozilla19", because I think that's the release where most of the patches landed, though somewhere in 18 and some in 20.