1346004 - Provide separate Tabs lists

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Jan Henning [:JanH]]

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.

Comment 1

[Jan Henning [:JanH]]

(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.

Comment 2

[Jan Henning [:JanH]]

Attachment: Bug 1346004 - Part 0 - Remove unneeded imports.

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

Comment 3

[Jan Henning [:JanH]]

Attachment: Bug 1346004 - Part 1 - Keep track of a tab type on the Java tab object.

As long as the tabs are opened in the same Gecko browser window, splitting the Java UI tabs list into multiple parts breaks too many assumptions, so the easier solution is to allow setting a type attribute on each tabs object, which will allow filtering of the tabs later on.

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

Comment 4

[Jan Henning [:JanH]]

Attachment: Bug 1346004 - Part 2 - Tabs helper functions for searching next/previous tab should take the Tab type into account.

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

Comment 5

[Jan Henning [:JanH]]

Attachment: Bug 1346004 - Part 3 - getNextTab() should fall back to browsing-type tabs if no tabs of the same type are available.

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.

Since we have to select some other tab instead in that case (so the selected tab in the tab list and the selected tab in Gecko remain in sync), getNextTab() therefore has to fall back to browsing-type tabs if no other tabs of the same type are available.

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

Comment 6

[Jan Henning [:JanH]]

Attachment: Bug 1346004 - Part 4 - Filter non-browsing tabs from the TabsLayout/TabStrip.

Custom tabs etc. shouldn't show up in the main browser activity.

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

Comment 7

[Jan Henning [:JanH]]

Attachment: Bug 1346004 - Part 5 - Replace Gecko customtab tab attribute by a generic type attribute.

This'll allow customising context menu/session store/... behaviour depending on the tab type.

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

Comment 8

[Jan Henning [:JanH]]

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.

Comment 9

[Jan Henning [:JanH]]

Comment on attachment 8848733 [details]
Bug 1346004 - Part 0 - Remove unneeded imports.

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

Comment 10

[Jan Henning [:JanH]]

Comment on attachment 8848734 [details]
Bug 1346004 - Part 1 - Keep track of a tab type on the Java tab object.

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

Comment 11

[Jan Henning [:JanH]]

Comment on attachment 8848735 [details]
Bug 1346004 - Part 2 - Tabs helper functions for searching next/previous tab should take the Tab type into account.

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

Comment 12

[Jan Henning [:JanH]]

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.

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

Comment 13

[Jan Henning [:JanH]]

Comment on attachment 8848737 [details]
Bug 1346004 - Part 4 - Filter non-browsing tabs from the TabsLayout/TabStrip.

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

Comment 14

[Jan Henning [:JanH]]

Comment on attachment 8848738 [details]
Bug 1346004 - Part 5 - Replace Gecko customtab tab attribute by a generic type attribute.

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

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8848733 [details]
Bug 1346004 - Part 0 - Remove unneeded imports.

https://reviewboard.mozilla.org/r/121630/#review124362

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

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! :)

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8848737 [details]
Bug 1346004 - Part 4 - Filter non-browsing tabs from the TabsLayout/TabStrip.

https://reviewboard.mozilla.org/r/121638/#review124370

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

Thank you so much for working on this. This is an important building block for custom tabs and web apps. :)

Comment 22

[Jan Henning [:JanH]]

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.

Comment 23

[Jan Henning [:JanH]]

Comment on attachment 8848733 [details]
Bug 1346004 - Part 0 - Remove unneeded imports.

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

Comment 24

[Jan Henning [:JanH]]

Comment on attachment 8848734 [details]
Bug 1346004 - Part 1 - Keep track of a tab type on the Java tab object.

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

Comment 25

[Jan Henning [:JanH]]

Comment on attachment 8848735 [details]
Bug 1346004 - Part 2 - Tabs helper functions for searching next/previous tab should take the Tab type into account.

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

Comment 26

[Jan Henning [:JanH]]

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.

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

Comment 27

[Jan Henning [:JanH]]

Comment on attachment 8848737 [details]
Bug 1346004 - Part 4 - Filter non-browsing tabs from the TabsLayout/TabStrip.

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

Comment 28

[Jan Henning [:JanH]]

Comment on attachment 8848738 [details]
Bug 1346004 - Part 5 - Replace Gecko customtab tab attribute by a generic type attribute.

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

Comment 29

[Jan Henning [:JanH]]

Comment on attachment 8848733 [details]
Bug 1346004 - Part 0 - Remove unneeded imports.

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

Comment 30

[Jan Henning [:JanH]]

Comment on attachment 8848734 [details]
Bug 1346004 - Part 1 - Keep track of a tab type on the Java tab object.

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

Comment 31

[Jan Henning [:JanH]]

Comment on attachment 8848735 [details]
Bug 1346004 - Part 2 - Tabs helper functions for searching next/previous tab should take the Tab type into account.

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

Comment 32

[Jan Henning [:JanH]]

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.

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

Comment 33

[Jan Henning [:JanH]]

Comment on attachment 8848737 [details]
Bug 1346004 - Part 4 - Filter non-browsing tabs from the TabsLayout/TabStrip.

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

Comment 34

[Jan Henning [:JanH]]

Comment on attachment 8848738 [details]
Bug 1346004 - Part 5 - Replace Gecko customtab tab attribute by a generic type attribute.

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

Comment 35

[Pulsebot]

Pushed by [e-mail]:
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

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e22cc28b03c8
https://hg.mozilla.org/mozilla-central/rev/d5baa00b8365
https://hg.mozilla.org/mozilla-central/rev/83e6ed905120
https://hg.mozilla.org/mozilla-central/rev/1fcdba303d88
https://hg.mozilla.org/mozilla-central/rev/bc88a273c527
https://hg.mozilla.org/mozilla-central/rev/a9799dcbb7d9