Last Comment Bug 732377 - Add the API for Paris bindings to nsXMLHttpRequest
: Add the API for Paris bindings to nsXMLHttpRequest
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
Depends on: 738625 741163 814050
Blocks: ParisBindings
  Show dependency treegraph
 
Reported: 2012-03-02 04:53 PST by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-11-21 09:29 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (51.37 KB, patch)
2012-03-02 04:53 PST, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
v1 (75.55 KB, patch)
2012-03-27 21:56 PDT, Peter Van der Beken [:peterv]
bzbarsky: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-03-02 04:53:04 PST
Created attachment 602312 [details] [diff] [review]
Patch

Some issues:

* long mozilla::dom::bindings::prototypes::XMLHttpRequestResponseType::value is long
* Need to be able to mark attribute getters infallible if the setter can throw
* void SetMozBackgroundRequest(bool, nsresult&) instead of nsresult SetMozBackgroundRequest(bool)? Meh.
* We'll want bug 687332, I guess.
* Inlining is hard because all the XML_HTTP_REQUEST_* defines are in the .cpp
* Bah, multiple inheritance
Comment 1 Peter Van der Beken [:peterv] 2012-03-27 21:56:25 PDT
Created attachment 610018 [details] [diff] [review]
v1
Comment 2 Boris Zbarsky [:bz] (TPAC) 2012-03-27 23:55:30 PDT
Comment on attachment 610018 [details] [diff] [review]
v1

Should we be using IMPL_STRING_GETTER for GetResponseText?

StaticAssertions() doesn't seem to be asserting anything.  Also, should that call be debug-only?

>+  mResponseType = ResponseType(aResponseType);

Why do you need the cast?

In GetStatus, the unsent/opened/error case should return 0, not NS_OK.

Also, GetStatus is actually infallible, right?  We should mark it so.

In SendAsBinary, no need to null-check the writable variant allocation, right?

GetUpload looks infallible to me.

The manual wrapping for event listeners is a bit annoying.  I wonder whether it's worth adding an IDL annotation that says "convert to this nsIFoo type" somewhere for that...  possibly just on set.  Certainly a followup if so.

r=me with the above nits picked.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-03-28 01:14:13 PDT
> return mozilla::dom::bindings::prototypes::XMLHttpRequestUpload::Wrap(cx, scope, this, triedToWrap);

Per recent discussion in dev.platform, can we reduce the use of namespaces here? How about mozilla::XMLHttpRequestUpload::Wrap at least?
Comment 4 Boris Zbarsky [:bz] (TPAC) 2012-03-28 01:54:07 PDT
I definitely plan to bring that up when we review the patches that would lead to code like that tomorrow.  Luckily, the patch in this bug has nothing like that in it.  ;)
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2012-03-28 03:24:33 PDT
(In reply to Boris Zbarsky (:bz) from comment #2)
> StaticAssertions() doesn't seem to be asserting anything.

It did in attachment 602312 [details] [diff] [review], fwiw
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-03-28 03:31:27 PDT
Comment on attachment 610018 [details] [diff] [review]
v1

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

::: content/base/src/nsXMLHttpRequest.cpp
@@ +783,5 @@
>  NS_IMETHODIMP
>  nsXMLHttpRequest::GetResponseXML(nsIDOMDocument **aResponseXML)
>  {
> +  nsresult rv = NS_OK;
> +  nsIDocument *responseXML = GetResponseXML(rv);

* on the left

@@ +1353,2 @@
>    }
> +  else {

} else {

@@ +2502,3 @@
>    char *data = static_cast<char*>(NS_Alloc(aBody.Length() + 1));
> +  if (!data) {
> +    aRv = NS_ERROR_OUT_OF_MEMORY;

NS_Alloc is infallible

@@ +2550,5 @@
> +  aDoc->GetInputEncoding(inputEncoding);
> +  if (!DOMStringIsNull(inputEncoding)) {
> +    CopyUTF16toUTF8(inputEncoding, aCharset);
> +  }
> +  else {

} else {

@@ +2616,5 @@
> +  aContentType.SetIsVoid(true);
> +  aCharset.Truncate();
> +
> +  PRInt32 length = JS_GetArrayBufferByteLength(aArrayBuffer);
> +  char* data = (char*)JS_GetArrayBufferData(aArrayBuffer);

static_cast

@@ +2717,5 @@
> +  }
> +
> +  const RequestBody& body = *aBody;
> +  RequestBody::Value value = body.GetValue();
> +  nsresult rv;

Declare rv where you need it, please
Comment 7 Boris Zbarsky [:bz] (TPAC) 2012-03-28 15:22:37 PDT
> Should we be using IMPL_STRING_GETTER for GetResponseText?

No, because IMPL_STRING_GETTER only works with infallible getters.

> NS_Alloc is infallible

See bug 705035.

> Certainly a followup if so.

Filed bug 740083.

> static_cast

reinterprest_cast.

Addressed the other nits from comment 6.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f4d6e20b6de
Comment 8 Matt Brubeck (:mbrubeck) 2012-03-29 08:54:32 PDT
https://hg.mozilla.org/mozilla-central/rev/5f4d6e20b6de

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