Closed Bug 933834 Opened 6 years ago Closed 6 years ago

Rename and handlify JS_ValueToString

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

No description provided.
Attached patch wip (obsolete) — Splinter Review
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.
Attached patch v1 (obsolete) — Splinter Review
Attachment #828906 - Attachment is obsolete: true
Attachment #832966 - Flags: review?(terrence)
Attached patch real v1Splinter Review
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+
Had to make 2 minor changes to make it compile everywhere.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf9ea7edd6e2
https://hg.mozilla.org/mozilla-central/rev/30cbd1abde1a
https://hg.mozilla.org/mozilla-central/rev/cf9ea7edd6e2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.