Closed Bug 678432 Opened 14 years ago Closed 14 years ago

Sending arraybuffers using XMLHttpRequest.send

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla9

People

(Reporter: hippopotamus.gong, Assigned: hippopotamus.gong)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 2 obsolete files)

Patch 2.
5.93 KB, patch
sicking
: review+
Details | Diff | Splinter Review
Changed to call JS_GetArrayBufferDataLength() which is an exposed API.
5.38 KB, patch
Details | Diff | Splinter Review
Implement the ability for XMLHttpRequest.send to take in arraybuffer data.
Assignee: nobody → shawn
Attached patch Patch 1. Implemented the feature (obsolete) — — Splinter Review
Attachment #552589 - Flags: review?(jonas)
Comment on attachment 552589 [details] [diff] [review] Patch 1. Implemented the feature Review of attachment 552589 [details] [diff] [review]: ----------------------------------------------------------------- Some drive by comments. I'm wary of only making this work on the main thread though. ::: content/base/public/nsIXMLHttpRequest.idl @@ +262,1 @@ > void send([optional] in nsIVariant body); You need to change the uuid. ::: content/base/src/nsXMLHttpRequest.cpp @@ +2091,5 @@ > + if (NS_SUCCEEDED(rv) && !JSVAL_IS_PRIMITIVE(realVal) && > + JS_WrapValue(aCx, &realVal) && JSVAL_IS_OBJECT(realVal)) { > + > + JSObject* obj = JSVAL_TO_OBJECT(realVal); > + if (js_IsArrayBuffer(obj)) { Can you just return early here? e.g. if (!js_IsArrayBuffer(obj)) return NS_ERROR_UNEXPECTED; @@ +2093,5 @@ > + > + JSObject* obj = JSVAL_TO_OBJECT(realVal); > + if (js_IsArrayBuffer(obj)) { > + PRInt32 abLength = js::ArrayBuffer::getByteLength(obj); > + char* data = static_cast<char*>(NS_Alloc(abLength + 1)); The manual memory management here is more than a little ugly. Why are you using NS_Alloc instead of just new, and can we use an nsAutoPtr instead of a raw ptr? @@ +2100,5 @@ > + } > + char* p = data; > + char* iter = (char*)JS_GetArrayBufferData(obj); > + for (PRInt32 i = 0; i < abLength; i++) { > + if (*iter & 0xFF00) { Why are we picking out this one character in particular? This needs comments if nothing else. @@ +2105,5 @@ > + NS_Free(data); > + return NS_ERROR_DOM_INVALID_CHARACTER_ERR; > + } > + *p++ = static_cast<char>(*iter++); > + } Implementing our own memcpy kinda sucks. @@ +2110,5 @@ > + *p = '\0'; > + > + nsCOMPtr<nsIInputStream> stream; > + nsresult rv = NS_NewByteInputStream(getter_AddRefs(stream), data, > + abLength, NS_ASSIGNMENT_ADOPT); Ok, are you using NS_Alloc so to get data to use the right allocator? @@ +2117,5 @@ > + } > + NS_ENSURE_SUCCESS(rv, rv); > + > + if (stream) { > + *aResult = stream.forget().get(); stream.forget(aResult); ::: content/base/test/Makefile.in @@ +69,5 @@ > # Split files arbitrarily in two groups to not run into too-long command lines > # which break on Windows (see bug 563151) > +_TEST_FILES1 = \ > + handleArrayBuffer.sjs \ > + test_bug5141.html \ As much as I hate them, use tabs to get the spacing right. ::: content/base/test/test_XHR.html @@ +1,5 @@ > <!DOCTYPE HTML> > <html> > <head> > <title>Test for XMLHttpRequest</title> > + <script type="text/javascript" src="handleArrayBuffer.sjs"></script> Why is this here? ::: dom/workers/XMLHttpRequestPrivate.cpp @@ +978,5 @@ > NS_ERROR("This should never fail!"); > } > } > > + nsresult rv = mProxy->mXHR->Send(variant, nsnull); So this won't work in workers?
Roping in bent to think about how to do this in workers.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2) Long time no see Kyle (: > I'm wary of only making this work on the main thread though. Yes the plan is to first carry out the main thread functionality then consider the worker thread implementation. > You need to change the uuid. Good catch! > ::: content/base/src/nsXMLHttpRequest.cpp > @@ +2091,5 @@ > > + if (NS_SUCCEEDED(rv) && !JSVAL_IS_PRIMITIVE(realVal) && > > + JS_WrapValue(aCx, &realVal) && JSVAL_IS_OBJECT(realVal)) { > > + > > + JSObject* obj = JSVAL_TO_OBJECT(realVal); > > + if (js_IsArrayBuffer(obj)) { > > Can you just return early here? > > e.g. > > if (!js_IsArrayBuffer(obj)) > return NS_ERROR_UNEXPECTED; I think everything that does not fit the type checks goes into a plain string? That's why I didn't return. What do you think? > @@ +2100,5 @@ > > + } > > + char* p = data; > > + char* iter = (char*)JS_GetArrayBufferData(obj); > > + for (PRInt32 i = 0; i < abLength; i++) { > > + if (*iter & 0xFF00) { > > Why are we picking out this one character in particular? This needs > comments if nothing else. This one came from SendAsBinary; I think that's a mask to check if we have characters whose encoding is out of the range. > @@ +2093,5 @@ > > + > > + JSObject* obj = JSVAL_TO_OBJECT(realVal); > > + if (js_IsArrayBuffer(obj)) { > > + PRInt32 abLength = js::ArrayBuffer::getByteLength(obj); > > + char* data = static_cast<char*>(NS_Alloc(abLength + 1)); > > The manual memory management here is more than a little ugly. Why are you > using NS_Alloc instead of just new, and can we use an nsAutoPtr instead of a > raw ptr? > > @@ +2105,5 @@ > > + NS_Free(data); > > + return NS_ERROR_DOM_INVALID_CHARACTER_ERR; > > + } > > + *p++ = static_cast<char>(*iter++); > > + } > > Implementing our own memcpy kinda sucks. > > @@ +2110,5 @@ > > + *p = '\0'; > > + > > + nsCOMPtr<nsIInputStream> stream; > > + nsresult rv = NS_NewByteInputStream(getter_AddRefs(stream), data, > > + abLength, NS_ASSIGNMENT_ADOPT); > > Ok, are you using NS_Alloc so to get data to use the right allocator? Jonas said he will comment later on this issue. > > + if (stream) { > > + *aResult = stream.forget().get(); > > stream.forget(aResult); Great. > > + handleArrayBuffer.sjs \ > > + test_bug5141.html \ > As much as I hate them, use tabs to get the spacing right. Right. > ::: content/base/test/test_XHR.html > > + <script type="text/javascript" src="handleArrayBuffer.sjs"></script> > > Why is this here? Because we need to send arraybuffer to this server-side script and check if we sent the correct data. > ::: dom/workers/XMLHttpRequestPrivate.cpp > > + nsresult rv = mProxy->mXHR->Send(variant, nsnull); > > So this won't work in workers? Workers is the next step (: Thanks a lot Kyle!
Comment on attachment 552589 [details] [diff] [review] Patch 1. Implemented the feature Review of attachment 552589 [details] [diff] [review]: ----------------------------------------------------------------- Rather than adding to test_XHR.html, add to test_XHRSendData.html, that is closer to what you want. Grab me on irc and I'll show how to do it. ::: content/base/src/nsXMLHttpRequest.cpp @@ +2082,5 @@ > if (sendable) { > return sendable->GetSendInfo(aResult, aContentType, aCharset); > } > + > + if (aCx) { Actually, I don't think you need to use a context here at all. You don't appear to be using it. Also, put the arraybuffer checks after all the existing checks. That way we have all the tests for d @@ +2088,5 @@ > + jsval realVal; > + nsresult rv = aBody->GetAsJSVal(&realVal); > + > + if (NS_SUCCEEDED(rv) && !JSVAL_IS_PRIMITIVE(realVal) && > + JS_WrapValue(aCx, &realVal) && JSVAL_IS_OBJECT(realVal)) { I don't think you need the JS_WrapValue here. Also, you don't need to check both !JSVAL_IS_PRIMITIVE and JSVAL_IS_OBJECT. They basically check the same thing. Just do the !JSVAL_IS_PRIMITIVE check. I'd also combine this with the js_IsArrayBuffer test as to create less nesting. You can write if (NS_SUCCEEDED(rv) && !JSVAL_IS_PRIMITIVE(realVal) && (obj = JSVAL_TO_OBJECT(realVal)) && js_IsArrayBuffer(obj)) { (And it's good that you're not early-returning here as we want to go through the catch-all code at the end of the function) @@ +2105,5 @@ > + NS_Free(data); > + return NS_ERROR_DOM_INVALID_CHARACTER_ERR; > + } > + *p++ = static_cast<char>(*iter++); > + } No need to do the memory copying yourself at all, just pass NS_ASSIGNMENT_COPY to the NS_NewByteInputStream function and it will take care of copying the data.
Attachment #552589 - Flags: review?(jonas) → review-
Attached patch Patch 2. — — Splinter Review
Attachment #552589 - Attachment is obsolete: true
Attachment #553980 - Flags: review?(jonas)
Comment on attachment 553980 [details] [diff] [review] Patch 2. Review of attachment 553980 [details] [diff] [review]: ----------------------------------------------------------------- r=me with that fixed. ::: content/base/src/nsXMLHttpRequest.cpp @@ +2089,5 @@ > + if (NS_SUCCEEDED(rv) && !JSVAL_IS_PRIMITIVE(realVal) && > + (obj = JSVAL_TO_OBJECT(realVal)) && > + (js_IsArrayBuffer(obj))) { > + > + aContentType.SetIsVoid(true); SetIsVoid takes a PRBool, so change 'true' to 'PR_TRUE'. Also move this down to where you change the charset. @@ +2097,5 @@ > + nsCOMPtr<nsIInputStream> stream; > + nsresult rv = NS_NewByteInputStream(getter_AddRefs(stream), data, > + abLength, NS_ASSIGNMENT_COPY); > + NS_ENSURE_SUCCESS(rv, rv); > + if (stream) { This nullcheck isn't needed. You've already checked that the function succeeded.
Attachment #553980 - Flags: review?(jonas) → review+
Attached patch Patch 3. (obsolete) — — Splinter Review
Fixed the issues mentioned by Jonas
http://hg.mozilla.org/mozilla-central/rev/8e143ea435d9
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
So, does this work off the main thread?
If worker-XHR uses structured clones to get the body argument from the worker thread to the main thread, as has been claimed, then it should. Would be great if someone wrote a test for this and for sending blobs.
I tried to test this on the train but I ran into two separate worker XHR bugs ... I might try this again next week.
Hey Kyle and Jonas! I have been super busy for the first week and probably the following week as well, but will definitely take a look when possible. Have a great weekend!
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: