Closed Bug 645922 Opened 9 years ago Closed 9 years ago

Remove js_TryJSON and JS_TryJSON


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)



(Keywords: dev-doc-complete, Whiteboard: fixed-in-tracemonkey)


(2 files, 1 obsolete file)

Attached patch PatchSplinter Review
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]

- lines are always nice.

Attachment #522553 - Flags: review?(brendan) → review+
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)
Attachment #523811 - Attachment is obsolete: true
Attachment #525817 - Flags: review?(jst)
Attachment #525817 - Flags: review?(jst) → review+

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
Closed: 9 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:

So this will be in 7.
Target Milestone: mozilla8 → mozilla7
#0> After bug 636079 these methods have no users

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.
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:
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).
Thanks Jeff -- 

The other page I hit in my searching that I'd recommend updating is:
I've updated/substantially rewritten the JSON page:

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:

I still could be missing other docs, so leaving d-d-n in case I've missed something.
Blocks: 672063
No longer blocks: 672063
Depends on: 672063
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.
Clearing tracking as we are tracking bug 672063
Duplicate of this bug: 621039
"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"

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