[gjs] coverage: Pass filenames instead of objects

[Jasper St. Pierre <jstpierre src gnome org>]

commit c0a168b237e23f977ce07c74693e2c2fe70cfbfc
Author: Jasper St. Pierre <jstpierre mecheye net>
Date:   Mon Feb 3 13:48:32 2014 -0500

    coverage: Pass filenames instead of objects
    
    This simplifies the code a bit as well and prepares us for passing
    import prefixes rather than explicit filenames.

 gjs/coverage.cpp                   |  129 ++++++++++++-----------------------
 gjs/jsapi-util.cpp                 |   33 ++++++---
 gjs/jsapi-util.h                   |    3 +
 installed-tests/js/testCoverage.js |   22 ++++---
 modules/coverage.js                |   18 +----
 5 files changed, 85 insertions(+), 120 deletions(-)
---
diff --git a/gjs/coverage.cpp b/gjs/coverage.cpp
index 59d21ff..0493f4b 100644
--- a/gjs/coverage.cpp
+++ b/gjs/coverage.cpp
@@ -941,86 +941,6 @@ gjs_coverage_init(GjsCoverage *self)
 {
 }
 
-static JSBool
-lazy_get_script_contents(JSContext *context,
-                         unsigned   argc,
-                         jsval     *vp)
-{
-    JSObject *this_object = JS_THIS_OBJECT(context, vp);
-
-    jsval filename_value;
-    if (!JS_GetProperty(context, this_object, "filename", &filename_value) ||
-        !JSVAL_IS_STRING(filename_value)) {
-        gjs_throw(context, "The 'filename' property on this is not set");
-        return JS_FALSE;
-    }
-
-    char *filename = NULL;
-
-    if (!gjs_string_to_utf8(context, filename_value, &filename)) {
-        gjs_throw(context, "Failed to convert filename to a string");
-        return JS_FALSE;
-    }
-
-    GFile *file = g_file_new_for_commandline_arg(filename);
-    GError *error = NULL;
-
-    char *script;
-    gsize script_len;
-
-    if (!g_file_load_contents(file,
-                              NULL,
-                              &script,
-                              &script_len,
-                              NULL,
-                              &error)) {
-        gjs_throw(context, "Failed to load contents for filename %s: %s", filename, error->message);
-        g_object_unref(file);
-        g_free(filename);
-        g_clear_error(&error);
-        return JS_FALSE;
-    }
-
-    g_object_unref(file);
-    g_free(filename);
-
-    JSString *script_jsstr = JS_NewStringCopyN(context, script, script_len);
-    JS_SET_RVAL(context, vp, STRING_TO_JSVAL(script_jsstr));
-
-    g_free(script);
-
-    return JS_TRUE;
-}
-
-
-static GArray *
-strv_to_js_script_info_value_array(JSContext *context, char **strv)
-{
-    GArray *script_filenames = g_array_new(TRUE, TRUE, sizeof(jsval));
-    g_array_set_size(script_filenames, g_strv_length(strv));
-
-    unsigned int i = 0;
-    for (; i < script_filenames->len; ++i) {
-        const char *filename = strv[i];
-        JSString *filename_str = JS_NewStringCopyZ(context, filename);
-        jsval filename_str_value = STRING_TO_JSVAL(filename_str);
-
-        JSObject *object = JS_NewObject(context, NULL, NULL, NULL);
-
-        JS_SetProperty(context, object, "filename", &filename_str_value);
-        JS_DefineFunction(context,
-                          object,
-                          "getContents",
-                          (JSNative) lazy_get_script_contents,
-                          0,
-                          GJS_MODULE_PROP_FLAGS);
-
-        g_array_index(script_filenames, jsval, i) = OBJECT_TO_JSVAL(object);
-    }
-
-    return script_filenames;
-}
-
 static JSClass coverage_global_class = {
     "GjsCoverageGlobal", JSCLASS_GLOBAL_FLAGS_WITH_SLOTS(GJS_GLOBAL_SLOT_LAST),
     JS_PropertyStub, JS_DeletePropertyStub, JS_PropertyStub, JS_StrictPropertyStub,
@@ -1112,8 +1032,51 @@ coverage_warning(JSContext *context,
     return JS_TRUE;
 }
 
+static JSBool
+coverage_get_file_contents(JSContext *context,
+                           unsigned   argc,
+                           jsval     *vp)
+{
+    JSBool ret = JS_FALSE;
+    JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
+    char *filename = NULL;
+    GFile *file = NULL;
+    char *script = NULL;
+    gsize script_len;
+    JSString *script_jsstr;
+    GError *error = NULL;
+
+    if (!gjs_parse_call_args(context, "getFileContents", "s", args,
+                             "filename", &filename))
+        goto out;
+
+    file = g_file_new_for_commandline_arg(filename);
+
+    if (!g_file_load_contents(file,
+                              NULL,
+                              &script,
+                              &script_len,
+                              NULL,
+                              &error)) {
+        gjs_throw(context, "Failed to load contents for filename %s: %s", filename, error->message);
+        goto out;
+    }
+
+    args.rval().setString(JS_NewStringCopyN(context, script, script_len));
+    ret = JS_TRUE;
+
+ out:
+    g_clear_error(&error);
+    if (file)
+        g_object_unref(file);
+    g_free(filename);
+    g_free(script);
+    return ret;
+}
+
 static JSFunctionSpec coverage_funcs[] = {
     { "warning", JSOP_WRAPPER (coverage_warning), 1, GJS_MODULE_PROP_FLAGS },
+    { "getFileContents", JSOP_WRAPPER (coverage_get_file_contents), 1, GJS_MODULE_PROP_FLAGS },
     { NULL },
 };
 
@@ -1180,8 +1143,7 @@ bootstrap_coverage(GjsCoverage *coverage)
         JSObject *coverage_statistics_constructor = JSVAL_TO_OBJECT(coverage_statistics_prototype_value);
 
         /* Now create the array to pass the desired script names over */
-        GArray *filenames = strv_to_js_script_info_value_array(context, priv->covered_paths);
-        JSObject *filenames_js_array = JS_NewArrayObject(context, filenames->len, (jsval *) filenames->data);
+        JSObject *filenames_js_array = gjs_build_string_array(context, -1, priv->covered_paths);
 
         jsval coverage_statistics_constructor_arguments[] = {
             OBJECT_TO_JSVAL(filenames_js_array)
@@ -1194,13 +1156,10 @@ bootstrap_coverage(GjsCoverage *coverage)
 
         if (!coverage_statistics) {
             gjs_throw(context, "Failed to create coverage_statitiscs object");
-            g_array_unref(filenames);
             return FALSE;
         }
 
         priv->coverage_statistics = coverage_statistics;
-
-        g_array_unref(filenames);
     }
 
     return TRUE;
diff --git a/gjs/jsapi-util.cpp b/gjs/jsapi-util.cpp
index 44f4534..c588cf9 100644
--- a/gjs/jsapi-util.cpp
+++ b/gjs/jsapi-util.cpp
@@ -229,22 +229,17 @@ gjs_throw_abstract_constructor_error(JSContext *context,
     gjs_throw(context, "You cannot construct new instances of '%s'", name);
 }
 
-JSObject*
-gjs_define_string_array(JSContext   *context,
-                        JSObject    *in_object,
-                        const char  *array_name,
-                        gssize       array_length,
-                        const char **array_values,
-                        unsigned     attrs)
+JSObject *
+gjs_build_string_array(JSContext   *context,
+                       gssize       array_length,
+                       char       **array_values)
 {
     GArray *elems;
     JSObject *array;
     int i;
 
-    JS_BeginRequest(context);
-
     if (array_length == -1)
-        array_length = g_strv_length((char**)array_values);
+        array_length = g_strv_length(array_values);
 
     elems = g_array_sized_new(FALSE, FALSE, sizeof(jsval), array_length);
 
@@ -257,6 +252,23 @@ gjs_define_string_array(JSContext   *context,
     array = JS_NewArrayObject(context, elems->len, (jsval*) elems->data);
     g_array_free(elems, TRUE);
 
+    return array;
+}
+
+JSObject*
+gjs_define_string_array(JSContext   *context,
+                        JSObject    *in_object,
+                        const char  *array_name,
+                        gssize       array_length,
+                        const char **array_values,
+                        unsigned     attrs)
+{
+    JSObject *array;
+
+    JSAutoRequest ar(context);
+
+    array = gjs_build_string_array(context, array_length, (char **) array_values);
+
     if (array != NULL) {
         if (!JS_DefineProperty(context, in_object,
                                array_name, OBJECT_TO_JSVAL(array),
@@ -264,7 +276,6 @@ gjs_define_string_array(JSContext   *context,
             array = NULL;
     }
 
-    JS_EndRequest(context);
     return array;
 }
 
diff --git a/gjs/jsapi-util.h b/gjs/jsapi-util.h
index 607d157..f4e8b8c 100644
--- a/gjs/jsapi-util.h
+++ b/gjs/jsapi-util.h
@@ -247,6 +247,9 @@ JSObject*   gjs_construct_object_dynamic     (JSContext       *context,
                                               JSObject        *proto,
                                               unsigned         argc,
                                               jsval           *argv);
+JSObject*   gjs_build_string_array           (JSContext       *context,
+                                              gssize           array_length,
+                                              char           **array_values);
 JSObject*   gjs_define_string_array          (JSContext       *context,
                                               JSObject        *obj,
                                               const char      *array_name,
diff --git a/installed-tests/js/testCoverage.js b/installed-tests/js/testCoverage.js
index a2ba2d0..2960fd2 100644
--- a/installed-tests/js/testCoverage.js
+++ b/installed-tests/js/testCoverage.js
@@ -819,25 +819,27 @@ function testConvertFunctionCountersToArrayIsSorted() {
                       });
 }
 
-const MockFiles = [
-    {
-        filename: 'filename',
-        getContents: function() {
-            return "let f = function() { return 1; };"
-        }
-    }
-];
+const MockFiles = {
+    'filename': "let f = function() { return 1; };",
+};
+
+const MockFilenames = Object.keys(MockFiles);
 
+Coverage.getFileContents = function(filename) {
+    if (MockFiles[filename])
+        return MockFiles[filename];
+    throw new Error("Non existent");
+};
 
 function testCoverageStatisticsContainerFetchesValidStatisticsForFile() {
-    let container = new Coverage.CoverageStatisticsContainer(MockFiles);
+    let container = new Coverage.CoverageStatisticsContainer(MockFilenames);
     let statistics = container.fetchStatistics('filename');
 
     JSUnit.assertNotEquals(undefined, statistics);
 }
 
 function testCoverageStatisticsContainerThrowsForNonExistingFile() {
-    let container = new Coverage.CoverageStatisticsContainer(MockFiles);
+    let container = new Coverage.CoverageStatisticsContainer(MockFilenames);
 
     JSUnit.assertRaises(function() {
         container.fetchStatistics('nonexistent');
diff --git a/modules/coverage.js b/modules/coverage.js
index 3f2fe41..b425235 100644
--- a/modules/coverage.js
+++ b/modules/coverage.js
@@ -473,24 +473,14 @@ function _populateKnownFunctions(functions, nLines) {
  *
  * filename: a string describing a filename, potentially in
  * pendingFiles
- * pendingFiles: an array of an object with the following properties:
- * - filename: a string describing its filename
- * - getContents: a method to get the contents of a file
+ * pendingFiles: an array of filenames
  */
 function _createStatisticsForFoundFilename(filename, pendingFiles) {
-    let pendingIndex = -1;
-
-    for (let i = 0; i < pendingFiles.length; i++) {
-        if (filename == pendingFiles[i].filename) {
-            pendingIndex = i;
-            break;
-        }
-    }
-
+    let pendingIndex = pendingFiles.indexOf(filename);
     if (pendingIndex !== -1) {
-        let file = (pendingFiles.splice(pendingIndex, 1))[0];
+        pendingFiles.splice(pendingIndex, 1);
 
-        let contents = file.getContents();
+        let contents = getFileContents(filename);
         let reflection = Reflect.parse(contents);
         let nLines = _getNumberOfLinesForScript(contents);