Created attachment 348040 [details] [review]
jsapi-util-args: Functions with only optional args

An oversight in the original implementation: arguments were not
considered optional if all the arguments were optional.

The System.dumpHeapComplete() patch requires this fix to gjs_parse_call_args(), as well.

Review of attachment 348033 [details] [review]:

Thanks! A few small comments.

::: modules/system.cpp
@@ +109,3 @@
+        return false;
+
+    FILE *file;

What is the use case for the mode argument? Why not always "w"?

@@ +113,3 @@
+        fclose(file);
+    }
+    if (!gjs_parse_call_args(context, "dumpHeapComplete", argv, "|ss",

style: braces around the else clause and 4-space indent

@@ +175,3 @@
     JS_FS("refcount", gjs_refcount, 1, GJS_MODULE_PROP_FLAGS),
     JS_FS("breakpoint", gjs_breakpoint, 0, GJS_MODULE_PROP_FLAGS),
+    JS_FS("dumpHeapComplete", gjs_dump_heap_complete, 0, GJS_MODULE_PROP_FLAGS),

Thinking about the name some more, "Complete" doesn't actually refer to anything outside of the SpiderMonkey API, so we should probably just call it "dumpHeap".

Review of attachment 348040 [details] [review]:

This one looks good.

(In reply to Philip Chimento from comment #3)
> +    FILE *file;
> 
> What is the use case for the mode argument? Why not always "w"?

Appending instead of truncating the file


> +    JS_FS("dumpHeapComplete", gjs_dump_heap_complete, 0,
> GJS_MODULE_PROP_FLAGS),
> 
> Thinking about the name some more, "Complete" doesn't actually refer to
> anything outside of the SpiderMonkey API, so we should probably just call it
> "dumpHeap".

Sure no problem

Created attachment 348114 [details] [review]
Add System.dumpHeapComplete() method

Review of attachment 348114 [details] [review]:

::: gjs/jsapi-wrapper.h
@@ +36,3 @@
 #endif
 #include <jsapi.h>
+#include <jsfriendapi.h>

I forgot to mention the first time, this will need a rebase on master, it looks like this is applied to a mozjs31 point in the tree.

::: modules/system.cpp
@@ +104,3 @@
+    FILE *file;
+
+{

I missed this last time, but the signature has to be "|fs" so that the filename string is converted to filename encoding.

@@ +106,3 @@
+    if (!gjs_parse_call_args(context, "dumpHeap", argv, "|ss",
+                             "filename", &filename,
+    JS::CallArgs argv = JS::CallArgsFromVp(argc, vp);

I'm still not convinced of the usefulness of being able to append / overwrite. At least, not enough to balance the low-level C fopen() API leaking into JS here. Could we not just always append except if the file doesn't exist?

Comment on attachment 348040 [details] [review]
jsapi-util-args: Functions with only optional args

Attachment 348040 [details] pushed as cf0ac43 - jsapi-util-args: Functions with only optional args

I rebased it and amended it to always append, so we can commit it for now. If you feel like allowing an extra parameter for overwriting, that can certainly be added in the future.