435474 - Provide a readline function for xpcshell to read a line from stdin

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Add ReadLine to xpcshell - v1

In bug 300841, readline capabilities were added to js shell. I'd like this to work for xpcshell too.

Since xpcshell already provides a GetLine function, I'm using that instead of the js_fgets from bug 300841's jsscan.c.

This patch doesn't address the problems from bug 105707, when a new patch (after 6 years!) for that bug is created, the line length issue needs to be tackled for this change too.

Comments welcome.

Comment 1

[Brendan Eich [:brendan]]

Comment on attachment 322290 [details] [diff] [review]
Add ReadLine to xpcshell - v1

Really want mrbkap to look at this. Also would like to unify js and xpc shells (there's an old bug asking for that), not diverge them further.

/be

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 322290 [details] [diff] [review]
Add ReadLine to xpcshell - v1

>Index: js/src/xpconnect/shell/xpcshell.cpp
>@@ -173,16 +206,71 @@ my_ErrorReporter(JSContext *cx, const ch
> JS_STATIC_DLL_CALLBACK(JSBool)
>+ReadLine(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+{
>+    // While 4096 might be quite arbitrary, this is something to be fixed in
>+    // bug 105707. It is also the same limit as in ProcessFile.
>+    char buf[4096];
>+    char *promptBytes;

This isn't needed.

>+    /* If a prompt was specified, construct the string */

I'd rather see this written as:

if (argc > 0) {
    str = JS_ValueToString(cx, argv[0]);
    if (!str)
      return JS_FALSE;
    argv[0] = STRING_TO_JSVAL(str);
} else {
    str = JSVAL_TO_STRING(JS_GetEmptyStringValue(cx));
}

Note a couple of things:
- JS_ValueToString reports errors (such as OOM) so you don't have to.
- Generally, xpcshell.cpp doesn't overbrace one-line if conditions.
- Assigning the returned string back into argv[0] is a good habit to prevent GC hazards.

>+    rv = GetLine(cx, buf, gInFile, promptBytes);

Then this becomes:
    rv = GetLine(cx, buf, gInFile, JS_GetStringBytes(str));

>+    free(promptBytes);

And no need to free promptBytes.

>+    if (!rv) {
>+        return JS_FALSE;
>+    }

Overbracing here, I'm not going to point this out every time.

>+    /* Strip newline character */
>+    if (strlen(buf)) {
>+        buf[strlen(buf) - 1] = '\0';
>+    }

If you're going to call strlen multiple times, I'd rather see you stick it in a local variable to avoid counting the characters 3 times.

>+    /* Treat the empty string specially. */
>+    if (strlen(buf) == 0) {

This isn't a big deal if you stash the strlen beforehand, a better way to write this test would be:
    if (buf[0] == '\0')

I'm also not certain this is needed. It seems like JS_NewString* deals with 0-length strings fine. If this came from my ReadLine, the reason there was to avoid the shrinking realloc with 0 size.

>+     /* Turn buf into a JSString */
>+    str = JS_NewStringCopyZ(cx, buf);

Since you've stashed the string length above, this can be a JS_NewStringCopyN.

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Add ReadLine to xpcshell - v2

This patch takes all review comments into account. Sorry that this patch diverges xpcshell and jsshell more, but the changes required for bug 209176 are too massive for me to take care of now. That would require a lot more testing and possibly more knowledge about the works of jsshell/xpcshell and the js api.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 339663 [details] [diff] [review]
Add ReadLine to xpcshell - v2

A couple more nitpicks. Sorry for taking so long to get to this :-/

>+    /* Get a line from the infile */
>+    JSBool rv = GetLine(cx, buf, gInFile, JS_GetStringBytes(str));
>+
>+    if (!rv)
>+        return JS_FALSE;

No need for single-use 'rv' here. Just do:

if (!GetLine(cx, buf, gInFile, JS_GetStringBytes(str))
    return JS_FALSE;

>+    /* Strip newline character added by GetLine() */
>+    unsigned int buflen = strlen(buf);
>+    if (buf[0] != '\0')
>+        buf[buflen - 1] = '\0';

It looks like (because of the call to GetLine, buf[buflen - 1] might not be a newline. This should be:

if (buf[buflen - 1] == '\n')
    buf[--buflen] = '\0';

(notice the --buflen, to avoid copying the null character in the JS_NewStringCopyN).

I didn't look closely enough before. The second reason that the ReadLine in js.cpp handles the empty string specially is to indicate EOF to the script. As the patch is written currently, it's impossible to tell an empty line from EOF. So, after the call to strlen, it seems like we should have:

if (buflen == 0 && feof(gInFile)) {
    *rval = JSVAL_NULL;
    return JS_TRUE;
}

if (buflen > 0 && buf[buflen - 1] == '\n') {
    buf[--buflen] = '\0';
}

Comment 5

[Daniel Boelzle [:dbo]]

Attachment: Add ReadLine to xpcshell - v3

Updated to reflect comments. I've removed the '\0' termination since it's constructed passing the length anyway.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 345267 [details] [diff] [review]
Add ReadLine to xpcshell - v3

Cool, thanks.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 345267 [details] [diff] [review]
Add ReadLine to xpcshell - v3

We'll get to this after beta 2.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 345267 [details] [diff] [review]
Add ReadLine to xpcshell - v3

a191=beltzner

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Patch for checkin

This is v3 for checkin to 1.9.1 (includes checkin message and user)

Comment 10

[Shawn Wilsher :sdwilsh]

In case anyone comes here looking for what I backed out, I meant to type in bug 463824 as the bug number, not this one.

Comment 11

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/43900a377c77