Closed Bug 697858 Opened 13 years ago Closed 12 years ago

Restore tabs from previous session, including history

Categories

(Firefox for Android Graveyard :: General, defect, P2)

ARM
Android
defect

Tracking

(firefox11 fixed, firefox12 verified, fennec11+)

VERIFIED FIXED
Firefox 12
Tracking Status
firefox11 --- fixed
firefox12 --- verified
fennec 11+ ---

People

(Reporter: mbrubeck, Assigned: bnicholson)

References

Details

Attachments

(4 files, 4 obsolete files)

We do not restore the back/forward history of tabs from your previous session.

Note that the old SessionStore.js component from XUL Fennec is currently in the tree but is not built or packaged because it does not work in Native Fennec (bug 696203).
Assignee: nobody → bnicholson
Priority: -- → P2
Please make sure the use case described in Bug 656062, particularly Bug 656062 comment 2, works properly when this is done.
Attached patch WIP patch v1 (obsolete) — Splinter Review
This patch restores the tabs after a crash.  We still need to:
- restore favicons for each tab
- restore back/forward history
- reopen closed tabs
- change/remove 60 minute session timeout
Attached patch WIP patch v2 (obsolete) — Splinter Review
back/forward history added
Attachment #580249 - Attachment is obsolete: true
Attachment #581062 - Flags: feedback?(mark.finkle)
Some questions regarding the existing implementation:

Why do we delete the session information after an hour?
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/components/SessionStore.js#97

If we still want to delete session information after a crash, how can we differentiate between a crash and being killed by Android? As discussed in CoJaBo's bugs, we probably don't want to delete the session unless we have an actual crash, if at all.

Here,
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/bindings/browser.js#455
We send the session history on DOMContentLoaded, not on pageshow, so we won't necessarily have the correct history index after restoration.

Also, in onTabSelect,
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/components/SessionStore.js#436
we don't save the state after switching tabs, meaning the selected tab after restoration will not necessarily match the tab we were looking at.

Were these implemented like this intentionally, for performance reasons?
Comment on attachment 581062 [details] [diff] [review]
WIP patch v2

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>-    // Broadcast a UIReady message so add-ons know we are finished with startup
>-    let event = document.createEvent("Events");
>-    event.initEvent("UIReady", true, false);
>-    window.dispatchEvent(event);

Keep

>diff --git a/mobile/android/components/SessionStore.js b/mobile/android/components/SessionStore.js
>+      case "pageshow": {
>+        let browser = aEvent.currentTarget;
>+        this.onTabLoad(window, browser);
>+        break;
>+      }
>+      case "DOMContentLoaded": {
>+        let browser = aEvent.currentTarget;
>+        this.onContentLoaded(window, browser);
>+        break;
>+      }

We talked about moving to only use pageshow and check aEvent.persisted to see if we are just moving through bf-cache

>           let tabData = tabs[i];
>+          let isSelected = i + 1 == selected;
>+          let currentEntry = tabData.entries[tabData.index - 1];

currentEntry -> entry

Note: the reason we used the funky "select all tabs until we it the one that is really selected" was due to the XUL deck not liking having nothing selected. So we tricked it and always had a tab selected, even if it was the wrong one. Eventually we looped to the real selected tab and could stop the trick.

>-          let params = { getAttention: false, delayLoad: true };
>+          let params = { selected: isSelected, delayLoad: !isSelected, title: currentEntry.title };
>+          let tab = window.BrowserApp.addTab(currentEntry.url, params);
> 
>-          // We must have selected tabs as soon as possible, so we let all tabs be selected
>-          // until we get the real selected tab. Then we stop selecting tabs. The end result
>-          // is that the right tab is selected, but we also don't get a bunch of errors

Yeah, this comment sums it up. As loong as life is better now, we can avoid it, but make sure we can.

>+          // Make sure the browser has its session data for the delay reload
>+          if (i + 1 != selected) {

if (!isSelected)  ?

>-            // Recreate the thumbnail if we are delay loading the tab
>-            let canvas = tab.chromeTab.thumbnail;
>-            canvas.setAttribute("restored", "true");
>-            canvas.removeAttribute("empty");

How do we handle the thumbnail now? Oh, we won't need to until Sriram lands the "use thumbnails in tab menu" patches
Attachment #581062 - Flags: feedback?(mark.finkle) → feedback+
Blocks: 711572
Blocks: 711574
Blocks: 711578
Attachment #581062 - Attachment is obsolete: true
Attachment #582472 - Flags: review?(mark.finkle)
Attachment #582473 - Flags: review?(mark.finkle)
Attachment #582474 - Flags: review?(mark.finkle)
Comment on attachment 582472 [details] [diff] [review]
(1/3) tab/session history restore

This looks good. The Java code is likely bittrotted alittle.
Attachment #582472 - Flags: review?(mark.finkle) → review+
Comment on attachment 582473 [details] [diff] [review]
(2/3) undo close tabs

I want to try and avoid sending a new "Tab:ClosedChange" message and see if we can add this data to an existing message, like Tab:Added and/or TabClosed
Comment on attachment 582474 [details] [diff] [review]
(3/3) save restore state in bundle

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>-    if ("arguments" in window && window.arguments[0])
>-      uri = window.arguments[0];
>+    let shouldRestoreSession = false;

shouldRestoreSession -> shouldRestore

>+    if ("arguments" in window) {
>+      if (window.arguments[0])
>+        uri = window.arguments[0];
>+      shouldRestoreSession = window.arguments[1];
>+    }

We need to check for window.arguments[1] before attempting to use it.

        if (window.arguments.length == 2)
          shouldRestore = window.arguments[1];

>diff --git a/mobile/android/components/BrowserCLH.js b/mobile/android/components/BrowserCLH.js
>-function openWindow(aParent, aURL, aTarget, aFeatures, aArgs) {
>-  let argString = null;
>-  if (aArgs && !(aArgs instanceof Ci.nsISupportsArray)) {
>-    argString = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
>-    argString.data = aArgs;
>+function openWindow(aParent, aURL, aTarget, aFeatures, aUrl, aSessionRestore) {
>+  let argsArray = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
>+  let urlString = null;
>+  let sessionRestoreBool = Cc["@mozilla.org/supports-PRBool;1"].createInstance(Ci.nsISupportsPRBool);
>+
>+  if (aUrl) {
>+    urlString = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
>+    urlString.data = aUrl;
>   }
>+  sessionRestoreBool.data = aSessionRestore;
> 
>-  return Services.ww.openWindow(aParent, aURL, aTarget, aFeatures, argString || aArgs);
>+  argsArray.appendElement(urlString, false);
>+  argsArray.appendElement(sessionRestoreBool, false);
>+  return Services.ww.openWindow(aParent, aURL, aTarget, aFeatures, argsArray);
> }

I'd be happier if you sent aArgs as a JS array, and converted it to an nsISupportsArray. Kinda like this:
http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#260

>   handle: function fs_handle(aCmdLine) {
>     let urlParam = "about:home";
>+    let sessionRestore = false;
>     try {
>         urlParam = aCmdLine.handleFlagWithParam("remote", false);
>+        sessionRestore = aCmdLine.handleFlag("sessionrestore", false);
>     } catch (e) {
>       // Optional so not a real error
>     }

Use a second try/catch so wemake sure we actually attempt to handle the "sessionrestore" flag

>       } else {
>-        browserWin = openWindow(null, "chrome://browser/content/browser.xul", "_blank", "chrome,dialog=no,all", urlParam);
>+        browserWin = openWindow(null, "chrome://browser/content/browser.xul", "_blank", "chrome,dialog=no,all", urlParam, sessionRestore);

Given the changes to openWindow to handle a string or an array, I'd rather see this as:

          browserWin = openWindow(null, "chrome://browser/content/browser.xul", "_blank", "chrome,dialog=no,all", [urlParam, sessionRestore]);
Attachment #582474 - Flags: review?(mark.finkle) → review-
Comment on attachment 582473 [details] [diff] [review]
(2/3) undo close tabs


General nit: We should be referring to this feature as "Undo Closed Tab" not "Undo Close Tab"

I only mention that cause it affects your variable names and string entities, as well as actual strings. I only really care about the strings themselves.

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+            } else if (event.equals("Tab:ClosedChange")) {
>+                sUndoCloseTabEnabled = message.getBoolean("buttonEnabled");

I'd rather see the messaged called "Tab:CanUndo" and the data be an boolean called "allow".

>+    public boolean undoCloseTab() {
>+        Log.i(LOGTAG, "Undo close tab requested");

Let's drop this for checkin

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!ENTITY undo_close_tab "Undo Close Tab">

"Undo Closed Tab"

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

> var BrowserApp = {

>+  _closedTabButtonEnabled: false,

I really would like to kill this

>+  updateClosedTabCount: function updateClosedTabCount(closedTabCount) {

And move this to SessionStore.js as a private method

>+  undoCloseTab: function undoCloseTab(aTab) {

No data is sent. No need for aTab

>+    } else if (aTopic == "Tab:UndoClose") {
>+      this.undoCloseTab(this.getTabForId(parseInt(aData)));

No data is sent, right?

>diff --git a/mobile/android/components/SessionStore.js b/mobile/android/components/SessionStore.js

>     if (!aNoNotification)
>       this.saveStateDelayed();
>+
>+    // update the browser with the number of closed tabs
>+    aWindow.BrowserApp.updateClosedTabCount(this.getClosedTabCount(aWindow));

This is in onTabRemove. We only need to do this if count == 1 right?

>     // Put back the extra data
>     tab.browser.__SS_extdata = closedTab.extData;
> 
>+    // update the browser with the number of closed tabs
>+    aWindow.BrowserApp.updateClosedTabCount(this.getClosedTabCount(aWindow));

This is in undoCloseTab. We only need to send now if count == 0 right?

>           tab.browser.__SS_extdata = tabData.extData;
>           self._restoreHistory(tabData, tab.browser.sessionHistory);
>         }
> 
>+        // recover closed tabs data
>+        self._windows[window.__SSID].closedTabs = data.windows[0].closedTabs;
>+        window.BrowserApp.updateClosedTabCount(self.getClosedTabCount(window));

Yeah, we need to do this all the time

How is this patch different than bug 701725? I think this would be a good first patch for that bug. We do not need "undo closed tab" for the initial sessionrestore landing. It might be better to keep them separate. Feel free to move this patch to bug 701725.
Attachment #582473 - Flags: review?(mark.finkle) → review-
https://hg.mozilla.org/mozilla-central/rev/9d3e6a6901f7
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Whiteboard: [fennec-aurora]
This patch waits for the "sessionstore-windows-restored" before adding tabs or restoring the session in case of a failure. I was getting a NPE in GeckoApp.java with this patch at getGeckoViewportMetrics (likely a race condition that assumes a tab exists), so that's what that null check is for.
Attachment #583629 - Flags: review?(mark.finkle)
Attached patch save restore state in bundle (obsolete) — Splinter Review
Attachment #582474 - Attachment is obsolete: true
Attachment #583630 - Flags: review?(mark.finkle)
Attachment #583630 - Flags: feedback?(blassey.bugs)
reopened until the rest of the patches are landed
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 583629 [details] [diff] [review]
follow-up patch: handle session restore failures

Just noticed some minor typos. I changed:

+  restore: function init(uri) {
to
+  restore: function restore(uri) {

and

+  observe: function(aSubject, aTopic, aData) {
to
+  observe: function observe(aSubject, aTopic, aData) {
Attachment #583630 - Flags: feedback?(blassey.bugs) → feedback+
So what's the story for a yesterdays nightly build? When I open multiple tabs and explicitly close Firefox via Quit, those tabs are not getting restored when I start Firefox again. The start page gets loaded but there is no link which could be used to restore the last session.
Comment on attachment 583630 [details] [diff] [review]
save restore state in bundle

> function openWindow(aParent, aURL, aTarget, aFeatures, aArgs) {

I'm not exactly happy with this function impl yet. I know it's hard to test the case where aArgs could be null, but I think we need to impl the function as if it could be null.

If aArgs is null, we want to pass null to Services.ww.openWindow and then browser.js would get a null window.arguments. browser.js is already setup to handle that condition.

>-  let argString = null;
>-  if (aArgs && !(aArgs instanceof Ci.nsISupportsArray)) {
>-    argString = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
>-    argString.data = aArgs;
>+  let argsArray = Cc["@mozilla.org/supports-array;1"].createInstance(Ci.nsISupportsArray);

Set argsArray to null here and init it to a nsISupportsArray inside the | if (aArgs) | block below

>+  let urlString = null;
>+  let sessionRestoreBool = Cc["@mozilla.org/supports-PRBool;1"].createInstance(Ci.nsISupportsPRBool);

Move these to inside the | if (aArgs) | block

>+
>+  if (aArgs[0]) {

    if (aArgs) {

>+    urlString = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
>+    urlString.data = aArgs[0];
>   }
>+  sessionRestoreBool.data = aArgs[1];

Move this into the aArgs block

>+  argsArray.AppendElement(urlString, false);
>+  argsArray.AppendElement(sessionRestoreBool, false);

Move these to inside the | if (aArgs) |  block

>+  return Services.ww.openWindow(aParent, aURL, aTarget, aFeatures, argsArray);

At this point we should have argsArray as null or as a filled array.

Does this make sense to you?

r- only to fix up this method
Attachment #583630 - Flags: review?(mark.finkle) → review-
Comment on attachment 583629 [details] [diff] [review]
follow-up patch: handle session restore failures

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js


>+    // restore the previous session
>+    if (shouldRestore || ss.shouldRestore())
>+      SessionStore.restore(uri);

I know this is not my normal opinion on code, but I'd rather have this helper code integrated into BrowserApp.startup

http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/browser.js#403

Instead of "dummyCleaner" you can call it "sessionRestorer"


>+    else {
>+      this.addTab(uri);
>+      this.startupFinished();

>+  startupFinished: function startupFinished() {

Yes, we keep this

>+var SessionStore = {

Remove this


r- just to see a new patch
Attachment #583629 - Flags: review?(mark.finkle) → review-
Hardware: All → ARM
Attachment #583630 - Attachment is obsolete: true
Attachment #585946 - Flags: review?(mark.finkle)
Attachment #585946 - Flags: approval-mozilla-aurora?
Comment on attachment 585946 [details] [diff] [review]
save restore state in bundle

[Triage Comment]
Please re-nominate once r+'d, landed on m-c, and baked for a day.
Attachment #585946 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Comment on attachment 585946 [details] [diff] [review]
save restore state in bundle

> BrowserCLH.prototype = {

>+    try {
>+        restoreSession = aCmdLine.handleFlag("restoresession", false);
>+    } catch (e) {
>+      // Optional so not a real error
>+    }

      } catch (e) { /* Optional */ }

LGTM. I bitrot the BrowserCLH part though with a recent landing on inbound. Sorry.
Attachment #585946 - Flags: review?(mark.finkle) → review+
tracking-fennec: --- → 11+
Whiteboard: [fennec-aurora]
(In reply to Brian Nicholson (:bnicholson) from comment #28)
> Restore state landed on mozilla-inbound:
> http://hg.mozilla.org/integration/mozilla-inbound/rev/af32a0da45c9

One of three changesets backed out of inbound due to test coalescing making it hard to identify which caused the native android test failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1927c7905f5e
There was a typo in browser.js (s/uri/url). Fix passes try tests:
https://tbpl.mozilla.org/?tree=Try&rev=324f883c5e89

Pushed fix to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/03d77ca70f27
https://hg.mozilla.org/mozilla-central/rev/03d77ca70f27
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Comment on attachment 582472 [details] [diff] [review]
(1/3) tab/session history restore

[Approval Request Comment]
Restoring from session is a core feature we want for release. Parity with other Mozilla browsers too.

This has been baking on m-c for a while and has had the fallout issues fixed.
Attachment #582472 - Flags: approval-mozilla-aurora?
Comment on attachment 585946 [details] [diff] [review]
save restore state in bundle

This is now ready for moving to aurora.
Attachment #585946 - Flags: approval-mozilla-aurora- → approval-mozilla-aurora?
Mark - Should I be able to see the fix in today's or tomorrow's build?
(In reply to Gabriela from comment #34)
> Mark - Should I be able to see the fix in today's or tomorrow's build?

You can see it in today's Nightly.

Verified on Nightly (12.0a1)
Samsung Nexus S (Android 4.0.3)
Mozilla/5.0 (Android; Linux armv7l; rv:12.a01) Gecko/20120113 Firefox/12.0a1 Fennec/12.0a1
Status: RESOLVED → VERIFIED
Flags: in-litmus?(fennec)
Whiteboard: [QA+]
As I understand, session is only restored after a crash (or app is killed)?

I would very much like a way to be able to open the tabs from a previous visit, after I quit Fennec. Is there a way or bug filed about this?
Depends on: 718240
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #36)
> As I understand, session is only restored after a crash (or app is killed)?
> 
> I would very much like a way to be able to open the tabs from a previous
> visit, after I quit Fennec. Is there a way or bug filed about this?

See bug 712970
Comment on attachment 582472 [details] [diff] [review]
(1/3) tab/session history restore

[Triage Comment]
Mobile only - approved for Aurora.
Attachment #582472 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #585946 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Testcase added: https://litmus.mozilla.org/show_test.cgi?id=44977
Flags: in-litmus?(fennec) → in-litmus+
Whiteboard: [QA+]
Blocks: 810981
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: