Last Comment Bug 645922 - Remove js_TryJSON and JS_TryJSON
: Remove js_TryJSON and JS_TryJSON
Status: RESOLVED FIXED
fixed-in-tracemonkey
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
: 621039 (view as bug list)
Depends on: 636079 672063
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-28 19:04 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-04-09 07:19 PDT (History)
9 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (1.99 KB, patch)
2011-03-28 19:04 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
brendan: review+
Details | Diff | Splinter Review
Patch, with JS_TryJSON removal outside js/src (3.71 KB, patch)
2011-04-02 12:51 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Better patch, actually works (29.79 KB, patch)
2011-04-13 14:33 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
jst: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-03-28 19:04:10 PDT
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.
Comment 1 Brendan Eich [:brendan] 2011-04-01 15:28:30 PDT
Comment on attachment 522553 [details] [diff] [review]
Patch

- lines are always nice.

/be
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-02 12:51:01 PDT
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.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-02 14:32:13 PDT
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.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-13 14:33:40 PDT
Created attachment 525817 [details] [diff] [review]
Better patch, actually works
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-28 14:28:05 PDT
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.
Comment 6 Chris Leary [:cdleary] (not checking bugmail) 2011-07-05 18:03:59 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/0908448636c5
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-08 16:08:19 PDT
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.
Comment 8 vitar 2011-07-11 11:31:14 PDT
#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?
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-11 12:11:28 PDT
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 Joe Siegrist 2011-07-11 13:36:54 PDT
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
Comment 11 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-11 14:46:29 PDT
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 Joe Siegrist 2011-07-11 15:09:12 PDT
Thanks Jeff -- 

The other page I hit in my searching that I'd recommend updating is:   https://developer.mozilla.org/en/nsIJSON
Comment 13 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-12 11:12:01 PDT
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.
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-12 13:29:41 PDT
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.
Comment 15 christian 2011-08-11 14:56:20 PDT
Does this have binary add-on compatibility impact? If so, do you think the impact is large?
Comment 16 Jorge Villalobos [:jorgev] 2011-08-11 15:12:10 PDT
This was reverted in bug 672063, so there shouldn't be any interface changes here.
Comment 17 christian 2011-09-14 17:25:32 PDT
Clearing tracking as we are tracking bug 672063
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2011-10-09 11:31:56 PDT
*** Bug 621039 has been marked as a duplicate of this bug. ***
Comment 19 Serge Gautherie (:sgautherie) 2012-04-09 07:19:52 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.