649133 - Implement XHR2 responseType/response attributes

Status: RESOLVED FIXED

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

Description

[Masatoshi Kimura [:emk]]

Attachment: patch

mozResponseArrayBuffer is based on the older XHR2 spec. It isn't interoperable with latest spec and WebKit.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I don't understand what the goal of setting SetCacheAsFile _after_ the load is done is.  I'm also not sure why we think it's ok to cache as file for no-store responses and the like.  Or does HTTP ignore the SetCacheAsFile bit in those cases?

The code for text should probably not copy the text; it can be shared instead.  This would work automatically if using XPConnect's argument conversions or those in quickstubs instead of rolling your own.

It's not clear to me why JS_GetGlobalObject(cx) is the right scope for wrapping; the caller's scope would seem like a better idea.

Comment 2

[Masatoshi Kimura [:emk]]

(In reply to comment #1)
> I don't understand what the goal of setting SetCacheAsFile _after_ the load is
> done is.
I just attempted to map HEADERS_RECEIVED state to XML_HTTP_REQUEST_* flags.
You're right it it meeningless to call SetCacheAsFile outside OnStartRequest
per the idl description.

> I'm also not sure why we think it's ok to cache as file for no-store
> responses and the like.  Or does HTTP ignore the SetCacheAsFile bit in those
> cases?
I didn't consider about that, but fortunately SetCacheAsFile checked whether no-store directive is present before creating cached data.
Then, what should be done when no-store response was too large to store the memory?

> The code for text should probably not copy the text; it can be shared instead. 
> This would work automatically if using XPConnect's argument conversions or
> those in quickstubs instead of rolling your own.
I have to share the single "response" attribute for various data types. Is it possible to use XPConnect or quickstubs for scriptable attributes to return ArrayBuffer?

> It's not clear to me why JS_GetGlobalObject(cx) is the right scope for
> wrapping; the caller's scope would seem like a better idea.
How can I obtain the caller's scope? JS_GetScopeChain(cx)?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I just attempted to map HEADERS_RECEIVED state to XML_HTTP_REQUEST_* flags.

Right.  It's the check for REQUEST_LOADED in SetResponseType that I'm confused about...  Should that be checking for states _before_ "loaded"?

> but fortunately SetCacheAsFile checked whether no-store directive is present
> before creating cached data

Excellent.

> Then, what should be done when no-store response was too large to store the
> memory?

We should fail somehow (report an error, etc).

> Is it possible to use XPConnect or quickstubs for scriptable attributes to
> return ArrayBuffer?

It's certainly quite possible to do this in a custom quickstub in general.  I'm not sure whether quickstubs suppport "jsval" arguments, though...  In any case, we should be sharing the text, not copying it.

> How can I obtain the caller's scope? JS_GetScopeChain(cx)?

I believe so, yes.  There's also the question of whether GetCurrentJSContext() is the right thing to do, I guess.

Comment 4

[Masatoshi Kimura [:emk]]

(In reply to comment #3)
> > I just attempted to map HEADERS_RECEIVED state to XML_HTTP_REQUEST_* flags.
> Right.  It's the check for REQUEST_LOADED in SetResponseType that I'm confused
> about...  Should that be checking for states _before_ "loaded"?
Hm, there are some discrepancies about readyState names between XHR2 spec and our source.
# XHR2 name        Our internal name
0 UNSENT           UNINITIALIZED
1 OPENED           LOADING
2 HEADERS_RECEIVED LOADED
3 LOADING          INTERACTIVE
4 DONE             COMPLETED
I believe we should match the state using the numeric value rather than the name.
For example, we change the state to LOADED in OnStartRequest (all headers are received, but data may not be available yet). and change the state to INTERACTIVE in OnDataAvailable (that is, body byte has arrived). So I think our LOADED and INTERACTIVE corresponds to XHR2 HEADERS_RECEIVED and LOADING, respectively.
Per spec, we can change the responseType if we are at (not before) the HEADERS_RECEIVED state.
Please correct if I am misunderstanding.

Comment 5

[Boris Zbarsky [:bzbarsky]]

The spec says:

  If the state is not OPENED or HEADERS_RECEIVED raise an INVALID_STATE_ERR
  exception and terminate these steps. 

(as of today's editor's draft).  We seem to be only testing for HEADERS_RECEIVED.

Comment 6

[Masatoshi Kimura [:emk]]

> We seem to be only testing for HEADERS_RECEIVED.
Initially I always called SetCacheAsFile from SetResponseType, but it had no effect because nsHttpChannel::mCacheEntry is not yet initialized at this point. So I had to move SetCacheAsFile call into OnStartRequest. If the state was OPENED when SetResponseType was called, SetCacheAsFile will be called at the time of OnStartRequest.
However, if the state is HEADERS_RECEIVED, it will never be called because OnStartRequest is already dispatched! So I had to leave HEADERS_RECEIVED case inside SetResponseType.
Does this explanation tell you anything?

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: patch v2

Changes:
* Added a comment explaining why OPENED state is not checked in SetResponseType.
* Updated readyState names and type to match the spec.
* Used a custom quickstub for "response" attribute.
* Added an OOM check (nsCString::Append is fallible).
* Discard mResponseBody when cache file is available.

Comment 8

[Boris Zbarsky [:bzbarsky]]

You should probably stick with sicking reviewing this... I don't know when I'll get to a point where I can do a really thorough review here.

You probably also want to include your new file in the diff....

Comment 9

[Masatoshi Kimura [:emk]]

Attachment: patch v2

Oops, forgotten to hg add... Thanks for pointing out.

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: updated to tip

Comment 11

[Brendan Eich [:brendan]]

Is this going in for 6?

/be

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 528428 [details] [diff] [review]
updated to tip

Review of attachment 528428 [details] [diff] [review]:

First off: This is AWESOME! Super happy that someone is picking up implementing these properties! And the implementation is really great! It does need fixes, but the general approach is spot on!

Is there a reason you couldn't simply make the quickstubs always call GetResponse and put the implementation in there? I think you'll end up with getting the string stuff shared even if the implementation lives in the class itself. The current solution is pretty ugly so it would be nice to do something closer to your initial patch, though with the string sharing fixed.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +665,5 @@
   NS_ENSURE_ARG_POINTER(aResponseXML);
   *aResponseXML = nsnull;
+  if (mResponseType == XML_HTTP_RESPONSE_TYPE_BLOB) {
+    return NS_ERROR_DOM_INVALID_STATE_ERR;
+  }

Shouldn't this test be

if (mResponseType != XML_HTTP_RESPONSE_TYPE_DOCUMENT &&
    mResponseType != XML_HTTP_RESPONSE_TYPE_DEFAULT) {
  return ...;
}

Same in GetResponseText

@@ +843,5 @@
   JSContext *cx = nsContentUtils::GetCurrentJSContext();
   if (!cx)
     return NS_ERROR_FAILURE;
 
+  if (mResponseType == XML_HTTP_RESPONSE_TYPE_BLOB) {

This should be

if (mResponseType != XML_HTTP_RESPONSE_TYPE_DEFAULT) {

Unfortunately that means that you would need to make your new caller to this function call into an internal version of this function which doesn't have this check.

Alternatively, you could remove mozResponseArrayBuffer from the idl interface entirely. People should use the .response/.responseType instead. That way you can make this function internal to the implementation and call it as you do now. Possibly with a more convenient name :)

@@ +921,5 @@
+  // If the given value is not the empty string, "arraybuffer",
+  // "blob", "document", or "text" terminate these steps.
+
+  // If the state is OPENED, SetCacheAsFile is no effect here
+  // because the channel doesn't initialize the cache entry yet.

"If the state is OPENED, SetCacheAsFile _would have_ no effect here
because the channel _hasn't initialized_ the cache entry yet."

@@ +1608,5 @@
+    nsCOMPtr<nsIFileChannel> fc = do_QueryInterface(request);
+    if (fc) {
+      fc->GetFile(getter_AddRefs(file));
+    }
+  }

If |file| still is null here, should we create our own file and start streaming to it? I don't know if necko ever refuses to cache a file for example.

@@ +1725,2 @@
   // Set up responseXML
+  PRBool parseBody = mResponseType != XML_HTTP_RESPONSE_TYPE_BLOB;

Again, shouldn't this be

PRBool parseBody =
  mResponseType == XML_HTTP_RESPONSE_TYPE_DOCUMENT ||
  mResponseType == XML_HTTP_RESPONSE_TYPE_DEFAULT?

?

@@ +1897,5 @@
+      mChannel->GetContentType(contentType);
+      mResponseBlob =
+        new nsDOMMemoryFile(mResponseBody.BeginWriting(),
+                            mResponseBody.Length(), EmptyString(),
+                            NS_ConvertASCIItoUTF16(contentType));

I don't think this works. nsDOMMemoryFile assumes that it owns the buffer passed to it, but the buffer passed here is still owned by the string. So as soon as the string goes away nsDOMMemoryFile will have a dangling pointer.

Unfortunately I think the best solution for now is to simply copy the data and give nsDOMMemoryFile a pointer to the new copy.

Ideally we should change mResponseBody to be a raw malloc'ed buffer, but that'd be better to do in a separate patch.

@@ +1898,5 @@
+      mResponseBlob =
+        new nsDOMMemoryFile(mResponseBody.BeginWriting(),
+                            mResponseBody.Length(), EmptyString(),
+                            NS_ConvertASCIItoUTF16(contentType));
+      mResponseBodyUnicode.SetIsVoid(PR_TRUE);

Why is the SetIsVoid call needed here? If mResponseType == XML_HTTP_RESPONSE_TYPE_BLOB we should never use mResponseBodyUnicode right?

::: content/base/test/test_XHR.html
@@ +103,5 @@
+is(ab.byteLength, "hello pass\n".length, "wrong arraybuffer byteLength");
+
+u8v = new Uint8Array(ab);
+ok(String.fromCharCode([u8v[0], u8v[1], u8v[2], u8v[3], u8v[4]]), "hello", "wrong values");
+

Write a separate function which checks if an ArrayBuffer contains the same data as a string so that you check the whole string.

I'm sure such a function would be used elsewhere too.

@@ +119,5 @@
+i32v = new Int32Array(ab);
+u32v = new Uint32Array(ab, 8);
+ok(u8v[0] == 0xaa && u8v[1] == 0xee && u8v[2] == 0x00 && u8v[3] == 0x03, "wrong initial 4 bytes");
+is(i32v[1], -1, "wrong value, expected -1 (0xffffffff)");
+is(u32v[0], 0xbbbbbbbb, "wrong value, expected 0xbbbbbbbb");

For example here :)

You can create a binary string using "\xaa\xee\x00" syntax. I don't think you need to create multiple views and make sure that they give a coherent result. The tests for views should cover that in general.

@@ +138,5 @@
+fr.onload = function() {
+  var v = fr.result;
+  ok(String.fromCharCode([v[0], v[1], v[2], v[3], v[4]]), "hello", "wrong values");
+};
+fr.readAsBinaryString(b);

You need to ensure that the onload callback here happens before the test finishes. Right now these tests are probably not run at all.

To do this add a SimpleTest.waitForExplicitFinish() call in the beginning of the test, and then call SimpleTest.finish() once all the onload handlers have fired. Probably using some sort of counter for how many results are pending.

Additionally, it'd be nice to have a factored out function which can test if a blob contains a expected value (though obviously such a function can't deliver a result synchronously).

@@ +156,5 @@
+  var v = fr.result;
+  ok(v.charCodeAt(0) == 0xaa && v.charCodeAt(1) == 0xee &&
+     v.charCodeAt(2) == 0x00 && v.charCodeAt(3) == 0x03, "wrong initial 4 bytes");
+};
+fr.readAsBinaryString(b);

You're missing tests for xhr.responseType="document" and responseType="text". Specifically test that those return the expected value for .result.

And for all values of .responseType you need to test that accessing .responseText and .responseXML throws when appropriate.

You should also test that holding the blob alive past the XHR objects lifetime and then reading from it still works. You can ensure that the XHR object dies by releasing all references to it and then calling SpecialPowers.gc();

::: js/src/xpconnect/src/dom_quickstubs.qsconf
@@ +510,5 @@
 
     'nsIDOMTouchPoint': 'nsIDOMTouchEvent',
     'nsIDOMTouchList': 'nsIDOMTouchEvent',
+
+    'nsIDOMBlob': 'nsIDOMFile',

Why this change?

Comment 13

[Masatoshi Kimura [:emk]]

> Is there a reason you couldn't simply make the quickstubs always call
> GetResponse and put the implementation in there?
xpc_qsStringToJsval isn't callable from the outside. xpc_qsStringToJsval uses XPCStringConvert::ReadableToJSVal which is also js internal function.
To replicate a ReadableToJSVal function, I have to add a string finalizer using JS_AddExternalStringFinalizer, call JS_NewExternalString with the finalizer index, and remove the finalizer using JS_RemoveExternalStringFinalizer on exit. So I gave up reinventing the wheel to implement a non-quickstubbed version of GetResponse function. Is there a better way to share the string outside the quickstub?
I referred CanvasRenderingContext2D::strokeStyle/fillStyle to write a code handling variable return types.

> +
> +    'nsIDOMBlob': 'nsIDOMFile',
> 
> Why this change?
Because nsIDOMBlob doesn't live in a separate idl file. I got a compile error without this change.

I'll fix other points soon.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why can't you just #include "xpcprivate.h" and call XPCStringConvert::ReadableToJSVal? You might have to modify
content/base/src/Makefile.in so that it includes that directory, but that should be fine.

(As a side note, the interface for XPCStringConvert::ReadableToJSVal is kind'a crappy, but that's for a separate bug).

Comment 15

[Masatoshi Kimura [:emk]]

Oh, I misunderstood that "xpcprivate.h" was not callable outside the js.

(In reply to comment #12)
> If |file| still is null here, should we create our own file and start streaming
> to it? I don't know if necko ever refuses to cache a file for example.
* Necko will not create a disk cache if the response have a "no-store" directive. Of course we should not create our own file in this case.
* Cache entries may not be stored in separate files unless cache data is large enough. It is not optimal to create separate files for small cache data.
Although there may be other unusual cases of the cache file being not created, I don't think it will compensate the cost of implementing our own cache management. The nsDOMMemoryFile fallback already exists.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah. We should look into creating a separate mechanism creating temporary files which are automatically deleted once the last reference to their referring Blob(s) go away. But until we have that I think the current patch is fine.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, and I realized that you should test that setting .responseType only works when it's supposed to, and throws otherwise. Also test that setting it to "blob" works both before readystate is HEADERS_RECEIVED and during the onreadystate eventhandler (i.e. check that both places where you call SetCacheAsFile "works")


On a separate note, I think it would be really cool to add experimental support for JSON. Basically allow .responseType be set to "moz-json" which would make .response be the result of JSON parsing the response. This is extra nice since our JSON parser supports streaming IIRC.

If you want to do that, please do it as a separate patch, or a separate bug. Up to you.

Comment 18

[Jeff Walden [:Waldo]]

Streaming JSON is going away, so you'd have to buffer up the JSON data as it was received, to then pass it off for a single JS_ParseJSON call.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sorry, one more comment. When .responseType is set to "document" you don't need to store the received data. Just forward to the parser without any additional action.

Comment 20

[Masatoshi Kimura [:emk]]

Attachment: patch v3

Comment 21

[Masatoshi Kimura [:emk]]

Filed depending bugs for comment #16 & comment #17.

Comment 22

[Masatoshi Kimura [:emk]]

Attachment: patch v3

Sorry, the previous patch didn't build.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 531052 [details] [diff] [review]
patch v3

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

Minusing for the quickstubs stuff, but this is looking great otherwise!

::: content/base/src/CustomQS_XMLHttp.h
@@ +39,5 @@
> +
> +#include "nsIXMLHttpRequest.h"
> +
> +static JSBool
> +nsIXMLHttpRequest_GetResponse(JSContext *cx, JSObject *obj, jsid id, jsval *vp)

So you're basically adding this because of bug 604198, right? I don't think it's worth the extra complexity just to get this function faster until that bug is fixed. This function won't be on any hot codepaths for a while, and before that happens we should just fix bug 604198.

It doesn't even look like you have to exclude this function in dom_quickstubs.qsconf since mozResponseArrayBuffer isn't there, which means that as soon as bug 604198 this will automatically get faster.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +935,5 @@
> +static
> +nsresult ConvertNativeToJSVal(JSContext *cx,
> +                              nsISupports *aCOMObj,
> +                              const nsIID& aIID,
> +                              jsval *aResult)

Make this function call nsContentUtils::WrapNative. You can use the second version which doesn't require an IID:

http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsContentUtils.h#1666

Pass null for aHolder and PR_TRUE for aAllowWrapping.

(I'll leave it up to you if it's worth keeping this function around after that).

@@ +963,5 @@
> +/* readonly attribute jsval response; */
> +NS_IMETHODIMP nsXMLHttpRequest::GetResponse(jsval *aResult)
> +{
> +  nsresult rv = NS_OK;
> +  JSContext *cx = nsContentUtils::GetCurrentJSContext();

Instead of doing this, annotate the function with [implicit_jscontext] in the idl. That way xpconnect will pass the context to you.

@@ +1556,5 @@
>      return NS_ERROR_FAILURE;
>    }
>  
> +  if (xmlHttpRequest->mResponseType == XML_HTTP_RESPONSE_TYPE_BLOB) {
> +    if (xmlHttpRequest->mResponseBlob) {

Combine into a single if-statement

::: content/base/test/test_XHR.html
@@ +65,5 @@
>  }
>  
> +// test response (responseType='document')
> +function checkResponseTextAccess(xhr) {
> +  didthrow = false;

use 'var didthrow = false' just to make sure you're not colliding with a global variable. Same in checkResponseXMLAccess.

Nit: Could you name the function checkResponseTextAccessThrows or some such to make it more clear what it tests? Same for the following two functions.

@@ +75,5 @@
> +  try { xhr.responseXML } catch (e) { didthrow = true; }
> +  ok(didthrow, "should have thrown when accessing responseXML");
> +}
> +function checkSetResponseType(xhr) {
> +  try { xhr.responseType = 'document'; } catch (e) { didthrow = true; }

This is missing initializing didthrow entirely.

@@ +147,5 @@
> +is(xhr.status, 200, "wrong status");
> +checkResponseTextAccess(xhr);
> +checkResponseXMLAccess(xhr);
> +b = xhr.response;
> +ok(b != null, "should have a non-null blob");

Nit: you can just do: ok(b, ...);

@@ +172,5 @@
> +    is(b.size, 12, "wrong blob size");
> +
> +    fr = new FileReader();
> +    fr.onload = function() {
> +      ok(fr.result, "\xaa\xee\0\x03\xff\xff\xff\xff\xbb\xbb\xbb\xbb", "wrong values");

This looks wrong, should this be 'is' instead of 'ok'?

Comment 24

[Masatoshi Kimura [:emk]]

Attachment: patch v4

Resolved review comments.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 531347 [details] [diff] [review]
patch v4

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

r=me with the one test fix below! Let me know if you want me to land and I can just fix it. Or attach a new patch.

::: content/base/test/test_XHR.html
@@ +106,5 @@
> +function arraybuffer_equals_to(ab, s) {
> +  is(ab.byteLength, s.length, "wrong arraybuffer byteLength");
> +
> +  u8v = new Uint8Array(ab);
> +  ok(String.fromCharCode(u8v), s, "wrong values");

This should be 'is' right? Sorry, missed this last round. Will the .fromCharCode call really work? If it doesn't once you switch to 'is', it might work if you do

is(String.fromCharCode.apply(String, u8v), s, "wrong values");

Comment 26

[Masatoshi Kimura [:emk]]

Attachment: patch v4.1: for check in

(In reply to comment #25)
> r=me with the one test fix below! Let me know if you want me to land and I
> can just fix it. Or attach a new patch.
Thank you. Please land the patch.
> is(String.fromCharCode.apply(String, u8v), s, "wrong values");
works.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Arg, forgot one more comment. We really should prefix these new properties. I.e. call them 'mozResponseType' and 'mozResponse'. Let me know if this isn't ok with you, otherwise I'll check in an additional patch on top of yours that renames them.

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in! Thanks for all the work, very excited to have this in firefox 6!

I went ahead and did the prefixing before landing. Do let me know if you didn't want that. Feel bad that I didn't catch that earlier to give you a chance to have an opinion on it.

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

http://hg.mozilla.org/mozilla-central/rev/618cad1b1743

Leaving the honors of marking this FIXED to you.

Comment 30

[Masatoshi Kimura [:emk]]

I didn't consider prefixing because WebKit didn't, but I don't mind about that so much. Indeed the spec may change again.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

That is not a good state to be; Webkit not prefixing and Gecko prefixing.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Masatoshi, Jonas, how stable is this new feature, I mean spec-vise?

Comment 33

[Masatoshi Kimura [:emk]]

As of 8 August 2010, the property name was responseBody.
As of 27 August 2010, responseBlob/asBlob properties were added.
As of 18 September 2010, responseBody was renamed to responseArrayBuffer.
As of 1 December 2010, responseBlob and responseArrayBuffer were merged to response, and asBlob was renamed to responseType.
After that, property names have not been changed until now.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Yeah, I think we should just unprefix here.  People are writing demos using .response and thinking that it's fine to do that, since it's not prefixed in webkit.

So I think we should unprefix and post to the list explaining that we're unprefixing and why (because Webkit has already poisoned the well, so it sort of doesn't matter now).

Comment 35

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That works for me. If someone posts an unprefixing patch, i'll mail the list.

Comment 36

[Masatoshi Kimura [:emk]]

Attachment: unprefixing patch

Comment 37

[Masatoshi Kimura [:emk]]

Attachment: unprefixing patch

Sorry, a wrong interface was bumped.

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 532959 [details] [diff] [review]
unprefixing patch

You don't need to bump the iid when unprefixing since the change is binary compatible. But it also doesn't hurt so r=me either way.

Comment 39

[Boris Zbarsky [:bzbarsky]]

For what it's worth, resolved checkin-needed bugs don't show up in the usual checkin-needed searches.  :(

I pushed http://hg.mozilla.org/projects/cedar/rev/099889dac1e9

Is there a followup bug to make this work in web workers, by the way?

Comment 40

[Masatoshi Kimura [:emk]]

(In reply to comment #39)
> For what it's worth, resolved checkin-needed bugs don't show up in the usual
> checkin-needed searches.  :(
>
> I pushed http://hg.mozilla.org/projects/cedar/rev/099889dac1e9
Thank you, I should have reopened the bug until the followup patch has been landed.

> Is there a followup bug to make this work in web workers, by the way?
Filed bug 658178.

Comment 41

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/099889dac1e9

Comment 42

[Masatoshi Kimura [:emk]]

Documented:
https://developer.mozilla.org/en/XMLHttpRequest
https://developer.mozilla.org/en/Using_XMLHttpRequest
https://developer.mozilla.org/en/Firefox_6_for_developers

Comment 43

[Boris Zbarsky [:bzbarsky]]

Jonas, you're still going to do that post, right?

Comment 44

[Johnny Stenback (:jst)]

Tracking for Firefox 6, this is a new feature, and we took the followup fix for bug 658683 for Aurora 6.

Comment 45

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We probably need to fix both Bug 661582 and Bug 661583 before we ship this.

Comment 46

[Asa Dotzler [:asa]]

Is there any work left to do that we know needs to happen to make this feature shippable in Firefox 6?

Comment 47

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #46)
> Is there any work left to do that we know needs to happen to make this
> feature shippable in Firefox 6?

Yes, we should take the patch in Bug 669049.

Comment 48

[Johnny Stenback (:jst)]

Bug 669049 landed, so we're good for 6 and 7 here.

Comment 49

[Andreas Gal :gal]

Looks like the typed array response type doesn't support getting partial data during onprogress events. That really blows. Also, whats the progress with standardization here?

Comment 50

[Andreas Gal :gal]

(happy to make a patch if people agree that the feature in #49 is useful, since responseText does support that)

Comment 51

[Boris Zbarsky [:bzbarsky]]

The support for that in responseText causes major performance problems if someone actually makes use of the support in question.  I believe this was explicitly avoided when defining this API.

As far as standardization, http://dev.w3.org/2006/webapi/XMLHttpRequest-2/#the-response-attribute (which, note, says that there is no support for partial data, esp. because such support makes no sense for some of the response types).

Comment 52

[Andreas Gal :gal]

Thanks bz, I was looking at the wrong version of the doc.

For the byte array response type this should be reasonably performant. We allocate an array buffer when the download starts, and every time you get the property you get an updated view (zero copy, just an allocation).

What do you think?

Comment 53

[Boris Zbarsky [:bzbarsky]]

> We allocate an array buffer when the download starts

You don't know the size of the data when the download starts, and array buffers are not resizable, right?

Comment 54

[Boris Zbarsky [:bzbarsky]]

I guess we could speculate based on the (unreliable, in general) Content-Length header.  There would still be weirdness wherein the API would allocate a new view in some cases but not others, but maybe that's ok...

Comment 55

[Andreas Gal :gal]

Right. Thats a tad wonky. I just remembered that without r/o array buffers we have to copy here every time anyway otherwise the caller can muck with the data and the next call gets the modified data via the shared buffer. Clown shoes.

How about a new API.

getPartialResult(offset, length)

The return value depends on the selected result type.

Comment 56

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Please file a new bug or start a newsgroup thread regarding any additional API.

For what it's worth, we do *not* create a new arraybuffer every time .response is accessed. Instead the same mutable arraybuffer is returned every time. Yes, that means that the page can shoot itself in the foot if it modifies the result while there are several consumers of the result, some of which did not expect the buffer to mutate.

That's been the case ever since XMLHttpRequest was invented in that .responseXML has always been mutable. I've never heard of any problems caused by this though, so I'm not too worried about it.

Comment 57

[Andreas Gal :gal]

Mhm, not creating a new arraybuffer seems dangerous to me, but I agree that you do have to go out of your way to trip over it.

Where should I post about this?

Comment 58

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ideally on the public-webapps list at W3C which is where all discussions regarding the XHR standard is happening. The WHATWG list is also followed by a lot of the stakeholders.

If you're not on either of those lists, a bug is probably the best choice, but a post to dev-platform works too.

Comment 59

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux x86_64; rv:6.0) Gecko/20100101 Firefox/6.0

Could you please provide a test case in order to have this issue verified?