[Webkit-unassigned] [Bug 49633] Add .responseType and .response to XMLHttpRequest

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49633





--- Comment #17 from Chris Rogers <crogers at google.com>  2010-11-22 15:29:49 PST ---
Alexey, thanks for having a look.  I've posted a new patch addressing most of your comments:

(In reply to comment #14)
> This looks really good to me. Not sure if we want more baking time for discussion, keeping at r? in case we do.
> 
> As for string vs. named type discussion - let's do whatever is required to get other browser vendors on board with the new API, this doesn't seem to be a major issue. I haven't been following e-mail discussion in enough detail to tell who had a very strong opinion on that.
> 

I had brought up the issue of integer enum values vs. string on the list, but the only response I got was to use strings.  Jonas Sicking from Mozilla first used string constants in his example:
http://lists.w3.org/Archives/Public/public-webapps/2010OctDec/0336.html



> +    else if (responseType == "blob") {
> +#if ENABLE(XHR_RESPONSE_BLOB)
> +        m_responseTypeCode = ResponseTypeBlob;
> +#else
> +        ec = SYNTAX_ERR;
> +#endif
> 
> It would be cleaner to just let it fall through to default section.

FIXED


> 
> +    case ResponseTypeBlob:
> +        return "blob";
> 
> I'm somewhat surprised that this is not inside an #if (ditto for enum values in header). This is not a big deal - if this has made other code significantly more readable in your opinion, let's keep it.

I think it's a bit more readable as is.



> 
> -    bool asBlob() const { return m_asBlob; }
> +    bool asBlob() const { return responseTypeCode() == ResponseTypeBlob; }
> 
> Our normal naming scheme is for asXXX() and toXXX() functions to return XXX objects.

I didn't add this method.  It was already there to support the 'asBlob' attribute in the .idl file which is in the current proposed spec.  This patch should make it unnecessary to have 'asBlob' but, for now, I left it in with the
behavior unchanged.  I spoke with Michael Nordman earlier about this, and he thought we could probably just remove it, but he's on vacation right now so maybe it's safer to remove
it in another patch?


> 
> +responseXML serialized
> +<!DOCTYPE doc><doc>
> +  <foo xmlns="foobar">One</foo> <x:bar xmlns:x="barfoo">Two</x:bar>
> +  <d id="id3">Three</d>
> +</doc>
> 
> There is a downside to making this test depend on minute details of XML serialization. E.g., it's likely to catch someone off guard when fixing bug 25206. 

I'm happy to remove this part of the test, but I wanted to cover the case where I check for the case when .responseType is set to "document".  Is there a simpler way you can recommend?


> 
> I'd love to see tests for what happens when accessing .response when the load is only partially done, and in case the request has been aborted.

Tests have been added for accessing .response when .responseState != DONE.  This caught a bug in my previous patch, which I also fixed.  My behavior is to 
require that the response be DONE before an ArrayBuffer .response is available (similar to .responseXML).  I think this will cover the 99% use case, and we can always add the capability
to access it progressively as data comes in later on if it's thought to be important.

I also added a new test for abort behavior.


> 
> > Index: LayoutTests/fast/xmlhttprequest/resources/balls-of-the-orient.aif
> 
> We've got tons of resource files to request, why add another huge one? If you intend to use it for testing audio API later, then the location may not be so good.

I understand.  I just wanted to use a file in the xmlhttprequest directory because I was worried that if I referenced a different file (maybe in the media directory) that someone might
change it for a different test and wouldn't realize that my tests also relied on it.  If you think this is not a concern, then I'll change to reference a file in the media directory.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.