1017912 - Restore tab in correct position when undoing close tab

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

Right now we basically just add back a new tab with the URL of the tab you just closed. A few things we can do to improve this:

* Keep track of the tab order, to insert the tab back in the correct place.
* Restore session history. We already store this, we would just need to add it back. I also see us storing docshell info, can we actually restore the docshells somehow.

See also: bug 732752.

Comment 1

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #0)

> * Restore session history. We already store this, we would just need to add
> it back. I also see us storing docshell info, can we actually restore the
> docshells somehow.

Doh, we already do this:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#847

Morphing this bug to just be about the tab order.

Comment 2

[:Margaret Leibovic]

Attachment: WIP - Restore tab in correct position when undoing close tab

This patch just tacks some extra functionality on top of our current mess of a tabs system.

At first glance it seemed to work, but I ran into problems when closing a tab after restoring another tab. I'm worried that it would be tricky to get this logic right. I think this is a nice-to-have for the undo-close-tab feature, but I would want to make sure we're not breaking anything, since adding/closing tabs is pretty core functionality.

Comment 3

[Brian Nicholson (:bnicholson)]

Comment on attachment 8431979 [details] [diff] [review]
WIP - Restore tab in correct position when undoing close tab

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

::: mobile/android/base/Tabs.java
@@ +195,5 @@
>              lazyRegisterBookmarkObserver();
>              mTabs.put(id, tab);
> +
> +            if (tabIndex > -1) {
> +                mOrder.add(tabIndex, tab);

I'm assuming this feature will be used only for undoing the last closed tab via from super toast? The closed indices are always relative to the tabs in the array at that time, so restoring tabs out-of-order would break things if we did this on the closed tabs panel. For instance, if you closed tabs at indices 2 and then 1, but then restored the tab at index 2 first, you'd get an array out of bounds exception here.

::: mobile/android/chrome/content/browser.js
@@ +858,5 @@
>      }
>      return null;
>    },
>  
> +  getTabIndexForBrowser: function getTabIndexForBrowser(aBrowser) {

Note that private and normal tabs are intertwined together in the same array. I think this *should* be OK since we're using the same array to get and restore the tab indices, but just pointing this out as something to keep in mind as you test this.

::: mobile/android/components/SessionStore.js
@@ +445,5 @@
>      tabData.index = aHistory.index;
>      tabData.attributes = { image: aBrowser.mIconURL };
>      tabData.desktopMode = aWindow.BrowserApp.getTabForBrowser(aBrowser).desktopMode;
>      tabData.isPrivate = aBrowser.docShell.QueryInterface(Ci.nsILoadContext).usePrivateBrowsing;
> +    tabData.tabIndex = aWindow.BrowserApp.getTabIndexForBrowser(aBrowser);

If I'm correct that this will only work for tabs reopened via the super toast, that means we only need to hold at most one index (corresponding to the last closed tab being shown by the toast). Storing these indices any longer than that means we're just holding onto a bunch of useless data, so I'd recommend against having every tab hold its index. Instead, the index could be a short-lived value that we keep only as long as the toast is shown. Giving this index to the super toast button callback might be the best option.

Comment 4

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #3)
> Comment on attachment 8431979 [details] [diff] [review]
> WIP - Restore tab in correct position when undoing close tab
> 
> Review of attachment 8431979 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/Tabs.java
> @@ +195,5 @@
> >              lazyRegisterBookmarkObserver();
> >              mTabs.put(id, tab);
> > +
> > +            if (tabIndex > -1) {
> > +                mOrder.add(tabIndex, tab);
> 
> I'm assuming this feature will be used only for undoing the last closed tab
> via from super toast? The closed indices are always relative to the tabs in
> the array at that time, so restoring tabs out-of-order would break things if
> we did this on the closed tabs panel. For instance, if you closed tabs at
> indices 2 and then 1, but then restored the tab at index 2 first, you'd get
> an array out of bounds exception here.

Yeah, I think it makes sense to just do this for the undo toast. It's too confusing to figure out what should happen for restoring tabs that were closed who know how long ago.

Comment 5

[:Margaret Leibovic]

Comment on attachment 8431979 [details] [diff] [review]
WIP - Restore tab in correct position when undoing close tab

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

::: mobile/android/chrome/content/browser.js
@@ +931,5 @@
>  
>      let newTab = new Tab(aURI, aParams);
> +
> +    if (typeof aParams.tabIndex == "number") {
> +      this._tabs.splice(aParams.tabIndex, 1, newTab);

/me shakes fist at JS array methods... the second parameter here indicates how many items to *remove*, so it should be 0.

Comment 6

[:Margaret Leibovic]

Attachment: Restore tab in correct position when undoing close tab

This new version of the patch just holds onto the index of the most recently closed tab in memory in SessionStore.js, instead of making it part of the tab data. I kinda hacked the event detail to pass the index along from browser.js, let me know what you think of that.

Also, I found that my incorrect use of splice was what caused the bugs I was running into before. With this new version of the patch I haven't been able to uncover any incorrect behavior.

Comment 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 8434458 [details] [diff] [review]
Restore tab in correct position when undoing close tab

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

This looks fine, though I'm starting to wonder if holding onto the closed tab without actually closing it might make some of these bugs easier. It would help with an "undo all" action like you mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=817716#c29. For this bug, restoring a stubbed tab would "just work" since it will still be in the correct position in the list. It would also allow us to restore correct tab positions from closed UI panel, whereas an index-based solution does not.

Comment 8

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #7)
> Comment on attachment 8434458 [details] [diff] [review]
> Restore tab in correct position when undoing close tab
> 
> Review of attachment 8434458 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine, though I'm starting to wonder if holding onto the closed
> tab without actually closing it might make some of these bugs easier. It
> would help with an "undo all" action like you mentioned in
> https://bugzilla.mozilla.org/show_bug.cgi?id=817716#c29. For this bug,
> restoring a stubbed tab would "just work" since it will still be in the
> correct position in the list. It would also allow us to restore correct tab
> positions from closed UI panel, whereas an index-based solution does not.

Hm, maybe we can think about this as we work on the recent tabs panel for Fx33. We would need to think about how many stubbed tabs we would want to keep around, and what the memory impact of that would be compared to the session store data we hang onto. Also, if we fix bug 1015516, we'd be able to persist those closed tabs between sessions, which we wouldn't be able to do with the stubbed tab approach (unless we did both things, but then that would take up even more memory).

Comment 9

[:Margaret Leibovic]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0dc174324955

Comment 12

[Teodora Vermesan (:TeoVermesan)]

Open 3 pages in normal/private tabs: google.com, mail.yahoo.com and news.google.com and close the second tab: mail.yahoo.com, undo it, the tab will be restored between google.com and news.google.com, to its original position, so:
Verified fixed on:
Device: LG Nexus 4 (Android 4.4.2)
Build: Firefox for Android 33.0a1 (2014-07-02)

Comment 13

[Teodora Vermesan (:TeoVermesan)]

Will this be uplifted to Aurora?

Comment 14

[Carsten Book [:Tomcat]]

(In reply to Teodora Vermesan (:TeoVermesan) from comment #13)
> Will this be uplifted to Aurora?

not yet, i guess will be automatically uplifted to aurora during the merge but so far it was not uplifted also i see no uplift request for aurora for now

Comment 15

[Teodora Vermesan (:TeoVermesan)]

Marking as verified based on comment 12.

Comment 16

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Is it possible to get an animation to ease in the removed tab? I think we talked about this before but I really think it'd feel awesome. I can split this off into a separate bug.

Comment 17

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

^ https://bugzilla.mozilla.org/show_bug.cgi?id=1034649

Comment 18

[:Margaret Leibovic]

(In reply to Teodora Vermesan (:TeoVermesan) from comment #13)
> Will this be uplifted to Aurora?

This does not need to be uplifted to 32 because bug 701725 was disabled.

(In reply to Anthony Lam (:antlam) from comment #16)
> Is it possible to get an animation to ease in the removed tab? I think we
> talked about this before but I really think it'd feel awesome. I can split
> this off into a separate bug.

Let's file a follow-up about that.

Comment 19

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Margaret Leibovic from comment #18)
> Let's file a follow-up about that.

Got one right here! :) 

https://bugzilla.mozilla.org/show_bug.cgi?id=1034649