732752 - Load session history when 'loading tabs from last time'

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

About Home content lists the tabs open in the previous session. Tapping one of them will open the URL in a new tab. We pull the data from the session restore data, iirc.

We should open the full session history instead of only the last open URL. This gives us better session restore capabilities. It also gives us a separate path for restoring the last browsing session after a crash or force kill.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: patch

Adds a new method to SessionStore, restoreLastTabs(), which takes an array of tab indexes.

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: patch v2

minor cleanup

Comment 3

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

Comment on attachment 604575 [details] [diff] [review]
patch v2

I am changing your design a little. When we want to restore all tabs, we should just use the existing ss.restoreLastSession(...) method.

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

>+    public void restoreTabs(ArrayList<Integer> indexes) {

We only need to pass 1 index, which can be -1 for all tabs

restoreTabs(ArrayList<Integer> indexes)  -> restoreTab(Integer index)

>+        String indexArray = new JSONArray(indexes).toString();
>+        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Session:RestoreTabs", indexArray));

"Session:RestoreTabs" -> "Session:Restore"

Let's use better JSON in the message. Drop the array (it's only all tabs or one tab now) and send something like "{ tab: index }" or null, where null means all tabs:

  JSONObject json = new JSONObject();
  json.put("tab", index);
  GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Session:Restore", json));

(I think that would work)

>-        final ArrayList<String> lastTabUrlsList = new ArrayList<String>();
>+        final ArrayList<Integer> lastTabIndexes = new ArrayList<Integer>();

You should not need the array at all now (EDIT: I am wrong)

>                     container.findViewById(R.id.last_tab_row).setOnClickListener(new View.OnClickListener() {
>                         public void onClick(View v) {
>-                            GeckoApp.mAppContext.loadUrlInTab(url);
>+                            ArrayList<Integer> indexArray = new ArrayList<Integer>();
>+                            indexArray.add(tabIndex);
>+                            restoreTabs(indexArray);

no need for the array:
  restoreTab(tabIndex)

>-        int numLastTabs = lastTabUrlsList.size();
>+        int numLastTabs = lastTabIndexes.size();
>         if (numLastTabs > 0) {

OK. I guess you do need the array

>                         openAll.setOnClickListener(new LinkTextView.OnClickListener() {
>                             public void onClick(View v) {
>-                                for (String url : lastTabUrlsList)
>-                                    GeckoApp.mAppContext.loadUrlInTab(url);
>+                                restoreTabs(lastTabIndexes);

Just call the raw event:
  GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Session:Restore", null));

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

>+    } else if (aTopic == "Session:RestoreTabs") {
>+      let ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
>+      ss.restoreLastTabs(JSON.parse(aData));

  if (aData)
    ss.restoreTab(0, JSON.parse(aData).tab);
  else
    ss.restoreLastSession(false, true);

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

>+  /**
>+   * Restore tabs and their history from the previous session
>+   * @param aIndexes array of tab indexes to restore
>+   */
>+  void restoreLastTabs(in jsval aIndexes);

We only need this method to operate on a single tab now. Let's make this a little more like the xxxCloseTab APIs:

  void restoreTab(in integer aWindow, in integer aTab);

We can ignore the 'aWindow' param for now

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

>+  _getLastSessionData: function _getLastSessionData(aArgs) {

aArgs -> aCallback

>+  _restoreTab: function _restoreLastTabsHelper(aTabData, aSelected) {

_restoreLastTabsHelper -> _restoreTab


>+  restoreLastTabs: function ss_restoreLastTabs(aIndexes) {

    restoreTab: function ss_restoreLastTabs(aWindow, aTab) {

--------------------------

* Let me know if this sounds like a crazy change.
* As I was reviewing this and remembering the "about:home" skipping issues we talked about, I considered falling back to a simpler idea: tapping a single item in about:home would _not_ restore all the session. But we could make "Open all tabs from last time" be like desktop's "Restore previous session" - only that would restore all the session.

It removes the issue of the individual tab items, simpler, mimics desktop firefox better and removes almost all the code changes in the patch since it would only use the existing "ss.restoreLastSession" code.

What are your thoughts on this new, simpler idea?

Comment 4

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Comment on attachment 604575 [details] [diff] [review]
> patch v2
> 
> I am changing your design a little. When we want to restore all tabs, we
> should just use the existing ss.restoreLastSession(...) method.
> 

This was what I was going to do at first, but there are a number of things we want to do differently for automatic vs manual restores:
1) We originally didn't want to restore about: tabs with the session, so we would need a way to differentiate between automatic vs manual restores. Allowing the restoreTabs() function to specify exactly which tabs to restore meant we wouldn't have to duplicate the logic to exclude about: tabs in both Java and Gecko.
2) Another issue related to not restoring about: tabs would be knowing which tab to select when clicking "restore all tabs". It's possible for the last selected tab to have been an about: tab, so we would have to handle that special case in restoreLastSession(). This was simplified in restoreTabs() by just selecting the last restored tab.
3) I may be wrong, but we probably don't want to notify observers for session restore events if we're doing a manual restore. If the user does a manual restore, it means session restore can happen at any time during the browser session and there can be multiple restorations, which may violate the assumptions of the observers.

> * As I was reviewing this and remembering the "about:home" skipping issues
> we talked about, I considered falling back to a simpler idea: tapping a
> single item in about:home would _not_ restore all the session. But we could
> make "Open all tabs from last time" be like desktop's "Restore previous
> session" - only that would restore all the session.
> 
> It removes the issue of the individual tab items, simpler, mimics desktop
> firefox better and removes almost all the code changes in the patch since it
> would only use the existing "ss.restoreLastSession" code.
> 
> What are your thoughts on this new, simpler idea?

So does this mean we don't show about: tabs in "tabs from last time", but do restore them when restoring all tabs? This would simplify the patch since we would no longer have problems #1 and #2 listed above (though we'd still have to figure out something for #3 if the notifications are an issue). Desktop Firefox does restore the session for individual tabs from about:sessionrestore, but it's probably not a big deal to be different here.

Unfortunately, this brings back the problem of building about:tabs when doing a session restore. That is, the user:
* opens Fennec
* clicks "restore all tabs" (resulting in the previous session + an extra about:home tab)
* quits Fennec
* opens Fennec again
* clicks "restore all tabs" (resulting in the previous session + an extra about:home tab)
etc., where each quit/restore produces an additional about:home tab. This could be an annoyance if people rely on this feature to resume from where they left off.

If we go with this approach, how about we change "Restore all tabs from last time" -> "Restore last session"?

Comment 5

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

I talked to Brian a bit last week. This bug needs to be scoped better. I am taking it off the blocking list and will get Ian and Madhava to provide some input.

Comment 6

[Kevin Brosnan [Ex-Mozilla]]

miniusing to get off the radar

Comment 8

[:Margaret Leibovic]

I want to try to revive this.

Comment 9

[:Margaret Leibovic]

Attachment: WIP

I didn't get very far on this, but wanted to upload what I have so far in case someone else wants to work on this.

I basically did the easy part, now we just need to actually restore the tabs :)

One tricky bit is that the recently closed tabs come from a message from gecko, and the tabs from last time come from SessionParser.

For the case of restoring a single tab, we need to match up the item the user clicked on with the actual tab that's stored in session history, and currently we don't store any data to help us do that. I was thinking of storing an index and just sending a message to gecko to restore the single tab (since when the user clicks on one tab, they probably want to see its contents, so stubbing it doesn't get us anything).

For the "open all" case, we could just go through the stored tabs and stub them all out. For the recently closed tabs, we would need to get more information from gecko to do this, but I think it should be straightforward to do this for tabs from last time with SessionParser, since this is what we actually do for automatic session restore.

Whether restoring one or many tabs, we should make sure to update the session store data so that we don't display them in the list anymore.

Comment 10

[Brian Nicholson (:bnicholson)]

Since fixing this bug means restoring the tab's history -- and not just its last URL -- we'll want to tinker with this logic, too: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/RecentTabsPanel.java?rev=01357a4a5326#314.

Comment 12

[:Margaret Leibovic]

Vivek told me he wants to take this bug, which is awesome!

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 14

[:Margaret Leibovic]

This should really get fixed. I don't think it's a P5.

Comment 15

[Brian Nicholson (:bnicholson)]

Dumping some comments from our IRC chat just to make things a bit more visible:

SessionStore.js has a _restoreHistory method [1], so we need to figure out a way to call that from the recent tabs panel.

To do that, here's what I'd do as a first try:
1) Add a JSONObject tabData argument to addRow().
2) Get the tab data using tab.getTabObject(), then pass along at [2] as your new addRow arg.
3) Change lines at [3] to send a message to Gecko instead of calling mNewTabsListener.
  ** For a final patch we should abstract this in an interface, but messaging Gecko directly will be easiest for a first iteration.
4) SessionStore.js can handle the message, calling _restoreHistory() for each tab with the data passed along.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#766
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/RecentTabsPanel.java#340.
[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/RecentTabsPanel.java#145

Comment 16

[Vivek Balakrishnan[:vivek]]

Attachment: session_restore.patch

Implemented restore entire history for tabs from recent tabs panel.

Comment 17

[Brian Nicholson (:bnicholson)]

Comment on attachment 8533422 [details] [diff] [review]
session_restore.patch

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

Haven't tried this out yet, but it looks pretty good so far.

::: mobile/android/base/home/RecentTabsPanel.java
@@ +79,5 @@
> +        } catch (JSONException e) {
> +            Log.e(LOGTAG, "JSON error", e);
> +        }
> +
> +        GeckoAppShell.sendRequestToGecko(new GeckoRequest("Session:RecentTab", json) {

By dropping the OnNewTabsListener interface and messing Gecko directly, we're moving control from BrowserApp into the tabs panel. The more coupled to Gecko a class is, the less modular it becomes. OTOH, this class already has Gecko messaging/listeners at [1], so some coupling is already present. I think I'm OK with this change, but let's see what Margaret says since she wrote much of this class.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/RecentTabsPanel.java#170

@@ +82,5 @@
> +
> +        GeckoAppShell.sendRequestToGecko(new GeckoRequest("Session:RecentTab", json) {
> +            @Override
> +            public void onResponse(NativeJSObject nativeJSObject) {
> +            }

Any empty onResponse handler means you probably don't want to be using GeckoRequest here. Instead, just use GeckoEvent:

GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Session:RecentTab", json.toString()));

::: mobile/android/components/SessionStore.js
@@ +85,5 @@
>          observerService.addObserver(this, "application-background", true);
>          observerService.addObserver(this, "ClosedTabs:StartNotifications", true);
>          observerService.addObserver(this, "ClosedTabs:StopNotifications", true);
>          observerService.addObserver(this, "last-pb-context-exited", true);
> +        Messaging.addListener(this.restoreTabsWithHistory.bind(this), "Session:RecentTab");

With the normal events API, use observerService.addObserver() like the lines above. You'll also need to add an entry in the observe() method in this file that calls restoreTabsWithHistory() when the event is received.

@@ +764,5 @@
> +    let window = Services.wm.getMostRecentWindow("navigator:browser");
> +    for (let i = 0; i < data.tabs.length; i++) {
> +      let tabData = JSON.parse(data.tabs[i]);
> +      let params = {
> +        selected: true,

There's only one selected tab, so we shouldn't be setting this to true for every tab being restored. How about we set selected to true for the last tab (i == tabCount - 1)?

@@ +774,5 @@
> +      this._restoreHistory(tabData, tab.browser.sessionHistory);
> +    }
> +
> +    // Empty return for Java.
> +    return {};

Then you can remove the empty return (unused for events).

Comment 18

[:Margaret Leibovic]

Comment on attachment 8533422 [details] [diff] [review]
session_restore.patch

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

I agree with everything bnicholson had to say. This is looking good, I like this approach! I'm okay with this being coupled to Gecko, since it's by nature pretty tightly coupled with session store, so I don't see us ever wanting to use this class in a non-Gecko environment.

I haven't tested this out myself yet, either. I can test a build with an updated patch.

::: mobile/android/base/home/RecentTabsPanel.java
@@ +90,4 @@
>      private static final class ClosedTab {
>          public final String url;
>          public final String title;
> +        public final String tabObject;

Nit: I would call this tabData (or just data) to be consistent with the "data" property we're using to create this.

@@ +250,5 @@
>          if (c == null || !c.moveToFirst()) {
>              return;
>          }
>  
> +        final List<String> datas = new ArrayList<String>();

Nit: Call this dataList, like you do up above (the word "data" is already plural, so it seems odd to see the word "datas" :)

Comment 19

[Vivek Balakrishnan[:vivek]]

Attachment: session_restore.patch

Addressed review comment from @Margaret and @bnicholson

An apk to test can be found at :
https://www.dropbox.com/s/qvhoyj5vjqj6hnq/fennec-37.0a1.en-US.android-arm.apk?dl=0

Comment 20

[Vivek Balakrishnan[:vivek]]

Attachment: session_restore.patch

Minor nit corrected

Comment 21

[Richard Newman [:rnewman]]

I think this might feed into Bug 1108426, which will mean flinging a whole stack of URLs into tab-open. Knowledge transfer or more work for vivek? :D

Comment 22

[Vivek Balakrishnan[:vivek]]

rnewman: I assigned myself to the bug 1108426.

Comment 23

[Brian Nicholson (:bnicholson)]

Comment on attachment 8534063 [details] [diff] [review]
session_restore.patch

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

Looks good to me!

::: mobile/android/components/SessionStore.js
@@ +85,5 @@
>          observerService.addObserver(this, "application-background", true);
>          observerService.addObserver(this, "ClosedTabs:StartNotifications", true);
>          observerService.addObserver(this, "ClosedTabs:StopNotifications", true);
>          observerService.addObserver(this, "last-pb-context-exited", true);
> +        observerService.addObserver(this, "Session:RecentTab", true);

Session:RecentTab -> Session:RestoreRecentTabs, so the message is an action. (I was going to suggest just Session:RestoreTabs at first, but this could cause some confusion with the other Session:Restore observer.)

@@ +763,5 @@
>      return shEntry;
>    },
>  
> +  // This function iterates through a list of tab data restoring history for each of them.
> +  restoreTabsWithHistory: function ss_restoreTabsWithHistory(data) {

Please prefix with _ since this is a private method not exposed in the IDL.

@@ +955,5 @@
>          let lastEntry = tab.entries[tab.entries.length - 1];
>          return {
>            url: lastEntry.url,
> +          title: lastEntry.title || "",
> +          data: tab

We don't technically need to send the url/title anymore since this data can all be obtained from the tab object, but I guess it's fine to duplicate them to keep things simple on the Java end.

Comment 24

[:Margaret Leibovic]

Comment on attachment 8534063 [details] [diff] [review]
session_restore.patch

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

Looks good, thanks!

::: mobile/android/components/SessionStore.js
@@ +176,5 @@
>            window.closedTabs = window.closedTabs.filter(tab => !tab.isPrivate);
>          }
>          this._lastClosedTabIndex = -1;
>          break;
> +      case "Session:RecentTab":

Please put braces around this case statement, since you're declaring a variable inside.

Comment 25

[Vivek Balakrishnan[:vivek]]

Attachment: session_restore.patch

Minor nits mentioned in review+ comment addressed.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Can we please run this through Try first? :)

Comment 27

[Vivek Balakrishnan[:vivek]]

Try server logs:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=24f973b59b67

Comment 28

[Carsten Book [:Tomcat]]

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

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/65df46e2abc9