587031 - Entering/Exiting Private Browsing mode from within Tab Candy (Panorama) is presently ambiguous

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Aaron Train [:aaronmt]]

Currently on [1], when one enters Private Browsing mode, the browser is put in a state where it is unclear to the enduser what action has been taken. While the browser does correctly enter Private Browsing mode, it is unclear when the user is sitting in the Tab Candy screen. 

Ideally, Tab Candy should close and go back to regular tab browsing, or perhaps it should indicate that the browser is presently in Private Browsing mode - akin to the titlebar in regular tabbed browsing at the very least. 

UX opinions?

[1] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b4pre) Gecko/20100813 Minefield/4.0b4pre

Comment 3

[jag (Peter Annema)]

I'd say it should just exit Panorama and show the single "Private Browsing" tab. When you then exit Private Browsing I don't think it should switch back to Panorama, just show the most recently viewed group of tabs.

Comment 4

[Dietrich Ayala (:dietrich)]

Yep, need to figure this out, blocking+ and requesting ui-wanted and cc'ing aza.

Comment 5

[Tan Wei Lin Jason [:Ryuji]]

Right now Entering and exiting Private Browsing Mode will cause groups to disappear leaving tabs to be out of their groups. Should I report this as a separate bug or ?

Comment 6

[Ian Gilman [:iangilman]]

(In reply to comment #5)
> Right now Entering and exiting Private Browsing Mode will cause groups to
> disappear leaving tabs to be out of their groups. Should I report this as a
> separate bug or ?

I'd say that's bug 594644.

Comment 7

[Ian Gilman [:iangilman]]

Attachment: patch v2 (requires 595371, 595374, 600741)

Comment 8

[(no longer active)]

Comment on attachment 479640 [details] [diff] [review]
patch v2 (requires 595371, 595374, 600741)

Here's a bunch of comments.  Firstly, _privateBrowsingEntered is not really useful, you can just access the property on gPrivateBrowsing directly.  The getter is quite efficient (it just returns a boolean variable).

+        // If we're currently in the process of entering private browsing,
+        // we don't want to go to the Tab View UI. 
+        if (gPrivateBrowsing.privateBrowsingEnabled &&
+            !self._privateBrowsingEntered)
+          return; 

I don't really understand this condition.  Is there any conceivable state where this evaluates to true?

The test is not really testing what the patch adds (and what comment 0) complains about.  I guess the issue in question is what happens when you switch to PB mode (and out of it) when tabview is visible, right?

Also, PB uses sessionstore's getBrowserState and setBrowserState APIs behind the scenes, so we need to make sure that what tabview does with sessionstore is correctly reflected in those APIs as well.  I'm kind of certain that it's the case, but we need to have tests to make sure of that.

Further more, it would be nice to test everything that tabview does in PB mode as well.  I guess we could move tests which make sense in that situation to a sub-directory, and once run them outside of PB mode, and once again inside this mode.

Comment 9

[(no longer active)]

Also, the test needs to make sure that about:privatebrowsing is actually shown when you enter the PB mode, and the correct session (with all the windows/tabs/tabview data) is restored when you exit this mode.  In addition, the test needs to make sure that when you enter the PB mode, all of the tabview specific data is cleared (along with the rest of the sessionstore data).

Comment 10

[Ian Gilman [:iangilman]]

(In reply to comment #8)
> Comment on attachment 479640 [details] [diff] [review]
> patch v2 (requires 595371, 595374, 600741)
> 
> Here's a bunch of comments.  Firstly, _privateBrowsingEntered is not really
> useful, you can just access the property on gPrivateBrowsing directly.  The
> getter is quite efficient (it just returns a boolean variable).
> 
> +        // If we're currently in the process of entering private browsing,
> +        // we don't want to go to the Tab View UI. 
> +        if (gPrivateBrowsing.privateBrowsingEnabled &&
> +            !self._privateBrowsingEntered)
> +          return; 
> 
> I don't really understand this condition.  Is there any conceivable state where
> this evaluates to true?

The time span we need to be aware of is from the moment private browsing becomes enabled and the tabs start getting removed, to the time the tabs are done being removed and private browsing sends out its "enter" notification. During this time, Panorama needs to respond differently to the tabs going away than it normally would. That check above evaluates to true precisely during that time period. If there's a better way to get this info, please let me know. 
 
> The test is not really testing what the patch adds (and what comment 0)
> complains about.  I guess the issue in question is what happens when you switch
> to PB mode (and out of it) when tabview is visible, right? 

Oops! Good point; I'll add that. 

In this patch I'm also fixing the fact that it takes you into Panorama if you initiate private browsing when you're not in Panorama. 

> Also, PB uses sessionstore's getBrowserState and setBrowserState APIs behind
> the scenes, so we need to make sure that what tabview does with sessionstore is
> correctly reflected in those APIs as well.  I'm kind of certain that it's the
> case, but we need to have tests to make sure of that.

I'm not sure what you mean by this. Perhaps we need another bug for this?

> Further more, it would be nice to test everything that tabview does in PB mode
> as well.  I guess we could move tests which make sense in that situation to a
> sub-directory, and once run them outside of PB mode, and once again inside this
> mode.

This definitely needs to go in another bug.

(In reply to comment #9)
> Also, the test needs to make sure that about:privatebrowsing is actually shown
> when you enter the PB mode, and the correct session (with all the
> windows/tabs/tabview data) is restored when you exit this mode.  In addition,
> the test needs to make sure that when you enter the PB mode, all of the tabview
> specific data is cleared (along with the rest of the sessionstore data).

Also outside the scope of this bug. Probably a lot of it will be appropriate for bug 594644, but perhaps additional bugs need to be filed?

I'll probably need help with some of these tests when it comes to them... I hope you don't mind!

Comment 11

[(no longer active)]

(In reply to comment #10)
> (In reply to comment #8)
> > Comment on attachment 479640 [details] [diff] [review]
> > patch v2 (requires 595371, 595374, 600741)
> > 
> > Here's a bunch of comments.  Firstly, _privateBrowsingEntered is not really
> > useful, you can just access the property on gPrivateBrowsing directly.  The
> > getter is quite efficient (it just returns a boolean variable).
> > 
> > +        // If we're currently in the process of entering private browsing,
> > +        // we don't want to go to the Tab View UI. 
> > +        if (gPrivateBrowsing.privateBrowsingEnabled &&
> > +            !self._privateBrowsingEntered)
> > +          return; 
> > 
> > I don't really understand this condition.  Is there any conceivable state where
> > this evaluates to true?
> 
> The time span we need to be aware of is from the moment private browsing
> becomes enabled and the tabs start getting removed, to the time the tabs are
> done being removed and private browsing sends out its "enter" notification.
> During this time, Panorama needs to respond differently to the tabs going away
> than it normally would. That check above evaluates to true precisely during
> that time period. If there's a better way to get this info, please let me know. 

Unless I'm missing something, this check does not do what you want.  This is where we send the private-browsing notification: <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#520>.  The private browsing status (what nsIPrivateBrowsingService.privateBrowsingEnabled returns) is set on line 506, and setting the session state (what removes tabs and opens new ones) is done inside _onAfterPrivateBrowsingModeChange on line 523.  This means that when you receive the private-browsing notification, you're setting your boolean variable, and from that point on it remains in sync with nsIPrivateBrowsingService.privateBrowsingEnabled.

To do what you want, you have to first note that there are two setBrowserState calls.  The first one which happens before private-browsing basically sets the browser state to a blank state.  To trap that, you need to observe private-browsing-change-granted, and in that notification, set up an observer for sessionstore-browser-state-restored, and remove the observer when you receive the notification.  This should happen before private-browsing, except if we're quitting (we don't set the browser state to an empty session in that case).  You can detect that case by checking in private-browsing whether the first param (which is an nsISupportsPRBool) is set to true.

The second setBrowserState call happens after private-browsing has been sent, to either open about:privatebrowsing if we're entering the PB mode, or restore the user's old session if we're leaving it.  To trap that, you should setup another sessionstore-browser-state-restored observer and remove it when you receive the notification.  That's going to happen after private-browsing has been fired.

Sorry that this is so complex.  To see how things actually work, see the privateBrowsingEnabled setter <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#481> and also the _onBeforePrivateBrowsingModeChange and _onAfterPrivateBrowsingModeChange methods.

Out of curiosity, how did you test that this code does actually what you want?  Because you've tested it the wrong way.  :-)

> In this patch I'm also fixing the fact that it takes you into Panorama if you
> initiate private browsing when you're not in Panorama. 

Fair enough, but we should test all possible interactions.

> > Also, PB uses sessionstore's getBrowserState and setBrowserState APIs behind
> > the scenes, so we need to make sure that what tabview does with sessionstore is
> > correctly reflected in those APIs as well.  I'm kind of certain that it's the
> > case, but we need to have tests to make sure of that.
> 
> I'm not sure what you mean by this. Perhaps we need another bug for this?

zpao would probably know if tabview is safe with set/getBrowserState.  Feel free to move this part to another bug though.

> > Further more, it would be nice to test everything that tabview does in PB mode
> > as well.  I guess we could move tests which make sense in that situation to a
> > sub-directory, and once run them outside of PB mode, and once again inside this
> > mode.
> 
> This definitely needs to go in another bug.

OK.

> (In reply to comment #9)
> > Also, the test needs to make sure that about:privatebrowsing is actually shown
> > when you enter the PB mode, and the correct session (with all the
> > windows/tabs/tabview data) is restored when you exit this mode.  In addition,
> > the test needs to make sure that when you enter the PB mode, all of the tabview
> > specific data is cleared (along with the rest of the sessionstore data).
> 
> Also outside the scope of this bug. Probably a lot of it will be appropriate
> for bug 594644, but perhaps additional bugs need to be filed?

Maybe.  I tried to give you an overview of what needs to happen for PB support, but feel free to spread it across multiple bugs if needed.  And make sure to CC me as well so that I can help you with the implementation.

> I'll probably need help with some of these tests when it comes to them... I
> hope you don't mind!

Not at all!

Comment 12

[Ian Gilman [:iangilman]]

Attachment: patch v3 (requires 595371, 595374, 600741)

(In reply to comment #11)
> Unless I'm missing something, this check does not do what you want.  

I assure you it does what I want; it evaluates to true during the first sessionrestore (between when the status is set and the "private-browsing" notification is sent). At any rate, it's certainly a hack, so thank you for filling me in on the right way to do it! Fixed. 

> Out of curiosity, how did you test that this code does actually what you want? 

I just ran and observed. Without that code, if you're not in the Panorama UI and you hit PB, you're sent into the Panorama UI. With that code, you're not. I realize that's not the scenario described in the bug, but that's the scenario that the code in question is designed to deal with. 

> Maybe.  I tried to give you an overview of what needs to happen for PB support,
> but feel free to spread it across multiple bugs if needed.  And make sure to CC
> me as well so that I can help you with the implementation.

I've put several of your comments into bug 594644, as well as starting bug 601778.

I've updated the patch. Note that I'm subscribing to all sessionstore-browser-state-restored notifications; that'll be necessary for full support of sessionrestore, so I might as well get it started now.

Comment 13

[(no longer active)]

Comment on attachment 480776 [details] [diff] [review]
patch v3 (requires 595371, 595374, 600741)

So, about the test, it looks much better, but still, you need to make sure that about:privatebrowsing is opened when you enter private browsing, and your tabs are restored correctly when you exit PB.

I took a brief look at the transition logic, but honestly I'm too exhasted to follow all the code paths right now (been debugging a memory leak for the past three days, with success as of a few minutes ago!).  Can you document what the transitions should look like?

Comment 14

[Ian Gilman [:iangilman]]

Attachment: patch v4 (requires 595371, 595374, 600741)

(In reply to comment #13)
> So, about the test, it looks much better, but still, you need to make sure that
> about:privatebrowsing is opened when you enter private browsing, and your tabs
> are restored correctly when you exit PB.

Done.

> I took a brief look at the transition logic, but honestly I'm too exhasted to
> follow all the code paths right now (been debugging a memory leak for the past
> three days, with success as of a few minutes ago!).  Can you document what the
> transitions should look like?

We're keeping track of the transition with UI._privateBrowsing.transitionStage and .transitionMode. The stage is 0 if not transitioning, 1 if just started ("change-granted"), 2 after the first sessionrestore, 3 after the "private-browsing" notification, and then back to 0 after the second sessionrestore. The mode is "" if not transitioning, otherwise it's "enter" or "exit" as appropriate. 

When transitioning to PB, we exit Panorama if necessary (making note of the fact that we were there so we can return after PB) and make sure we don't reenter Panorama due to all of the session restore tab manipulation (which otherwise we might). 

When transitioning away from PB, we reenter Panorama if we had been there directly before PB. 

Does that answer your question?

Comment 15

[(no longer active)]

(In reply to comment #14)
> Created attachment 481655 [details] [diff] [review]
> patch v4 (requires 595371, 595374, 600741)
> 
> (In reply to comment #13)
> > So, about the test, it looks much better, but still, you need to make sure that
> > about:privatebrowsing is opened when you enter private browsing, and your tabs
> > are restored correctly when you exit PB.
> 
> Done.

The test looks awesome!

> > I took a brief look at the transition logic, but honestly I'm too exhasted to
> > follow all the code paths right now (been debugging a memory leak for the past
> > three days, with success as of a few minutes ago!).  Can you document what the
> > transitions should look like?
> 
> We're keeping track of the transition with UI._privateBrowsing.transitionStage
> and .transitionMode. The stage is 0 if not transitioning, 1 if just started
> ("change-granted"), 2 after the first sessionrestore, 3 after the
> "private-browsing" notification, and then back to 0 after the second
> sessionrestore. The mode is "" if not transitioning, otherwise it's "enter" or
> "exit" as appropriate. 
> 
> When transitioning to PB, we exit Panorama if necessary (making note of the
> fact that we were there so we can return after PB) and make sure we don't
> reenter Panorama due to all of the session restore tab manipulation (which
> otherwise we might). 
> 
> When transitioning away from PB, we reenter Panorama if we had been there
> directly before PB. 
> 
> Does that answer your question?

Yes!

Comment 16

[(no longer active)]

Comment on attachment 481655 [details] [diff] [review]
patch v4 (requires 595371, 595374, 600741)

>+  // Variable: _privateBrowsing
>+  // Keeps track of info related to private browsing, including: 
>+  //   transitionStage - what step we're on in entering/exiting PB
>+  //   transitionMode - whether we're entering or exiting PB
>+  //   wasInTabView - whether TabView was visible before we went into PB
>+  _privateBrowsing: {},

Nit: declare the object properties right here, instead of in the init function.

>+      if (currentTab && currentTab.tabItem)
>+        currentTab.tabItem.setZoomPrep(false);

What is this change doing here?!

>+        self._privateBrowsing.transitionStage = 1;
>+        self._privateBrowsing.transitionMode = aData;

Please refrain from using aData in this way.  In the future, it is possible for us to add data parameters besides enter and exit, in which case this will break.  Please check to make sure that aData is either "enter" or "exit" explicitly.

Also, please document the transitions somewhere in the code like you did in comment 14.
f=me with the above addressed.

Comment 17

[Ian Gilman [:iangilman]]

Attachment: patch v5 (requires 595374, 600741)

(In reply to comment #16)
> Nit: declare the object properties right here, instead of in the init function.

Done.

> >+      if (currentTab && currentTab.tabItem)
> >+        currentTab.tabItem.setZoomPrep(false);
> 
> What is this change doing here?!

Good point! It belongs in bug 595374. Done.

> Please refrain from using aData in this way.  In the future, it is possible for
> us to add data parameters besides enter and exit, in which case this will
> break.  Please check to make sure that aData is either "enter" or "exit"
> explicitly.

Done.

> Also, please document the transitions somewhere in the code like you did in
> comment 14.

Done.

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 482407 [details] [diff] [review]
patch v5 (requires 595374, 600741)

looks good, r=me

Comment 19

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/7d732041dbb7

Comment 20

[Tracy Walker [:tracy]]

verified with nightly minefield builds of 20101021