1015516 - Save closedTabs when saving session to disk

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

This is a follow-up to bug 1004850, which would make it more useful when the user's session is restored. Right now we just stored closed tabs in memory, but we could also write them to disk if we want to restore them.

Comment 1

[:Margaret Leibovic]

Attachment: WIP - Save closedTabs when saving session to disk

I think this should do the right thing, but I'm having a hard time testing it. I tried flipping the session restore pref to always restore my session, then killing and re-launching fennec, but I'm finding that state.windows[0].closedTabs is always undefined.

How does writing to sessionstore.js and sessionstore.bak work? I see that we're writing to sessionstore.js in _saveState, but reading from sessionstore.bak in restoreLastSession.

Comment 2

[Brian Nicholson (:bnicholson)]

Comment on attachment 8504981 [details] [diff] [review]
WIP - Save closedTabs when saving session to disk

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

This looks OK to me at first glance. I guess it'd be worth throwing in some logs where you're setting normalWin.closedTabs to make sure it's getting saved in the first place (and has the expected values).

Comment 3

[:Margaret Leibovic]

Vivek, maybe you would also be interested in taking this bug after wrapping up bug 732752.

Comment 4

[Vivek Balakrishnan[:vivek]]

Margaret, thanks I will take this bug.

Comment 5

[Vivek Balakrishnan[:vivek]]

Attachment: 1015516_part2.patch

This is in continuation to Margaret's WIP.

As discussed over irc, handled closedTabs in SessionParser and cleaned up restoreLastSession method to process data only from Java.

Comment 6

[Brian Nicholson (:bnicholson)]

Comment on attachment 8538135 [details] [diff] [review]
1015516_part2.patch

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

Oops, didn't notice this patch was built on top of Margaret's WIP before I marked it obsolete. But I'd go ahead and just merge them into a single patch -- her patch was pretty small, and it didn't do anything on its own.

::: mobile/android/base/GeckoApp.java
@@ +1649,5 @@
>              // stub the restored tabs immediately. This allows the UI to be
>              // updated before Gecko has restored.
>              if (mShouldRestore) {
>                  final JSONArray tabs = new JSONArray();
> +                final JSONObject closedTabs = new JSONObject();

Since closedTabs is an array, I think it would be better for this to be a JSONArray instead of an object with a single array attached to it.

@@ +1673,5 @@
>                      }
> +
> +                    @Override
> +                    public void onClosedTabsRead(final JSONArray closedTabData) throws JSONException {
> +                        closedTabs.put("closedTabs", closedTabData);

...then this would be changed to | closedTabs = closedTabData |.

::: mobile/android/base/SessionParser.java
@@ +51,5 @@
>      };
>  
>      abstract public void onTabRead(SessionTab tab);
>  
> +    // Bug 1015516 : Placeholder method that must be overloaded to handle closedTabs while parsing session data.

Nit: Please remove the "Bug 1015516" part. Only outstanding bugs need to be annotated in comments. You can also use JavaDoc-style commenting since this is a public method.

Comment 7

[Vivek Balakrishnan[:vivek]]

Attachment: 1015516.patch

A new patch with review comments addressed. 
This patch folds Margaret's and my changes to a single patch.

A try run for changes as separate patch can be found at:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4aa36cf716d

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 8538937 [details] [diff] [review]
1015516.patch

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

Looks good to me!

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/cac34649ce45

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cac34649ce45