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

RESOLVED FIXED in mozilla19

Status

()

defect
--
trivial
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Waldo, Assigned: escozzia)

Tracking

Trunk
mozilla19
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++][mentor=jwalden][leave open])

Attachments

(16 attachments, 2 obsolete attachments)

4.37 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
314.54 KB, patch
Details | Diff | Splinter Review
31.08 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
18.38 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
13.20 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
18.92 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
15.31 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
32.51 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
18.54 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
26.83 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
9.79 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
14.57 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
18.49 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
104.16 KB, patch
Details | Diff | Splinter Review
4.83 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
30.71 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
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.
Can I take this one? I want to start contributing to Mozilla, and this seems like a great place to start!
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?
(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!
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!
Attachment #715838 - Flags: review?
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.
Attachment #715838 - Flags: review? → review?(jwalden+bmo)
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.  :-)
Attachment #715838 - Flags: review?(jwalden+bmo) → review+
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f00aa5f44b0
Whiteboard: [lang=c++][mentor=jwalden] → [lang=c++][mentor=jwalden][leave open]
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! :)
Attachment #716337 - Flags: review?(jwalden+bmo)
(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/.
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.
Bam, new patch. It's a subset of the previous one, so I'm marking that one obsolete.
Attachment #716337 - Attachment is obsolete: true
Attachment #716337 - Flags: review?(jwalden+bmo)
Attachment #718213 - Flags: review?(jwalden+bmo)
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.
Okay fixed.
Attachment #718213 - Attachment is obsolete: true
Attachment #718213 - Flags: review?(jwalden+bmo)
Attachment #718214 - Flags: review?(jwalden+bmo)
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!
Attachment #718214 - Flags: review?(jwalden+bmo) → review+
All right, new patch then!

This one handles around 12 files, which picks up most of the smaller directories in a single go.
Attachment #721982 - Flags: review?(jwalden+bmo)
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!)
Attachment #721982 - Flags: review?(jwalden+bmo) → review+
Attachment #725776 - Flags: review?(jwalden+bmo)
Attachment #725777 - Flags: review?(jwalden+bmo)
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.
Attachment #725779 - Flags: review?(jwalden+bmo)
Attachment #725776 - Flags: review?(jwalden+bmo) → review+
Assignee: nobody → escozzia
Attachment #725777 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #725778 - Flags: review?(jwalden+bmo) → review+
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?
Attachment #725779 - Flags: review?(jwalden+bmo) → review+
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 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.
Attachment #717675 - Flags: review?(jwalden+bmo)
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.
Attachment #729921 - Flags: review?(jwalden+bmo)
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
Attachment #731229 - Flags: review?(jwalden+bmo)
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!
Attachment #729910 - Flags: review?(jwalden+bmo) → review+
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 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.
Attachment #729912 - Flags: review?(jwalden+bmo) → review+
Attachment #729913 - Flags: review?(jwalden+bmo) → review+
Attachment #729914 - Flags: review?(jwalden+bmo) → review+
Attachment #729916 - Flags: review?(jwalden+bmo) → review+
Attachment #731229 - Flags: review?(jwalden+bmo) → review+
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.  :-(
Attachment #729921 - Flags: review?(jwalden+bmo)
(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
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.
Attachment #736559 - Flags: review?(jwalden+bmo)
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!
Attachment #736559 - Flags: review?(jwalden+bmo) → review+
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.
Jose, is this still on your radar?
Flags: needinfo?(escozzia)
(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.
Flags: needinfo?(escozzia)
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 …).
Maybe this bug was superseded by bug 952650.
(In reply to Masatoshi Kimura [:emk] from comment #55)
> Maybe this bug was superseded by bug 952650.

Nicholas, can you please clarify?
Flags: needinfo?(n.nethercote)
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.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(n.nethercote)
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.