1340828 - Use SessionHistory.jsm on Android

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jan Henning [:JanH]]

As far as I can see, the session history code thankfully hasn't diverged very far between desktop and mobile, so it should be relatively straightforward to swap in SessionHistory.jsm and replace our own copy of the code.

Going forward, this should simplify the maintenance of this, since Desktop/Core people won't have to remember any more that any fixes in session history saving/restoring need to be ported over to Fennec as well.

Comment 1

[Jan Henning [:JanH]]

Attachment: Bug 1340828 - Part 1 - Move SessionHistory.jsm to toolkit.

We want to use this for Android, too. Once again, moving the file necessitates fixing a few ESLint errors that weren't yet checked for under the old directory.

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

Comment 2

[Jan Henning [:JanH]]

Attachment: Bug 1340828 - Part 2 - Make life easier for Fennec.

Life would easier if the restore function could return a reference to the nsISHistoryInternal interface retrieved from the DocShell, so that Fennec's main session store code can use that to trigger the needed reload afterwards instead of having to retrieve it again independently.

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

Comment 3

[Jan Henning [:JanH]]

Attachment: Bug 1340828 - Part 3 - Switch Fennec's session store over to SessionHistory.jsm.

Fennec's session store code was forked from Desktop a few years ago. Since then, the Desktop session store code has been refactored and modularised, which allows us to pull in again the bits the are compatible with our session store architecture and don't require any mobile-specific handling.

The handling of session history is such a case - with the exception of the "wyciwyg" filtering, any differences between SessionHistory.jsm and our equivalent in sessionstore.js seems to stem from Desktop bugfixes that were never ported over to our copy of the code.

Switching Fennec's session store over to use SessionHistory.jsm will prevent those sort of errors from occurring again in the future and hopefully simplify the maintenance of the session history code going forward.

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

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8843565 [details]
Bug 1340828 - Part 3 - Switch Fennec's session store over to SessionHistory.jsm.

https://reviewboard.mozilla.org/r/113676/#review119200

Awesome!

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8843563 [details]
Bug 1340828 - Part 1 - Move SessionHistory.jsm to toolkit.

https://reviewboard.mozilla.org/r/113672/#review119206

I'm very happy that Fennec is able to use this code! And even happier with the ESLint <3 ;-)

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8843564 [details]
Bug 1340828 - Part 2 - Make life easier for Fennec.

https://reviewboard.mozilla.org/r/113674/#review119208

LGTM!

Comment 7

[Jan Henning [:JanH]]

Comment on attachment 8843565 [details]
Bug 1340828 - Part 3 - Switch Fennec's session store over to SessionHistory.jsm.

https://reviewboard.mozilla.org/r/113676/#review119218

::: mobile/android/components/SessionStore.js:1293
(Diff revision 1)
> -    aHistory.getEntryAtIndex(activeIndex, true);
>  
> +    // SessionHistory.jsm will have force set the active history item,
> +    // but we still need to reload it in order to finish the process.
>      try {
> -      aHistory.QueryInterface(Ci.nsISHistory).reloadCurrentEntry();
> +      history.reloadCurrentEntry();

Note to myself before landing:
In practice it seems to work anyway, but since SessionHistory.jsm returns an `nsISHistoryInternal` interface, I technically still need to QI this to `nsISHistory` before calling `reloadCurrentEntry()`.

Comment 8

[Jan Henning [:JanH]]

Comment on attachment 8843563 [details]
Bug 1340828 - Part 1 - Move SessionHistory.jsm to toolkit.

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

Comment 9

[Jan Henning [:JanH]]

Comment on attachment 8843564 [details]
Bug 1340828 - Part 2 - Make life easier for Fennec.

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

Comment 10

[Jan Henning [:JanH]]

Comment on attachment 8843565 [details]
Bug 1340828 - Part 3 - Switch Fennec's session store over to SessionHistory.jsm.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/94b9be873261
Part 1 - Move SessionHistory.jsm to toolkit. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/bcea389b60b1
Part 2 - Make life easier for Fennec. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/a9c30ad75f43
Part 3 - Switch Fennec's session store over to SessionHistory.jsm. r=sebastian

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/94b9be873261
https://hg.mozilla.org/mozilla-central/rev/bcea389b60b1
https://hg.mozilla.org/mozilla-central/rev/a9c30ad75f43