1347515 - Remove nsIJSON

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Henri Sivonen (:hsivonen) (temporarily not reading bugmail)]

We should remove nsIJSON once support for XPCOM extensions has been dropped.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 1 - nsIJSON.encodeFromJSVal,

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 2 - nsIJSON.decodeToJSVal

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 3 - nsIJSON.decodeFromStream

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 4 - dom/json

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8926832 [details] [diff] [review]
part 3 - nsIJSON.decodeFromStream

Review of attachment 8926832 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +316,5 @@
>                            .createInstance(Ci.nsIFileInputStream);
>        inputStream.init(new FileUtils.File(this._path),
>                         FileUtils.MODE_RDONLY, FileUtils.PERMS_FILE, 0);
>        try {
> +        let data = jSON.parse(NetUtil.readInputStreamToString(inputStream,

I don't think this will run correctly? jSON shouldn't be valid?

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Most everything looks good here, and will r+ pretty quick after that JSON typo is fixed.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 3 - nsIJSON.decodeFromStream

Yes, I haven't pushed the patch to try. This is green.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: comm

comm repository has to be updated. I have to admit that I have not tested this patch. Jorgk, can you please review and test this patch? Thanks.

Comment 12

[Jorg K (CEST = GMT+2)]

Thanks, I will.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3c81ef73700
Get rid of nsIJSON.encodeFromJSVal, r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/e26f2365ea7c
Get rid of nsIJSON.decodeToJSVal, r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/08f21a0e7ebe
Get rid of nsIJSON.decodeFromStream, r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/31b7eb194662
Get rid of dom/json, r=qdot

Comment 14

[Tom S [:evilpie]]

I think this means we can remove the special parsing code we had for this API from the JS engine.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8927053 [details] [diff] [review]
comm

Review of attachment 8927053 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/modules/imSmileys.jsm
@@ +5,5 @@
>  Components.utils.import("resource:///modules/imServices.jsm");
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyGetter(this, "gTextDecoder", () => {
> +  return new TextDecoder();

Why do it like this when you're doing it differently in calendar/test/unit/head_consts.js?

Comment 16

[Andrea Marchesini [:baku]]

> Why do it like this when you're doing it differently in
> calendar/test/unit/head_consts.js?

This is for testing. We don't care too much of avoiding creating a single TextDecoder. But for imSmileys.jsm, we can optimize the number of TextDecoder objects.

Comment 17

[Andrea Marchesini [:baku]]

(In reply to Tom Schuster [:evilpie] from comment #14)
> I think this means we can remove the special parsing code we had for this
> API from the JS engine.

Can you file a follow up? Maybe pointing to the code you are talking about? Thanks

Comment 18

[Andrea Marchesini [:baku]]

I filed bug 1416155

Comment 19

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/a3c81ef73700
https://hg.mozilla.org/mozilla-central/rev/e26f2365ea7c
https://hg.mozilla.org/mozilla-central/rev/08f21a0e7ebe
https://hg.mozilla.org/mozilla-central/rev/31b7eb194662

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8927053 [details] [diff] [review]
comm

Thanks, I'll get this landed now. Sadly we're busted by the packaging changes also landed here, so I can't test it. So we might have to follow up in case of problems.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ebac2c6a8ba8
remove dom_json.xpt from TB/IB/SM package manifests. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/916c30e50af1
Remove use of nsIJSON in Thunderbird's calendar and chat. rs=jorgk

Comment 22

[:gerard-majax]

This regressed containers: https://github.com/mozilla/multi-account-containers/issues/949#issuecomment-343994922

Comment 23

[Tanvi Vyas[:tanvi]]

(In reply to Alexandre LISSY :gerard-majax from comment #22)
> This regressed containers:
> https://github.com/mozilla/multi-account-containers/issues/949#issuecomment-
> 343994922

Can we pull this out of the first beta because of the regression?  And then land it as part of the second beta that goes out, along with bug 1416468?  If this goes into beta, it will cause irrevocable damage to Containers beta users.  (20% of folks who have installed https://addons.mozilla.org/en-US/firefox/addon/multi-account-containers/ are on beta.)

Alternatively, we would have to get bug 1416468 into the first beta, and it might be too late for that.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Julien - Data loss in profile here, so we probably should take some action here quickly.  We should probably rebuild beta 3 and retest even if that causes some delay in 58.0b3 release.

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

A note to clarify what landed where. We build 58.0b1 and shipped it last Tuesday to dev edition users. Because of some build issues we didn't ship 58.0b2.  58.0b3 in theory would ship this Wednesday to beta users for the first time.  So, this regression was never shipped to non-nightly users.

Comment 26

[Gerry Chang [:gchang]]

This cause bug 1416468 regression. Back out first for the first 58 beta.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

This shouldn't have landed during a Nightly soft freeze right before the final merge to Beta. Anyway, backout pushed to Beta.

https://hg.mozilla.org/releases/mozilla-beta/rev/f7eb3bbcd5f830244a4234b1269c3a73e750330b

Comment 28

[Jorg K (CEST = GMT+2)]

Backed out from comm-beta (TB 58):
https://hg.mozilla.org/releases/comm-beta/rev/778fc98a593fac351fedb4477fd2ce39629bc9cd
https://hg.mozilla.org/releases/comm-beta/rev/b4b33e1604e113149a9c5401f8248b687a0dd79e