952650 - Kill all methods like JSVAL_IS_* and JSVAL_TO_*

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Tom S [:evilpie]]

This could be relatively straightforward work for somebody touching a lot of code.

Comment 2

[Rodrigo Rodriguez]

I would like to contribute, but since I am quite new here I will need some direction.

Comment 3

[Tom S [:evilpie]]

So in this file: https://mxr.mozilla.org/mozilla-central/source/js/public/Value.h?force=1#1771. There are methods like JSVAL_IS_NULL. For all of these methods there is an equivalent method on JS::Value already like isNull().

So basically what you want to do is grep (or use mxr) to search for all occurrences of for example JSVAL_IS_NULL and replace that with JSValue::isNull. And do the same thing for all other methods like it. I would suggest you try to do the easy ones like IS_NULL or IS_UNDEFINED first. We also have methods that extract specific types from JS::Values like toInt32() or toNumber(), which replace the old JSVAL_TO_INT etc.

This wiki page has a table on how these methods map to each other: https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/Jsval#jsval_is_not_particularly_type-safe.

I will also give you a concrete example: Here https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCVariant.cpp#65 you would replace:
 if (!JSVAL_IS_NULL(val))
with
 if (!val.isNull())

Please create one patch for every function that you replace and remove.

Comment 4

[Tom S [:evilpie]]

Oh and I also assigned the bug to you. Good luck and thanks for the help!

Comment 5

[Rodrigo Rodriguez]

The methods that I am to replace the depreciated methods with are defined here: https://mxr.mozilla.org/mozilla-central/source/js/public/Value.h?force=1#1006, right? Just to be clear.

The wiki page shows all the methods that will eventually get replaced?

Okay, will do.

Comment 6

[Tom S [:evilpie]]

(In reply to Rodrigo Rodriguez from comment #5)
> The methods that I am to replace the depreciated methods with are defined
> here:
> https://mxr.mozilla.org/mozilla-central/source/js/public/Value.
> h?force=1#1006, right? Just to be clear.
> 
Yes you should replace the old methods with the new ones that are defined at line and beneath of your link.

> The wiki page shows all the methods that will eventually get replaced?
I believe so. One suggestion along the way, OBJECT_TO_JSVAL is kind of complicated, I would do that one last.
> 
> Okay, will do.
Perfect :)
A small tip: It's easier to understand to what you are responding if you use the reply feature, which puts the text you respond to into your comment.

Comment 7

[Rodrigo Rodriguez]

I traded out the methods here: https://mxr.mozilla.org/mozilla-central/source/js/jsd/jsd_val.cpp#99, would this be an acceptable method to switch out? Will the depreciated method be located throughout the entire source tree?

Comment 8

[Rodrigo Rodriguez]

(In reply to Tom Schuster [:evilpie] from comment #6)
> (In reply to Rodrigo Rodriguez from comment #5)
> > Okay, will do.
> Perfect :)
> A small tip: It's easier to understand to what you are responding if you use
> the reply feature, which puts the text you respond to into your comment.
Test test ... making sure I got the format down.

I have created a diff file in the .hg directory, do I just upload it to the attachments section from there or is there some kind of special routine I have to go through?

Comment 9

[Rodrigo Rodriguez]

Attachment: bug952650_depreciatedMethodfixes.diff

Comment 10

[Tom S [:evilpie]]

>I traded out the methods here: https://mxr.mozilla.org/mozilla-central/source/js/jsd/jsd_val.cpp#99, would this be an >acceptable method to switch out? Will the depreciated method be located throughout the entire source tree?
Yes and yes.

>I have created a diff file in the .hg directory, do I just upload it to the attachments section from there or is >there some kind of special routine I have to go through?
Oh yeah awesome that worked!
However I just realized
>Please create one patch for every function that you replace and remove.
was badly phrased.
What I meant is, remove all the instances of a function eg. JSVAL_IS_NULL and attach a patch for that. So basically with your patch applied there should be no JSVAL_IS_NULL left.

Oh and in this case, you would need "return jsdval->val.isNull();"

Comment 11

[Rodrigo Rodriguez]

(In reply to Tom Schuster [:evilpie] from comment #10)
> Yes and yes.
ok, cool.
> Oh yeah awesome that worked!
> However I just realized
> >Please create one patch for every function that you replace and remove.
> was badly phrased.
> What I meant is, remove all the instances of a function eg. JSVAL_IS_NULL
> and attach a patch for that. So basically with your patch applied there
> should be no JSVAL_IS_NULL left.
So if I were to do this correctly, that diff file that I have attached ( for the "JSVAL_IS_NULL" case ) should contain every change I make that involves JSVAL_IS_NULL to val.isNULL().  

> Oh and in this case, you would need "return jsdval->val.isNull();"
opps, I will make sure to get more informed on this data type.

Comment 12

[Rodrigo Rodriguez]

Attachment: bug952650_KillJSVAL_isNull.diff

Replaced JSVAL_IS_NULL( val ) with isNull().

Comment 13

[Rodrigo Rodriguez]

Attachment: bug952650_KillJSVAL_isNull.diff

replaced method JSVAL_IS_NULL(val) with val.isNull().

Also corrected some syntax errors from previous diff file.

Comment 14

[Tom S [:evilpie]]

Comment on attachment 8362406 [details] [diff] [review]
bug952650_KillJSVAL_isNull.diff

This looks really good! Thank you. However I am going to ask somebody else to review it.
If you want to continue, JSVAL_IS_VOID would be the obvious option.

Comment 15

[Rodrigo Rodriguez]

(In reply to Tom Schuster [:evilpie] from comment #14)
> This looks really good! Thank you. However I am going to ask somebody else
> to review it.
Thanks! I ran an update and I had a patch conflict with: https://bugzilla.mozilla.org/attachment.cgi?id=8362406&action=diff#a/dom/file/LockedFile.cpp_sec2

Looks like the update completely removed the use of JSVAL_IS_NULL in this case, but I am not completely sure if that is what happened since I am new, and haven't seen something like that.

> If you want to continue, JSVAL_IS_VOID would be the obvious option.
JSVAL_IS_VOID has to be replaced with val.isUndefined()?

Comment 16

[Terrence Cole [:terrence]]

Comment on attachment 8362406 [details] [diff] [review]
bug952650_KillJSVAL_isNull.diff

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

Looks good! r=me

Comment 17

[Rodrigo Rodriguez]

Attachment: bug952650_KILLJSVAL_isUndefined.diff

replaced JSVAL_IS_VOID( val ) with val.isUndefined()

Comment 18

[Rodrigo Rodriguez]

Attachment: bug952650_KillJSVAL_isBoolean.diff

replaced JSVAL_IS_BOOLEAN( val ) with val.isBoolean()

Comment 19

[Rodrigo Rodriguez]

Attachment: bug952650_KillJSVAL_toBoolean.diff

replaced JSVAL_TO_BOOLEAN( val ) with val.toBoolean()

Comment 20

[Rodrigo Rodriguez]

Attachment: bug952650_KillJSVAL_isDouble.diff

replaced JSVAL_IS_DOUBLE( val ) with val.isDouble()

Comment 21

[Rodrigo Rodriguez]

Attachment: bug952650_KillJSVAL_toDouble.diff

replaced JSVAL_TO_DOUBLE( val ) with val.toDouble()

Comment 22

[Rodrigo Rodriguez]

Attachment: bug952650_KillJSVAL_isString.diff

replaced JSVAL_IS_STRING( val ) with val.isString()

Comment 23

[Masatoshi Kimura [:emk]]

Comment on attachment 8362877 [details] [diff] [review]
bug952650_KillJSVAL_isBoolean.diff

>--- a/js/src/ctypes/CTypes.cpp
>+++ b/js/src/ctypes/CTypes.cpp
>@@ -1576,17 +1576,17 @@ static JS_ALWAYS_INLINE bool IsNegative(
>-  if (JSVAL_IS_BOOLEAN(val)) {
>+  if (val.isUndefined()) {

val.isBoolean()

Comment 24

[Rodrigo Rodriguez]

Attachment: bug952650_KillJSVAL_isBoolean.diff

opps, thank you Masatoshi Kimura for pointing that out.

Comment 25

[Rodrigo Rodriguez]

Attachment: bug952650_KillJSVAL_toString.diff

replaced JSVAL_TO_STRING( val ) with val.toString()

Comment 26

[Rodrigo Rodriguez]

Attachment: bug952650_KillJSVAL_isInt32.diff

replaced JSVAL_IS_INT( val ) with val.isInt32()

Comment 27

[Rodrigo Rodriguez]

Attachment: bug952650_KillJSVAL_toInt32.diff

replaced JSVAL_TO_INT( val ) with val.toInt32()

Comment 28

[Rodrigo Rodriguez]

Attachment: bug952650_KillJSVAL_isNumber.diff

replaced JSVAL_IS_NUMBER( val ) with val.isNumber()

Comment 29

[Tom S [:evilpie]]

Comment on attachment 8362861 [details] [diff] [review]
bug952650_KILLJSVAL_isUndefined.diff

WoW, nice work so far! I think we should get some different people to review this. Do you have try server access yet?

Comment 30

[Rodrigo Rodriguez]

Attachment: bug952650_KillJSVAL_toInt32.diff

replaced JSVAL_TO_INT( val ) with val.toInt32()

removed some syntax errors

Comment 31

[Rodrigo Rodriguez]

(In reply to Tom Schuster [:evilpie] from comment #29)
> Comment on attachment 8362861 [details] [diff] [review]
> bug952650_KILLJSVAL_isUndefined.diff
> 
> WoW, nice work so far! I think we should get some different people to review
> this. Do you have try server access yet?

I'm not sure what you mean by server access, can you explain?

Comment 32

[Tom S [:evilpie]]

It's basically a continuous integration server, that runs all our tests across different devices and platforms. You can read about it here: https://wiki.mozilla.org/ReleaseEngineering/TryServer. You can also checkout our IRC channel #jsapi on irc.mozilla.org, so we can have a bit more direct communication.

Comment 33

[Rodrigo Rodriguez]

Attachment: bug952650_KillJSVAL_toInt32.diff

replaced JSVAL_TO_INT( val ) with val.toInt32()

removed some syntax errors

Comment 34

[Rodrigo Rodriguez]

Attachment: jsval-patch

Comment 35

[Nicholas Nethercote [inactive]]

Comment on attachment 8363258 [details] [diff] [review]
bug952650_KillJSVAL_toBoolean.diff

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

I'm happy to review some. This one looks fine, though it doesn't appear to remove the definition of the JSVAL_TO_BOOLEAN macro, which is something I expected.

Comment 36

[Rodrigo Rodriguez]

(In reply to Nicholas Nethercote [:njn] from comment #35)
> Comment on attachment 8363258 [details] [diff] [review]
> bug952650_KillJSVAL_toBoolean.diff
> 
> Review of attachment 8363258 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm happy to review some. This one looks fine, though it doesn't appear to
> remove the definition of the JSVAL_TO_BOOLEAN macro, which is something I
> expected.
I wasn't sure if I was suppose to remove the depreciated definitions or not. Do you suggest I remove the definitions I have replaced?

Comment 37

[Nicholas Nethercote [inactive]]

> Do you suggest I remove the definitions I have replaced?

Yes! No point having an unused macro hanging around, esp. when there's a preferred function that does the same thing :)

Comment 38

[Rodrigo Rodriguez]

(In reply to Nicholas Nethercote [:njn] from comment #37)
> > Do you suggest I remove the definitions I have replaced?
> 
> Yes! No point having an unused macro hanging around, esp. when there's a
> preferred function that does the same thing :)

Okay, I'll do that right now then!

Comment 39

[Rodrigo Rodriguez]

Attachment: bug952650_KillJSVAL_removeDepreMacro.diff

Removed following unused macros ...
JSVAL_IS_NULL( val )
JSVAL_IS_VOID( val )
JSVAL_IS_INT( val )
JSVAL_TO_INT( val )
JSVAL_IS_DOUBLE( val )
JSVAL_TO_DOUBLE( val )
JSVAL_IS_NUMBER( val )
JSVAL_IS_STRING( val )
JSVAL_TO_STRING( val )
JSVAL_IS_BOOLEAN( val )
JSVAL_TO_BOOLEAN( val )

Comment 40

[Nicholas Nethercote [inactive]]

Comment on attachment 8363498 [details] [diff] [review]
bug952650_KillJSVAL_toInt32.diff

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

::: js/xpconnect/src/XPCConvert.cpp
@@ +1230,5 @@
>          double number;
>          bool isResult = false;
>  
>          if (s.isInt32()) {
> +            rv = (nsresult) s.isInt32();

Should be toInt32, not isInt32!

::: security/manager/ssl/src/nsCrypto.cpp
@@ +950,5 @@
>      JS_ReportError(cx, "%s%s", JS_ERROR,
>                     "passed in non-integer for key size");
>      return NS_ERROR_FAILURE;
>    }
> +  keySize = argv[0].isInt32();

Ditto.

Comment 41

[Nicholas Nethercote [inactive]]

Comment on attachment 8362406 [details] [diff] [review]
bug952650_KillJSVAL_isNull.diff

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

::: js/jsd/jsd_val.cpp
@@ -95,5 @@
>  
>  bool
>  jsd_IsValueNull(JSDContext* jsdc, JSDValue* jsdval)
>  {
> -    return jsdval.isNull();

That's weird... in my copy of the repo, this line looks like:

    return JSVAL_IS_NULL(jsdval->val);

Comment 42

[Nicholas Nethercote [inactive]]

Comment on attachment 8366488 [details] [diff] [review]
bug952650_KillJSVAL_removeDepreMacro.diff

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

This looks fine, but there are still some to be done: JSVALUE_TO_OBJECT should be replaced with toObjectOrNull()... and can the *_TO_JSVAL ones be converted?

Comment 43

[Rodrigo Rodriguez]

(In reply to Nicholas Nethercote [:njn] from comment #42)
> Comment on attachment 8366488 [details] [diff] [review]
> bug952650_KillJSVAL_removeDepreMacro.diff
> 
> Review of attachment 8366488 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine, but there are still some to be done: JSVALUE_TO_OBJECT
> should be replaced with toObjectOrNull()... and can the *_TO_JSVAL ones be
> converted?

I was told by Tom that replacing that function could get a little tricky. I have yet to replaced it because I haven't yet figured out what's stopping me from just switching it out.

Comment 44

[Nicholas Nethercote [inactive]]

The only tricky thing I can see is that there is both toObject() and toObjectOrNull(). The assertions within those two are different, and one returns a pointer and the other a reference, but JSVAL_TO_OBJECT() is functionally identical to toObjectOrNull() as far as I can tell.

Comment 45

[Rodrigo Rodriguez]

(In reply to Nicholas Nethercote [:njn] from comment #44)
> The only tricky thing I can see is that there is both toObject() and
> toObjectOrNull(). The assertions within those two are different, and one
> returns a pointer and the other a reference, but JSVAL_TO_OBJECT() is
> functionally identical to toObjectOrNull() as far as I can tell.

Okay, I will upload a patch tonight to replace those functions.

Comment 46

[Nicholas Nethercote [inactive]]

FYI: Bug 842186 was an old bug that was about removing jsval; it seems to have some outstanding patches but hadn't seen action in 9 months so I closed it.

Comment 47

[Biru [:poiru]]

Rodrigo, are you still working on this?

Comment 48

[Till Schneidereit [:till]]

Un-assigning based on the lack of feedback. Rodrigo, if you still want to work on this, please shout.

Otherwise, this can be taken up by someone else. Most of these patches probably have to be rebased, but I'm hopeful that that won't be too complicated.

Comment 49

[Nicholas Nethercote [inactive]]

I'm going to finish this off and land it. I'm reassigning to Rodrigo since he's done 95% of the work and it'll be his name on the landed patches.

Comment 50

[Nicholas Nethercote [inactive]]

I'll post the rebased version of the first 11 patches and carry the r+'s over. The only change of note that I made was that I move the removal of each JSVAL_* function into the relevant patch.

Then I'll post new patches that remove the few cases that Rodrigo didn't do.

Comment 51

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove JSVAL_IS_NULL

Comment 52

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove JSVAL_IS_VOID

Comment 53

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove JSVAL_TO_BOOLEAN

Comment 54

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Remove JSVAL_IS_DOUBLE

Comment 55

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Remove JSVAL_TO_DOUBLE

Comment 56

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Remove JSVAL_IS_STRING

Comment 57

[Jeff Walden [:Waldo]]

I assume the actual removals (as possibly opposed to changing all uses) are going to wait til after the merge?  There's not really any point to removing APIs we're not going to use in branch patches, right now.  And embedders being given an extra cycle for conversions is probably what they want, as well.

Comment 58

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Remove JSVAL_IS_BOOLEAN

Comment 59

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Remove JSVAL_TO_STRING

Comment 60

[Nicholas Nethercote [inactive]]

Attachment: (part 9) - Remove JSVAL_IS_INT

Comment 61

[Nicholas Nethercote [inactive]]

Attachment: (part 10) - Remove JSVAL_IS_NUMBER

Comment 62

[Nicholas Nethercote [inactive]]

Attachment: (part 11) - Remove JSVAL_TO_INT

Comment 63

[Nicholas Nethercote [inactive]]

Attachment: (part 12) - Remove JSVAL_TO_OBJECT

This removes JSVAL_TO_OBJECT and replaces all calls to it with
toObjectOrNull(), which is the equivalent function. Some of these could
actually be toObject(), I'm sure, but I certainly don't want to go through them
and try to work that out :)

Comment 64

[Nicholas Nethercote [inactive]]

Attachment: (part 13) - Remove JSVAL_IS_PRIMITIVE

Comment 65

[Nicholas Nethercote [inactive]]

Attachment: (part 14) - Remove JSVAL_IS_GCTHING

Comment 66

[Nicholas Nethercote [inactive]]

Attachment: (part 15) - Remove JSVAL_TO_GCTHING

This one is dead, which makes things easy.

Comment 67

[Nicholas Nethercote [inactive]]

Attachment: (part 16) - Remove JSVAL_TO_PRIVATE

Comment 68

[Nicholas Nethercote [inactive]]

And with that, all the JSVAL_{IS,TO}_* functions are gone. The *_TO_JSVAL functions still exist, but that can be fodder for another bug.

Comment 69

[Till Schneidereit [:till]]

Comment on attachment 8413499 [details] [diff] [review]
(part 12) - Remove JSVAL_TO_OBJECT

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

r=me. It's a good thing that "JSVAL_TO_OBJECT" and ".toObjectOrNull" are exactly the same length, so this doesn't require any rewrapping.

::: js/src/tests/js1_8_1/regress/regress-452498-123.js
@@ +44,5 @@
>  // Requires -j:
>  // =====
>    (function(){ eval("for (x in ['', {}, '', {}]) { this; }" )})();
>  
> +// Assertion failure: fp->thisp == fp->argv[-1].toObjectOrNull(), at ../jstracer.cpp:4172

Given that the source coordinates for this don't even exist anymore (instead of just having moved around), I doubt this is useful. But it also doesn't hurt, I suppose.

Comment 70

[Till Schneidereit [:till]]

Comment on attachment 8413503 [details] [diff] [review]
(part 13) - Remove JSVAL_IS_PRIMITIVE

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

Given that !isPrimitive === isObject, it would be nice to actually use that, so can you maybe do an s/\!(\w+).isPrimitive\(/$1.isObject(/ on the patch file? (Or, to catch all cases in CTypes.cpp, too: /\!(\w+\[?\d?\]?).isPrimitive\(/.

r=me with that.

::: js/src/ctypes/CTypes.cpp
@@ +3808,5 @@
>    JS_ASSERT(objTypeProto);
>    JS_ASSERT(CType::IsCTypeProto(objTypeProto));
>  
>    jsval valCTypes = JS_GetReservedSlot(objTypeProto, SLOT_CTYPES);
> +  JS_ASSERT(!valCTypes.isPrimitive());

So you don't think asserting this twice is extra-safe?

@@ +3991,5 @@
>    RootedObject thisObj(cx, nullptr);
>    if (args.length() >= 2) {
>      if (args[1].isNull()) {
>        thisObj = nullptr;
> +    } else if (!args[1].isPrimitive()) {

This and a handful of other usages in this file wouldn't be caught by the simpler regexp I gave above, but would make more sense as `args[n].isObject()` calls, too.

::: js/src/tests/js1_8_1/regress/regress-452498-119.js
@@ +30,5 @@
>      eval("for(let y in [false]) var x, x = 0");
>    }
>    f();
>  
> +// Assertion failure: !regs.sp[-2].isPrimitive(), at ../jsinterp.cpp:3243

I realize that I actually like the idea behind these comments. They just don't really work, though.

Comment 71

[Till Schneidereit [:till]]

Comment on attachment 8413505 [details] [diff] [review]
(part 14) - Remove JSVAL_IS_GCTHING

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

nice. r=me

Comment 72

[Till Schneidereit [:till]]

Comment on attachment 8413509 [details] [diff] [review]
(part 15) - Remove JSVAL_TO_GCTHING

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

obviously. r=me

Comment 73

[Till Schneidereit [:till]]

Comment on attachment 8413511 [details] [diff] [review]
(part 16) - Remove JSVAL_TO_PRIVATE

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

This doesn't really kill jsval, though, right? No need to change that - we can, and probably should, do it in the follow-up bug about removing *_TO_JSVAL macros - but the description should reflect what's done here.

In that spirit: in part 15, I saw that there's a JSVAL_IS_UNIVERSAL in jsapi.h. That is only used in XMLHttpRequest, which could be changed to !isGCThing.
Also, there are JSVAL_IS_SPECIFIC_BOOLEAN, which is only used in isTrue and isFalse, and JSVAL_IS_MAGIC, which is only used in isMagic.

r=me with this feedback addressed.

Also, \o/

Comment 74

[Nicholas Nethercote [inactive]]

> This doesn't really kill jsval, though, right? No need to change that - we
> can, and probably should, do it in the follow-up bug about removing
> *_TO_JSVAL macros - but the description should reflect what's done here.

You mean the bug title?

> In that spirit: in part 15, I saw that there's a JSVAL_IS_UNIVERSAL in
> jsapi.h. That is only used in XMLHttpRequest, which could be changed to
> !isGCThing.

Ok, I'll remove that in an additional patch. I'll assume a pre-emptive r=till.

> Also, there are JSVAL_IS_SPECIFIC_BOOLEAN, which is only used in isTrue and
> isFalse,

That one should be called JSVAL_IS_SPECIFIC_BOOLEAN_IMPL, because it's similar to JSVAL_IS_SPECIFIC_INT32_IMPL...

> and JSVAL_IS_MAGIC, which is only used in isMagic.

... it's actually JSVAL_IS_MAGIC_IMPL. There are numerous JSVAL_IS_*_IMPL functions remaining. All these _IMPL functions take a |jsval_layout| rather than a |jsval|, so they don't need to be removed in order to kill off |jsval|.

Comment 75

[Nicholas Nethercote [inactive]]

BTW, thanks for the very fast reviews.

Comment 76

[Till Schneidereit [:till]]

(In reply to Nicholas Nethercote [:njn] from comment #74)
> > This doesn't really kill jsval, though, right? No need to change that - we
> > can, and probably should, do it in the follow-up bug about removing
> > *_TO_JSVAL macros - but the description should reflect what's done here.
> 
> You mean the bug title?

Err, yes. :) I don't really know where that came from, sorry.

> 
> > In that spirit: in part 15, I saw that there's a JSVAL_IS_UNIVERSAL in
> > jsapi.h. That is only used in XMLHttpRequest, which could be changed to
> > !isGCThing.
> 
> Ok, I'll remove that in an additional patch. I'll assume a pre-emptive
> r=till.

Yay.

> 
> > Also, there are JSVAL_IS_SPECIFIC_BOOLEAN, which is only used in isTrue and
> > isFalse,
> 
> That one should be called JSVAL_IS_SPECIFIC_BOOLEAN_IMPL, because it's
> similar to JSVAL_IS_SPECIFIC_INT32_IMPL...
> 
> > and JSVAL_IS_MAGIC, which is only used in isMagic.
> 
> ... it's actually JSVAL_IS_MAGIC_IMPL. There are numerous JSVAL_IS_*_IMPL
> functions remaining. All these _IMPL functions take a |jsval_layout| rather
> than a |jsval|, so they don't need to be removed in order to kill off
> |jsval|.

Note to self: after switching between a Splinter tab, a mxr tab and your IDE too many times, verify that you didn't completely mix up the things you just saw. Sorry for the noise on this stuff.

> BTW, thanks for the very fast reviews.

You're very welcome!

Comment 77

[Nicholas Nethercote [inactive]]

Attachment: (part 13b) - Convert many !v.isPrimitive() calls to v.isObject()

> Given that !isPrimitive === isObject, it would be nice to actually use that,
> so can you maybe do an s/\!(\w+).isPrimitive\(/$1.isObject(/ on the patch
> file? (Or, to catch all cases in CTypes.cpp, too:
> /\!(\w+\[?\d?\]?).isPrimitive\(/.

This turned out to be a good idea. There were lots of cases like this:

> if (!val.isPrimitive()) {
>   JSObject *obj = val.toObjectOrNull();
>   ...
> }

where it became clear that toObject() could be used instead of toObjectOrNull():

> if (val.isObject()) {
>   JSObject *obj = &val.toObject();  // Or perhaps |JSObject &obj = val.toObject();|
>   ...
> }

I've done them in this follow-up patch.

Comment 78

[Nicholas Nethercote [inactive]]

Attachment: (part 17) - Remove JSVAL_IS_UNIVERSAL

Comment 79

[Nicholas Nethercote [inactive]]

Attachment: (part 18) - Rename JSVAL_IS_SPECIFIC_BOOLEAN

Comment 80

[Till Schneidereit [:till]]

Comment on attachment 8414095 [details] [diff] [review]
(part 13b) - Convert many !v.isPrimitive() calls to v.isObject()

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

Oh, even much nicer than what I had in mind!

Comment 81

[Till Schneidereit [:till]]

Comment on attachment 8414107 [details] [diff] [review]
(part 17) - Remove JSVAL_IS_UNIVERSAL

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

Obviously

Comment 82

[Till Schneidereit [:till]]

Comment on attachment 8414109 [details] [diff] [review]
(part 18) - Rename JSVAL_IS_SPECIFIC_BOOLEAN

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

Very certainly yes

Comment 83

[Nicholas Nethercote [inactive]]

My try push was green except for some Android crashes:
https://tbpl.mozilla.org/?tree=Try&rev=8be09a4fde46

But I rebased and re-pushed and Android was green:
https://tbpl.mozilla.org/?tree=Try&rev=cece2118a57c

So I think I'm clear to land.

Comment 84

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24dedc2a7064
https://hg.mozilla.org/integration/mozilla-inbound/rev/24f49a2ca817
https://hg.mozilla.org/integration/mozilla-inbound/rev/840f2ac959d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/899b41829e76
https://hg.mozilla.org/integration/mozilla-inbound/rev/77173a0f4085
https://hg.mozilla.org/integration/mozilla-inbound/rev/47e4ad6fbe3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/061b0af0e45b
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdc9683e85ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/762a93154b1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/31a8179c225b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5711e3806d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/a61fdeb956a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cdc75f1615b
https://hg.mozilla.org/integration/mozilla-inbound/rev/582a4beff7b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/eadb3ac8bc7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/a97a14bb56df
https://hg.mozilla.org/integration/mozilla-inbound/rev/697506d9c324
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f2c7485b6a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/51eaa71a1dfb

Comment 85

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/24dedc2a7064
https://hg.mozilla.org/mozilla-central/rev/24f49a2ca817
https://hg.mozilla.org/mozilla-central/rev/840f2ac959d4
https://hg.mozilla.org/mozilla-central/rev/899b41829e76
https://hg.mozilla.org/mozilla-central/rev/77173a0f4085
https://hg.mozilla.org/mozilla-central/rev/47e4ad6fbe3c
https://hg.mozilla.org/mozilla-central/rev/061b0af0e45b
https://hg.mozilla.org/mozilla-central/rev/bdc9683e85ba
https://hg.mozilla.org/mozilla-central/rev/762a93154b1e
https://hg.mozilla.org/mozilla-central/rev/31a8179c225b
https://hg.mozilla.org/mozilla-central/rev/d5711e3806d0
https://hg.mozilla.org/mozilla-central/rev/a61fdeb956a6
https://hg.mozilla.org/mozilla-central/rev/7cdc75f1615b
https://hg.mozilla.org/mozilla-central/rev/582a4beff7b0
https://hg.mozilla.org/mozilla-central/rev/eadb3ac8bc7f
https://hg.mozilla.org/mozilla-central/rev/a97a14bb56df
https://hg.mozilla.org/mozilla-central/rev/697506d9c324
https://hg.mozilla.org/mozilla-central/rev/2f2c7485b6a6
https://hg.mozilla.org/mozilla-central/rev/51eaa71a1dfb

Comment 86

[Andrew McCreight [:mccr8]]

The changes to the two Skia files were never uplifted to the Skia repo, so they got reverted when the version of Skia was updated. Somebody will need to do that.