Closed Bug 1238796 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox47 verified)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- verified

People

(Reporter: antlam, Assigned: varunnaganathan912, Mentored)

References

Details

Attachments

(1 file, 2 obsolete files)

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?
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).
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.
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.
(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.
Hi,I want to work on this.Could someone guide me to the code for the implementation of the private browsing tabs?
Thanks
I'll give :mcomella a ping to see if he has cycles :)
Flags: needinfo?(michael.l.comella)
(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
Flags: needinfo?(michael.l.comella)
Mentor: michael.l.comella, 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);
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!
Attachment #8716488 - Flags: review?(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.
Attachment #8716488 - Flags: review?(margaret.leibovic) → review+
Hi,
Updated the patch to match all changes specified by you.
Please review.
Thanks!
Attachment #8717160 - Flags: review?(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.
Attachment #8717160 - Flags: review?(margaret.leibovic) → review+
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.
Attachment #8716488 - Attachment is obsolete: true
Assignee: nobody → varunnaganathan912
Hi,made the changes.
Also I was unable to figure out how to mark patches as obsolete.
Do guide me on that.
Thanks!
Attachment #8717542 - Flags: review?(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".
Attachment #8717160 - Attachment is obsolete: true
Attachment #8717542 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
(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.
https://hg.mozilla.org/mozilla-central/rev/9fd143cd5996
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
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)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.