743843 - Components.Exception should provide an options object API

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Split off from bug 738119. This bug is for the necessary refactoring of the Components.Exception constructor, which is already done/tested and might as well land.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Factor out Exception argument parsing into a helper class. v1

For ease of reviewing, note that there are only 2 changes to the big switch statement:
1 - return ThrowAndFail(...) -> return false.
2 - eMsgBytes -> messageBytes

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Factor individual argument parsers into helper methods. v1

For ease of reviewing, note that the only change to the parsers has been to replace |argv[i]| with |v|.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Ditch the confusing switch-based parsing and add some comments. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Introduce the options object for Components.Exception. v1

Comment 5

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

Comment on attachment 613416 [details] [diff] [review]
Part 4 - Introduce the options object for Components.Exception. v1

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +1941,5 @@
> +        jsval v;
> +
> +        if (!getOption(obj, "result", &v) ||
> +            (!v.isUndefined() && !parseResult(v)))
> +            return false;

I find it hard to parse the condition... Maybe two ifs would work better

Comment 6

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

Comment on attachment 613414 [details] [diff] [review]
Part 2 - Factor individual argument parsers into helper methods. v1

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +1862,5 @@
> +            case 3:
> +                if (!parseStack(argv[2]))
> +                    return false;
> +            case 2:
> +                if (parseResult(argv[1]))

Er, missing '!'?

@@ +1865,5 @@
> +            case 2:
> +                if (parseResult(argv[1]))
> +                    return false;
> +            case 1:
> +                if (parseMessage(argv[0]))

Same here. Good that you remove this code again...

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 613413 [details] [diff] [review]
Part 1 - Factor out Exception argument parsing into a helper class. v1

Switching review over to Ms2ger, given his interest. ;-)

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 613416 [details] [diff] [review]
Part 4 - Introduce the options object for Components.Exception. v1

Flagging mrbkap for a quick sr on the new api.

Comment 9

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

Comment on attachment 613413 [details] [diff] [review]
Part 1 - Factor out Exception argument parsing into a helper class. v1

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +1837,5 @@
> +{
> +    ExceptionArgParser(JSContext *context,
> +                       nsXPConnect *xpconnect) : eMsg("exception")
> +                                               , eResult(NS_ERROR_FAILURE)
> +                                               , cx(context)

Please indent as

ExceptionArgParser(JSContext *context, nsXPConnect *xpconnect)
    : eMsg("exception")
    , eResult(NS_ERROR_FAILURE)
    , cx(context)

@@ +1843,5 @@
> +    {}
> +
> +    // Public exception parameter values. During construction, these are
> +    // initialized to the appropriate defaults.
> +    const char*             eMsg;

Can't say I like those 'e' prefixes.

@@ +1849,5 @@
> +    nsCOMPtr<nsIStackFrame> eStack;
> +    nsCOMPtr<nsISupports>   eData;
> +
> +    // Parse the constructor arguments into the above |eFoo| parameter values.
> +    bool parse(uint32_t argc, jsval *argv) {

JS::Value

@@ +1851,5 @@
> +
> +    // Parse the constructor arguments into the above |eFoo| parameter values.
> +    bool parse(uint32_t argc, jsval *argv) {
> +
> +        // all params are optional - grab any passed in

Drop the empty line

Comment 10

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

Comment on attachment 613414 [details] [diff] [review]
Part 2 - Factor individual argument parsers into helper methods. v1

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +1880,5 @@
> +    /*
> +     * Parsing helpers.
> +     */
> +
> +    bool parseMessage(jsval v) {

const JS::Value&. Also for the others.

@@ +1881,5 @@
> +     * Parsing helpers.
> +     */
> +
> +    bool parseMessage(jsval v) {
> +        JSString* str = JS_ValueToString(cx, v);

* to the right

@@ +1882,5 @@
> +     */
> +
> +    bool parseMessage(jsval v) {
> +        JSString* str = JS_ValueToString(cx, v);
> +        if (!str || !(eMsg = messageBytes.encode(cx, str)))

if (!str)
    return false;

eMsg = messageBytes.encode(cx, str)
return !!eMsg;

@@ +1890,5 @@
> +
> +    bool parseResult(jsval v) {
> +        if (!JS_ValueToECMAInt32(cx, v, (int32_t*) &eResult))
> +            return false;
> +        return true;

return JS_ValueToECMAInt32...

@@ +1894,5 @@
> +        return true;
> +    }
> +
> +    bool parseStack(jsval v) {
> +        if (JSVAL_IS_NULL(v)) {

v.isNull()

@@ +1897,5 @@
> +    bool parseStack(jsval v) {
> +        if (JSVAL_IS_NULL(v)) {
> +            // eStack has already been initialized to null.
> +            return true;
> +        } else {

No else after a return

@@ +1898,5 @@
> +        if (JSVAL_IS_NULL(v)) {
> +            // eStack has already been initialized to null.
> +            return true;
> +        } else {
> +            if (JSVAL_IS_PRIMITIVE(v) ||

if (!v.isObject())
    return false;

@@ +1899,5 @@
> +            // eStack has already been initialized to null.
> +            return true;
> +        } else {
> +            if (JSVAL_IS_PRIMITIVE(v) ||
> +                NS_FAILED(xpc->WrapJS(cx, JSVAL_TO_OBJECT(v),

return NS_SUCCEEDED(...

&v.toObject()

@@ +1901,5 @@
> +        } else {
> +            if (JSVAL_IS_PRIMITIVE(v) ||
> +                NS_FAILED(xpc->WrapJS(cx, JSVAL_TO_OBJECT(v),
> +                                      NS_GET_IID(nsIStackFrame),
> +                                      (void**)getter_AddRefs(eStack))))

The (void**) cast shouldn't be necessary.

@@ +1907,5 @@
> +        }
> +        return true;
> +    }
> +
> +    bool parseData(jsval v) {

Same.

Comment 11

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

Comment on attachment 613416 [details] [diff] [review]
Part 4 - Introduce the options object for Components.Exception. v1

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +1937,5 @@
>          return true;
>      }
>  
> +    bool parseOptionsObject(JSObject &obj) {
> +        jsval v;

JS::Value

@@ +1941,5 @@
> +        jsval v;
> +
> +        if (!getOption(obj, "result", &v) ||
> +            (!v.isUndefined() && !parseResult(v)))
> +            return false;

Please make it so

::: js/xpconnect/tests/chrome/Makefile.in
@@ +69,5 @@
>  		test_bug658560.xul \
>  		test_bug679861.xul \
>  		test_APIExposer.xul \
>  		test_bug664689.xul \
> +		test_bug738119.xul \

If you keep it here, please move test_bug706301.xul above it

::: js/xpconnect/tests/chrome/test_bug738119.xul
@@ +1,5 @@
> +<?xml version="1.0"?>
> +<?xml-stylesheet type="text/css" href="chrome://global/skin"?>
> +<?xml-stylesheet type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css"?>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=738119

Might as well use the bug number for this bug.

@@ +28,5 @@
> +  is(e1.data, window, "User data should get set properly");
> +
> +  // Test the options object.
> +  var e2 = Components.Exception('foo', { result: Components.results.NS_BINDING_ABORTED,
> +                                         data: window });

Please pass extra arguments here and test they aren't used

Comment 12

[Bobby Holley (:bholley)]

Thanks for the careful review, Ms2ger. :-)

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3bdc8131e7d3

Comment 13

[Bobby Holley (:bholley)]

Looks green. Pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/39cf985bb1ce
http://hg.mozilla.org/integration/mozilla-inbound/rev/b80749916305
http://hg.mozilla.org/integration/mozilla-inbound/rev/b31df4d97584
http://hg.mozilla.org/integration/mozilla-inbound/rev/d4a0adaa9328

Comment 14

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

(In reply to Ms2ger from comment #10)
> Comment on attachment 613414 [details] [diff] [review]
> Part 2 - Factor individual argument parsers into helper methods. v1
> 
> Review of attachment 613414 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/xpconnect/src/XPCComponents.cpp
> @@ +1880,5 @@
> > +    /*
> > +     * Parsing helpers.
> > +     */
> > +
> > +    bool parseMessage(jsval v) {
> 
> const JS::Value&. Also for the others.

Hmm, did the 'const' go missing?

Comment 15

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/d4a0adaa9328
https://hg.mozilla.org/mozilla-central/rev/b31df4d97584
https://hg.mozilla.org/mozilla-central/rev/b80749916305
https://hg.mozilla.org/mozilla-central/rev/39cf985bb1ce
https://hg.mozilla.org/mozilla-central/rev/69fdff02f85d

Comment 16

[Bobby Holley (:bholley)]

(In reply to Ms2ger from comment #14)
> (In reply to Ms2ger from comment #10)
> > const JS::Value&. Also for the others.
> 
> Hmm, did the 'const' go missing?

Oops, yeah. Sorry about that. :-(