Closed
Bug 476928
Opened 16 years ago
Closed 16 years ago
Slow closing of tabs with the given testcase when entering Private Browsing mode
Categories
(Firefox :: Tabbed Browser, defect)
Tracking
()
VERIFIED
FIXED
Firefox 3.6a1
People
(Reporter: whimboo, Assigned: zeniko)
References
Details
(Keywords: verified1.9.1)
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dietrich
:
review+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre Ubiquity/0.1.5 ID:20090204020327
See bug 473843 comment 10 and 11 for an explanation. The given sessionstore.js (attachment 359929 [details]) shows this nicely at least with the following tryserver build:
https://build.mozilla.org/tryserver-builds/2009-01-30_01:53-ehsan.akhgari@gmail.com-pbseamless/
Copying Simons comment over here:
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) {
Assignee | ||
Comment 1•16 years ago
|
||
This should IMO be fixed in our tabbed browser implementation. The cited SessionStore change would only be a wall-paper solution for the impending 3.1 release.
Component: Session Restore → Tabbed Browser
QA Contact: session.restore → tabbed.browser
Comment 2•16 years ago
|
||
How about modifying removeAllTabsBut to accept another parameter which overrides the confirmation? I think (I haven't tested this recently) that closes the tabs much more efficiently. Then sessionstore can use that to close all the tabs instead of doing it manually.
Reporter | ||
Comment 3•16 years ago
|
||
This is something which should really be fixed for FF3.1. Otherwise this behavior could bring up a lot of annoyed users.
Flags: blocking-firefox3.1?
Comment 4•16 years ago
|
||
(In reply to comment #1)
> This should IMO be fixed in our tabbed browser implementation. The cited
> SessionStore change would only be a wall-paper solution for the impending 3.1
> release.
I filed bug 478750 for that.
This should be about the most expedient way to fix the issue at hand, be it through calling another method for closing all tabs (as per comment 2) or just hard coding that we start closing from the first tab (as per comment 1).
Either way, it doesn't block, as it won't affect all users. Wanted, though, so we'd take a patch that was tested and simple. Whipping up something like from comment 1 seems easiest.
Flags: wanted-firefox3.1+
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1-
Reporter | ||
Comment 5•16 years ago
|
||
Simon, does it mean we only need your proposed fix I mentioned again in comment 0 to fix it for FF3.1? If yes, I've created a tryserver build. It's available here: http://tinyurl.com/chht6r
I'll have a test and report back.
Reporter | ||
Comment 6•16 years ago
|
||
Simon, that doesn't work. Doing it that way we lose the content of all tabs after a restart. Feel free to test such a tryserver build.
Assignee | ||
Comment 7•16 years ago
|
||
So, my original wall-paper didn't work as well as I had envisioned. This one does: it cleans up the tab closing logic and as a side-effect also fixes this bug for good.
Dietrich: This three-liner is wanted for 3.1.
Comment 8•16 years ago
|
||
Nice, this makes it a lot smoother for me. I was also thinking of turning off smoothScroll temporarily, but I don't think it's necessary anymore and would also probably be more risky this late.
Reporter | ||
Comment 9•16 years ago
|
||
Thanks Simon. I've started a tryserver build to check it on Windows and OS X.
Reporter | ||
Comment 10•16 years ago
|
||
That looks good. Tryserver builds are available here: http://tinyurl.com/ccwxxk
Comment 11•16 years ago
|
||
Comment on attachment 362619 [details] [diff] [review]
cleaned up tab closing
>
> // when overwriting tabs, remove all superflous ones
>- for (t = openTabCount - 1; t >= newTabCount; t--) {
>- tabbrowser.removeTab(tabbrowser.mTabs[t]);
>- }
>+ if (aOverwriteTabs && newTabCount < openTabCount)
>+ Array.slice(tabbrowser.mTabs, newTabCount, openTabCount)
>+ .forEach(tabbrowser.removeTab, tabbrowser);
nit: style in this file seems to be to use brackets even for single-line if() blocks (eg, the code immediately below this change).
r=me otherwise
>
> if (aOverwriteTabs) {
> this.restoreWindowFeatures(aWindow, winData);
> }
> if (winData.cookies) {
> this.restoreCookies(winData.cookies);
> }
> if (winData.extData) {
Attachment #362619 -
Flags: review?(dietrich) → review+
Assignee | ||
Comment 12•16 years ago
|
||
We're not that consistent, but sure.
Attachment #362619 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 13•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
Assignee | ||
Updated•16 years ago
|
Attachment #363517 -
Flags: approval1.9.1?
Reporter | ||
Comment 14•16 years ago
|
||
Looks good with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090222 Minefield/3.2a1pre ID:20090222035031. Marking verified.
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 15•16 years ago
|
||
The patch is still waiting for approval on 1.9.1. It's a highly visible behavior users with a lot of tabs will notice.
Due to we don't have it in beta 3 I'ld ask for a relnote.
Keywords: relnote
Updated•16 years ago
|
Attachment #363517 -
Flags: approval1.9.1? → approval1.9.1+
Comment 16•16 years ago
|
||
a=beltzner for 191 landing
Don't think this is worthy of a relnote. It's UI choppiness that will be improved for the next beta.
Keywords: relnote
Updated•16 years ago
|
Keywords: checkin-needed
Comment 17•16 years ago
|
||
Keywords: checkin-needed → fixed1.9.1
Reporter | ||
Comment 18•16 years ago
|
||
It's improved a lot on 1.9.1. Verfied with builds on Windows and OS X:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090309 Shiretoko/3.1b4pre
Simon, there is still some flickering visible. Means when closing all tabs the content of the first tab is visible for some milliseconds. Shall I file a follow-up bug for further optimizations?
Keywords: fixed1.9.1 → verified1.9.1
Assignee | ||
Comment 19•16 years ago
|
||
(In reply to comment #18)
> Shall I file a follow-up bug for further optimizations?
Sure, at least to investigate if we can improve the transition any further.
Reporter | ||
Comment 20•16 years ago
|
||
Filed bug 482580.
Reporter | ||
Comment 21•16 years ago
|
||
Sorry, but this is not fixed. Something has been changed. When I have verified this bug on 1.9.1 two days ago it seems fine but the same old behavior is present for Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090311 Shiretoko/3.1b4pre Ubiquity/0.1.5 ID:20090311030630
You only have to open about 10 tabs and select e.g. the second one. When entering the PB mode we still close all the tabs step by step. There is no change to the behavior as we had before the patch went into 1.9.1. This really affects 1.9.1 only, trunk doesn't show this behavior.
Keywords: verified1.9.1
Comment 22•16 years ago
|
||
Could you please try to find a regression range? This may be a new bug which causes this to regress from the time that the patch landed on 1.9.1.
Updated•16 years ago
|
Keywords: regressionwindow-wanted
Reporter | ||
Comment 23•16 years ago
|
||
I tried it with the same build I've mentioned in comment 18 and I'm still able to see this behavior. That's why my assumption is that the patch doesn't work on 1.9.1.
Assignee | ||
Comment 24•16 years ago
|
||
Not closing the selected tab (which will lead to different tabs being selected while closing superfluous tabs) reduces the flickering further.
Henrik: Can you manually apply these four lines to your nsSessionStore.js and see if they help.
Attachment #367105 -
Flags: review?(dietrich)
Assignee | ||
Updated•16 years ago
|
Keywords: regressionwindow-wanted
Comment 25•16 years ago
|
||
Comment on attachment 367105 [details] [diff] [review]
fix part II (never close the selected tab) (checked in)
this bug is already marked fixed. if this is a further optimization, please file a new bug and ask for review there. otherwise, re-open this one.
Assignee | ||
Comment 26•16 years ago
|
||
Reopening, since the bug as filed wasn't fixed by the original patch (but should be with both patches combined).
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Updated•16 years ago
|
Attachment #367105 -
Flags: review?(dietrich) → review+
Comment 27•16 years ago
|
||
Comment on attachment 367105 [details] [diff] [review]
fix part II (never close the selected tab) (checked in)
r=me
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Whiteboard: [checkin needed for part II]
Reporter | ||
Updated•16 years ago
|
Attachment #363517 -
Attachment description: for check-in → for check-in (checked-in)
Reporter | ||
Comment 28•16 years ago
|
||
Simon, could you create a tryserver build for 1.9.1 so we could test and dont have to wait for checkin on 1.9.1?
Assignee | ||
Comment 29•16 years ago
|
||
(In reply to comment #28)
I can't, I'm sorry. Then again, you can easily apply the patch yourself: Just copy the four lines to the correct place in nsSessionStore.js (and remove the initial "+" on each line).
Comment 30•16 years ago
|
||
I just uploaded the patch to the try server.
Comment 31•16 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [checkin needed for part II]
Comment 32•16 years ago
|
||
1.9.1-based try server builds are available here:
<https://build.mozilla.org/tryserver-builds/2009-03-14_00:25-ehsan.akhgari@gmail.com-1237015451/>
Reporter | ||
Comment 33•16 years ago
|
||
(In reply to comment #29)
> (In reply to comment #28)
> I can't, I'm sorry. Then again, you can easily apply the patch yourself: Just
> copy the four lines to the correct place in nsSessionStore.js (and remove the
> initial "+" on each line).
I missed that. But thank you Ehsan. I've tried this 1.9.1 build and it works fine on OS X. Seems like everything will be fine now. Thanks Simon.
Assignee | ||
Updated•16 years ago
|
Attachment #367105 -
Flags: approval1.9.1?
Assignee | ||
Updated•16 years ago
|
Whiteboard: [part II needs 1.9.1 approval and landing]
Reporter | ||
Comment 34•16 years ago
|
||
Verified fixed on Windows and OS X with:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090322 Minefield/3.6a1pre ID:20090322035551
Status: RESOLVED → VERIFIED
Comment 35•16 years ago
|
||
Backed out changeset 6f44d1dd4764 (the first patch on 191) as part of trying to track down the Mac Ts regression being tracked by bug 486236. I would re-open this but it's still landed on Trunk. I would remove the fixed-191 keyword, but it doesn't have one. So I'm going to put it in the whiteboard so that we don't totally lose track of it.
Whiteboard: [part II needs 1.9.1 approval and landing] → [part II needs 1.9.1 approval and landing] [backed out on 191 branch - see comment 35]
Comment 36•16 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ce9c2499760d
Backout changeset
Comment 37•16 years ago
|
||
That backout didn't seem to change things on 8.8.2 fast talos after the first
run, and those runs were pretty consistent at detecting the regression.
http://graphs-new.mozilla.org/graph.html#tests=[{%22test%22:16,%22branch%22:3,%22machine%22:49}]
Sorry for the annoyance.
Reporter | ||
Comment 38•16 years ago
|
||
Johnathan, will it be checked-in again or do we need a combined patch now to fix the remaining problem on this bug?
Comment 39•16 years ago
|
||
Whimboo - probably better asked of zeniko and dietrich. The first part was approved to land, and can reland, but if it makes more sense to bundle it into a complete fix, that works too.
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Whiteboard: [part II needs 1.9.1 approval and landing] [backed out on 191 branch - see comment 35] → [1.9.1 landing needed for part I][part II needs 1.9.1 approval and landing]
Comment 40•16 years ago
|
||
Keywords: checkin-needed
Whiteboard: [1.9.1 landing needed for part I][part II needs 1.9.1 approval and landing] → [part II needs 1.9.1 approval]
Comment 41•16 years ago
|
||
Comment on attachment 367105 [details] [diff] [review]
fix part II (never close the selected tab) (checked in)
a191=beltzner
Attachment #367105 -
Flags: approval1.9.1? → approval1.9.1+
Comment 42•16 years ago
|
||
Keywords: fixed1.9.1
Whiteboard: [part II needs 1.9.1 approval]
Updated•16 years ago
|
Attachment #367105 -
Attachment description: fix part II (never close the selected tab) → fix part II (never close the selected tab) (checked in)
Reporter | ||
Comment 43•16 years ago
|
||
Verified fixed on 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090411 Shiretoko/3.5b4pre ID:20090411030607 and on WindowsXP.
Keywords: fixed1.9.1 → verified1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•