906227 - LastTabsPage shows "Switch to tab" items but always creates a new tab

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[Lucas Rocha (:lucasr)]

I wonder if we should disable the "switch to tab" behaviour in the Last Tabs page. Thoughts?

Comment 1

[Ian Barlow (:ibarlow)]

What about keeping it and modifying its behaviour such that if the item has an open tab already, we don't create a new one?

That would be my first choice, though I imagine this is probably more complex technically. So if we need to start with your proposal and move to this, I would be open to that.

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This switches to the tab if one exists. Additionally open all tabs won't open tabs that are already "switchable".

Comment 4

[:Margaret Leibovic]

Comment on attachment 799085 [details] [diff] [review]
Patch

Review of attachment 799085 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/BrowserApp.java
@@ +2145,5 @@
>      // HomePager.OnNewTabsListener
>      @Override
>      public void onNewTabs(String[] urls) {
>          for (String url : urls) {
> +            if (!TextUtils.isEmpty(url)) {

Why this change?

::: mobile/android/base/home/LastTabsPage.java
@@ +124,5 @@
>                  }
>  
>                  final String url = c.getString(c.getColumnIndexOrThrow(Combined.URL));
> +                final int tabId = Tabs.getInstance().getTabIdForUrl(url);
> +                if (tabId < 0) {

I don't love that we need to check for the tab id like this, since the point of the ALLOW_SWITCH_TO_TAB flag was that we could hide away that Tabs-dependent logic in onUrlOpen.

Bug 887268 replaced mUrlOpenListener with mNewTabsListener so that we would always open the items in a new tab, but now we have two different cases (open in a new tab or switch to the tab), maybe we should consider getting rid of NewTabsListener and just create a new flag to pass to onUrlOpen (e.g. Flags.NEW_TAB). Then in onUrlOpen, if we don't switch to the tab, we can check this flag and open the tab in a new tab.

However, one issue with this is that onUrlOpen just expects one url, so we would need to change that to support multiple urls, or change the logic in openAllTabs.

Comment 5

[Sriram Ramasubramanian [:sriram]]

(In reply to :Margaret Leibovic from comment #4)
> Comment on attachment 799085 [details] [diff] [review]
> Patch
> 
> Review of attachment 799085 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +2145,5 @@
> >      // HomePager.OnNewTabsListener
> >      @Override
> >      public void onNewTabs(String[] urls) {
> >          for (String url : urls) {
> > +            if (!TextUtils.isEmpty(url)) {
> 
> Why this change?

We create an array of size X. But if there are few tabs already opened (with switch-to-tab enabled), then we might not copy X entries to the array. The last few cells are empty. This leaves the gecko to be in a weird state as the url is "undefined" or "null".

> 
> ::: mobile/android/base/home/LastTabsPage.java
> @@ +124,5 @@
> >                  }
> >  
> >                  final String url = c.getString(c.getColumnIndexOrThrow(Combined.URL));
> > +                final int tabId = Tabs.getInstance().getTabIdForUrl(url);
> > +                if (tabId < 0) {
> 
> I don't love that we need to check for the tab id like this, since the point
> of the ALLOW_SWITCH_TO_TAB flag was that we could hide away that
> Tabs-dependent logic in onUrlOpen.
> 
> Bug 887268 replaced mUrlOpenListener with mNewTabsListener so that we would
> always open the items in a new tab, but now we have two different cases
> (open in a new tab or switch to the tab), maybe we should consider getting
> rid of NewTabsListener and just create a new flag to pass to onUrlOpen (e.g.
> Flags.NEW_TAB). Then in onUrlOpen, if we don't switch to the tab, we can
> check this flag and open the tab in a new tab.
> 
> However, one issue with this is that onUrlOpen just expects one url, so we
> would need to change that to support multiple urls, or change the logic in
> openAllTabs.

So we don't need NewTabsListener anymore? All calls will go through UrlOpenListener, and switch to tab if needed?

Comment 6

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> (In reply to :Margaret Leibovic from comment #4)
> > Comment on attachment 799085 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 799085 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/base/BrowserApp.java
> > @@ +2145,5 @@
> > >      // HomePager.OnNewTabsListener
> > >      @Override
> > >      public void onNewTabs(String[] urls) {
> > >          for (String url : urls) {
> > > +            if (!TextUtils.isEmpty(url)) {
> > 
> > Why this change?
> 
> We create an array of size X. But if there are few tabs already opened (with
> switch-to-tab enabled), then we might not copy X entries to the array. The
> last few cells are empty. This leaves the gecko to be in a weird state as
> the url is "undefined" or "null".

That seems kind of fragile. Wouldn't it be better to just pass an array that only contains the urls we know we want to open? However, if you implement my idea to get rid of NewTabsListener this wouldn't be an issue anymore.

> > 
> > ::: mobile/android/base/home/LastTabsPage.java
> > @@ +124,5 @@
> > >                  }
> > >  
> > >                  final String url = c.getString(c.getColumnIndexOrThrow(Combined.URL));
> > > +                final int tabId = Tabs.getInstance().getTabIdForUrl(url);
> > > +                if (tabId < 0) {
> > 
> > I don't love that we need to check for the tab id like this, since the point
> > of the ALLOW_SWITCH_TO_TAB flag was that we could hide away that
> > Tabs-dependent logic in onUrlOpen.
> > 
> > Bug 887268 replaced mUrlOpenListener with mNewTabsListener so that we would
> > always open the items in a new tab, but now we have two different cases
> > (open in a new tab or switch to the tab), maybe we should consider getting
> > rid of NewTabsListener and just create a new flag to pass to onUrlOpen (e.g.
> > Flags.NEW_TAB). Then in onUrlOpen, if we don't switch to the tab, we can
> > check this flag and open the tab in a new tab.
> > 
> > However, one issue with this is that onUrlOpen just expects one url, so we
> > would need to change that to support multiple urls, or change the logic in
> > openAllTabs.
> 
> So we don't need NewTabsListener anymore? All calls will go through
> UrlOpenListener, and switch to tab if needed?

I think that would be cleaner. What do you think? Lucas probably also has an opinion here, since he's the one who worked on bug 887268.

Comment 7

[Lucas Rocha (:lucasr)]

The reason for the NewTabsListener is described in https://bugzilla.mozilla.org/show_bug.cgi?id=887268#c9. We have to handle all new tab URLs in one go because HomePager fragments are not guaranteed to be around once we open a new tab or load a new URL.

If you want to reuse OnUrlOpenListener here, you'd have to change it to receive multiple URLs, which is not a nice and clean solution I think.

Comment 8

[Lucas Rocha (:lucasr)]

One relevant aspect of this bug is that we should ensure that we select one of the last tabs even if all URLs in the 'last tabs' list are marked with "Switch to tab". Otherwise tapping on "Open all..." will not work consistently. Or we should at least show a toast notification like "All tabs are already open" or something.

Comment 9

[Sriram Ramasubramanian [:sriram]]

So do you want me to not open tabs when they already exist, when the url is sent as a part of NewTabsListener's onNewTabs() ?

Comment 10

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #9)
> So do you want me to not open tabs when they already exist, when the url is
> sent as a part of NewTabsListener's onNewTabs() ?

Yes, that would be how I'd expect this to work I guess. And this seems to be what ibarlow suggested on comment #1 above.

Comment 11

[:Margaret Leibovic]

Comment on attachment 799085 [details] [diff] [review]
Patch

Review of attachment 799085 [details] [diff] [review]:
-----------------------------------------------------------------

Setting this to r- because I don't like passing null entries in the urls array that onNewTabs handles. I think we should just make an array of the URLs we do want to open. Given recent comments, I think I am okay with this approach of using mUrlOpenListener for the switch-to-tab case and mNewTabsListener for the other cases, but I'd like to see a comment added explaining why this is special. Also maybe a comment in openAllTabs explaining what lucasr mentioned about the fragment lifecycle.

We also need to add something to handle the case where all the URLs are already open in tabs. In that case, openAllInTabs wouldn't find a URL to open, so we should probably do what lucasr suggested and select one of them. But that is very much an edge case, perhaps one we can handle in a follow-up bug.

Comment 12

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #10)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #9)
> > So do you want me to not open tabs when they already exist, when the url is
> > sent as a part of NewTabsListener's onNewTabs() ?
> 
> Yes, that would be how I'd expect this to work I guess. And this seems to be
> what ibarlow suggested on comment #1 above.

Aaah! Even my patch does the same thing. My questions is, say I send 10 urls through onNewTabs() and 5 of them are already opened, do I open the rest 5? Basically use onNewTabs() and don't care about the ones opened. Or, use onUrlOpenListener??

Comment 13

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #12)
> (In reply to Lucas Rocha (:lucasr) from comment #10)
> > (In reply to Sriram Ramasubramanian [:sriram] from comment #9)
> > > So do you want me to not open tabs when they already exist, when the url is
> > > sent as a part of NewTabsListener's onNewTabs() ?
> > 
> > Yes, that would be how I'd expect this to work I guess. And this seems to be
> > what ibarlow suggested on comment #1 above.
> 
> Aaah! Even my patch does the same thing. My questions is, say I send 10 urls
> through onNewTabs() and 5 of them are already opened, do I open the rest 5?
> Basically use onNewTabs() and don't care about the ones opened. Or, use
> onUrlOpenListener??

An onNewTabs() call should result in all URLs having a respective tab, which means a new tab should be created if necessary. So, to answer your question: you should create tabs for the 5 URLs that are not already open in tabs.

Comment 14

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

As per the conclusion,

This send all the tabs in "last-session" to onNewTabs(). Now if the tab cannot be switched to (as in, if its not open), then a new tab is created.

Comment 15

[:Margaret Leibovic]

Comment on attachment 808880 [details] [diff] [review]
Patch

Review of attachment 808880 [details] [diff] [review]:
-----------------------------------------------------------------

Well this is a lot nicer! I just have one possible suggestion, but I'd be fine if we landed it as-is.

::: mobile/android/base/BrowserApp.java
@@ +2275,4 @@
>          for (String url : urls) {
> +            if (!maybeSwitchToTab(url, flags)) {
> +                openUrl(url, true);
> +            }

This logic is identical to what's in onUrlOpen, so I wonder if we should just call onUrlOpen(url, flags); here instead.

Comment 16

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #15)
> This logic is identical to what's in onUrlOpen, so I wonder if we should
> just call onUrlOpen(url, flags); here instead.

Drive-by: It's usually not a good idea to call listeners directly out of their intended context (it's error-prone and might lead to confusion). If you want to avoid code duplication here, I suggest creating a private method that is used on both onNewTabs() and onUrlOpen().

Comment 17

[Sriram Ramasubramanian [:sriram]]

I agree with lucasr. Also, adding a small method for a single line of code is not that good when proguard is not enabled. Hence pushing as-is.
https://hg.mozilla.org/integration/fx-team/rev/a6e1c75e5aa4

Comment 18

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/a6e1c75e5aa4

Comment 19

[Lucas Rocha (:lucasr)]

Sriram, request uplift to Aurora?

Comment 20

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 808880 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New tabs are created when there are existing tabs with same URL in last-tabs section.
User impact if declined: Annoying to have more tabs of same url.
Testing completed (on m-c, etc.): Landed 15 days back.
Risk to taking this patch (and alternatives if risky): None. All good. 
String or IDL/UUID changes made by this patch: None.

Comment 21

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f3c46c1e3aa5