:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Description • 12 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 12 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 3 • 12 years ago

> 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?

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 12 years ago

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.  ;)

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 5 • 12 years ago

(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

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 6 • 12 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 12 years ago

> 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

	Matt Brubeck (:mbrubeck)
	Comment 8 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/5f4d6e20b6de