1469021 - Implement well formed JSON.stringify

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Yulia Startsev [:yulia]]

Currently in stage 2 in the TC39 process. More information: https://github.com/tc39/proposal-well-formed-stringify

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Driveby procrastination.

Comment 2

[André Bargull [:anba]]

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?

Comment 3

[Jeff Walden [:Waldo]]

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

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Final patch for eventual landing, r=anba

Comment 5

[ExE Boss]

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.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Er, whoops

Comment 7

[ExE Boss]

Comment on attachment 8998690 [details] [diff] [review]
Er, whoops

Requesting a review of the updated patch.

Comment 8

[Jeff Walden [:Waldo]]

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.

Comment 10

[Mathias Bynens]

This proposal is now at stage 3.

Comment 11

[Tom S [:evilpie]]

We can land this, I doesn't look like the proposal changed before advancing.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/55c19b23576a

Comment 14

[Chris Mills [:cmills]]

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.

Comment 15

[Florian Scholz (Open Web Docs)]

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?

Comment 16

[Jeff Walden [:Waldo]]

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

Comment 17

[Florian Scholz (Open Web Docs)]

Looks excellent to me (as always)! Thanks a lot for looking into it.

And yes, MDN's diff viewer is messy :/