1474139 - Move content-sessionStore.js to a JSM

Status: RESOLVED FIXED

(Firefox :: Session Restore, enhancement, P1)

Description

[Kris Maglione [:kmag]]

Aside from causing a separate copy to be loaded into every content frameloader, loading this script as a framescript puts it in a different compartment from the rest of the JSMs, which means it creates a lot of extra wrapper, JS prototype, and self-hosted JS overhead.

The bulk of the script should be a JSM loaded by a frame script.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1474139: Part 1 - Refactor content-sessionSstore to not depend on running in a frame script global.

Review commit: https://reviewboard.mozilla.org/r/255622/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255622/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1474139: Part 2 - Move content-sessionStore to a separate JSM.

Review commit: https://reviewboard.mozilla.org/r/255624/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255624/

Comment 3

[Mike de Boer [:mikedeboer]]

Kris, sorry for the delay here. The patches are rather big :) They look good though and I'm making progress.

Comment 4

[Dale Harvey (:daleharvey)]

Actively worked on so P1

Comment 5

[Mike de Boer [:mikedeboer]]

Looking very good so far, prolly r+ tomorrow. Thanks for you patience!

Comment 6

[Kris Maglione [:kmag]]

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> Looking very good so far, prolly r+ tomorrow. Thanks for you patience!

Cheers

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8990556 [details]
Bug 1474139: Part 1 - Refactor content-sessionSstore to not depend on running in a frame script global.

https://reviewboard.mozilla.org/r/255622/#review264172

I have to say that I like that you introduced the 'Handler' generalization class.

::: commit-message-bfade:1
(Diff revision 1)
> +Bug 1474139: Part 1 - Refactor content-sessionSstore to not depend on running in a frame script global. r?mikedeboer

nit: 'content-sessionStore' (-s)

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8990557 [details]
Bug 1474139: Part 2 - Move content-sessionStore to a separate JSM.

https://reviewboard.mozilla.org/r/255624/#review264320

I see what you did here, but please try to use `hg mv` to make the diff smaller (the content-sessionStore.js deletions will be omitted in that case). r=me with that change!

Comment 9

[Mike de Boer [:mikedeboer]]

We have a comprehensive test suite, so when it's green, I'd be comfortably with landing this.

Comment 10

[Kris Maglione [:kmag]]

(In reply to Mike de Boer [:mikedeboer] from comment #8)
> I see what you did here, but please try to use `hg mv` to make the diff
> smaller (the content-sessionStore.js deletions will be omitted in that
> case). r=me with that change!

Unfortunately, that doesn't work. `hg mv` just registers a copy and a delete. Re-adding the original file in the same commit just undoes the delete, and you get the same diff you'd get with just a `cp`.

Comment 11

[Mike de Boer [:mikedeboer]]

It looks like I missed something here, because tests are failing - I assume you're on it?

Comment 12

[Kris Maglione [:kmag]]

(In reply to Mike de Boer [:mikedeboer] from comment #11)
> It looks like I missed something here, because tests are failing - I assume
> you're on it?

Yeah, sorry, I forgot to change `onLoadStarted() {` to `onLoadStarted: () => {` in restoreHistory. Already fixed.

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8990556 [details]
Bug 1474139: Part 1 - Refactor content-sessionSstore to not depend on running in a frame script global.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255622/diff/1-2/

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8990557 [details]
Bug 1474139: Part 2 - Move content-sessionStore to a separate JSM.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255624/diff/1-2/

Comment 15

[Kris Maglione [:kmag]]

(I also apparently made a classic Vim-user error and changed this.contentRestoreInitialized to this.ontentRestoreInitialized just before committing... which broke a bunch of things that worked when I first tested :/)

Comment 16

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5106a0c2566570158dfaf129ad42a4ff811c158
Bug 1474139: Part 1 - Refactor content-sessionSstore to not depend on running in a frame script global. r=mikedeboer

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccfb086c668d4454ee4484dc1d22951e9a3bc6c3
Bug 1474139: Part 2 - Move content-sessionStore to a separate JSM. r=mikedeboer

Comment 17

[Mike de Boer [:mikedeboer]]

(In reply to Kris Maglione [:kmag] from comment #15)
> (I also apparently made a classic Vim-user error and changed
> this.contentRestoreInitialized to this.ontentRestoreInitialized just before
> committing... which broke a bunch of things that worked when I first tested
> :/)

Heh, and I focused my review on logical errors, not typos ;-)

It seems to have all worked out - fingers crossed of course, but usually our unit tests give a very good indication. Thanks!

Comment 18

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a8131b3319381ef00054af0de6ce6d811552ad8
Bug 1474139: Follow-up: Fix test that relied on Timer.jsm being implicitly imported. r=bustage,test-only CLOSED TREE

Comment 19

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/835430ce6d62e9ce56ceaca7b0a7ba8ae1f6830f
Bug 1474139: Follow-up: Fix another test that relied on Timer.jsm being implicitly imported. r=bustage CLOSED TREE

Comment 20

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/d5106a0c2566
https://hg.mozilla.org/mozilla-central/rev/ccfb086c668d
https://hg.mozilla.org/mozilla-central/rev/1a8131b33193
https://hg.mozilla.org/mozilla-central/rev/835430ce6d62