Julian Reschke Reporter
	Description • 13 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 1 • 13 years ago

Masatoshi, would you be interested in working on this?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 2 • 13 years ago

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.

	Julian Reschke Reporter
	Comment 3 • 13 years ago

(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.

	Pablo Flouret
	Comment 4 • 13 years ago

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

	Masatoshi Kimura [:emk] Assignee
	Comment 5 • 13 years ago

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

	Julian Reschke Reporter
	Comment 6 • 13 years ago

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

	Masatoshi Kimura [:emk] Assignee
	Comment 7 • 13 years ago

(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.

	Julian Reschke Reporter
	Comment 8 • 13 years ago

(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.

	Julian Reschke Reporter
	Comment 9 • 13 years ago

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

	Masatoshi Kimura [:emk] Assignee
	Comment 10 • 13 years ago

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

	Anne (:annevk)
	Comment 11 • 13 years ago

(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.

	Anne (:annevk)
	Comment 12 • 13 years ago

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.

	Masatoshi Kimura [:emk] Assignee
	Comment 13 • 13 years ago

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

	Julian Reschke Reporter
	Comment 14 • 13 years ago

(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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 15 • 13 years ago

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.

	Masatoshi Kimura [:emk] Assignee
	Comment 17 • 13 years ago

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

	Ed Morley [:emorley]
	Comment 18 • 13 years ago

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

	Ed Morley [:emorley]
	Comment 19 • 13 years ago

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