727530 - XHR for data URIs should support content-type header field

Status: RESOLVED FIXED

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

Description

[Julian Reschke]

Bug 702820 has introduced XHR support for data URIs (RFC 2397). These URIs can carry an internet media type, thus the response should return the content-type header field accordingly.

Comment 1

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

Masatoshi, would you be interested in working on this?

Comment 2

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

First off don't think this is agreed upon spec behavior in the standards community yet. data: URIs don't have headers any more than file: URIs or javascript: URIs.

That said, we probably should do this, both for data: URIs and blob: URIs. We also should expose a better .status/.statusText such that those properties return consistent results independent of protocol.

Comment 3

[Julian Reschke]

(In reply to Jonas Sicking (:sicking) from comment #2)
> First off don't think this is agreed upon spec behavior in the standards
> community yet. data: URIs don't have headers any more than file: URIs or
> javascript: URIs.
> ...

Yes. But we should avoid a case where a spec documents something just because Opera and Firefox do it that way, when Firefox does it that way just because Opera did so, and the Opera implementation obviously is incomplete.

Comment 4

[Pablo Flouret]

Looks like bug 716491 was filed to look into the status/statusText thing.

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: patch

This patch only support content-type. status/statusText support will be much harder and have their own bug.

Comment 6

[Julian Reschke]

Comment on attachment 598008 [details] [diff] [review]
patch

I think you should have a test case where the media type is not specified, such as

  data:,foo

Comment 7

[Masatoshi Kimura [:emk]]

(In reply to Julian Reschke from comment #6)
> I think you should have a test case where the media type is not specified,
> such as
>   data:,foo
What should be returned? Currently we infer the mime type as text/plain in this case.

Comment 8

[Julian Reschke]

(In reply to Masatoshi Kimura [:emk] from comment #7)
> (In reply to Julian Reschke from comment #6)
> > I think you should have a test case where the media type is not specified,
> > such as
> >   data:,foo
> What should be returned? Currently we infer the mime type as text/plain in
> this case.

<http://greenbytes.de/tech/webdav/rfc2397.html#rfc.section.2>, so I'd make it

  text/plain;charset=US-ASCII

Also, it might be good to have a test that an optional ";base64" doesn't leak into the media type value.

Comment 9

[Julian Reschke]

Also, it would probably good to have a test for fragment identifiers in data URIs, such as

  data:,foo#bar -> foo

and

  data:,foo%23bar -> foo#bar

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: patch v2

Changes:
- mState check was lacked somehow.
- Added a charset parameter if present.
- Added more test cases per comment #6, #8, and #9.

Comment 11

[Anne (:annevk)]

(In reply to Julian Reschke from comment #8)
>   text/plain;charset=US-ASCII

Every implementation uses windows-1252. I don't think we should change that.

Comment 12

[Anne (:annevk)]

And actually even that might not be entirely okay. E.g. for "data:,¢" the result from Chrome seems more desirable. Similarly if you lead with a BOM.

Comment 13

[Masatoshi Kimura [:emk]]

I didn't change the implementation. Our implementation complemented US-ASCII even before the patch.
http://hg.mozilla.org/mozilla-central/file/f88a05e00f47/netwerk/protocol/data/nsDataHandler.cpp#l200

Comment 14

[Julian Reschke]

(In reply to Anne van Kesteren from comment #11)
> (In reply to Julian Reschke from comment #8)
> >   text/plain;charset=US-ASCII
> 
> Every implementation uses windows-1252. I don't think we should change that.

What's relevant here is the declaration, not the encoding the UA applied when generating the text property. That's not different from "http" URIs, as far as I can tell.

Comment 15

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

Comment on attachment 598197 [details] [diff] [review]
patch v2

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

r=me with that fixed.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +1338,5 @@
>        *_retval = ToNewCString(visitor->Headers());
> +  } else if (mState & (XML_HTTP_REQUEST_UNSENT |
> +                       XML_HTTP_REQUEST_OPENED | XML_HTTP_REQUEST_SENT)) {
> +    // If the state is UNSENT or OPENED,
> +    // return empty string and terminate these steps.

Do this part as a early-return check up top in the function instead. Empty if-statements are always confusing to read, and technically we don't need to check for http-channels in those states.

@@ +1389,5 @@
> +        NS_SUCCEEDED(mChannel->GetStatus(&status)) && 
> +        NS_SUCCEEDED(status) &&
> +        header.LowerCaseEqualsASCII("content-type")) {
> +      rv = mChannel->GetContentType(_retval);
> +      if (rv != NS_ERROR_NOT_AVAILABLE) {

Check NS_SUCCEEDED(rv) instead here.

@@ +1396,5 @@
> +            !value.IsEmpty()) {
> +          _retval.Append(";charset=");
> +          _retval.Append(value);
> +        }
> +        return rv;

Which means that you can just return NS_OK here (or preferably just use an else around the SetIsVoid call.

Comment 16

[Masatoshi Kimura [:emk]]

Attachment: patch for check in; r=jonas

Comment 17

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=d869f24f99e3

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/560cfb993c9d

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/560cfb993c9d