1352997 - Improve custom tab/web app tab selection logic

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement, P1)

Description

[Jan Henning [:JanH]]

While working on bug 1351739, I've noticed some problems that should be tackled in a subsequent step:
- Occasionally, the activities can get confused about which tab they should select when switching between different activities
- For easier restoring of state from the task switcher, I'm keeping the original intent around for custom tab/web app activities [1, 2], which means that when running onCreate we need to figure out whether we should open a new tab from the URL in the intent as usual, or whether there's some already existing tab that should be selected instead.

[1] At the moment the intent is reset in GeckoApp (https://dxr.mozilla.org/mozilla-central/rev/38894655c89e68bcd8f45d31a0d3005f2c2b53db/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1285) when onCreate runs with a savedInstanceState present.
[2] Especially for custom tabs, since the intent there can contain all sorts of customisations.

Comment 1

[Jan Henning [:JanH]]

Attachment: Bug 1352997 - Part 1 - Register GeckoApp's onTabsChangedListener earlier.

For BrowserApp we want to switch the last selected tab tracking to use tab selection events instead, so we need to register the listener earlier in order to catch the initial selection of the startup tab as well.

Review commit: https://reviewboard.mozilla.org/r/128040/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128040/

Comment 2

[Jan Henning [:JanH]]

Attachment: Bug 1352997 - Part 3 - Re-implement tracking of last selected tab for BrowserApp.

Currently, we basically take a snapshot of the currently selected tab when pausing an activity and then later re-select that tab ID when switching back from another activity within our application. In practice, this doesn't seem entirely fool-proof, so when switching between our normal UI (BrowserApp) and custom tabs or web apps we can eventually end up with the wrong tab being selected in the wrong activity.

In this part, we'll rip out the current code and replace it by a new implementation for BrowserApp - following parts will then cover custom tabs and web apps.

As BrowserApp is our normal tabbed browsing interface, we can simply track all tab switches for BROWSING-type tabs as they happen, which ensures that our data is always up-to-date. Because tab IDs are not unique across Gecko restarts, while the savedInstanceState can carry even across (OOM-)kills, we now additionally also store the respective tab object's hash to make sure that the tab we're trying to select is really the same one it was when the activity was last running.

For BrowserApp this is less important because on a full startup, this behaviour will be overridden by session restore anyway (although we can still hit it if only BrowserApp gets destroyed while Gecko keeps running, or if BrowserApp is launched after some other activity has already loaded Gecko), but it'll be quite relevant for web apps and custom tabs which don't have that benefit.

As it stands, this patch temporarily break behaviour around activity restoring for custom tabs/web apps, but tearing the old implementation out in one go was easier and the patch needs to be split somewhere.

Review commit: https://reviewboard.mozilla.org/r/128074/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128074/

Comment 3

[Jan Henning [:JanH]]

Attachment: Bug 1352997 - Part 4 - Remember the tab selected by session restoring if somebody other than BrowserApp is starting up first.

The first activity to run triggers Gecko startup and therefore session restore. Since the selected tab stored in the session file is only of interest for BrowserApp, we need to store it somewhere safe if some other activity (e.g. custom tab/web app) starts up first.

This is because currently everything needs to share the same Gecko browser window, so those other activities selecting a tab of their own when starting up will necessarily override session restore's tab selection.

Review commit: https://reviewboard.mozilla.org/r/127710/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127710/

Comment 4

[Jan Henning [:JanH]]

Attachment: Bug 1352997 - Part 5 - Implement common behaviour for custom tabs/web apps and switch over the former.

This implements the common behaviour for restoring the correct tab when switching to/from custom tab and web app activities. Unlike our normal UI, those activities are basically single tab activities, that is each activity is linked to a certain Gecko tab, with no facilities (bugs aside) for the user to directly load/select a different tab within that activity.

Therefore, here we basically update the selected tab only when the activity is starting up and initially creating its new (or, especially once tab type switching will be implemented, taking over an existing) content tab.

When subsequently restoring, we then check whether the tab is still available. If it is, we select it, if not, we fall back to opening a new tab based on the available intent data.

Review commit: https://reviewboard.mozilla.org/r/127712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127712/

Comment 5

[Jan Henning [:JanH]]

Attachment: Bug 1352997 - Part 6 - Switch over web apps and implement additional startup logic for them.

Web Apps are single task activities, but Android's task switcher will only ever return the intent that originally created the activity and will never ever update its stored intent for subsequent launches via onNewIntent, so we have to do this ourselves.

Additionally, web apps have some additional logic when being launched via a new intent that checks whether the currently loaded page matches the scope of the web app intent and then resets it if necessary. We now hook up this logic to the new SingleTabActivity wiring.

Review commit: https://reviewboard.mozilla.org/r/127714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127714/

Comment 6

[Jan Henning [:JanH]]

Attachment: Bug 1352997 - Part 2 - Provide dedicated methods for typical homepage operations.

That is figuring out whether a homepage has been set (but not caring about the specific page), or else getting the homepage URL with an automatic fallback (to about:home) if no homepage has been set.

Review commit: https://reviewboard.mozilla.org/r/128182/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128182/

Comment 7

[Jan Henning [:JanH]]

Comment on attachment 8856153 [details]
Bug 1352997 - Part 1 - Register GeckoApp's onTabsChangedListener earlier.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128040/diff/1-2/

Comment 8

[Jan Henning [:JanH]]

Comment on attachment 8856154 [details]
Bug 1352997 - Part 3 - Re-implement tracking of last selected tab for BrowserApp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128074/diff/1-2/

Comment 9

[Jan Henning [:JanH]]

Comment on attachment 8856155 [details]
Bug 1352997 - Part 4 - Remember the tab selected by session restoring if somebody other than BrowserApp is starting up first.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127710/diff/1-2/

Comment 10

[Jan Henning [:JanH]]

Comment on attachment 8856156 [details]
Bug 1352997 - Part 5 - Implement common behaviour for custom tabs/web apps and switch over the former.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127712/diff/1-2/

Comment 11

[Jan Henning [:JanH]]

Comment on attachment 8856157 [details]
Bug 1352997 - Part 6 - Switch over web apps and implement additional startup logic for them.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127714/diff/1-2/

Comment 12

[Jan Henning [:JanH]]

Comment on attachment 8856153 [details]
Bug 1352997 - Part 1 - Register GeckoApp's onTabsChangedListener earlier.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128040/diff/2-3/

Comment 13

[Jan Henning [:JanH]]

Comment on attachment 8856262 [details]
Bug 1352997 - Part 2 - Provide dedicated methods for typical homepage operations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128182/diff/1-2/

Comment 14

[Jan Henning [:JanH]]

Comment on attachment 8856154 [details]
Bug 1352997 - Part 3 - Re-implement tracking of last selected tab for BrowserApp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128074/diff/2-3/

Comment 15

[Jan Henning [:JanH]]

Comment on attachment 8856155 [details]
Bug 1352997 - Part 4 - Remember the tab selected by session restoring if somebody other than BrowserApp is starting up first.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127710/diff/2-3/

Comment 16

[Jan Henning [:JanH]]

Comment on attachment 8856156 [details]
Bug 1352997 - Part 5 - Implement common behaviour for custom tabs/web apps and switch over the former.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127712/diff/2-3/

Comment 17

[Jan Henning [:JanH]]

Comment on attachment 8856157 [details]
Bug 1352997 - Part 6 - Switch over web apps and implement additional startup logic for them.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127714/diff/2-3/

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8856153 [details]
Bug 1352997 - Part 1 - Register GeckoApp's onTabsChangedListener earlier.

https://reviewboard.mozilla.org/r/128040/#review131952

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:432
(Diff revision 3)
> +            if (resetFormAssist && mFormAssistPopup != null) {
> +                mFormAssistPopup.hide();

I don't really understand why the formAssist code is part of resetOptionsMenu()? Can't it be its own method?

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8856262 [details]
Bug 1352997 - Part 2 - Provide dedicated methods for typical homepage operations.

https://reviewboard.mozilla.org/r/128182/#review131954

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8856154 [details]
Bug 1352997 - Part 3 - Re-implement tracking of last selected tab for BrowserApp.

https://reviewboard.mozilla.org/r/128074/#review131956

Please make sure to test this with "Don't keep activities" enable in Developer settings. I remember this uncovering a bunch of issues with some of the previous iterations.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1158
(Diff revision 3)
> +        Tabs tabs = Tabs.getInstance();
> +        Tab tabToSelect = tabs.getTab(mLastSelectedTabId);

nit: final

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1161
(Diff revision 3)
> +        if (tabToSelect != null && tabToSelect.hashCode() == mLastSelectedTabHash &&
> +                tabToSelect.getType() == TabType.BROWSING) {

I don't think this is necessarily true. While two equal tabs will generate the same hashcode, it is not guaranteed that two equal hashcodes mean the tabs equal too (although this might be rare in practice).

A equals B => hash(A) equals hash(B)

But the reverse is not true:

hash(A) equals hash(B) => A equals B

Is what we are actually looking for here a truly unique id? Could we generate a UUID for every tab and use this for comparison? I haven't followed all the code yet to know whether this is something that needs to survive tab restoration?

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1166
(Diff revision 3)
> +        if (tabToSelect != null && tabToSelect.hashCode() == mLastSelectedTabHash &&
> +                tabToSelect.getType() == TabType.BROWSING) {
> +            tabs.selectTab(mLastSelectedTabId);
> +        } else {
> +            if (!tabs.selectLastTab(TabType.BROWSING)) {
> +                Tabs.getInstance().loadUrl(Tabs.getHomepageForStartupTab(this), Tabs.LOADURL_NEW_TAB);

nit: you already have "tabs" as a reference to the instance.

Comment 21

[Jan Henning [:JanH]]

Comment on attachment 8856153 [details]
Bug 1352997 - Part 1 - Register GeckoApp's onTabsChangedListener earlier.

https://reviewboard.mozilla.org/r/128040/#review131952

> I don't really understand why the formAssist code is part of resetOptionsMenu()? Can't it be its own method?

I was trying too hard to emulate the previous fall-through behaviour or something like that... I'll separate it out.

Comment 22

[Jan Henning [:JanH]]

Comment on attachment 8856154 [details]
Bug 1352997 - Part 3 - Re-implement tracking of last selected tab for BrowserApp.

https://reviewboard.mozilla.org/r/128074/#review131956

I already did a bit, although clearly not enough [1], because I've noticed an issue with web apps manifest parsing there. The tab selection logic itself seems to work though if I open both the normal UI and a custom tab/web app and switch between the two using the task switcher.

[1] The problem is that in combination with activity switching I'm running into [bug 1325021](https://bugzilla.mozilla.org/show_bug.cgi?id=1325021).

> I don't think this is necessarily true. While two equal tabs will generate the same hashcode, it is not guaranteed that two equal hashcodes mean the tabs equal too (although this might be rare in practice).
> 
> A equals B => hash(A) equals hash(B)
> 
> But the reverse is not true:
> 
> hash(A) equals hash(B) => A equals B
> 
> Is what we are actually looking for here a truly unique id? Could we generate a UUID for every tab and use this for comparison? I haven't followed all the code yet to know whether this is something that needs to survive tab restoration?

Depending on how Java's default object hash implementation looks like, this might or might not be unlikely enough, but you have a point here. Tab IDs are unique per session until we are killed/shutdown and have to restart, so as we've discussed, using a per-session (i.e. application startup) UUID instead of the hash code will allow to uniquely identify each tab object, while at the same time avoid having to generate a UUID for each tab.

Comment 23

[Jan Henning [:JanH]]

Just FYI, later today I'm going to push an updated version with one or two small fixes I've since discovered and that's also been rebased for the recent CustomTabsActivity/WebAppActivity changes that landed at the beginning of this week.

Comment 24

[Jan Henning [:JanH]]

Comment on attachment 8856153 [details]
Bug 1352997 - Part 1 - Register GeckoApp's onTabsChangedListener earlier.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128040/diff/3-4/

Comment 25

[Jan Henning [:JanH]]

Comment on attachment 8856262 [details]
Bug 1352997 - Part 2 - Provide dedicated methods for typical homepage operations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128182/diff/2-3/

Comment 26

[Jan Henning [:JanH]]

Comment on attachment 8856154 [details]
Bug 1352997 - Part 3 - Re-implement tracking of last selected tab for BrowserApp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128074/diff/3-4/

Comment 27

[Jan Henning [:JanH]]

Comment on attachment 8856155 [details]
Bug 1352997 - Part 4 - Remember the tab selected by session restoring if somebody other than BrowserApp is starting up first.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127710/diff/3-4/

Comment 28

[Jan Henning [:JanH]]

Comment on attachment 8856156 [details]
Bug 1352997 - Part 5 - Implement common behaviour for custom tabs/web apps and switch over the former.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127712/diff/3-4/

Comment 29

[Jan Henning [:JanH]]

Comment on attachment 8856157 [details]
Bug 1352997 - Part 6 - Switch over web apps and implement additional startup logic for them.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127714/diff/3-4/

Comment 30

[Jan Henning [:JanH]]

Attachment: Bug 1352997 - Part 7 - Don't switch activities if we're closing a tab while exiting the activity.

Closing the currently selected tab will select another tab, which can trigger an activity switch if the tab types differ. We don't want that if we're about to close the activity, as that'll bring that activity into the foreground instead of simply walking back along the activity history stack.

To give an example: Without this patch, closing the last custom tab will select a normal browsing tab, which will trigger an activity switch, so instead of returning to the user's previous activity, pressing back will send us to BrowserApp.

There's one additional catch:
If we change our onDone() behaviour and no longer finish() the SingleTabActivity when exiting (in order to avoid a costly restart of Gecko should the user return to us soon), this means that the activity that has just been exited could be brought back into the foreground via the onResume() codepath.

In that case the mLastActiveGeckoApp-check won't be triggered if no other GeckoApp-based activity was active in the meantime, so we wouldn't reselect/reopen the activity's desired tab, but instead display the tab that was selected when closing the previous tab. Therefore, we track this via an additional flag that is set for this case.

Review commit: https://reviewboard.mozilla.org/r/130308/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130308/

Comment 31

[Jan Henning [:JanH]]

Comment on attachment 8856153 [details]
Bug 1352997 - Part 1 - Register GeckoApp's onTabsChangedListener earlier.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128040/diff/4-5/

Comment 32

[Jan Henning [:JanH]]

Comment on attachment 8856262 [details]
Bug 1352997 - Part 2 - Provide dedicated methods for typical homepage operations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128182/diff/3-4/

Comment 33

[Jan Henning [:JanH]]

Comment on attachment 8856154 [details]
Bug 1352997 - Part 3 - Re-implement tracking of last selected tab for BrowserApp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128074/diff/4-5/

Comment 34

[Jan Henning [:JanH]]

Comment on attachment 8856155 [details]
Bug 1352997 - Part 4 - Remember the tab selected by session restoring if somebody other than BrowserApp is starting up first.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127710/diff/4-5/

Comment 35

[Jan Henning [:JanH]]

Comment on attachment 8856156 [details]
Bug 1352997 - Part 5 - Implement common behaviour for custom tabs/web apps and switch over the former.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127712/diff/4-5/

Comment 36

[Jan Henning [:JanH]]

Comment on attachment 8856157 [details]
Bug 1352997 - Part 6 - Switch over web apps and implement additional startup logic for them.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127714/diff/4-5/

Comment 37

[Julian Chu [:walkingice]]

Comment on attachment 8856153 [details]
Bug 1352997 - Part 1 - Register GeckoApp's onTabsChangedListener earlier.

https://reviewboard.mozilla.org/r/128040/#review133130

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:1679
(Diff revision 5)
>  
> -        Tabs.registerOnTabsChangedListener(this);
> -
>          initializeChrome();
>  
> +        mInitialized = true;

this line is moved from the beginning of this method. Is it necessary?

Comment 38

[Julian Chu [:walkingice]]

Comment on attachment 8856262 [details]
Bug 1352997 - Part 2 - Provide dedicated methods for typical homepage operations.

https://reviewboard.mozilla.org/r/128182/#review133132

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:1381
(Diff revision 4)
> +     * @return The user's homepage, or about:home if none is set.
> +     */
> +    @NonNull
> +    public static String getHomepageForStartupTab(Context context) {
> +        final String homepage = Tabs.getHomepage(context);
> +        return !TextUtils.isEmpty(homepage) ? homepage : AboutPages.HOME;

If we swap return value, then we can just use `TextUtils.isEpmty(homepage)` which avoid double-negative. This is more about personal programming style so I am good with current implementation. :-)

Does it make sense also to apply this method in `getHomepageForNewTab` ?

Comment 39

[Jan Henning [:JanH]]

Comment on attachment 8856153 [details]
Bug 1352997 - Part 1 - Register GeckoApp's onTabsChangedListener earlier.

https://reviewboard.mozilla.org/r/128040/#review133130

> this line is moved from the beginning of this method. Is it necessary?

Hmm, basically I've moved the setting of `mInitialized` to true to the point where `Tabs.registerOnTabsChangedListener(this)` was formerly called. I'm not sure if there's anything in the tab change listeners that actually needs stuff from `initializeChrome()` - the only thing I could find now was the form assist popup, and that one's got a null check - but I probably wanted to be on the safe side, hence I then moved it to below the `initializeChrome()` call.

Since both `initialize()` and the tab change listeners are called on the main thread and there's nothing in `initialize()` between the beginning and `initializeChrome()` that would cause any tab change events, the whole thing is probably not strictly necessary, so I suppose I could equally leave `mInitialized = true` where it formerly was...

Comment 40

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8856154 [details]
Bug 1352997 - Part 3 - Re-implement tracking of last selected tab for BrowserApp.

https://reviewboard.mozilla.org/r/128074/#review133748

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:179
(Diff revision 5)
>  
>          mIsInitialResume = true;
>  
>          mRefWatcher = LeakCanary.install(this);
>  
> +        sSessionUUID = UUID.randomUUID();

I'd call toString() here and use the hex representation.

Comment 41

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8856155 [details]
Bug 1352997 - Part 4 - Remember the tab selected by session restoring if somebody other than BrowserApp is starting up first.

https://reviewboard.mozilla.org/r/127710/#review133750

Comment 42

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8856156 [details]
Bug 1352997 - Part 5 - Implement common behaviour for custom tabs/web apps and switch over the former.

https://reviewboard.mozilla.org/r/127712/#review133754

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:1673
(Diff revision 5)
> +        if (ThreadUtils.isOnUiThread()) {
> +            onTabOpenFromIntent(newTab);
> +        } else {
> +            ThreadUtils.postToUiThread(new Runnable() {
> +                @Override
> +                public void run() {
> +                    onTabOpenFromIntent(newTab);
> +                }
> +            });
> +        }

Is this check really required to synchronize some flow or is this just an optimization?

::: mobile/android/base/java/org/mozilla/gecko/SingleTabActivity.java:20
(Diff revision 5)
> +
> +import static org.mozilla.gecko.Tabs.INTENT_EXTRA_SESSION_UUID;
> +import static org.mozilla.gecko.Tabs.INTENT_EXTRA_TAB_ID;
> +import static org.mozilla.gecko.Tabs.INVALID_TAB_ID;
> +
> +public abstract class SingleTabActivity extends GeckoApp {

nit: There are a bunch of finals that could be added to the various references in the methods of this class.

Comment 43

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8856157 [details]
Bug 1352997 - Part 6 - Switch over web apps and implement additional startup logic for them.

https://reviewboard.mozilla.org/r/127714/#review133756

Comment 44

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8858362 [details]
Bug 1352997 - Part 7 - Don't switch activities if we're closing a tab while exiting the activity.

https://reviewboard.mozilla.org/r/130308/#review133758

Comment 45

[Julian Chu [:walkingice]]

Comment on attachment 8856154 [details]
Bug 1352997 - Part 3 - Re-implement tracking of last selected tab for BrowserApp.

https://reviewboard.mozilla.org/r/128074/#review133788

Comment 46

[Julian Chu [:walkingice]]

(In reply to Jan Henning [:JanH] from comment #39)
> Comment on attachment 8856153 [details]
> Bug 1352997 - Part 1 - Register GeckoApp's onTabsChangedListener earlier.
> 
> https://reviewboard.mozilla.org/r/128040/#review133130
> 
> > this line is moved from the beginning of this method. Is it necessary?
> 
> Hmm, basically I've moved the setting of `mInitialized` to true to the point
> where `Tabs.registerOnTabsChangedListener(this)` was formerly called. I'm
> not sure if there's anything in the tab change listeners that actually needs
> stuff from `initializeChrome()` - the only thing I could find now was the
> form assist popup, and that one's got a null check - but I probably wanted
> to be on the safe side, hence I then moved it to below the
> `initializeChrome()` call.
> 
> Since both `initialize()` and the tab change listeners are called on the
> main thread and there's nothing in `initialize()` between the beginning and
> `initializeChrome()` that would cause any tab change events, the whole thing
> is probably not strictly necessary, so I suppose I could equally leave
> `mInitialized = true` where it formerly was...

Understood.

I also didn't see any actual difference in that change, that's the reason I had the question. I was worrying whether there is any black magic I haven't found. If that setting was in the beginning of the method, then any code-reader can easily understand it is just a flag to indicate whether `initialize()` is invoked or not. Now it is not in the beginning, people might guess is there any intention behind it. That my two cents :-)

Comment 47

[Julian Chu [:walkingice]]

Comment on attachment 8856155 [details]
Bug 1352997 - Part 4 - Remember the tab selected by session restoring if somebody other than BrowserApp is starting up first.

https://reviewboard.mozilla.org/r/127710/#review134286

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:1909
(Diff revision 5)
>              }
>              throw new SessionRestoreException("No tabs could be read from session file");
>          }
>  
> +        SharedPreferences.Editor prefsEdit = getSharedPreferencesForProfile().edit();
> +        onSessionRestoringFinished(prefsEdit, parser.getStoredSelectedTabId());

`onSessionRestoringFinished` implementation in parent class is saving preferences, in child class is removing preferences. Opposite actions in same method is bit hard to understand. I guess the removing in `BrowserApp.onSessionRestoringFinished` is to ensure the preferences is wiped.(since `BrowserApp.restoreLastSelectedTab` already do it once)

If this is correct, keep `BrowserApp.onSessionRestoringFinished` should also work. But overwrite a method to make it empty is bad idea.

Instead of involving `onSessionRestoringFinished`, if we to add a method `needToSaveStartupTab()`, then save preference like

```
if (needToSaveStartupTab()) {
    .....
    prefsEdit.putInt(STARTUP_SELECTED_TAB, tab.getId());
    .....
}
```

BrowserApp can just return false in that method. How do you think?

Comment 48

[Julian Chu [:walkingice]]

Comment on attachment 8856155 [details]
Bug 1352997 - Part 4 - Remember the tab selected by session restoring if somebody other than BrowserApp is starting up first.

https://reviewboard.mozilla.org/r/127710/#review134286

> `onSessionRestoringFinished` implementation in parent class is saving preferences, in child class is removing preferences. Opposite actions in same method is bit hard to understand. I guess the removing in `BrowserApp.onSessionRestoringFinished` is to ensure the preferences is wiped.(since `BrowserApp.restoreLastSelectedTab` already do it once)
> 
> If this is correct, keep `BrowserApp.onSessionRestoringFinished` should also work. But overwrite a method to make it empty is bad idea.
> 
> Instead of involving `onSessionRestoringFinished`, if we to add a method `needToSaveStartupTab()`, then save preference like
> 
> ```
> if (needToSaveStartupTab()) {
>     .....
>     prefsEdit.putInt(STARTUP_SELECTED_TAB, tab.getId());
>     .....
> }
> ```
> 
> BrowserApp can just return false in that method. How do you think?

`needToSaveStartupTab` is a bad naming I know :P

Comment 49

[Julian Chu [:walkingice]]

Comment on attachment 8856156 [details]
Bug 1352997 - Part 5 - Implement common behaviour for custom tabs/web apps and switch over the former.

https://reviewboard.mozilla.org/r/127712/#review134312

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:2316
(Diff revision 5)
> +        if (tabToCheck == null) {
> +            return false;
> +        }
> +        return !intent.hasExtra(Tabs.INTENT_EXTRA_SESSION_UUID) ||
> +                GeckoApplication.getSessionUUID()
> +                        .equals(intent.getUnsafe().getSerializableExtra(Tabs.INTENT_EXTRA_SESSION_UUID));

its bit hard to read this line. could we put more semantic expressions here?

::: mobile/android/base/java/org/mozilla/gecko/SingleTabActivity.java:74
(Diff revision 5)
> +
> +        // If the tab we've stored is still existing and valid select it...
> +        if (tabToSelect != null && GeckoApplication.getSessionUUID().equals(mLastSessionUUID) &&
> +                tabs.currentActivityMatchesTab(tabToSelect)) {
> +            tabs.selectTab(mLastSelectedTabId);
> +        // ...otherwise fall back to the intent data and open a new tab.

I think this line should move into `else` block

::: mobile/android/base/java/org/mozilla/gecko/SingleTabActivity.java:88
(Diff revision 5)
> +    }
> +
> +    /**
> +     * @return True if we're going to select an existing tab, false if we want to load a new tab.
> +     */
> +    private boolean decideTabAction(SafeIntent intent, @Nullable Bundle savedInstanceState) {

since you already added an annotation for bundle. Intent might need one.

Comment 50

[Julian Chu [:walkingice]]

Comment on attachment 8856157 [details]
Bug 1352997 - Part 6 - Switch over web apps and implement additional startup logic for them.

https://reviewboard.mozilla.org/r/127714/#review134320

Comment 51

[Julian Chu [:walkingice]]

Comment on attachment 8858362 [details]
Bug 1352997 - Part 7 - Don't switch activities if we're closing a tab while exiting the activity.

https://reviewboard.mozilla.org/r/130308/#review134324

Comment 52

[Jan Henning [:JanH]]

Comment on attachment 8856154 [details]
Bug 1352997 - Part 3 - Re-implement tracking of last selected tab for BrowserApp.

https://reviewboard.mozilla.org/r/128074/#review133748

> I'd call toString() here and use the hex representation.

Duh - you're right, it's easier than serialising and deserialising and converting to a string and parsing from a string back and forth...

Comment 53

[Jan Henning [:JanH]]

Comment on attachment 8856156 [details]
Bug 1352997 - Part 5 - Implement common behaviour for custom tabs/web apps and switch over the former.

https://reviewboard.mozilla.org/r/127712/#review133754

> Is this check really required to synchronize some flow or is this just an optimization?

Good question. Since in some cases (when loadStartupTab is called from the background thread, e.g. in conjunction with tab queues) this should work even if we always dispatch, so I think I just wanted to avoid a redispatch where not required. I've also seen this pattern elsewhere in our codebase, e.g. [`notifyListeners()` in Tabs.java](https://dxr.mozilla.org/mozilla-central/rev/c0ea5ed7f91a6be996a4a3c5ab25e2cdf6b4377e/mobile/android/base/java/org/mozilla/gecko/Tabs.java#780).

Comment 54

[Jan Henning [:JanH]]

Comment on attachment 8856156 [details]
Bug 1352997 - Part 5 - Implement common behaviour for custom tabs/web apps and switch over the former.

https://reviewboard.mozilla.org/r/127712/#review134312

> its bit hard to read this line. could we put more semantic expressions here?

Sebastian's suggestion of just storing and handling the string representation of the UUID directly has already simplified this a bit and I hope my additional change of this line has helped here further.

Comment 55

[Jan Henning [:JanH]]

Comment on attachment 8856262 [details]
Bug 1352997 - Part 2 - Provide dedicated methods for typical homepage operations.

https://reviewboard.mozilla.org/r/128182/#review133132

> If we swap return value, then we can just use `TextUtils.isEpmty(homepage)` which avoid double-negative. This is more about personal programming style so I am good with current implementation. :-)
> 
> Does it make sense also to apply this method in `getHomepageForNewTab` ?

Yup, I originally thought to return the homepage first and the fall-back value in the "else" part, but avoiding the double-negative has something going for it. I've also taken up your suggestion for `getHomepageForNewTab()`.

Comment 56

[Jan Henning [:JanH]]

Comment on attachment 8856155 [details]
Bug 1352997 - Part 4 - Remember the tab selected by session restoring if somebody other than BrowserApp is starting up first.

https://reviewboard.mozilla.org/r/127710/#review134286

> `needToSaveStartupTab` is a bad naming I know :P

I've rewritten this as you suggested.

Also thinking some more about it, it's not absolutely necessary to remove the data here - any outdated tab data from a previous session would still be detected through the session UUID and consequently ignored. This means we can defer removing it until after we've actually read it, and therefore avoid a prefs access during session restoring for the probably more common case where BrowserApp starts up first.

Comment 57

[Jan Henning [:JanH]]

Comment on attachment 8856153 [details]
Bug 1352997 - Part 1 - Register GeckoApp's onTabsChangedListener earlier.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128040/diff/5-6/

Comment 58

[Jan Henning [:JanH]]

Comment on attachment 8856262 [details]
Bug 1352997 - Part 2 - Provide dedicated methods for typical homepage operations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128182/diff/4-5/

Comment 59

[Jan Henning [:JanH]]

Comment on attachment 8856154 [details]
Bug 1352997 - Part 3 - Re-implement tracking of last selected tab for BrowserApp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128074/diff/5-6/

Comment 60

[Jan Henning [:JanH]]

Comment on attachment 8856155 [details]
Bug 1352997 - Part 4 - Remember the tab selected by session restoring if somebody other than BrowserApp is starting up first.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127710/diff/5-6/

Comment 61

[Jan Henning [:JanH]]

Comment on attachment 8856156 [details]
Bug 1352997 - Part 5 - Implement common behaviour for custom tabs/web apps and switch over the former.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127712/diff/5-6/

Comment 62

[Jan Henning [:JanH]]

Comment on attachment 8856157 [details]
Bug 1352997 - Part 6 - Switch over web apps and implement additional startup logic for them.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127714/diff/5-6/

Comment 63

[Jan Henning [:JanH]]

Comment on attachment 8858362 [details]
Bug 1352997 - Part 7 - Don't switch activities if we're closing a tab while exiting the activity.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130308/diff/1-2/

Comment 64

[Julian Chu [:walkingice]]

Comment on attachment 8856155 [details]
Bug 1352997 - Part 4 - Remember the tab selected by session restoring if somebody other than BrowserApp is starting up first.

https://reviewboard.mozilla.org/r/127710/#review136114

Comment 65

[Jan Henning [:JanH]]

Comment on attachment 8856153 [details]
Bug 1352997 - Part 1 - Register GeckoApp's onTabsChangedListener earlier.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128040/diff/6-7/

Comment 66

[Jan Henning [:JanH]]

Comment on attachment 8856262 [details]
Bug 1352997 - Part 2 - Provide dedicated methods for typical homepage operations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128182/diff/5-6/

Comment 67

[Jan Henning [:JanH]]

Comment on attachment 8856154 [details]
Bug 1352997 - Part 3 - Re-implement tracking of last selected tab for BrowserApp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128074/diff/6-7/

Comment 68

[Jan Henning [:JanH]]

Comment on attachment 8856155 [details]
Bug 1352997 - Part 4 - Remember the tab selected by session restoring if somebody other than BrowserApp is starting up first.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127710/diff/6-7/

Comment 69

[Jan Henning [:JanH]]

Comment on attachment 8856156 [details]
Bug 1352997 - Part 5 - Implement common behaviour for custom tabs/web apps and switch over the former.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127712/diff/6-7/

Comment 70

[Jan Henning [:JanH]]

Comment on attachment 8856157 [details]
Bug 1352997 - Part 6 - Switch over web apps and implement additional startup logic for them.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127714/diff/6-7/

Comment 71

[Jan Henning [:JanH]]

Comment on attachment 8858362 [details]
Bug 1352997 - Part 7 - Don't switch activities if we're closing a tab while exiting the activity.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130308/diff/2-3/

Comment 72

[Jan Henning [:JanH]]

Comment on attachment 8856153 [details]
Bug 1352997 - Part 1 - Register GeckoApp's onTabsChangedListener earlier.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128040/diff/7-8/

Comment 73

[Jan Henning [:JanH]]

Comment on attachment 8856262 [details]
Bug 1352997 - Part 2 - Provide dedicated methods for typical homepage operations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128182/diff/6-7/

Comment 74

[Jan Henning [:JanH]]

Comment on attachment 8856154 [details]
Bug 1352997 - Part 3 - Re-implement tracking of last selected tab for BrowserApp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128074/diff/7-8/

Comment 75

[Jan Henning [:JanH]]

Comment on attachment 8856155 [details]
Bug 1352997 - Part 4 - Remember the tab selected by session restoring if somebody other than BrowserApp is starting up first.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127710/diff/7-8/

Comment 76

[Jan Henning [:JanH]]

Comment on attachment 8856156 [details]
Bug 1352997 - Part 5 - Implement common behaviour for custom tabs/web apps and switch over the former.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127712/diff/7-8/

Comment 77

[Jan Henning [:JanH]]

Comment on attachment 8856157 [details]
Bug 1352997 - Part 6 - Switch over web apps and implement additional startup logic for them.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127714/diff/7-8/

Comment 78

[Jan Henning [:JanH]]

Comment on attachment 8858362 [details]
Bug 1352997 - Part 7 - Don't switch activities if we're closing a tab while exiting the activity.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130308/diff/3-4/

Comment 79

[Jan Henning [:JanH]]

Comment on attachment 8856153 [details]
Bug 1352997 - Part 1 - Register GeckoApp's onTabsChangedListener earlier.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128040/diff/8-9/

Comment 80

[Jan Henning [:JanH]]

Comment on attachment 8856262 [details]
Bug 1352997 - Part 2 - Provide dedicated methods for typical homepage operations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128182/diff/7-8/

Comment 81

[Jan Henning [:JanH]]

Comment on attachment 8856154 [details]
Bug 1352997 - Part 3 - Re-implement tracking of last selected tab for BrowserApp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128074/diff/8-9/

Comment 82

[Jan Henning [:JanH]]

Comment on attachment 8856155 [details]
Bug 1352997 - Part 4 - Remember the tab selected by session restoring if somebody other than BrowserApp is starting up first.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127710/diff/8-9/

Comment 83

[Jan Henning [:JanH]]

Comment on attachment 8856156 [details]
Bug 1352997 - Part 5 - Implement common behaviour for custom tabs/web apps and switch over the former.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127712/diff/8-9/

Comment 84

[Jan Henning [:JanH]]

Comment on attachment 8856157 [details]
Bug 1352997 - Part 6 - Switch over web apps and implement additional startup logic for them.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127714/diff/8-9/

Comment 85

[Jan Henning [:JanH]]

Comment on attachment 8858362 [details]
Bug 1352997 - Part 7 - Don't switch activities if we're closing a tab while exiting the activity.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130308/diff/4-5/

Comment 86

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 1fa30faa49c6 -d e919535c7872: rebasing 392135:1fa30faa49c6 "Bug 1352997 - Part 1 - Register GeckoApp's onTabsChangedListener earlier. r=sebastian,walkingice"
merging mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
merging mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
rebasing 392136:0ef78ab099bf "Bug 1352997 - Part 2 - Provide dedicated methods for typical homepage operations. r=sebastian,walkingice"
merging mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
merging mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
merging mobile/android/base/java/org/mozilla/gecko/Tabs.java
rebasing 392137:e9e42fb17bfc "Bug 1352997 - Part 3 - Re-implement tracking of last selected tab for BrowserApp. r=sebastian,walkingice"
merging mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
merging mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
merging mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java
merging mobile/android/base/java/org/mozilla/gecko/Tabs.java
merging mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/GeckoApp.java! (edit, then use 'hg resolve --mark')
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 87

[Jan Henning [:JanH]]

Comment on attachment 8856153 [details]
Bug 1352997 - Part 1 - Register GeckoApp's onTabsChangedListener earlier.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128040/diff/9-10/

Comment 88

[Jan Henning [:JanH]]

Comment on attachment 8856262 [details]
Bug 1352997 - Part 2 - Provide dedicated methods for typical homepage operations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128182/diff/8-9/

Comment 89

[Jan Henning [:JanH]]

Comment on attachment 8856154 [details]
Bug 1352997 - Part 3 - Re-implement tracking of last selected tab for BrowserApp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128074/diff/9-10/

Comment 90

[Jan Henning [:JanH]]

Comment on attachment 8856155 [details]
Bug 1352997 - Part 4 - Remember the tab selected by session restoring if somebody other than BrowserApp is starting up first.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127710/diff/9-10/

Comment 91

[Jan Henning [:JanH]]

Comment on attachment 8856156 [details]
Bug 1352997 - Part 5 - Implement common behaviour for custom tabs/web apps and switch over the former.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127712/diff/9-10/

Comment 92

[Jan Henning [:JanH]]

Comment on attachment 8856157 [details]
Bug 1352997 - Part 6 - Switch over web apps and implement additional startup logic for them.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127714/diff/9-10/

Comment 93

[Jan Henning [:JanH]]

Comment on attachment 8858362 [details]
Bug 1352997 - Part 7 - Don't switch activities if we're closing a tab while exiting the activity.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130308/diff/5-6/

Comment 94

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29dc2947feaf
Part 1 - Register GeckoApp's onTabsChangedListener earlier. r=sebastian,walkingice
https://hg.mozilla.org/integration/autoland/rev/49f48bd6b958
Part 2 - Provide dedicated methods for typical homepage operations. r=sebastian,walkingice
https://hg.mozilla.org/integration/autoland/rev/674241dce78b
Part 3 - Re-implement tracking of last selected tab for BrowserApp. r=sebastian,walkingice
https://hg.mozilla.org/integration/autoland/rev/5e3f399d311e
Part 4 - Remember the tab selected by session restoring if somebody other than BrowserApp is starting up first. r=sebastian,walkingice
https://hg.mozilla.org/integration/autoland/rev/86c1de1e154e
Part 5 - Implement common behaviour for custom tabs/web apps and switch over the former. r=sebastian,walkingice
https://hg.mozilla.org/integration/autoland/rev/a977e2d9610e
Part 6 - Switch over web apps and implement additional startup logic for them. r=sebastian,walkingice
https://hg.mozilla.org/integration/autoland/rev/7e3ac5b323e7
Part 7 - Don't switch activities if we're closing a tab while exiting the activity. r=sebastian,walkingice

Comment 95

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29dc2947feaf
https://hg.mozilla.org/mozilla-central/rev/49f48bd6b958
https://hg.mozilla.org/mozilla-central/rev/674241dce78b
https://hg.mozilla.org/mozilla-central/rev/5e3f399d311e
https://hg.mozilla.org/mozilla-central/rev/86c1de1e154e
https://hg.mozilla.org/mozilla-central/rev/a977e2d9610e
https://hg.mozilla.org/mozilla-central/rev/7e3ac5b323e7