Rename and handlify JS_ValueToString

RESOLVED FIXED in mozilla28

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

unspecified
mozilla28
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 828906 [details] [diff] [review]
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.
(Assignee)

Comment 2

5 years ago
Created attachment 832966 [details] [diff] [review]
v1
Attachment #828906 - Attachment is obsolete: true
Attachment #832966 - Flags: review?(terrence)
(Assignee)

Comment 3

5 years ago
Created attachment 832968 [details] [diff] [review]
real v1

Dammit, sorry.
Attachment #832966 - Attachment is obsolete: true
Attachment #832966 - Flags: review?(terrence)
Attachment #832968 - Flags: review?(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.
Attachment #832968 - Flags: review?(terrence)
Attachment #832968 - Flags: review?(bzbarsky)
Attachment #832968 - Flags: review+
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
Attachment #832968 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 7

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

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/30cbd1abde1a
https://hg.mozilla.org/mozilla-central/rev/cf9ea7edd6e2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.