Created attachment 175701 [details] [review]
Provide a useful toString for importer and module objects.

SpiderMonkey provides a disgustingly horrible default
toString for identification: [object Object]. This
patch provides a more useful toString and also uses
the string created for it during the native module
importer.

Review of attachment 175701 [details] [review]:

It'd be nice to know a specific reason you want this; you could say for example "When debugging, it's convenient to be able to see the file path to the module" (or something).

::: gjs/importer.c
@@ +65,3 @@
+        strval = g_strdup("[Root Module]");
+    else
+        strval = g_strdup_printf("[Module %s]", gjs_string_get_ascii(context, module_path));

Rather than "Module", I suggest toString() overrides use JS_GetClass(), then class->name so there's consistency.  In this case that will be "GjsFileImporter"

@@ +67,3 @@
+        strval = g_strdup_printf("[Module %s]", gjs_string_get_ascii(context, module_path));
+
+    retval = STRING_TO_JSVAL(JS_NewStringCopyZ(context, strval));

You need to g_free(strval);

@@ +131,3 @@
+            module_path_buf = g_strdup_printf("%s.%s",
+                                              gjs_string_get_ascii(context, parent_module_path),
+                                              module_name);

You're not handling the possibility here that __modulePath__ is not null, but also not a string (then gjs_string_get_ascii will assert or crash).

I suggest:

if (JSVAL_IS_STRING(parent_module_path))
  ..
else
  ..

Also note that after bug 635707 lands, gjs_string_get_ascii will malloc().  I plan to land that patch before this one, so be prepared to rebase.  If you don't it will leak here.

@@ +132,3 @@
+                                              gjs_string_get_ascii(context, parent_module_path),
+                                              module_name);
+        module_path = STRING_TO_JSVAL(JS_NewStringCopyZ(context, module_path_buf));

g_free(module_path_buf);

::: gjs/native.c
@@ +137,3 @@
+    if (gjs_object_get_property(context, module_obj, "__modulePath__", &value) &&
+        JSVAL_IS_STRING(value))
+        module_id = gjs_string_get_ascii(context, value);

This will malloc() after bug 635707.

Created attachment 175718 [details] [review]
Provide a useful toString for importer and module objects.

SpiderMonkey provides a disgustingly horrible default
toString for identification: [object Object]. This
patch provides a more useful toString and also uses
the string created for it during the native module
importer.

Created attachment 175797 [details] [review]
Provide a useful toString for importer and module objects.

SpiderMonkey provides a disgustingly horrible default
toString for identification: [object Object]. This
patch provides a more useful toString and also uses
the string created for it during the native module
importer.

Created attachment 177175 [details] [review]
Provide a useful toString for importer and module objects.

SpiderMonkey provides a disgustingly horrible default
toString for identification: [object Object]. When
debugging, it's useful to have information like
the imported path of a module, which we don't store.

Created attachment 177652 [details] [review]
Provide a useful toString for importer and module objects.

SpiderMonkey provides a disgustingly horrible default toString for
identification: [object Object]. When debugging, it's useful to have
information like the imported path of a module, which we currently
don't display.

Review of attachment 177652 [details] [review]:

Two memory leaks and a style comment.

::: gjs/importer.c
@@ +132,3 @@
+        else
+            module_path_buf = g_strdup_printf("%s.%s",
+                                              gjs_string_get_ascii(context, parent_module_path),

Leaks.

@@ +300,3 @@
+
+    retval = STRING_TO_JSVAL(JS_NewStringCopyZ(context, strval));
+    g_free (strval);

Inconsistent style; gjs has no space between identifier and paren.

::: gjs/native.c
@@ +201,3 @@
     {
+        char *path = get_module_path(context, module_obj);
+        native_module = lookup_native_module (context, path, TRUE);

Leaks path.

Created attachment 179802 [details] [review]
Provide a useful toString for importer and module objects.

SpiderMonkey provides a disgustingly horrible default toString for
identification: [object Object]. When debugging, it's useful to have
information like the imported path of a module, which we currently
don't display.

Review of attachment 179802 [details] [review]:

::: gjs/importer.c
@@ +126,3 @@
+            return JS_FALSE;
+
+        char *module_path_buf;

ANSI C only, do this in the beginning of a branch.

@@ +296,3 @@
+    g_assert(!JSVAL_IS_NULL(module_path));
+
+    char *path = gjs_string_get_ascii(context, module_path);

Ditto see above

Created attachment 181152 [details] [review]
Provide a useful toString for importer and module objects.

SpiderMonkey provides a disgustingly horrible default toString for
identification: [object Object]. When debugging, it's useful to have
information like the imported path of a module, which we currently
don't display.

Created attachment 342920 [details] [review]
importer: Useful toString for importer and module objects

SpiderMonkey doesn't have a useful default toString(): [object Object].
When debugging, it's useful to have information like the imported path of
a module, which we previously didn't display.

Original patch by Jasper St. Pierre; rewritten for current GJS by Philip
Chimento.

I rebased this, but it needed enough rewriting that it's worth a review by someone else.

Review of attachment 342920 [details] [review]:

Feel free to push with this nit fixed.

::: gjs/importer.cpp
@@ +126,3 @@
+        if (parent_module_path.isNull())
+            module_path_buf = g_strdup(module_name);
+                                           &parent_module_path))

Nit: it's a bit awkward to have the if block without curly braces and the else block with curly braces.

Attachment 342920 [details] pushed as 4c74e71 - importer: Useful toString for importer and module objects