1505572 - get*State should return an object instead of a serialized string

Status: NEW

(Firefox :: Session Restore, enhancement, P2)

Description

[:Gijs (he/him)]

All the non-sessionstore consumers of these methods immediately parse the JSON back out again. This is a waste of CPU cycles (encoding and then immediately decoding again). We should just expose a non-JSON API.

Mike, do you know why these APIs return JSON at all? Is it just for IPC purposes or something? Or to avoid consumers changing the values when we pass object references, or something else?

Comment 1

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs (he/him) from comment #0)
> Or to avoid consumers changing the values when we
> pass object references, or something else?

This. At the moment I can't really think of a way to do this other than using `Cu.cloneInto` to make a structured clone (copy) of the data.

Comment 2

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #1)
> At the moment I can't really think of a way to do this other than
> using `Cu.cloneInto` to make a structured clone (copy) of the data.

Yes, but (as I'm sure you know) that should still be measurably faster than doing that via JSON.parse(JSON.stringify()) :)

Comment 3

[Mike de Boer [:mikedeboer]]

Indeed, so I'd be very happy to see that change. I can work on that somewhere in the future, but perhaps else can find the cycles before I can...? ;-)

Comment 4

[Mike Conley (:mconley) (:⚙️)]

We also have structuredClone now.

Comment 5

[:Gijs (he/him)]

I think this might be WFM? Mark did a bunch of heroic work around this, IIRC.

Comment 6

[Mark Banner (:standard8)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #4)

We also have structuredClone now.

The bugs we've already fixed here, switched to using Cu.cloneInto, since that's what was already being used in the non-string cases. I'm not sure which is better between the two for this case.

(In reply to :Gijs (he/him) from comment #5)

I think this might be WFM? Mark did a bunch of heroic work around this, IIRC.

There's still two functions to do:

• getBrowserState - this doesn't appear to have any uses outside tests, but lots of those tests do JSON.parse straight away...
• getTabState - there's a couple of uses here which would avoid the stringify -> parse cycle when dealing with lazy tabs in the re-opening container or swapping with a different browser. Again, there's also lots of tests that would benefit as well.