Closed
Bug 765464
Opened 12 years ago
Closed 12 years ago
Throw TypeError for binding exceptions
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: Ms2ger, Assigned: Ms2ger)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 1 obsolete file)
1.91 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
5.39 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
12.99 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 2•12 years ago
|
||
Yay, C++
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #636177 -
Flags: review?(khuey)
Comment on attachment 636177 [details] [diff] [review] Part b: Throw some TypeErrors Review of attachment 636177 [details] [diff] [review]: ----------------------------------------------------------------- This is good, but I think it can be better. ::: dom/bindings/Errors.msg @@ +19,5 @@ > + * be replaced with a string value when the error is reported. > + */ > + > +MSG_DEF(MSG_INVALID_ENUM_VALUE, 0, "Value is not a valid value of the expected enumeration.") > +MSG_DEF(MSG_MISSING_ARGUMENTS, 0, "Not enough arguments.") I think that these should take some arguments. For MSG_INVALID_ENUM_VALUE we should report the value passed, and the enumeration type name if possible. For MSG_MISSING_ARGUMENTS we should report the (method, interface) pair invoked. So these would say something like: Value 'foo' is not a valid value for enumeration 'Bar'. Not enough arguments to XMLHttpRequest.setRequestHeader.
Attachment #636177 -
Flags: review?(khuey)
Assignee | ||
Updated•12 years ago
|
Attachment #636176 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #638663 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #636177 -
Attachment is obsolete: true
Attachment #638664 -
Flags: review?(khuey)
Comment on attachment 638664 [details] [diff] [review] Part c: Throw some better TypeErrors Review of attachment 638664 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/BindingUtils.h @@ +36,5 @@ > +#undef MSG_DEF > + Err_Limit > +}; > + > +extern bool Why extern? @@ +437,5 @@ > + > + NS_LossyConvertUTF16toASCII deflated(static_cast<const PRUnichar*>(chars), > + length); > + return ThrowErrorMessage(cx, MSG_INVALID_ENUM_VALUE, deflated.get(), type); > +} Write this as two separate template specializations and avoid the if.
Attachment #638664 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 8•12 years ago
|
||
Comment on attachment 638663 [details] [diff] [review] Part b: Add an exnType to JSErrorReport This causes test failures in dom/tests/mochitest/bugs/test_onerror_message.html; see <https://tbpl.mozilla.org/?tree=Try&rev=48d631151de1>. I tried to investigate, but gdb hates me.
Comment 9•12 years ago
|
||
https://mxr.mozilla.org/mozilla-central/source/js/src/jsexn.cpp?rev=13a8fa3afd28#1108 |report.exnType = JSEXN_NONE;| is needed here, I think.
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #9) > https://mxr.mozilla.org/mozilla-central/source/js/src/jsexn. > cpp?rev=13a8fa3afd28#1108 > |report.exnType = JSEXN_NONE;| is needed here, I think. Yep, that fixed it. Thanks!
Comment 11•12 years ago
|
||
Comment on attachment 636176 [details] [diff] [review] Part a: Introduce JS_ReportErrorNumberVA Review of attachment 636176 [details] [diff] [review]: ----------------------------------------------------------------- (The r+ is intentional, notwithstanding my comment below.) ::: js/src/jsapi.h @@ +5402,5 @@ > +#ifdef va_start > +extern JS_PUBLIC_API(void) > +JS_ReportErrorNumberVA(JSContext *cx, JSErrorCallback errorCallback, > + void *userRef, const unsigned errorNumber, va_list ap); > +#endif Ugh I hate our error-reporting APIs so much.
Attachment #636176 -
Flags: review?(jwalden+bmo) → review+
Comment 12•12 years ago
|
||
Comment on attachment 638663 [details] [diff] [review] Part b: Add an exnType to JSErrorReport Review of attachment 638663 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsfriendapi.h @@ +781,5 @@ > /* Implemented in jsexn.cpp. */ > > /* > + * Get an error type name from a JSExnType constant. > + * Retursn NULL for invalid arguments and JSEXN_INTERNALERR ptyo
Attachment #638663 -
Flags: review?(jwalden+bmo) → review+
Comment 13•12 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #4) > So these would say something like: > > Value 'foo' is not a valid value for enumeration 'Bar'. > Not enough arguments to XMLHttpRequest.setRequestHeader. Not to bikeshed too much, but. This should either be XMLHttpRequest.prototype.setRequestHeader, if we think developers understand the JS prototyping mechanism. Or it should be something like "...to the setRequestHeader method on XMLHttpRequest objects, or somesuch. The problem is XMLHttpRequest.setRequestHeader implies a "class-static" method, not a method "on" instances. We should be careful not to muddy the waters here, in case a method exists both class-statically and on instances.
Assignee | ||
Comment 14•12 years ago
|
||
(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #13) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #4) > > So these would say something like: > > > > Value 'foo' is not a valid value for enumeration 'Bar'. > > Not enough arguments to XMLHttpRequest.setRequestHeader. > > Not to bikeshed too much, but. This should either be > XMLHttpRequest.prototype.setRequestHeader, if we think developers understand > the JS prototyping mechanism. Or it should be something like "...to the > setRequestHeader method on XMLHttpRequest objects, or somesuch. The problem > is XMLHttpRequest.setRequestHeader implies a "class-static" method, not a > method "on" instances. We should be careful not to muddy the waters here, > in case a method exists both class-statically and on instances. FWIW, we already use strings like XMLHttpRequest.setRequestHeader in quickstub exceptions.
Assignee | ||
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ba8f96c49185 https://hg.mozilla.org/mozilla-central/rev/811a6217b1b9 https://hg.mozilla.org/mozilla-central/rev/1ed8454e7090
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•