Support unprefixed responseType == "json" in XMLHttpRequest

RESOLVED FIXED in Firefox 10

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: hsivonen, Assigned: hsivonen)

Tracking

({dev-doc-complete})

Trunk
mozilla11
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox10 fixed)

Details

(Whiteboard: [qa-], URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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)

Updated

6 years ago
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 years ago
Created attachment 579667 [details] [diff] [review]
WIP: code, no tests yet
(Assignee)

Comment 3

6 years ago
Created attachment 579694 [details] [diff] [review]
Replace moz-json with spec-compliant json
Attachment #579667 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: dev-doc-needed
(Assignee)

Updated

6 years ago
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.

Updated

6 years ago
Attachment #579694 - Flags: review?(bugs) → review+
Duplicate of this bug: 708522
(Assignee)

Comment 7

6 years ago
(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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
(Assignee)

Comment 11

6 years ago
Created attachment 580410 [details] [diff] [review]
Backport to string-frozen branches without the console message

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

Updated

6 years ago
Attachment #580410 - Flags: review?(bugs) → review+
(Assignee)

Comment 12

6 years ago
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?

Updated

6 years ago
Attachment #580410 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 13

6 years ago
Landed for Aurora / Firefox 10:
https://hg.mozilla.org/releases/mozilla-aurora/rev/5487edfae2de
status-firefox10: --- → fixed
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-]
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.