Closed Bug 765464 Opened 12 years ago Closed 12 years ago

Throw TypeError for binding exceptions

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

      No description provided.
Looking into this
Assignee: nobody → Ms2ger
Attached patch Part b: Throw some TypeErrors (obsolete) — Splinter Review
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 #636176 - Flags: review?(jwalden+bmo)
Attachment #638663 - Flags: review?(jwalden+bmo)
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+
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.
(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 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 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+
(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.
(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.
Blocks: 774705
Depends on: 776861
No longer depends on: 776861
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.