Add the API for Paris bindings to nsXMLHttpRequest

RESOLVED FIXED in mozilla14

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: peterv)

Tracking

(Blocks: 1 bug)

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
Depends on: 738625
(Assignee)

Comment 1

5 years ago
Created attachment 610018 [details] [diff] [review]
v1
Assignee: nobody → peterv
Attachment #602312 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Attachment #610018 - Flags: review?(bzbarsky)
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.
Attachment #610018 - Flags: review?(bzbarsky) → review+
> 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?
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.  ;)
(Reporter)

Comment 5

5 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
(Reporter)

Comment 6

5 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
> 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
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/5f4d6e20b6de
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 741163
Depends on: 814050
You need to log in before you can comment on or make changes to this bug.