917798 - Handlify JS exception APIs

Status: RESOLVED FIXED

Description

[Jon Coppeard (:jonco)]

Comment 1

[Jon Coppeard (:jonco)]

Created attachment 806804 [details] [diff] [review]
handlify-exception-apis

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 806804 [details] [diff] [review]
handlify-exception-apis

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

r=me on the JS and JSD changes.

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 806804 [details] [diff] [review]
handlify-exception-apis

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

::: dom/bindings/Exceptions.cpp
@@ +70,5 @@
>    if (!obj) {
>      return false;
>    }
>  
> +  JS::Rooted<JS::Value> exn(aCx, OBJECT_TO_JSVAL(obj));

I think we're now officially using JS::RootedValue everywhere.

This should also be JS::ObjectValue(obj).

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +456,5 @@
>      JSString *str = ::JS_NewUCStringCopyN(cx, (jschar *)ucex.get(),
>                                            ucex.Length());
>  
>      if (str) {
> +      JS::Rooted<JS::Value> exn(cx, STRING_TO_JSVAL(str));

RootedValue, StringValue(str)

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +61,5 @@
>  
>  static nsresult
>  ReportError(JSContext *cx, const char *msg)
>  {
> +    RootedValue exn(cx, STRING_TO_JSVAL(JS_NewStringCopyZ(cx, msg)));

StringValue

Comment 4

[Boris Zbarsky [:bz] (still digging out from vacation mail)]

Comment on attachment 806804 [details] [diff] [review]
handlify-exception-apis

>+      if (aRetValue)

Curlies, please.  r=me

Comment 5

[Boris Zbarsky [:bz] (still digging out from vacation mail)]

> I think we're now officially using JS::RootedValue everywhere.

Er, we are?  In non-JS code?  Where was that decided, exactly?  I saw nothing about it on .platform.

Comment 6

[Luke Wagner [:luke]]

(In reply to Boris Zbarsky [:bz] from comment #5)
Well, what was "decided" (insofar as njn asking for a module owner decision and me providing one in https://groups.google.com/d/msg/mozilla.dev.tech.js-engine.internals/meWx5yxofYw/NKl_wsFCvL4J) was on using typedefs inside the JS engine.  A corollary is that it would be weird to use a different style inside and outside of the engine, so I expressed my preference for that.  I guess I should have followed up with a dev.platform post asking if we could adopt typedefs-only convention outside JS too; I mostly just forgot about the thread as comments died down.  (Note: the Handle/Rooted templates would still be exposed and used in exceptional cases like generic algorithms.)

Comment 7

[Luke Wagner [:luke]]

m.d.platform thread: https://groups.google.com/forum/#!topic/mozilla.dev.platform/DtN9842JqRo

Comment 8

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0874c0c422b1

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0874c0c422b1

Comment 10

[Florian Scholz [:fscholz] (MDN)]

Docs updated (thanks :arai):

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_GetPendingException
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_SetPendingException
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_ErrorFromException

Technical review appreciated.