919301 - RFE: When in private browsing stop showing "switch to tab" from normal browsing.

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, enhancement)

Description

[Biju]

RFE: When in private browsing stop showing "switch to tab" from normal browsing. (Firefox for Mobile)

If I am not going to a previously bookmarked site, My normal browsing habit is to go first to https://www.google.com/ then search and find site to go.

So many times I will have already opened google.com on a normal tab.
Now if try to do private browsing "switch to tab" for google.com on the normal tab will show up in the dropdown list.

Can we STOP displaying "switch to tab" from normal browsing in a private browsing mode.

Comment 1

[:Margaret Leibovic]

This will involve changing things in two places.

First, you'll need to update where we decide to show the "Switch to tab" UI in TwoLinePageRow:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/TwoLinePageRow.java#156

To do this, you should get the tab we're thinking of showing the UI for and, call tab.isPrivate() to see if it's a private tab.

Second, you'll need to update where we actually handle clicks on these items in BrowserApp.maybeSwitchToTab:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1270

You should do something similar and only switch to the tab if it's not private.

Comment 2

[Aaron Train [:aaronmt]]

Is this on par with what desktop does?

Comment 3

[:Margaret Leibovic]

(In reply to Aaron Train [:aaronmt] from comment #2)
> Is this on par with what desktop does?

Doing a quick test, switch-to-tab does not appear as an option in private windows on desktop.

Thinking about this bug a bit more, I realized what I described in comment 1 is "Don't show switch-to-tab for private tabs", which seems like a valid thing to do, but it is different than "Don't show switch-to-tab while *in* a private tab". The logic tweaks would happen in the same place, but let's get some feedback from ibarlow to see what he thinks we should do.

I also think this would be a good motivator for fixing bug 904217, since I'd want to see some tests for this behavior.

Comment 4

[Biju]

I am also happy with "Don't show switch-to-tab for private tabs"

Comment 5

[Lauren]

Hey guys, I'm looking for a good first bug to work on. If you decide on the logic, I'd be happy to take a stab at this.

Comment 6

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #3)
> (In reply to Aaron Train [:aaronmt] from comment #2)
> > Is this on par with what desktop does?
> 
> Doing a quick test, switch-to-tab does not appear as an option in private
> windows on desktop.
> 
> Thinking about this bug a bit more, I realized what I described in comment 1
> is "Don't show switch-to-tab for private tabs", which seems like a valid
> thing to do, but it is different than "Don't show switch-to-tab while *in* a
> private tab". The logic tweaks would happen in the same place, but let's get
> some feedback from ibarlow to see what he thinks we should do.
> 

I think both of those behaviours fit together nicely.

Comment 7

[:Margaret Leibovic]

Thanks for thinking about this, Ian!

Lauren, I'm assigning this bug to you. Let me know if you need more direction. You're also more than welcome to ask questions in #mobile on irc.mozilla.org. We're there to help you! :)

Comment 8

[Lauren]

Attachment: WIP

Comment 9

[Lauren]

So I ran into an interesting problem. I modified those 2 functions mentioned above and it seems to work - except for the case where the same URL is open in both private browsing and normal browsing.

Example:
Go to google.com in normal browsing
Switch to private browsing tab and go to google.com
Open a new tab in private browsing and start typing in Google.com
Expected behaviour: google should have a "Switch to tab" label under it
Actual behavior: google has the URL displayed under it

So this occurs because the function "getTabIdForUrl" only returns 1 Tab ID, which happens to not be the one we are looking for. Should I add another function in Tabs.java to find an array of Tab IDs?

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Lauren from comment #9)

> So this occurs because the function "getTabIdForUrl" only returns 1 Tab ID,
> which happens to not be the one we are looking for. Should I add another
> function in Tabs.java to find an array of Tab IDs?

Nice find. I think we should change the existing function to getTabIdsForUrl and make it return an array. We'd need to update any existing callers, but I think that's better than a new method.

I'm open to counterpoints though. It might make the existing code a little messier.

Comment 11

[Lauren]

Attachment: WIP

WIP

Comment 12

[Lauren]

Hi! I'm a bit stuck so I was wondering if I could get some feedback. It seems that the code I've modified (attached) works for normal browsing, but on private browsing the displayed URL never changes to "switch to tab" when there is a valid tab to switch to. Is there anything I've overlooked?

Comment 13

[:Margaret Leibovic]

Comment on attachment 812441 [details] [diff] [review]
WIP

Thanks for posting a WIP! When you upload a WIP, you can set the feedback? flag to request feedback from a certain individual. I'll take a look at this today.

Comment 14

[:Margaret Leibovic]

(In reply to Lauren from comment #12)
> Hi! I'm a bit stuck so I was wondering if I could get some feedback. It
> seems that the code I've modified (attached) works for normal browsing, but
> on private browsing the displayed URL never changes to "switch to tab" when
> there is a valid tab to switch to. Is there anything I've overlooked?

I can't reproduce this problem with your patch applied... I do see the "Switch to tab" text for other private tabs that I have open when I'm in private browsing mode.

Is it possible that what you're seeing is that we don't create history entries in private browsing mode, so those other private tabs aren't shown in the history list to begin with?

Comment 15

[:Margaret Leibovic]

Comment on attachment 812441 [details] [diff] [review]
WIP

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

Nice work, this is definitely on the right track. However, I have some suggestions to make this more straightforward.

::: mobile/android/base/BrowserApp.java
@@ +1278,3 @@
>              hideHomePager();
>          } else {
> +            tabs.selectTab(tabIds.get(0));

See my comment below... if we still only return one tab id, we won't need to worry about this.

::: mobile/android/base/Tabs.java
@@ +587,5 @@
>  
>      /**
>       * Looks for an open tab with the given URL.
>       *
>       * @return id of an open tab with the given URL; -1 if the tab doesn't exist.

Whatever we end up doing here, you should make sure you update the documentation accordingly.

@@ +589,5 @@
>       * Looks for an open tab with the given URL.
>       *
>       * @return id of an open tab with the given URL; -1 if the tab doesn't exist.
>       */
> +    public ArrayList<Integer> getTabIdsForUrl(String url, boolean isPrivate) {

Since we only actually ever want to use one tab id, not the whole array, using this parameter to ask either for a regular or private tab solves the problem for us, without needing to return an array of ids.

So I think we should keep this method as getTabIdForUrl, but keep this check for regular or private tabs.

@@ +595,4 @@
>          for (Tab tab : mOrder) {
> +            if ((TextUtils.equals(tab.getURL(), url) ||
> +                TextUtils.equals(ReaderModeUtils.getUrlFromAboutReader(tab.getURL()), url)) &&
> +                isPrivate == tab.isPrivate()) {

This if statement is starting to get a bit hairy, so I think we should break some of it out into a local variable. How about something like this instead:

boolean urlMatches = TextUtils.equals(tab.getURL(), url) ||
                     TextUtils.equals(ReaderModeUtils.getUrlFromAboutReader(tab.getURL()), url);
if (urlMatches && isPrivate == tab.isPrivate()) {
    ...
}

... actually, to make this more correct, I think we should check to see if we actually have an "about:reader" url before just calling getUrlFromAboutReader, since that method doesn't actually check to make sure we're parsing an "about:reader" url. So something like:

String tabUrl = tab.getURL();
if (ReaderModeUtils.isAboutReader(tabUrl)) {
    tabUrl = ReaderModeUtils.getUrlFromAboutReader(tabUrl);
}

if (TextUtils.equals(tabUrl, url) && (isPrivate == tab.isPrivate())) {
    ...
}

(Sorry this is a bit out of scope for your bug, but we're rearranging this logic anyway :)

Comment 16

[Lauren]

Attachment: bug-919301-fix.patch

Added all your suggestions to the patch!

Comment 17

[:Margaret Leibovic]

Comment on attachment 813426 [details] [diff] [review]
bug-919301-fix.patch

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

Looking good! I just have a few more comments to address, and then this will get an r+.

::: mobile/android/base/BrowserApp.java
@@ +73,5 @@
>  import java.io.FileNotFoundException;
>  import java.net.URLEncoder;
>  import java.util.EnumSet;
>  import java.util.Vector;
> +import java.util.ArrayList;

Another unused import.

::: mobile/android/base/Tabs.java
@@ +587,5 @@
>  
>      /**
>       * Looks for an open tab with the given URL.
>       *
>       * @return id of an open tab with the given URL; -1 if the tab doesn't exist.

Update this documentation to document the parameters that get passed to this method.

@@ +594,3 @@
>          for (Tab tab : mOrder) {
> +            String tabUrl = tab.getURL();
> +            if(ReaderModeUtils.isAboutReader(tabUrl)) {

Nit: Put a space between the if and the open paren.

::: mobile/android/base/home/TwoLinePageRow.java
@@ +29,5 @@
>  import android.view.LayoutInflater;
>  import android.widget.LinearLayout;
>  import android.widget.TextView;
>  
> +import java.util.ArrayList;

You don't need this import anymore.

@@ +151,5 @@
>          }
>      }
>  
>      /**
>       * Replaces the page URL with "Switch to tab" if there is already a tab open with that URL.

Update this documentation to indicate that we only look for open tabs that are private or not private, depending on the current tab.

Comment 18

[Lauren]

Attachment: bug-919301-fix.patch

all cleaned up now!

Comment 19

[:Margaret Leibovic]

Comment on attachment 813665 [details] [diff] [review]
bug-919301-fix.patch

Great! (Sorry for the slow response, I was away from bugzilla for a long weekend, and I'm just catching up now!)

Comment 20

[:Margaret Leibovic]

I'm setting the checkin-needed keyword on this bug, which means that someone with commit access will come along and land your patch for you!

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/5448a5ffcccf

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5448a5ffcccf

Comment 23

[Teodora Vermesan (:TeoVermesan)]

Test Case added in moztrap:
https://moztrap.mozilla.org/manage/case/10497/