473843 - Make transition into Private Browsing mode seamless

Status: VERIFIED FIXED

(Firefox :: Private Browsing, enhancement)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090114 Shiretoko/3.1b3pre Ubiquity/0.1.4 ID:20090114020333

The current UI behavior while entering Private Browsing mode is somewhat confusing. All windows get hidden for a while until a new window with the private browsing start page opens. It works fine the other way around. Leaving PB mode there is a seamless transition.

Safari which also has the PB mode included does it in a nice seamless way. There is no flickering visible at all.

We should probably do the same. The currently active window shouldn't be closed.

Ehsan, I did a search on Bugzilla but haven't found anything about that high visual behavior. Why that wasn't covered already?

No idea, if this qualifies for a blocker but lets try.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Nice try.

(Yes, we should do it. No, it doesn't block. There may even be bugs on file about making this better.)

Comment 2

[(no longer active)]

There may be a way to do this, CCing Simon to confirm.

Is it possible to use nsISessionStore::setBrowserState with a string parameter representing the "initial" state?  What I'm thinking is building a string representation of a state containing a single window and tab displaying about:privatebrowsing, and then calling nsISessionStore::setBrowserState with that string parameter and let the session store service handle closing of windows, and reusing the first window to display about:privatebrowsing.

This should make the transition into the private browsing mode as seamless as the transition out of it (because we use nsISessionStore::setBrowserState for the latter case).

Comment 3

[(no longer active)]

Attachment: Patch (v1)

Actually I have a patch which implements the idea I talked about in comment 2.  Henrik: I've submitted a try server build for you to test.

Comment 4

[Simon Bünzli]

Comment on attachment 359727 [details] [diff] [review]
Patch (v1)

Yeah, that's the way it should work, with one thing changed:

> +          let privateBrowisngState = Cu.evalInSandbox("(" + this._savedBrowserState + ")",
> +                                                      new Cu.Sandbox("about:blank"));
> +          // Remove all windows but the first one
> +          while (privateBrowisngState.windows.length > 1)
> +            privateBrowisngState.windows.pop();
> +          // Only display one tab showing about:privatebrowsing
> +          privateBrowisngState.windows[0].tabs = [{
> +              entries: [{
> +                url: "about:privatebrowsing"
> +              }]
> +            }];
> +          // The Closed Tabs list should be empty
> +          privateBrowisngState.windows[0]._closedTabs = [];

Do you really need any information about the current session's state? The above code still retains e.g. cookies, so I'd rather explicitly copy over what you need instead of trying to delete all the rest. You can start with

let privateBrowsingState = { windows: [{ "tabs": [{
  entries: [{ url: "about:privatebrowsing" }] }] }] };

AFAICT, you really only need the window's "sizemode" property if it's maximized (because we default to "normal" when the property is omitted).

>  #ifndef XP_WIN
>  #define BROKEN_WM_Z_ORDER
>  #endif

That's no longer needed, is it?

Comment 5

[(no longer active)]

I'll update the patch based on Simon's comments shortly, but in the mean time here are the try server builds for the current patch (the behavior should be the same as far as the transition is concerned):

<https://build.mozilla.org/tryserver-builds/2009-01-30_01:[e-mail]-pbseamless/>

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ehsan, I've take a look at the tryserver build and following I've noticed:

On OS X everything looks good! But on Windows all tabs get closed really slowly. Means if you have open more than 20 tabs and enter the PB mode, it takes about 3-4 seconds until each tab is closed and the pb page is displayed. That doesn't look nice and should be at the same speed as on OS X and when leaving the pb mode.

Comment 7

[(no longer active)]

(In reply to comment #6)
> Ehsan, I've take a look at the tryserver build and following I've noticed:
> 
> On OS X everything looks good! But on Windows all tabs get closed really
> slowly. Means if you have open more than 20 tabs and enter the PB mode, it
> takes about 3-4 seconds until each tab is closed and the pb page is displayed.
> That doesn't look nice and should be at the same speed as on OS X and when
> leaving the pb mode.

I get quick tab closing on Windows...  Are you talking about tabs on the first window, or tabs on other windows (which will get closed eventually)?

Simon, are you aware of any such performance problems in session store's setBrowserState implementation?

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #7)
> I get quick tab closing on Windows...  Are you talking about tabs on the first
> window, or tabs on other windows (which will get closed eventually)?

I've only one window open and it contains about 20 tabs. Haven't tested with additional windows open.

Comment 9

[Simon Bünzli]

(In reply to comment #7)
> Simon, are you aware of any such performance problems in session store's
> setBrowserState implementation?

None whatsoever.

Henrik: Do tabs generally take some time to close (even through the Close button or Ctrl+W)? And does the same happen in Safe Mode as well? Finally: Did your tabs contain specific URLs or does it take too long with arbitrary URLs loaded?

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Slow tab closing

It's interesting. The seen behavior only happens on Windows. I cannot reproduce it on OS X. I did some further investigation to this slow closing behavior and found out that it only happens with this special sessionstore.js.

I have reduced the amount of pages in this file as much as I can. Simon, I hope it will give you some insight whats going on, and I hope you and Ehsan can reproduce it too.

Comment 11

[Simon Bünzli]

Thanks, Henrik, with your sessionstore.js I can nicely reproduce this issue. The problem seems to be that _endRemoveTab in tabbrowser.xml does some performance intensive scrolling (sync'd) when the last tab is removed and we remove the tabs starting at the end. You shouldn't be seeing this issue, if you select the very first tab before entering PB mode.

To fix this, we could either scroll asynchronously in _endRemoveTab or just remove the tabs starting at the first tab to be removed (which fixes this issue for me):

>-    for (t = openTabCount - 1; t >= newTabCount; t--) {
>+    while (t <= tabbrowser.mTabs.length) {

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Shall I file it as a separate bug? That way we will not hinder this bug to  get checked into branch. Or is it the only line which needs to be changed? That way I think Ehsan can take care of it.

Comment 13

[(no longer active)]

(In reply to comment #12)
> Shall I file it as a separate bug? That way we will not hinder this bug to  get
> checked into branch. Or is it the only line which needs to be changed? That way
> I think Ehsan can take care of it.

Actually it would be great if you can file a separate bug on this, because with bug 476463, I think we can't fix this bug, and bug 476463 is a blocker, so it wins...  :(

Comment 14

[(no longer active)]

I'm giving up on this bug for now (read 3.1).  May look into this further after we ship 3.1.  If someone has a suggestion about how to fix this with bug 476463 fixed, then I'd be willing to take this again.

Comment 15

[Nochum Sossonko [:Natch]]

Right, I was hacking nsPrivateBrowsing a bit and realized that the session
restore kind of hinders the "do something after the pages are unloaded".
However, maybe some sort of mechanism can be added to session restore that
would allow this and not hinder this bug? I think it may be necessary anyhow,
unless _closeAllWindows is called both when entering and exiting private
browsing mode.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #13)
> Actually it would be great if you can file a separate bug on this, because with
> bug 476463, I think we can't fix this bug, and bug 476463 is a blocker, so it
> wins...  :(

Will do so. Meanwhile lets add bug 476463 to the dependency list.

Comment 17

[(no longer active)]

The patch I posted in bug 476463 should take care of this as well, but that's not final yet.

Comment 18

[(no longer active)]

Henrik, this should be fixed by bug 476463.  Can you please verify?

Comment 19

[(no longer active)]

Bug 476463 landed on 1.9.1...

Comment 20

[Jim Jeffery not reading bug-mail 1/2/11]

This new transition into Private browsing is worse then the total blank out and reopen of the window.

As each tab is saved/closed after clicking 'start private browsing' the window jumps from PB to your non-PB window several times if you have a lot of tabs open, I currently have 11 tabs open...

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090215 Minefield/3.2a1pre Firefox/3.0.4 ID:20090215112632

Comment 21

[Nochum Sossonko [:Natch]]

(In reply to comment #20)

See comment 11, which is probably what you are hitting. This bug in itself has been fixed on the private browsing side, the rest remains really with sessionstore or tabbrowser. IMHO the new look and feel is still better since the whole window doesn't close and reopen now...

Comment 22

[Nochum Sossonko [:Natch]]

(oh and it would probably be nice to remove the unnecessary session switching and tab removal, but that isn't what is causing the slow tab closing)

Comment 23

[(no longer active)]

Jim, can you please verify that the problem you've been observing is the same as comment 11?  You can verify this by checking out if the problem occurs when either the first or the last tab is selected.  Thanks!

Comment 24

[Nochum Sossonko [:Natch]]

Ehsan, I'm going to file a bug to have tabbrowsers's removeAllTabsBut accept another parameter which overrides the confirmation pref, then pb can use that which seems to be a lot smoother (although I'll retest this assumption before I do ;)

Comment 25

[Nochum Sossonko [:Natch]]

(In reply to comment #20)
> As each tab is saved/closed after clicking 'start private browsing' the window
> jumps from PB to your non-PB window several times if you have a lot of tabs
> open, I currently have 11 tabs open...

I'm not sure I understand this completely, what do you mean by PB to non-PB window? Are you transitioning into PB mode or out of it? Can you provide your sessionstore.js file?

Comment 26

[(no longer active)]

(In reply to comment #24)
> Ehsan, I'm going to file a bug to have tabbrowsers's removeAllTabsBut accept
> another parameter which overrides the confirmation pref, then pb can use that
> which seems to be a lot smoother (although I'll retest this assumption before I
> do ;)

Maybe that would be bug 476928, no?  Anyway, it'd be great if you can take over that bug (or a new one which you file).  :-)

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Excellent work. That looks fantastic! Verified with builds on OS X and Windows:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090215 Minefield/3.2a1pre ID:20090215020424

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090215 Shiretoko/3.1b3pre ID:20090215231652

On OS X the slow closing of tabs only happens with Shiretoko. Minefield doesn't show this problem. On Windows both builds are suffering from that. It should really be fixed for FF3.1.

Comment 28

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to comment #23)
> Jim, can you please verify that the problem you've been observing is the same
> as comment 11?  You can verify this by checking out if the problem occurs when
> either the first or the last tab is selected.  Thanks!

Yes, selecting the 1st tab then entering PB is smooth and seamless as expected.  If I select the last tab, or any in the middle of a string of tabs (usually have over 10 open) then I see them close (the tabs) one-by-one then enter PB.  

@Natch
It was an illusion on my part when I stated it was jumping back and forth, it was merely the tabs being closed before the transition into PB.  Sorry for the confusion.  

I think we can end this discussion and now move to the next step.  

Thanks for the hard work.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marcia, I've updated both Litmus tests to cover the seamless switch-over into PB mode.

Comment 30

[Nochum Sossonko [:Natch]]

Tentatively marking in-testsuite- as I don't think there is a reasonable way to test this (that the transition is seamless).

Comment 31

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P2 - Polish issue that is in a secondary interface, occasionally encountered, and is easily identifiable.

Only effects interactions with private browsing, so not a constant polish problem.