Closed Bug 727530 Opened 8 years ago Closed 8 years ago

XHR for data URIs should support content-type header field

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: julian.reschke, Assigned: emk)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 2 obsolete files)

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.
Masatoshi, would you be interested in working on this?
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.
(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.
Looks like bug 716491 was filed to look into the status/statusText thing.
Attached patch patch (obsolete) — Splinter Review
This patch only support content-type. status/statusText support will be much harder and have their own bug.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #598008 - Flags: review?(jonas)
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
(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.
(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.
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
Attached patch patch v2 (obsolete) — Splinter Review
Changes:
- mState check was lacked somehow.
- Added a charset parameter if present.
- Added more test cases per comment #6, #8, and #9.
Attachment #598008 - Attachment is obsolete: true
Attachment #598197 - Flags: review?(jonas)
Attachment #598008 - Flags: review?(jonas)
(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.
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.
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
(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 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.
Attachment #598197 - Flags: review?(jonas) → review+
Attachment #598197 - Attachment is obsolete: true
Attachment #598477 - Flags: review+
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/560cfb993c9d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.