1238796 - Say "Closed Private Browsing" in snackbar when closing a private tab

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Currently, we call out information about the tab that's closed and it can be displayed (even for just a brief second) over a normal tab.

This detail seems weird to me. I think we can improve this by offering a generic "Closed Private Browsing" message, maybe even without an "undo" action.

Thoughts?

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The title seems like a good detail to change but I don't think removing the UNDO button is necessary (and it could be annoying in the case that a tab is accidentally closed).

Comment 2

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Hm, maybe. But I also think the threshold for "annoying" is a bit higher in Private Browsing. The ability to "restore" a tab after closing it like that feels contradictory to the idea of "not leaving a trail behind".

I do think we should prioritize for privacy in PB through these little details though.

Comment 3

[:Margaret Leibovic]

I don't think we should remove the "undo" button in the snackbar. The snackbar only appears very briefly, so someone would have to be grabbing the phone right from your hand at that moment to take advantage of this.

And in the case where you accidentally close the tab, it would be really annoying to not have the "undo" button.

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Margaret Leibovic from comment #3)
> I don't think we should remove the "undo" button in the snackbar. The
> snackbar only appears very briefly, so someone would have to be grabbing the
> phone right from your hand at that moment to take advantage of this.
> 
> And in the case where you accidentally close the tab, it would be really
> annoying to not have the "undo" button.

Yeah, we could separate that issue out. Mike and you raise some fair points.

But I think changing the messaging in the snack bar (this bug) still makes sense here.

Comment 5

[varunnaganathan912]

Hi,I want to work on this.Could someone guide me to the code for the implementation of the private browsing tabs?
Thanks

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I'll give :mcomella a ping to see if he has cycles :)

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to varunnaganathan912 from comment #5)
> Hi,I want to work on this.Could someone guide me to the code for the
> implementation of the private browsing tabs?
> Thanks

Sure. I used our mxr code search system (mxr.mozilla.org) to search for "undo" in the mobile/android tree [1] and found the related code at [2]. It looks like it's in the browser.js file in a method called _handleTabClosed. Our javascript strings are defined in mobile/android/locales/en-US/chrome/browser.properties (such as [3]).

[1]: https://mxr.mozilla.org/mozilla-central/search?string=undo&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1320
[3]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/chrome/browser.properties#189

Comment 8

[:Margaret Leibovic]

Gah, I mid-aired with Mike! I was going to say...

This is where we show the "Undo closed tab" snackbar:
http://hg.mozilla.org/mozilla-central/annotate/f53533d9eb77/mobile/android/chrome/content/browser.js#l1311
 
You'll want to modify this to show a different message if it's a private tab. To find that out, you can use something like:

let isPrivate = PrivateBrowsingUtils.isBrowserPrivate(aTab.browser);

Comment 9

[varunnaganathan912]

Attachment: Changed message on closing of Private Tab

Hi,
I changed the message on closing of a Private Tab.
I have tried to follow the general convention and style of code already present.
Please Review!

Comment 10

[:Margaret Leibovic]

Comment on attachment 8716488 [details] [diff] [review]
Changed message on closing of Private Tab

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

This looks great, thanks! I have a few small style comments, so please attach a new patch with those issues addressed, and we can land this.

::: mobile/android/chrome/content/browser.js
@@ +1319,5 @@
> +
> +      if (isPrivate) {
> +        message = Strings.browser.GetStringFromName("privateClosedMessage.message");
> +      }
> +      else if (title) {

Nit: To follow our coding style, put the else on the same line as the close brace.

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +189,5 @@
>  undoCloseToast.message=Closed %S
>  
> +#Private Tab closed message
> +#LOCALIZATION NOTE (privateClosedMessage.message(: This message appears 
> +# when the user closes a private tab. 

Nit: Put a space between the # and the text, the second paren should be a close paren, and you should delete the trailing whitespace at the end of these lines.

Comment 11

[varunnaganathan912]

Attachment: Changed message on closing of Private Tab

Hi,
Updated the patch to match all changes specified by you.
Please review.
Thanks!

Comment 12

[:Margaret Leibovic]

Comment on attachment 8717160 [details] [diff] [review]
Changed message on closing of Private Tab

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

::: mobile/android/chrome/content/browser.js
@@ +1314,5 @@
>        let closedTabData = ss.getClosedTabs(window)[0];
>  
>        let message;
>        let title = closedTabData.entries[closedTabData.index - 1].title;
> +      //change

Looks like you have an extraneous comment in here... please remove this before we land it.

Comment 13

[:Margaret Leibovic]

Comment on attachment 8716488 [details] [diff] [review]
Changed message on closing of Private Tab

You should also mark old versions of patches as obsolete, to avoid confusion when looking at the bug.

Comment 14

[varunnaganathan912]

Attachment: Changed message on closing of Private Tab

Hi,made the changes.
Also I was unable to figure out how to mark patches as obsolete.
Do guide me on that.
Thanks!

Comment 15

[:Margaret Leibovic]

(In reply to varunnaganathan912 from comment #14)
> Created attachment 8717542 [details] [diff] [review]
> Changed message on closing of Private Tab
> 
> Hi,made the changes.
> Also I was unable to figure out how to mark patches as obsolete.
> Do guide me on that.
> Thanks!

When uploading a new patch, there's an "Obsoletes" section with checkboxes next to old patches. Alternately, on a single patch attachment there is an "obsolete" checkbox if you hit "edit details".

Comment 16

[varunnaganathan912]

(In reply to :Margaret Leibovic from comment #15)
> (In reply to varunnaganathan912 from comment #14)
> > Created attachment 8717542 [details] [diff] [review]
> > Changed message on closing of Private Tab
> > 
> > Hi,made the changes.
> > Also I was unable to figure out how to mark patches as obsolete.
> > Do guide me on that.
> > Thanks!
> 
> When uploading a new patch, there's an "Obsoletes" section with checkboxes
> next to old patches. Alternately, on a single patch attachment there is an
> "obsolete" checkbox if you hit "edit details".

Ok got it..
Thanks.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9fd143cd5996

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9fd143cd5996

Comment 19

[Teodora Vermesan (:TeoVermesan)]

Go to gmail.com on private browsing. Close it. "Closed Private Browsing" snackbar is displayed, so:
Verified as fixed using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 47.0a1 (2016-02-12)