645922 - Remove js_TryJSON and JS_TryJSON

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: 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]]

Comment on attachment 522553 [details] [diff] [review]
Patch

- lines are always nice.

/be

Comment 2

[Jeff Walden [:Waldo]]

Attachment: 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]]

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]]

Attachment: Better patch, actually works

Comment 5

[Jeff Walden [:Waldo]]

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)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/0908448636c5

Comment 7

[Jeff Walden [:Waldo]]

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]

#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]]

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]

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]]

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]

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]]

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]]

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

[LegNeato]

Does this have binary add-on compatibility impact? If so, do you think the impact is large?

Comment 16

[Jorge Villalobos [:jorgev] (he/him)]

This was reverted in bug 672063, so there shouldn't be any interface changes here.

Comment 17

[LegNeato]

Clearing tracking as we are tracking bug 672063

Comment 19

[Serge Gautherie (:sgautherie)]

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.