Closed
Bug 743843
Opened 12 years ago
Closed 12 years ago
Components.Exception should provide an options object API
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: bholley, Assigned: bholley)
References
Details
(Keywords: dev-doc-needed)
Attachments
(4 files)
7.58 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
4.46 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
2.21 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
6.12 KB,
patch
|
Ms2ger
:
review+
mrbkap
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
For ease of reviewing, note that there are only 2 changes to the big switch statement: 1 - return ThrowAndFail(...) -> return false. 2 - eMsgBytes -> messageBytes
Attachment #613413 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 2•12 years ago
|
||
For ease of reviewing, note that the only change to the parsers has been to replace |argv[i]| with |v|.
Attachment #613414 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #613415 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #613416 -
Flags: review?(bent.mozilla)
Comment 5•12 years ago
|
||
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•12 years ago
|
||
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...
Assignee | ||
Comment 7•12 years ago
|
||
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. ;-)
Attachment #613413 -
Flags: review?(bent.mozilla) → review?(Ms2ger)
Assignee | ||
Updated•12 years ago
|
Attachment #613414 -
Flags: review?(bent.mozilla) → review?(Ms2ger)
Assignee | ||
Updated•12 years ago
|
Attachment #613415 -
Flags: review?(bent.mozilla) → review?(Ms2ger)
Assignee | ||
Comment 8•12 years ago
|
||
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.
Attachment #613416 -
Flags: superreview?(mrbkap)
Attachment #613416 -
Flags: review?(bent.mozilla)
Attachment #613416 -
Flags: review?(Ms2ger)
Updated•12 years ago
|
Attachment #613416 -
Flags: superreview?(mrbkap) → superreview+
Comment 9•12 years ago
|
||
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
Attachment #613413 -
Flags: review?(Ms2ger) → review+
Comment 10•12 years ago
|
||
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.
Attachment #613414 -
Flags: review?(Ms2ger) → review+
Updated•12 years ago
|
Attachment #613415 -
Flags: review?(Ms2ger) → review+
Comment 11•12 years ago
|
||
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
Attachment #613416 -
Flags: review?(Ms2ger) → review+
Assignee | ||
Comment 12•12 years ago
|
||
Thanks for the careful review, Ms2ger. :-) Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3bdc8131e7d3
Assignee | ||
Comment 13•12 years ago
|
||
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
Flags: in-testsuite+
Target Milestone: --- → mozilla14
Comment 14•12 years ago
|
||
(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•12 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 16•12 years ago
|
||
(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. :-(
You need to log in
before you can comment on or make changes to this bug.
Description
•