857602 - Implement nsISessionStore.restored to easily know if the session has already been restored

Status: RESOLVED INVALID

(Firefox :: Session Restore, defect)

Description

[Alexandre Poirot [:ochameau]]

There is a common issue when you listen for sessionstore-windows-restored: you can't easily know if this event has already been dispatched and the session has already been restored.
It isn't much an issue in Firefox codebase, as we can register the event listener during firefox early startup, but it becomes way more complicated when you are an addon that is installed at any time.

In order to prevent any weak workaround in SDK code and ease running code after session restore, I'm suggesting to implement nsISessionStore.restored, a boolean that will become true when the session has been restored and the sessionstore-windows-restored event fired. And that on all platforms that supports this component.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 857602: Implement nsISessionStore.restored to indicate if session has already been restored.

Comment 2

[Alexandre Poirot [:ochameau]]

Here is a potential patch, I still need to test it on android and do the similar modification to metro.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 857602: Implement nsISessionStore.restored to indicate if session has already been restored.

Comment 4

[Alexandre Poirot [:ochameau]]

Pushed to try to see if metro and android test actually pass...
https://tbpl.mozilla.org/?tree=Try&rev=314270fba923

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 733444 [details] [diff] [review]
Bug 857602: Implement nsISessionStore.restored to indicate if session has already been restored.

I've seen that you reviewed some patches around this component...

So, this patch will be usefull for addons, or any code that can eventually be run on startup or later, and want to ensure running after the session is restored.
I added some tests, the sessionstore-windows-restored isn't tested yet whereas it is extremelly important that it actually fires as jetpack addons won't start if we miss it.
Unfortunately, I haven't found any test framework that allows running a test during firefox start in order to listen for this event. So I'm only ensure that `restored` is true during mochitest-browser.

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 733444 [details] [diff] [review]
Bug 857602: Implement nsISessionStore.restored to indicate if session has already been restored.

Gavin, Can you find me a reviewer for this bug?

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 733444 [details] [diff] [review]
Bug 857602: Implement nsISessionStore.restored to indicate if session has already been restored.

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

f+ for the non-metro, non-fennec version.

::: browser/components/sessionstore/test/browser_857602.js
@@ +3,5 @@
> +
> +function test() {
> +  ok(ss.restored, "nsISessionStore.restored is true as tests are run only " +
> +                  "after the session is restored.");
> +}

Nit: Could you add a comment mentioning why this is sufficient (i.e. ss.restored is initialized to false).

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 733444 [details] [diff] [review]
Bug 857602: Implement nsISessionStore.restored to indicate if session has already been restored.

r=me on the non-metro browser/ portions.

Flagging mbrubeck for metro, and bnicholson for mobile/

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 733444 [details] [diff] [review]
Bug 857602: Implement nsISessionStore.restored to indicate if session has already been restored.

Android parts look fine.

Comment 10

[Matt Brubeck (:mbrubeck)]

Comment on attachment 733444 [details] [diff] [review]
Bug 857602: Implement nsISessionStore.restored to indicate if session has already been restored.

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

::: browser/metro/components/test/Makefile.in
@@ +11,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +
> +MOCHITEST_BROWSER_FILES = \
> +	browser_857602.js \
> +	$(NULL)

This will actually run in desktop Firefox rather than in Metro Firefox.

To create a mochitest-browser-chrome test that runs in the Metro app, you'll need to install it to _tests/testing/mochitest/metro like we do in this directory:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/Makefile.in

You could modify rules.mk to look for a "MOCHITEST_METRO_FILES" variable to make this more convenient.  (We hadn't bothered doing this yet because we had only one directory of "metro-chrome" tests.)

::: browser/metro/components/test/browser_857602.js
@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +function test() {
> +  ok(ss.restored, "nsISessionStore.restored is true as tests are run only " +
> +                  "after the session is restored.");

There's no "ss" global in the Metro browser, so you'll need to call getService here.

Comment 11

[Matt Brubeck (:mbrubeck)]

By the way, we have no automated tests running for the Metro browser yet (we're actively working on it in bug 847442) so you won't be able to verify your test on Try.  I'm happy to test it for you if you don't have a Windows 8 machine to do it yourself.

Comment 12

[Brian Nicholson (:bnicholson)]

Comment on attachment 733444 [details] [diff] [review]
Bug 857602: Implement nsISessionStore.restored to indicate if session has already been restored.

Oh, I missed the test bits at the bottom.

I'm not familiar enough with the test build system to know if this will work. But I think we'll at least have the same problem regarding the ss global that Matt mentioned.

Passing review to Mark.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 733444 [details] [diff] [review]
Bug 857602: Implement nsISessionStore.restored to indicate if session has already been restored.

I agree with Brian, r+ on the code changes. However, we do not run browser-chrome or even mochi-chrome on Android. Your test will not run.

Sadly, we have no direct way to run a test like that. Even if we did, as Matt points out for Metro, there is no global "ss" object. You'd need to XPCOM getService.

I'll give an r+, but remove the test related code, since it won't run.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Yoric: some of the add-ons in your post at http://dutherenverseauborddelatable.wordpress.com/2013/05/23/add-on-breakage-continued-list-of-add-ons-that-will-probably-be-affected/ are using those properties for this purpose, so it would be good to get this patch landed in the same cycle as the __SS_* removals.