Last Comment Bug 749485 - Switch Paris bindings to using a struct for the result of fallible methods instead of a raw nsresult
: Switch Paris bindings to using a struct for the result of fallible methods in...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 745897
Blocks: ParisBindings
  Show dependency treegraph
 
Reported: 2012-04-26 19:23 PDT by Boris Zbarsky [:bz]
Modified: 2012-05-06 20:26 PDT (History)
7 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (99.80 KB, patch)
2012-04-26 19:25 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
With the ErrorInfo.h file too (101.25 KB, patch)
2012-04-26 20:06 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Renamed to ErrorResult and with most of Ms2ger's nits fixed (102.12 KB, patch)
2012-04-30 08:24 PDT, Boris Zbarsky [:bz]
peterv: review+
Details | Diff | Splinter Review
Updated to Peter's comments (101.62 KB, patch)
2012-05-04 10:19 PDT, Boris Zbarsky [:bz]
bent.mozilla: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2012-04-26 19:23:21 PDT
Patch coming up.  Longer-term, we can use this to pass out exception strings or whatnot, not just nsresults!

This applies on top of the patch in bug 745897.
Comment 1 Boris Zbarsky [:bz] 2012-04-26 19:25:02 PDT
Created attachment 618901 [details] [diff] [review]
Patch

Ben, could you review the worker stuff?  Peter, if you want to do the rest?  Or delegate it to Ben if desired?  ;)
Comment 2 Boris Zbarsky [:bz] 2012-04-26 20:06:13 PDT
Created attachment 618907 [details] [diff] [review]
With the ErrorInfo.h file too
Comment 3 Boris Zbarsky [:bz] 2012-04-26 22:19:53 PDT
roc suggests s/ErrorInfo/ErrorResult/, by the way.

For comparison, WebKit uses ExceptionCode, but they just have an integer there.
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2012-04-27 02:09:16 PDT
Comment on attachment 618907 [details] [diff] [review]
With the ErrorInfo.h file too

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

::: content/base/src/nsXMLHttpRequest.cpp
@@ +2522,4 @@
>  {
>    char *data = static_cast<char*>(NS_Alloc(aBody.Length() + 1));
>    if (!data) {
> +    return aRv.Throw(NS_ERROR_OUT_OF_MEMORY);

NS_Alloc is infallible...

::: content/canvas/src/WebGLContext.cpp
@@ +790,3 @@
>      JSObject* obj = GetContextAttributes(rv);
> +    if (rv.Failed())
> +        return rv.ErrorCode();

{}

::: content/canvas/src/WebGLContextGL.cpp
@@ +2743,4 @@
>      JS::Value v =
>          GetFramebufferAttachmentParameter(cx, target, attachment, pname, rv);
> +    if (rv.Failed())
> +        return rv.ErrorCode();

{}

@@ +3413,3 @@
>      JS::Value v = GetVertexAttrib(cx, index, pname, rv);
> +    if (rv.Failed())
> +        return rv.ErrorCode();

{}

::: dom/bindings/ErrorInfo.h
@@ +10,5 @@
> +
> +#ifndef mozilla_dom_ErrorInfo_h
> +#define mozilla_dom_ErrorInfo_h
> +
> +class ErrorInfo {

This should be in a namespace

@@ +17,5 @@
> +    mResult = NS_OK;
> +  }
> +
> +  void Throw(nsresult rv) {
> +    mResult = rv;

Assert NS_FAILED?

@@ +40,5 @@
> +    return mResult;
> +  }
> +
> +private:
> +  nsresult mResult;

You need to include nscore.h.
Comment 5 Boris Zbarsky [:bz] 2012-04-27 08:41:54 PDT
> NS_Alloc is infallible...

Who the hell did that?  And why did they not change this code in the process?  This MUST be a fallible allocation.

> This should be in a namespace

Er, yes.  Assume it's in mozilla::dom.  Though that _does_ make using it in headers a huge pain. :(

> Assert NS_FAILED?

I was considering that, yes.  Wasn't sure whether it was desirable in practice...

> You need to include nscore.h.

Yes.  Will do.
Comment 6 Josh Matthews [:jdm] (away until 9/3) 2012-04-27 12:49:19 PDT
(In reply to Boris Zbarsky (:bz) from comment #5)
> > NS_Alloc is infallible...
> 
> Who the hell did that?  And why did they not change this code in the
> process?  This MUST be a fallible allocation.

Bug 680556.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-27 14:33:15 PDT
(In reply to Boris Zbarsky (:bz) from comment #5)
> Er, yes.  Assume it's in mozilla::dom.  Though that _does_ make using it in
> headers a huge pain. :(

You could put it in the mozilla namespace, and/or add a typedef to bring it into the scope of specific classes.
Comment 8 Boris Zbarsky [:bz] 2012-04-27 16:41:02 PDT
Yeah, maybe I'll put it in mozilla.  Can't stick it int mfbt, but maybe that's ok.
Comment 9 Boris Zbarsky [:bz] 2012-04-30 07:46:37 PDT
Oh, and as far as comment 4 bracing nits, local style in that code is to not brace single-line if bodies.
Comment 10 Boris Zbarsky [:bz] 2012-04-30 08:24:01 PDT
Created attachment 619575 [details] [diff] [review]
Renamed to ErrorResult and with most of Ms2ger's nits fixed
Comment 11 Peter Van der Beken [:peterv] 2012-05-04 06:40:34 PDT
Comment on attachment 619575 [details] [diff] [review]
Renamed to ErrorResult and with most of Ms2ger's nits fixed

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

::: content/base/src/nsXMLHttpRequest.cpp
@@ +977,4 @@
>      nsCOMPtr<nsICharsetConverterManager> ccm =
> +      do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID, &rv);
> +    if (NS_FAILED(rv)) {
> +      return aRv.Throw(rv);

This looks a bit odd. I think I'd prefer:

    aRv.Throw(rv);
    return;

::: dom/workers/XMLHttpRequestEventTarget.h
@@ +28,5 @@
>    _Finalize(JSFreeOp* aFop) MOZ_OVERRIDE;
>  
>  #define IMPL_GETTER_AND_SETTER(_type)                                          \
>    JSObject*                                                                    \
> +  GetOn##_type(ErrorResult& aRv)                                                 \

Please correct the whitespace.

@@ +34,5 @@
>      return GetEventListener(NS_LITERAL_STRING(#_type), aRv);                   \
>    }                                                                            \
>                                                                                 \
>    void                                                                         \
> +  SetOn##_type(JSObject* aListener, ErrorResult& aRv)                            \

And here.
Comment 12 Boris Zbarsky [:bz] 2012-05-04 09:07:38 PDT
> I think I'd prefer

OK.  I'll leave that pattern as-is in the canvas code, since the canvas folks prefer it, undo it in the DOM code.

Thanks for the review!
Comment 13 Boris Zbarsky [:bz] 2012-05-04 10:19:55 PDT
Created attachment 621087 [details] [diff] [review]
Updated to Peter's comments
Comment 14 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-05 13:58:37 PDT
Comment on attachment 621087 [details] [diff] [review]
Updated to Peter's comments

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

I don't understand why we allow the operator= on ErrorResult. Shouldn't we just have Reset() or something to set to NS_OK?

::: dom/workers/EventListenerManager.cpp
@@ +283,5 @@
>  {
>    using namespace mozilla::dom::workers::events;
>  
>    if (!IsSupportedEventClass(aEvent)) {
>      aRv = NS_ERROR_FAILURE;

Er, shouldn't this be Throw() too?

::: dom/workers/EventTarget.cpp
@@ +44,5 @@
>  
>    JSString* type =
>      JS_NewUCStringCopyN(cx, aType.BeginReading(), aType.Length());
>    if (!type || !(type = JS_InternJSString(cx, type))) {
>      aRv = NS_ERROR_OUT_OF_MEMORY;

Throw?

::: dom/workers/XMLHttpRequest.h
@@ +79,5 @@
>    Notify(JSContext* aCx, Status aStatus) MOZ_OVERRIDE;
>  
>  #define IMPL_GETTER_AND_SETTER(_type)                                          \
>    JSObject*                                                                    \
> +  GetOn##_type(ErrorResult& aRv)                                                 \

Can you fix the \ alignment?
Comment 15 Boris Zbarsky [:bz] 2012-05-05 17:41:54 PDT
> I don't understand why we allow the operator= on ErrorResult.

Because there's a bunch of code that was doing:

  aRv = SomethingThatMightReturnError();

and I was trying to keep the size of the patch down.  We would have had to convert each of those to a local rv plus a branch...  I'm fine with doing that in followups; I agree that we should work on getting rid of the operator=.

> Er, shouldn't this be Throw() too?

Yes.  Will fix.  ;)

> Throw?

Yes.  Will fix.

> Can you fix the \ alignment?

Yes.
Comment 17 Phil Ringnalda (:philor) 2012-05-05 20:36:13 PDT
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b7be69a34372 for build bustage.
Comment 18 Boris Zbarsky [:bz] 2012-05-06 05:08:18 PDT
Weird.  It used to build!

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/4ba9cc4ee095 with the extra needed include.
Comment 19 Boris Zbarsky [:bz] 2012-05-06 05:17:59 PDT
Ah, it used to be ordered after another patch that indirectly added the include!
Comment 21 Masatoshi Kimura [:emk] 2012-05-06 20:26:47 PDT
(In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment #6)
> (In reply to Boris Zbarsky (:bz) from comment #5)
> > > NS_Alloc is infallible...
> > Who the hell did that?  And why did they not change this code in the
> > process?  This MUST be a fallible allocation.
> Bug 680556.
And I filed bug 705035.

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