Closed Bug 952650 Opened 10 years ago Closed 10 years ago

Kill all methods like JSVAL_IS_* and JSVAL_TO_*

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: evilpie, Assigned: n.nethercote)

Details

(Whiteboard: [lang=c++] [mentor=evilpie])

Attachments

(19 files, 18 obsolete files)

21.35 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
41.16 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
7.09 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
11.79 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
12.59 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
22.92 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
8.35 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
24.52 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
20.87 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
5.07 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
25.48 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
74.99 KB, patch
till
: review+
Details | Diff | Splinter Review
74.26 KB, patch
till
: review+
Details | Diff | Splinter Review
7.61 KB, patch
till
: review+
Details | Diff | Splinter Review
875 bytes, patch
till
: review+
Details | Diff | Splinter Review
19.23 KB, patch
till
: review+
Details | Diff | Splinter Review
22.89 KB, patch
till
: review+
Details | Diff | Splinter Review
1.97 KB, patch
till
: review+
Details | Diff | Splinter Review
2.06 KB, patch
till
: review+
Details | Diff | Splinter Review
      No description provided.
This could be relatively straightforward work for somebody touching a lot of code.
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [lang=c++] [mentor=evilpie]
Assignee: evilpies → nobody
I would like to contribute, but since I am quite new here I will need some direction.
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.
Oh and I also assigned the bug to you. Good luck and thanks for the help!
Assignee: nobody → rrodrigue96
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.
(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.
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?
(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?
>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();"
(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.
Attached patch bug952650_KillJSVAL_isNull.diff (obsolete) — Splinter Review
Replaced JSVAL_IS_NULL( val ) with isNull().
Attachment #8361887 - Attachment is obsolete: true
Attached patch bug952650_KillJSVAL_isNull.diff (obsolete) — Splinter Review
replaced method JSVAL_IS_NULL(val) with val.isNull().

Also corrected some syntax errors from previous diff file.
Attachment #8362396 - Attachment is obsolete: true
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.
Attachment #8362406 - Flags: review?(terrence)
(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 on attachment 8362406 [details] [diff] [review]
bug952650_KillJSVAL_isNull.diff

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

Looks good! r=me
Attachment #8362406 - Flags: review?(terrence) → review+
replaced JSVAL_IS_VOID( val ) with val.isUndefined()
replaced JSVAL_IS_BOOLEAN( val ) with val.isBoolean()
replaced JSVAL_TO_BOOLEAN( val ) with val.toBoolean()
replaced JSVAL_IS_DOUBLE( val ) with val.isDouble()
replaced JSVAL_TO_DOUBLE( val ) with val.toDouble()
replaced JSVAL_IS_STRING( val ) with val.isString()
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()
opps, thank you Masatoshi Kimura for pointing that out.
Attachment #8362877 - Attachment is obsolete: true
replaced JSVAL_TO_STRING( val ) with val.toString()
Attached patch bug952650_KillJSVAL_isInt32.diff (obsolete) — Splinter Review
replaced JSVAL_IS_INT( val ) with val.isInt32()
Attached patch bug952650_KillJSVAL_toInt32.diff (obsolete) — Splinter Review
replaced JSVAL_TO_INT( val ) with val.toInt32()
replaced JSVAL_IS_NUMBER( val ) with val.isNumber()
Attachment #8362861 - Flags: review?(evilpies)
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?
Attachment #8362861 - Flags: review?(evilpies) → review+
Attached patch bug952650_KillJSVAL_toInt32.diff (obsolete) — Splinter Review
replaced JSVAL_TO_INT( val ) with val.toInt32()

removed some syntax errors
Attachment #8363464 - Attachment is obsolete: true
(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?
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.
Attached patch bug952650_KillJSVAL_toInt32.diff (obsolete) — Splinter Review
replaced JSVAL_TO_INT( val ) with val.toInt32()

removed some syntax errors
Attachment #8363473 - Attachment is obsolete: true
Attached patch jsval-patch (obsolete) — Splinter Review
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.
Attachment #8363258 - Flags: review+
(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?
> 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 :)
(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!
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 )
Attachment #8363269 - Flags: review+
Attachment #8363278 - Flags: review+
Attachment #8363293 - Flags: review+
Attachment #8363433 - Flags: review+
Attachment #8363456 - Flags: review+
Attachment #8363353 - Flags: review+
Attachment #8363466 - Flags: review+
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.
Attachment #8363498 - Flags: review-
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 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?
Attachment #8366488 - Flags: review+
(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.
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.
(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.
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.
Rodrigo, are you still working on this?
Flags: needinfo?(rrodrigue96)
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.
Assignee: rrodrigue96 → nobody
Flags: needinfo?(rrodrigue96)
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.
Assignee: nobody → rrodrigue96
Attachment #8363540 - Attachment is obsolete: true
Attachment #8363540 - Attachment is patch: true
Attachment #8363540 - Attachment mime type: text/x-patch → text/plain
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.
Assignee: rrodrigue96 → n.nethercote
Status: NEW → ASSIGNED
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.
Attachment #8362406 - Attachment is obsolete: true
Attachment #8362861 - Attachment is obsolete: true
Attachment #8363258 - Attachment is obsolete: true
Attachment #8363269 - Attachment is obsolete: true
Attachment #8363278 - Attachment is obsolete: true
Attachment #8363293 - Attachment is obsolete: true
Attachment #8363353 - Attachment is obsolete: true
Attachment #8363433 - Attachment is obsolete: true
Attachment #8363456 - Attachment is obsolete: true
Attachment #8363466 - Attachment is obsolete: true
Attachment #8363498 - Attachment is obsolete: true
Attachment #8366488 - Attachment is obsolete: true
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 :)
Attachment #8413499 - Flags: review?(till)
This one is dead, which makes things easy.
Attachment #8413509 - Flags: review?(till)
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 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.
Attachment #8413499 - Flags: review?(till) → review+
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.
Attachment #8413503 - Flags: review?(till) → review+
Comment on attachment 8413505 [details] [diff] [review]
(part 14) - Remove JSVAL_IS_GCTHING

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

nice. r=me
Attachment #8413505 - Flags: review?(till) → review+
Comment on attachment 8413509 [details] [diff] [review]
(part 15) - Remove JSVAL_TO_GCTHING

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

obviously. r=me
Attachment #8413509 - Flags: review?(till) → review+
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/
Attachment #8413511 - Flags: review?(till) → 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.

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|.
Summary: Kill jsval and all methods like JSVAL_IS_* and JSVAL_TO_* → Kill all methods like JSVAL_IS_* and JSVAL_TO_*
BTW, thanks for the very fast reviews.
(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!
> 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.
Attachment #8414095 - Flags: review?(till)
Attachment #8413485 - Flags: review+
Attachment #8413486 - Flags: review+
Attachment #8413487 - Flags: review+
Attachment #8413488 - Flags: review+
Attachment #8413489 - Flags: review+
Attachment #8413491 - Flags: review+
Attachment #8413492 - Flags: review+
Attachment #8413494 - Flags: review+
Attachment #8413495 - Flags: review+
Attachment #8413496 - Flags: review+
Attachment #8413497 - Flags: review+
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!
Attachment #8414095 - Flags: review?(till) → review+
Comment on attachment 8414107 [details] [diff] [review]
(part 17) - Remove JSVAL_IS_UNIVERSAL

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

Obviously
Attachment #8414107 - Flags: review?(till) → review+
Comment on attachment 8414109 [details] [diff] [review]
(part 18) - Rename JSVAL_IS_SPECIFIC_BOOLEAN

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

Very certainly yes
Attachment #8414109 - Flags: review?(till) → review+
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.
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.
Depends on: 1177825
No longer depends on: 1177825
You need to log in before you can comment on or make changes to this bug.