[gjs] importer: Give module objects a [Symbol.toStringTag]

[Philip Chimento <pchimento src gnome org>]

commit 043343920980dd238ae170474a23f6ea7cf17763
Author: Philip Chimento <philip chimento gmail com>
Date:   Sat Jul 22 20:45:07 2017 -0700

    importer: Give module objects a [Symbol.toStringTag]
    
    Instead of defining a toString() function on module objects, give them a
    [Symbol.toStringTag] property. This avoids overwriting a toString()
    property that the module may have defined itself.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=781623

 gjs/importer.cpp                     |   44 +++++++++------------------------
 installed-tests/js/modules/foobar.js |    6 ++++
 installed-tests/js/testImporter.js   |   10 ++++++-
 3 files changed, 26 insertions(+), 34 deletions(-)
---
diff --git a/gjs/importer.cpp b/gjs/importer.cpp
index 5b48b3b..1de23d4 100644
--- a/gjs/importer.cpp
+++ b/gjs/importer.cpp
@@ -130,6 +130,7 @@ define_meta_properties(JSContext       *context,
     JS::RootedValue module_name_val(context, JS::NullValue());
     JS::RootedValue parent_module_val(context, JS::NullValue());
     JS::RootedValue module_path(context, JS::NullValue());
+    JS::RootedValue to_string_tag(context);
     if (parent_is_module) {
         module_name_val.setString(JS_NewStringCopyZ(context, module_name));
         parent_module_val.setObject(*parent);
@@ -150,6 +151,12 @@ define_meta_properties(JSContext       *context,
             module_path_buf = g_strdup_printf("%s.%s", parent_path.get(), module_name);
         }
         module_path.setString(JS_NewStringCopyZ(context, module_path_buf));
+
+        GjsAutoChar to_string_tag_buf = g_strdup_printf("GjsModule %s",
+                                                        module_path_buf.get());
+        to_string_tag.setString(JS_NewStringCopyZ(context, to_string_tag_buf));
+    } else {
+        to_string_tag.setString(JS_AtomizeString(context, "GjsModule"));
     }
 
     if (!JS_DefineProperty(context, module_obj,
@@ -164,7 +171,11 @@ define_meta_properties(JSContext       *context,
                            attrs))
         return false;
 
-    return true;
+    JS::RootedId to_string_tag_name(context,
+        SYMBOL_TO_JSID(JS::GetWellKnownSymbol(context,
+                                              JS::SymbolCode::toStringTag)));
+    return JS_DefinePropertyById(context, module_obj, to_string_tag_name,
+                                 to_string_tag, attrs);
 }
 
 static bool
@@ -252,29 +263,6 @@ cancel_import(JSContext       *context,
 }
 
 static bool
-module_to_string(JSContext *cx,
-                 unsigned   argc,
-                 JS::Value *vp)
-{
-    GJS_GET_THIS(cx, argc, vp, args, module);
-
-    JS::RootedValue module_path(cx);
-    if (!gjs_object_get_property(cx, module, GJS_STRING_MODULE_PATH,
-                                 &module_path))
-        return false;
-
-    g_assert(!module_path.isNull());
-
-    GjsAutoJSChar path(cx);
-    if (!gjs_string_to_utf8(cx, module_path, &path))
-        return false;
-    GjsAutoChar output = g_strdup_printf("[GjsModule %s]", path.get());
-
-    args.rval().setString(JS_NewStringCopyZ(cx, output));
-    return true;
-}
-
-static bool
 import_native_file(JSContext       *context,
                    JS::HandleObject obj,
                    const char      *name)
@@ -289,10 +277,6 @@ import_native_file(JSContext       *context,
     if (!define_meta_properties(context, module_obj, NULL, name, obj))
         return false;
 
-    if (!JS_DefineFunction(context, module_obj, "toString", module_to_string,
-                           0, 0))
-        return false;
-
     if (JS_IsExceptionPending(context)) {
         /* I am not sure whether this can happen, but if it does we want to trap it.
          */
@@ -452,10 +436,6 @@ import_file_on_module(JSContext       *context,
     if (!define_meta_properties(context, module_obj, full_path, name, obj))
         goto out;
 
-    if (!JS_DefineFunction(context, module_obj, "toString", module_to_string,
-                           0, 0))
-        goto out;
-
     if (!seal_import(context, obj, id, name))
         goto out;
 
diff --git a/installed-tests/js/modules/foobar.js b/installed-tests/js/modules/foobar.js
index cdd5cad..4ac0a24 100644
--- a/installed-tests/js/modules/foobar.js
+++ b/installed-tests/js/modules/foobar.js
@@ -2,3 +2,9 @@
 
 var foo = "This is foo";
 var bar = "This is bar";
+
+var toString = x => x;
+
+function testToString(x) {
+    return toString(x);
+}
diff --git a/installed-tests/js/testImporter.js b/installed-tests/js/testImporter.js
index e216561..e46781f 100644
--- a/installed-tests/js/testImporter.js
+++ b/installed-tests/js/testImporter.js
@@ -79,6 +79,10 @@ describe('Importer', function () {
         expect(foobar.bar).toEqual('This is bar');
     });
 
+    it('can import a module with a toString property', function () {
+        expect(foobar.testToString('foo')).toEqual('foo');
+    });
+
     it('makes deleting the import a no-op', function () {
         expect(delete imports.foobar).toBeFalsy();
         expect(imports.foobar).toBe(foobar);
@@ -98,8 +102,10 @@ describe('Importer', function () {
     });
 
     it('imports modules with a toString representation', function () {
-        expect(foobar.toString()).toEqual('[GjsModule foobar]');
-        expect(subFoobar.toString()).toEqual('[GjsModule subA.subB.foobar]');
+        expect(Object.prototype.toString.call(foobar))
+            .toEqual('[object GjsModule foobar]');
+        expect(subFoobar.toString())
+            .toEqual('[object GjsModule subA.subB.foobar]');
     });
 
     it('does not share the same object for a module on a different path', function () {