Last Comment Bug 743843 - Components.Exception should provide an options object API
: Components.Exception should provide an options object API
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Bobby Holley (busy)
:
Mentors:
Depends on:
Blocks: 738119
  Show dependency treegraph
 
Reported: 2012-04-09 15:16 PDT by Bobby Holley (busy)
Modified: 2012-04-16 08:42 PDT (History)
5 users (show)
bobbyholley: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Factor out Exception argument parsing into a helper class. v1 (7.58 KB, patch)
2012-04-09 15:19 PDT, Bobby Holley (busy)
Ms2ger: review+
Details | Diff | Review
Part 2 - Factor individual argument parsers into helper methods. v1 (4.46 KB, patch)
2012-04-09 15:19 PDT, Bobby Holley (busy)
Ms2ger: review+
Details | Diff | Review
Part 3 - Ditch the confusing switch-based parsing and add some comments. v1 (2.21 KB, patch)
2012-04-09 15:19 PDT, Bobby Holley (busy)
Ms2ger: review+
Details | Diff | Review
Part 4 - Introduce the options object for Components.Exception. v1 (6.12 KB, patch)
2012-04-09 15:19 PDT, Bobby Holley (busy)
Ms2ger: review+
mrbkap: superreview+
Details | Diff | Review

Description Bobby Holley (busy) 2012-04-09 15:16:03 PDT
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 (busy) 2012-04-09 15:19:04 PDT
Created attachment 613413 [details] [diff] [review]
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 (busy) 2012-04-09 15:19:20 PDT
Created attachment 613414 [details] [diff] [review]
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 (busy) 2012-04-09 15:19:33 PDT
Created attachment 613415 [details] [diff] [review]
Part 3 - Ditch the confusing switch-based parsing and add some comments. v1
Comment 4 Bobby Holley (busy) 2012-04-09 15:19:50 PDT
Created attachment 613416 [details] [diff] [review]
Part 4 - Introduce the options object for Components.Exception. v1
Comment 5 :Ms2ger 2012-04-10 01:43:50 PDT
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 2012-04-10 01:43:59 PDT
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 (busy) 2012-04-10 09:24:12 PDT
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 (busy) 2012-04-10 09:25:18 PDT
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 2012-04-15 14:15:18 PDT
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 2012-04-15 14:23:08 PDT
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 2012-04-15 14:29:42 PDT
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 (busy) 2012-04-15 16:48:32 PDT
Thanks for the careful review, Ms2ger. :-)

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3bdc8131e7d3
Comment 14 :Ms2ger 2012-04-16 01:11:53 PDT
(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 16 Bobby Holley (busy) 2012-04-16 08:42:02 PDT
(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. :-(

Note You need to log in before you can comment on or make changes to this bug.