The default bug view has changed. See this FAQ.

Remove js_TryJSON and JS_TryJSON

RESOLVED FIXED in mozilla7

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

({dev-doc-complete})

Trunk
mozilla7
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 1 obsolete attachment)

Created attachment 522553 [details] [diff] [review]
Patch

After bug 636079 these methods have no users.  They're extra effort as far as bug 645468 is concerned, and I can't see a use case for what they do anyway, so we should remove them.
Attachment #522553 - Flags: review?(brendan)
Comment on attachment 522553 [details] [diff] [review]
Patch

- lines are always nice.

/be
Attachment #522553 - Flags: review?(brendan) → review+
Created attachment 523811 [details] [diff] [review]
Patch, with JS_TryJSON removal outside js/src

Sigh.  I thought I'd built Mozilla with the patch here, but it seems I hadn't -- either that or new JS_TryJSON uses sneaked in (yeah, right).  So let's get a look at the non-js/src changes.

Brief recap: JS_TryJSON does work that JS_Stringify already does -- that is, calling a toJSON method on the provided object if there is one.  So there's no need to be calling JS_TryJSON here to "filter" the value being stringified, or anything like that.  Moreover, it's very likely even *wrong* to do so, because invoking toJSON twice means, taking the large view, you'd over-toJSON, like for something like this:

  {
    toJSON: function()
    {
      return { a: 5, toJSON: function() { return 17; } };
    }
  }

Calling JS_TryJSON on that before stringifying will stringify '17', but you really wanted to stringify '{"a":5}' (functions aren't JSON-serializable values, so the inner toJSON property would be skipped when serializing).

And further, the method is getting in the way of other cleanups/refactoring being done now, so it needs to go even ignoring the correctness issues it likely introduces.
Attachment #523811 - Flags: review?
Attachment #523811 - Flags: review? → review?(jst)
Comment on attachment 523811 [details] [diff] [review]
Patch, with JS_TryJSON removal outside js/src

Oops, this breaks some undocumented requirements for encoding/decoding primitives in nsIJSON.  Better patch shortly.
Attachment #523811 - Flags: review?(jst)
Created attachment 525817 [details] [diff] [review]
Better patch, actually works
Attachment #523811 - Attachment is obsolete: true
Attachment #525817 - Flags: review?(jst)

Updated

6 years ago
Attachment #525817 - Flags: review?(jst) → review+
http://hg.mozilla.org/tracemonkey/rev/0908448636c5

There are a couple changes here relevant to documentation.

First, nsIJSON.encode and nsIJSON.decode have been removed, and their uses should be replaced with JSON.stringify and JSON.parse, respectively.  The effort needed to switch from one to the other is almost negligible.

Second, JS_TryJSON has been removed.  I noted its obsolescence on the 1.8.5 release notes page, and there wasn't a page documenting it already, so I think this is fully documented, but I'm noting it just in case.
Keywords: dev-doc-needed
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla7
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/0908448636c5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla7 → mozilla8
This missed the last TM->m-c merge before the aurora cutoff due to a schedule misunderstanding, but it was part of a post-main-merge TM->aurora catchup merge:

http://hg.mozilla.org/releases/mozilla-aurora/rev/0908448636c5

So this will be in 7.
Target Milestone: mozilla8 → mozilla7

Comment 8

6 years ago
#0> After bug 636079 these methods have no users
ORLY?

Look into a current (1.3.9) stable Adblock Plus
 chrome://adblockplus-modules/content/FilterListener.jsm Line 382

And it's only one from a MANY addons ruined by this "fix"

#5> The effort needed to switch from one to the other
#5> is almost negligible.
So why not to make a simple dumb wrapper -- translator from an .encode into a .stringify calls? Is this so-o-o hard?
I meant no users in the Mozilla tree.  The "simple dumb wrapper" would not be so simple, because of certain quirks of nsIJSON.encode/decode that are not present in JSON.parse/JSON.stringify.  Specifically, the encode/decode methods are not helpful when working with a primitive value at the root -- 1, true, "foopy", etc.  I no longer remember the details, but I think basically they got censored to null at output, or similar, which unnecessarily throws away information compared to just doing what JSON.parse and JSON.stringify do.

In the long run, using JSON.parse and JSON.stringify will be better for everyone -- no need to drag in Mozilla-specific APIs to get stuff done, just use standard JS.  In the short run, some minor work might be required to transition.

Comment 10

6 years ago
While it may be better for everyone in the long run, making breaking changes like this without bothering to update the docs that things are being deprecated is rough on people trying to support Mozilla extensions:   

https://developer.mozilla.org/en/JSON
I confess lack of knowledge of the location of all that documentation, so I didn't update it.  Note the dev-doc-needed keyword here, which was intended (along with comment 5) to rectify this.  I also sent mail about a week ago to a few AMO people telling them about the change, asking them to add it to the list of things they scan for when doing automated addon compatibility checks.  (And I imagine they'll also make sure it gets added to any "Firefox # for addon developers" articles that may be written.)

I usually do update documentation for changes when I know what needs updating, and I can point to ample bugs demonstrating that I've done this in the past, if you're skeptical.  But for this bug, since it involves documentation with which I'm unfamiliar, I couldn't really do it and be certain I'd done it fully.

That said, I'm happy to update the JSON page now, although I doubt that's the end of what needs updating for this (and have no idea where/what that further documentation might be).

Comment 12

6 years ago
Thanks Jeff -- 

The other page I hit in my searching that I'd recommend updating is:   https://developer.mozilla.org/en/nsIJSON
I've updated/substantially rewritten the JSON page:

https://developer.mozilla.org/en/JSON

The old organization was pretty grab-baggy, the new one is a bit more targeted on doing stuff.  I also killed information on JSON.jsm and other really old stuff that's barely relevant these days.  It delegates details to the JS reference, since that's generally going to be better-maintained on JS details (it's the first place JS hackers are going to look for stuff to update, generally).

I haven't updated the nsIJSON page noted in comment 12 yet, because I'm not certain of the format for noting deletions and such.  I'm pinging sheppy about it as we speak.
And now nsIJSON is updated, too:

https://developer.mozilla.org/en/nsIJSON

I still could be missing other docs, so leaving d-d-n in case I've missed something.
Keywords: dev-doc-needed → dev-doc-complete

Updated

6 years ago
Blocks: 672063

Updated

6 years ago
tracking-firefox7: --- → +

Updated

6 years ago
No longer blocks: 672063
Depends on: 672063

Comment 15

6 years ago
Does this have binary add-on compatibility impact? If so, do you think the impact is large?
This was reverted in bug 672063, so there shouldn't be any interface changes here.

Comment 17

6 years ago
Clearing tracking as we are tracking bug 672063
tracking-firefox7: + → ---

Updated

6 years ago
Duplicate of this bug: 621039
http://hg.mozilla.org/comm-central/rev/69de1b1f58f7
"Unit test bustage fix - port bug 645922 to comm-central (Remove nsIJSON.encode/decode, because their functionality is subsumed by JSON.parse and JSON.stringify, and their idiosyncrasies are hindering code improvements). r=bustage fix"

+

http://hg.mozilla.org/comm-central/rev/56c513a746ce
"(BBv1) Fully resync' test_leftpane_corruption_handling.js (= what changeset 69de1b1f58f7 missed)."
to c-c/14 only.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.