Closed Bug 1352997 Opened 7 years ago Closed 7 years ago

Improve custom tab/web app tab selection logic

Categories

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

55 Branch
All
Android
enhancement

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: JanH, Assigned: JanH)

References

Details

Attachments

(7 files)

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.
Component: Graphics, Panning and Zooming → General
Blocks: 1353732
Blocks: 1353857
Blocks: 1347585
Priority: -- → P1
Blocks: 1346008
Blocks: 1351808
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?
Attachment #8856153 - Flags: review?(s.kaspari) → review+
Comment on attachment 8856262 [details]
Bug 1352997 - Part 2 - Provide dedicated methods for typical homepage operations.

https://reviewboard.mozilla.org/r/128182/#review131954
Attachment #8856262 - Flags: review?(s.kaspari) → review+
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.
Blocks: 1355824
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 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.
Attachment #8856153 - Flags: review?(cnevinchen)
Attachment #8856262 - Flags: review?(cnevinchen)
Attachment #8856154 - Flags: review?(cnevinchen)
Attachment #8856155 - Flags: review?(cnevinchen)
Attachment #8856156 - Flags: review?(cnevinchen)
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.
Attachment #8856153 - Flags: review?(cnevinchen) → review?(walkingice0204)
Attachment #8856154 - Flags: review?(cnevinchen) → review?(walkingice0204)
Attachment #8856155 - Flags: review?(cnevinchen) → review?(walkingice0204)
Attachment #8856156 - Flags: review?(cnevinchen) → review?(walkingice0204)
Attachment #8856157 - Flags: review?(cnevinchen) → review?(walkingice0204)
Attachment #8856262 - Flags: review?(cnevinchen) → review?(walkingice0204)
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?
Attachment #8856153 - Flags: review?(walkingice0204) → review+
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` ?
Attachment #8856262 - Flags: review?(walkingice0204) → review+
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 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.
Attachment #8856154 - Flags: review?(s.kaspari) → review+
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
Attachment #8856155 - Flags: review?(s.kaspari) → review+
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.
Attachment #8856156 - Flags: review?(s.kaspari) → review+
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
Attachment #8856157 - Flags: review?(s.kaspari) → review+
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
Attachment #8858362 - Flags: review?(s.kaspari) → review+
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
Attachment #8856154 - Flags: review?(walkingice0204) → review+
(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 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 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 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.
Attachment #8856156 - Flags: review?(walkingice0204) → review+
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
Attachment #8856157 - Flags: review?(walkingice0204) → review+
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
Attachment #8858362 - Flags: review?(walkingice0204) → review+
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 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 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 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 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 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
Attachment #8856155 - Flags: review?(walkingice0204) → review+
Blocks: 1359531
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)
Pushed by mozilla@buttercookie.de:
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
Depends on: 1360699
Depends on: 1360743
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: