1193386 - Wrong tab selected after restoring the browser

Status: RESOLVED FIXED

(Firefox for iOS :: Browser, defect)

Description

[Brian Nicholson (:bnicholson)]

STR:
1) Open a new tab.
2) Select another tab.
3) Close and reopen the browser.

After these steps, the tab from step 1 is selected. We should flush the session when the selected tab changes.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/901

Comment 2

[Richard Newman [:rnewman]]

I see that this will fix the bug, but it'll also do a full DB write and a full session store flush, with thumbnails, every time you switch tabs. That seems like a bad idea.

Can we just not persist isSelected in SavedTab, and instead aggressively persist an index in `prefs`?

Comment 3

[Brian Nicholson (:bnicholson)]

(In reply to Richard Newman [:rnewman] from comment #2)
> I see that this will fix the bug, but it'll also do a full DB write and a
> full session store flush, with thumbnails, every time you switch tabs. That
> seems like a bad idea.
> 
> Can we just not persist isSelected in SavedTab, and instead aggressively
> persist an index in `prefs`?

A couple points worth noting:
1) Selecting a tab is relatively infrequent operation versus loading a new URL. We already call storeChanges in the latter case.
2) storeChanges is much less expensive with bug 1187176 fixed.

Right now, storeChanges actually stores the selected tab as a boolean on the SavedTab, rather than an index. This is slightly safer since we don't need to worry about the index getting out of sync for whatever reason (i.e., one of the tabs has no history and is skipped). It also feels a bit messy to have our session data spanned across three sources: the keyed archive, screenshots on disk, and now an index in prefs. Also worth noting that we do a full session flush for each of these events (and more!) on Android.

I forgot about the DB flush, though. So how do you feel about a bool passed to storeChanges that allows us to bypass the `self.profile.storeTabs` call?

Comment 4

[Richard Newman [:rnewman]]

> 1) Selecting a tab is relatively infrequent operation versus loading a new
> URL. We already call storeChanges in the latter case.

Yeah, but we also do the thumbnail -> browser zoom at the same time on switch, so anything we can do to save cycles is worth it.

> 2) storeChanges is much less expensive with bug 1187176 fixed.

Yup.

> Right now, storeChanges actually stores the selected tab as a boolean on the
> SavedTab, rather than an index. This is slightly safer since we don't need
> to worry about the index getting out of sync for whatever reason (i.e., one
> of the tabs has no history and is skipped).

Yup, I quite agree.

> I forgot about the DB flush, though. So how do you feel about a bool passed
> to storeChanges that allows us to bypass the `self.profile.storeTabs` call?

I'd split it into two methods (one of which depends on the other) and just call the one you want.

This gets even better if we don't store thumbnails -- I don't mind at all if we write an updated set of open tabs, so long as we're not writing thumbnails or touching the DB!

Comment 5

[Brian Nicholson (:bnicholson)]

(In reply to Richard Newman [:rnewman] from comment #4)
> This gets even better if we don't store thumbnails -- I don't mind at all if
> we write an updated set of open tabs, so long as we're not writing
> thumbnails or touching the DB!

Yeah, we only write thumbnails that have changed, so there won't be many thumbnail writes. We should flush new thumbnails, though, because they're generated only when going to the tab tray. If we don't save them here, the screenshot from the previous tab may not be persisted at all.

Comment 6

[Brian Nicholson (:bnicholson)]

Updated.

Comment 7

[Stephan Leroux [:sleroux]]

Comment on attachment 8646428 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/901

LGTM

Comment 8

[Brian Nicholson (:bnicholson)]

https://github.com/mozilla/firefox-ios/commit/bdcd0c7a8c349931a54ed25fb5cebf1be214794c