Implement well formed JSON.stringify

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
8 months ago

People

(Reporter: yulia, Assigned: Waldo)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

()

Attachments

(1 attachment, 2 obsolete attachments)

Currently in stage 2 in the TC39 process. More information: https://github.com/tc39/proposal-well-formed-stringify
Posted patch Patch (obsolete) — Splinter Review
Driveby procrastination.
Attachment #8998344 - Flags: review?(andrebargull)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Comment on attachment 8998344 [details] [diff] [review]
Patch

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

The patch itself looks good to me, but I think our normal procedure is to wait until a proposal reaches stage 3 before we try to land the changes.

::: js/src/builtin/JSON.cpp
@@ +99,5 @@
> +                uint8_t x = c >> 4;
> +                MOZ_ASSERT(x < 10);
> +                *dstPtr++ = '0' + x;
> +
> +                *dstPtr++ = ToLowerHex(c % 16);

Maybe |c & 0xF| for consistency with the call below?
Attachment #8998344 - Flags: review?(andrebargull) → review+
(In reply to André Bargull [:anba] from comment #2)
> The patch itself looks good to me, but I think our normal procedure is to
> wait until a proposal reaches stage 3 before we try to land the changes.

Fair 'nuff.  Poked upstream a little about doing that, after making a suggestion to write out the change slightly differently.  Semantics-wise, tho, I can't imagine anything about the proposal changing, so it seemed safe enough to churn out -- and IMO even to land, if we cared.

> Maybe |c & 0xF| for consistency with the call below?

Sure, will post the revised patch so it's not just on my local system while we wait for upstream to take.
Attachment #8998344 - Attachment is obsolete: true
Comment on attachment 8998657 [details] [diff] [review]
Final patch for eventual landing, r=anba

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

It seems to me like you accidentally submitted an older patch.

::: js/src/builtin/JSON.cpp
@@ +99,5 @@
> +                uint8_t x = c >> 4;
> +                MOZ_ASSERT(x < 10);
> +                *dstPtr++ = '0' + x;
> +
> +                *dstPtr++ = ToLowerHex(c % 16);

As per Comment 2, this should be `ToLowerHex(c & 0xF)` for consistency.
Posted patch Er, whoopsSplinter Review
Attachment #8998657 - Attachment is obsolete: true
Comment on attachment 8998690 [details] [diff] [review]
Er, whoops

Requesting a review of the updated patch.
Attachment #8998690 - Flags: review?(andrebargull)
Comment on attachment 8998690 [details] [diff] [review]
Er, whoops

Second round of review isn't necessary.

It's an individual-reviewer sort of judgment call whether to flat-out deny review when an issue remains to be resolved, or r+ "with changes" and assume the submitter is competent to make the fixes.  The sum total of change from the original patch to this one is a straightforward application of the review comments (and a commit-message change), with no room for intelligence or judgment or anything like that, so it is unsurprising and understandable that as we've each written patches the other has reviewed and long engaged in the practice, r+ sight unseen as to the final patch would be how it would go.
Attachment #8998690 - Flags: review?(andrebargull) → review+
Priority: -- → P2
Duplicate of this bug: 1495210
This proposal is now at stage 3.
We can land this, I doesn't look like the proposal changed before advancing.
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55c19b23576a
Unicode-escape unpaired surrogates in JSON-quoted strings so that JSON-quoted strings are always valid Unicode without any embedded unpaired surrogates.  r=anba
https://hg.mozilla.org/mozilla-central/rev/55c19b23576a
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1496747
Note to docs team:

I've added a note about this to the Fx64 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#JavaScript

To finish this off, you should make sure this new proposal is properly represented in the stringify ref doc, update compat data, etc.
Added this here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#Well-formed_JSON.stringify

Also, a new row will be added to the compat table per https://github.com/mdn/browser-compat-data/pull/3124

Does this look good to you, :Waldo?
Flags: needinfo?(jwalden+bmo)
(In reply to Florian Scholz [:fscholz] (MDN) from comment #15)
> Added this here
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/JSON/stringify#Well-formed_JSON.stringify
> 
> Does this look good to you, :Waldo?

Somewhat, ish?  I made a couple additional changes in this regard, trying to better/more clearly describe what changed and whether anyone needs to care.  Mostly, users of JSON.stringify do not need to care about this change -- *unless* they pass a stringify result to code that isn't just consuming it as JSON text.  Maybe take a look at what I changed up?  I'd give a link to a diff, but the diff viewer is kind of making a mess of things too much.  :-(
Flags: needinfo?(jwalden+bmo) → needinfo?(fscholz)
Looks excellent to me (as always)! Thanks a lot for looking into it.

And yes, MDN's diff viewer is messy :/
Flags: needinfo?(fscholz)
You need to log in before you can comment on or make changes to this bug.