Closed Bug 1015516 Opened 6 years ago Closed 6 years ago

Save closedTabs when saving session to disk

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 37

People

(Reporter: Margaret, Assigned: vivek)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → margaret.leibovic
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.
Attachment #8504981 - Flags: feedback?(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).
Attachment #8504981 - Flags: feedback?(bnicholson) → feedback+
Vivek, maybe you would also be interested in taking this bug after wrapping up bug 732752.
Flags: needinfo?(vivekb.balakrishnan)
Margaret, thanks I will take this bug.
Flags: needinfo?(vivekb.balakrishnan)
Assignee: margaret.leibovic → vivekb.balakrishnan
Attached patch 1015516_part2.patch (obsolete) — Splinter Review
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.
Attachment #8538135 - Flags: review?(bnicholson)
Attachment #8504981 - Attachment is obsolete: true
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.
Attachment #8538135 - Flags: review?(bnicholson) → feedback+
Attached patch 1015516.patchSplinter Review
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
Attachment #8538135 - Attachment is obsolete: true
Attachment #8538937 - Flags: review?(bnicholson)
Comment on attachment 8538937 [details] [diff] [review]
1015516.patch

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

Looks good to me!
Attachment #8538937 - Flags: review?(bnicholson) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cac34649ce45
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
You need to log in before you can comment on or make changes to this bug.