933834 - Rename and handlify JS_ValueToString

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Tom S [:evilpie]]

Attachment: wip

For some reason this patch was like super annoying. I basically hit it with a wrench until it compiled. js.cpp can be super annoying.

Comment 2

[Tom S [:evilpie]]

Attachment: v1

Comment 3

[Tom S [:evilpie]]

Attachment: real v1

Dammit, sorry.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 832968 [details] [diff] [review]
real v1

Review of attachment 832968 [details] [diff] [review]:
-----------------------------------------------------------------

You are forgiven: seeing |n=1;if(n)| made it all worthwhile. This all looks fine to me, but we should have Boris take a quick look to make sure none of the new Rooted in gecko are likely to bite us. I'm particularly worried about the one in IdToString, since I'd guess that's probably heavily used.

::: ipc/testshell/XPCShellEnvironment.cpp
@@ -103,5 @@
>          fprintf(stdout, "%s%s", i ? " " : "", bytes.ptr());
>          fflush(stdout);
>      }
> -    n++;
> -    if (n)

Hahahahaha. I wonder what series of code reorganizations took this so far off the rails.

::: js/src/jsapi.cpp
@@ -411,5 @@
> -{
> -    RootedValue value(cx, valueArg);
> -    AssertHeapIsIdle(cx);
> -    CHECK_REQUEST(cx);
> -    assertSameCompartment(cx, value);

I guess we lose these assertions moving this to the header. I think this is probably fine as I can't image this conversion being too useful on its own.

::: js/xpconnect/src/XPCShellImpl.cpp
@@ -267,5 @@
>          fprintf(gOutFile, "%s%s", i ? " " : "", strBytes.ptr());
>          fflush(gOutFile);
>      }
> -    n++;
> -    if (n)

Twice? I guess someone copied this code and didn't fix it? O_o

::: js/xpconnect/src/qsgen.py
@@ -456,5 @@
>  
> -    'wstring':
> -        "    const PRUnichar *${name};\n"
> -        "    if (!xpc_qsJsvalToWcharStr(cx, ${argVal}, ${argPtr}, &${name}))\n"
> -        "        return false;\n",

I don't really know if it's okay to remove this, but I guess if Try is green that's fine.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 832968 [details] [diff] [review]
real v1

> I don't really know if it's okay to remove this,

It better be.  We have few quickstubs left and none should be using wstring.  No on should be using wstring.

IdToString is only used for localStorage.  I doubt an extra Rooted there matters.  And we should move that stuff to webidl anyway.  ;)

I would appreciate JS::Rooted<JS::Value>, not JS::RootedValue, in dom code until we mass-convert.

r=me with that

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bc46d4abd0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa66c9f42ff4

Comment 7

[Tom S [:evilpie]]

Had to make 2 minor changes to make it compile everywhere.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf9ea7edd6e2

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/30cbd1abde1a
https://hg.mozilla.org/mozilla-central/rev/cf9ea7edd6e2