Closed
Bug 1015516
Opened 10 years ago
Closed 10 years ago
Save closedTabs when saving session to disk
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 37
People
(Reporter: Margaret, Assigned: vivek)
References
Details
Attachments
(1 file, 2 obsolete files)
8.17 KB,
patch
|
bnicholson
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → margaret.leibovic
Reporter | ||
Comment 1•10 years ago
|
||
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 2•10 years ago
|
||
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+
Reporter | ||
Comment 3•10 years ago
|
||
Vivek, maybe you would also be interested in taking this bug after wrapping up bug 732752.
Flags: needinfo?(vivekb.balakrishnan)
Assignee | ||
Comment 4•10 years ago
|
||
Margaret, thanks I will take this bug.
Flags: needinfo?(vivekb.balakrishnan)
Assignee | ||
Updated•10 years ago
|
Assignee: margaret.leibovic → vivekb.balakrishnan
Assignee | ||
Comment 5•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8504981 -
Attachment is obsolete: true
Comment 6•10 years ago
|
||
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+
Assignee | ||
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 9•10 years ago
|
||
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•