Implement well formed JSON.stringify

RESOLVED FIXED in Firefox 64

Status

()

P2
normal
RESOLVED FIXED
9 months ago
4 months ago

People

(Reporter: yulia, Assigned: Waldo)

Tracking

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

unspecified
mozilla64
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

Currently in stage 2 in the TC39 process. More information: https://github.com/tc39/proposal-well-formed-stringify
Keywords: dev-doc-needed
Blocks: 1435830
(Assignee)

Comment 1

8 months ago
Posted patch Patch (obsolete) — Splinter Review
Driveby procrastination.
Attachment #8998344 - Flags: review?(andrebargull)
(Assignee)

Updated

8 months ago
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+
(Assignee)

Comment 3

8 months ago
(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.
(Assignee)

Comment 4

8 months ago
(Assignee)

Updated

8 months ago
Attachment #8998344 - Attachment is obsolete: true

Comment 5

8 months ago
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.
(Assignee)

Comment 6

8 months ago
Posted patch Er, whoopsSplinter Review
(Assignee)

Updated

8 months ago
Attachment #8998657 - Attachment is obsolete: true

Comment 7

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

Requesting a review of the updated patch.
Attachment #8998690 - Flags: review?(andrebargull)
(Assignee)

Comment 8

8 months ago
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

Comment 10

6 months ago
This proposal is now at stage 3.
Blocks: 1435811
No longer blocks: 1435830
We can land this, I doesn't look like the proposal changed before advancing.

Comment 12

6 months ago
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

Comment 13

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/55c19b23576a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox64: --- → fixed
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)
(Assignee)

Comment 16

4 months ago
(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)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.