Closed Bug 1346004 Opened 3 years ago Closed 3 years ago

Provide separate Tabs lists

Categories

(Firefox for Android :: General, enhancement)

All
Android
enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: JanH, Assigned: JanH)

References

Details

Attachments

(6 files)

Currently e.g. opening a custom tab through the CustomTabActivity will also open a new tab in the browser UI, since everything on the Java side shares a common Tabs instance.
(In reply to Sebastian Kaspari (:sebastian) from comment #7)
> (In reply to Jan Henning [:JanH] from comment #6)
> > Would somewhat de-singletonifying the Tabs object with one Tabs object per
> > group of tabs we want to keep separate be something you had in mind?
> 
> For now I thought it would be enough to keep the Tabs singleton and expose a
> filtered list of tabs to the browser. Custom Tabs and also Web Apps are more
> or less activities that only show a single tab. So we'd just need things
> like select/remove (for switching to the activity or closing it).
> [...]
> A first version might be just "a little hack" in the Tabs class. However
> from there we need to see how this affects session restore, or synced tabs,
> or history.

I've actually tried separate Tabs instances and got it sort of working, but yes, it introduced some complications that probably aren't worth the effort when (or are perhaps even caused by) having only one Gecko-side browser window.

So now I'm experimenting with having a TabType attribute and filtering things along the lines of private/non-private tabs, which seems to be less invasive for a similar effect.
No longer blocks: 889100
Depends on: 1347585
As long we open only browsing-type tabs (i.e. outside Nightly/Aurora? and as long as the corresponding option is not activated), I think this should be pretty safe. With custom tabs, there might be some weirdness, but then that's generally true of custom tabs.
Blocks: 1348686
Comment on attachment 8848733 [details]
Bug 1346004 - Part 0 - Remove unneeded imports.

https://reviewboard.mozilla.org/r/121630/#review124362
Attachment #8848733 - Flags: review?(s.kaspari) → review+
Comment on attachment 8848734 [details]
Bug 1346004 - Part 1 - Keep track of a tab type on the Java tab object.

https://reviewboard.mozilla.org/r/121632/#review124364

That's nice! :)
Attachment #8848734 - Flags: review?(s.kaspari) → review+
Comment on attachment 8848735 [details]
Bug 1346004 - Part 2 - Tabs helper functions for searching next/previous tab should take the Tab type into account.

https://reviewboard.mozilla.org/r/121634/#review124366

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:199
(Diff revision 2)
>       */
>      public synchronized int getDisplayCount() {
>          // Once mSelectedTab is non-null, it cannot be null for the remainder
>          // of the object's lifetime.
>          boolean getPrivate = mSelectedTab != null && mSelectedTab.isPrivate();
> +        TabType type = mSelectedTab != null ? mSelectedTab.getType() : TabType.BROWSING;

I wonder if this could be a problem when switching between browser and custom/webapp activities - a non browsing tab will be selected until we switch back to the last selected browser tab. But let's see.
Attachment #8848735 - Flags: review?(s.kaspari) → review+
Comment on attachment 8848736 [details]
Bug 1346004 - Part 3 - getNextTab() should fall back to browsing-type tabs if no tabs of the same type are available.

https://reviewboard.mozilla.org/r/121636/#review124368

::: commit-message-bb7a0:3
(Diff revision 2)
> +Bug 1346004 - Part 3 - getNextTab() should fall back to browsing-type tabs if no tabs of the same type are available. r?sebastian
> +
> +We always want at least one browsing type tab open, but the same doesn't hold true for other tab types. A CustomTabActivity instance e.g. only holds a single tab, so the lifetime of the tab is tied to the lifetime of the CustomTabActivity. If we're existing the activity, we just want to close the corresponding tab without opening a new replacement for it.

typo: existing -> exiting

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:470
(Diff revision 2)
>                  return parent;
>          }
>          return nextTab;
>      }
>  
> +    private Tab getFallbackNextTab(TabType type) {

maybe add a comment what "fallback" means in this case. without the other code and the commit message it's not obvious.
Attachment #8848736 - Flags: review?(s.kaspari) → review+
Comment on attachment 8848737 [details]
Bug 1346004 - Part 4 - Filter non-browsing tabs from the TabsLayout/TabStrip.

https://reviewboard.mozilla.org/r/121638/#review124370
Attachment #8848737 - Flags: review?(s.kaspari) → review+
Comment on attachment 8848738 [details]
Bug 1346004 - Part 5 - Replace Gecko customtab tab attribute by a generic type attribute.

https://reviewboard.mozilla.org/r/121640/#review124372
Attachment #8848738 - Flags: review?(s.kaspari) → review+
Thank you so much for working on this. This is an important building block for custom tabs and web apps. :)
Comment on attachment 8848735 [details]
Bug 1346004 - Part 2 - Tabs helper functions for searching next/previous tab should take the Tab type into account.

https://reviewboard.mozilla.org/r/121634/#review124366

> I wonder if this could be a problem when switching between browser and custom/webapp activities - a non browsing tab will be selected until we switch back to the last selected browser tab. But let's see.

Each GeckoApp instance keeps track of its last selected tab. On resuming we check [whether some other GeckoApp instance was active in the meantime](https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#2235) and then reselect the correct tab if necessary. I'm not sure whether this is absolutely foolproof, but in principle it looks like it should work.
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/e22cc28b03c8
Part 0 - Remove unneeded imports. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/d5baa00b8365
Part 1 - Keep track of a tab type on the Java tab object. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/83e6ed905120
Part 2 - Tabs helper functions for searching next/previous tab should take the Tab type into account. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/1fcdba303d88
Part 3 - getNextTab() should fall back to browsing-type tabs if no tabs of the same type are available. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/bc88a273c527
Part 4 - Filter non-browsing tabs from the TabsLayout/TabStrip. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/a9799dcbb7d9
Part 5 - Replace Gecko customtab tab attribute by a generic type attribute. r=sebastian
No longer depends on: 1347585
You need to log in before you can comment on or make changes to this bug.