Closed Bug 707142 Opened 13 years ago Closed 13 years ago

Support unprefixed responseType == "json" in XMLHttpRequest

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla11
Tracking Status
firefox10 --- fixed

People

(Reporter: hsivonen, Assigned: hsivonen)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [qa-])

Attachments

(2 files, 1 obsolete file)

The XHR spec now has a "json" responseType. We only have "moz-json".

Should add support for "json".

sicking, "moz-json" isn't even in the release channel yet. Do we need to keep supporting also "moz-json" or can we just replace it with "json"?

(It would be cool to get support for the unprefixed type into Aurora and Beta. That way, the prefixed version would never reach the release channel and cause legacy to be created.)
I don't think we need to support moz-json no.

If you attach a patch, feel free to ask for beta/aurora approval, I don't know how likely it is to get it.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attached patch WIP: code, no tests yet (obsolete) — Splinter Review
Attachment #579667 - Attachment is obsolete: true
Keywords: dev-doc-needed
Attachment #579694 - Flags: review?(bugs)
Comment on attachment 579694 [details] [diff] [review]
Replace moz-json with spec-compliant json

>--- a/content/base/src/nsXMLHttpRequest.cpp
>+++ b/content/base/src/nsXMLHttpRequest.cpp
> nsXMLHttpRequest::CreateResponseParsedJSON(JSContext* aCx)
> {
>   if (!aCx) {
>     return NS_ERROR_FAILURE;
>   }
>-
>+  // The Unicode converter has already zapped the BOM if there was one
>   if (!JS_ParseJSON(aCx,
>                     (jschar*)mResponseText.get(),

Can you make this static_cast<const jschar*> while you're here?
I'm personally not a fan of enforcing utf8-only for JSON. I don't really see the harm in allowing other encodings when explicitly declared through http-header. Consistency with other text-based formats seems better to me.

But I'll leave that up to you guys.
Attachment #579694 - Flags: review?(bugs) → review+
(In reply to Ms2ger from comment #4)
> Comment on attachment 579694 [details] [diff] [review]
> Replace moz-json with spec-compliant json
> 
> >--- a/content/base/src/nsXMLHttpRequest.cpp
> >+++ b/content/base/src/nsXMLHttpRequest.cpp
> > nsXMLHttpRequest::CreateResponseParsedJSON(JSContext* aCx)
> > {
> >   if (!aCx) {
> >     return NS_ERROR_FAILURE;
> >   }
> >-
> >+  // The Unicode converter has already zapped the BOM if there was one
> >   if (!JS_ParseJSON(aCx,
> >                     (jschar*)mResponseText.get(),
> 
> Can you make this static_cast<const jschar*> while you're here?

Sorry, already landed based on seeing smaug's r+ in bugmail:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0f8871174a5

(In reply to Jonas Sicking (:sicking) from comment #5)
> I'm personally not a fan of enforcing utf8-only for JSON. I don't really see
> the harm in allowing other encodings when explicitly declared through
> http-header. Consistency with other text-based formats seems better to me.
> 
> But I'll leave that up to you guys.

This is really a discussion for public-webapps, but forcing authors to use UTF-8 everywhere we can (i.e. when using new features) is good, because that way there's less of a chance of authors lazily creating apps that don't deal with personal names, addresses, etc. outside their immediate imagination. (See the thread Faruk started of the WHATWG list, too.)
(In reply to Jonas Sicking (:sicking) from comment #1)
> I don't think we need to support moz-json no.
> 
> If you attach a patch, feel free to ask for beta/aurora approval, I don't
> know how likely it is to get it.

It would be more practical to create a patch to disable "moz-json" for aurora and beta. We don't have to be in a hurry to accumulate a legacy.
I don't think we'll accumulate the legacy that fast. It seems more valuable to get author feedback as soon as possible.

A simple patch to just remove the "moz-" prefix but leave the throwing and charset as it is seems like it would be safe enough to take on branches.
https://hg.mozilla.org/mozilla-central/rev/b0f8871174a5
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
(In reply to Jonas Sicking (:sicking) from comment #9)
> I don't think we'll accumulate the legacy that fast. It seems more valuable
> to get author feedback as soon as possible.
> 
> A simple patch to just remove the "moz-" prefix but leave the throwing and
> charset as it is seems like it would be safe enough to take on branches.

I think as far as branch landings go, landing a backport of the real fix is as safe as disabling or merely renaming moz-json. It's extremely clear what the lines of this backport patch that wouldn't be part of a mere rename patch do.

This backport patch rebases the real fix to Aurora and removes the console message, since Aurora is string-frozen. (The patch works for Beta, too, with trivial merge conflict resolution.)
Attachment #580410 - Flags: review?(bugs)
Attachment #580410 - Flags: review?(bugs) → review+
Comment on attachment 580410 [details] [diff] [review]
Backport to string-frozen branches without the console message

Requesting approval for Firefox 10. The code changes here are very simple. The sooner we ship this to the release channel, the better chances we have to avoid the creation of Web content that uses the moz-prefixed version.
Attachment #580410 - Flags: approval-mozilla-aurora?
Attachment #580410 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Marking qa- as I don't think this is something QA needs to verify. Please mark qa+ if this is not the case.
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: