Build 2003120109, Windows 2000 During resizing the browser window Mozilla seems to be reflowing the documents in the tabs that are invisible. That seems somewhat unnecessary to me. I think it only needs to reflow the visible tab during the resizing and it should reflow all the tabs once the user lets go of the mousebutton (preferably the visible tab first). That should make the UI feel more responsive. Reproduce: 1. Open a huge document in the first tab 2. Open about:blank in the second tab, make this tab visible 3. Resize the window Result: It's very slow. Expected result: It should resize pretty fast since the visible document is nearly empty.

OS/2 certainly needs this no less than W98 or W2K.

Clarifying summary: 'invisible tabs' confused me.

That would have a considerable boost in speed perception - is that something that should be targeted for 1.7 ?

Markush, as much as I'd love to see this one fixed by 1.7, there's no point in setting the target milestone as long as the bug isn't assigned to anyone who can actually fix it. Feel free to cc some folks who can fix this since I can't do it.

This is a very quick and dirty hack. But something like this could be possible?

This version works also with demo#13 etc. Basically this patch delays the resize reflow until a tab is focused. Also the resize events are delayed. The performance boost is huge.

CC'ing bz

dbaron knows this code a lot better than I do. The patch looks ok to me in general, but the names of the functions and variables could use improvement (eg the setter should probably be setAllowResizeReflow instead of what it is....)

Function and variable names changed. Fewer function calls when resize reflow is disabled.

BTW, Opera and Konqueror seem to process the resize reflow when switching a tab, or at least the resize event is dispatched then.

This seems a bit like the wrong approach. It seems more like a stack is the wrong thing to use for the tabbrowser, and you really want something that doesn't resize the container for the background tabs in the first place. That said, maybe it's reasonable to do for now, but I'd like to talk to some other people about it...

So maybe modifying nsDeckFrame would be a better way to do this, but how to handle the resize event (I mean dispatching only when switching a tab)? Could we use this simple approach first and fix the nsDeckFrame later?

Tabs expect their "deck" to stretch all of its children to fit...

> Tabs expect their "deck" to stretch all of its children to fit... What does that exactly mean? Does it mean the tabs should be modified not to expect their deck to strech all of its children or that this can't be fixed at all? And why do tabs expect that?

What I meant was that if you have a tabbed dialog then it should size itself to fit the largest tab and not the current tab. Looking at that in reverse, it means that if we shrink the window we have to shrink all the <browser>s, which I think is why Smaug is looking at stopping the hidden browsers from reflowing, assuming that this doesn't cause follow-on issues like resize events occurring at the wrong time.

I see. Forgive me my rookieness when it comes to Mozilla's internals but to me it seems that we don't need to shrink all the <browser>s since nsDeckFrame::HideBox not only hides the invisible tabs but also resizes them to 0x0 and restores the size when the tab becomes visible again: http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsDeckFrame.cpp#149 http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsDeckFrame.cpp#164 Looking at it this way it seems rather surprising to me that Mozilla reflows the hidden <browser>s anyway, since they're hidden and 0x0. For what it's worth, I removed the ResizeView from nsDeckFrame::HideBox and nsDeckFrame::ShowBox and it didn't seem to make any difference, except that switching tabs seemed somewhat faster. Furthermore, nsDeckFrame::DoLayout seems to be running through all the deck's children hiding them all except the visible one. How usefull is this, considering the fact that nsDeckFrame::IndexChanged already does this? Just trying help :)

Don't worry, I'm one of the many who doesn't understand the vagaries of layout ;-) but I tried removing the resize view stuff and also the show/hide from IndexChanged without any obvious adverse effects...

I suspect the fix for this could live entirely inside nsDeckFrame.cpp. It probably doesn't have much to do with ResizeView, but it should have a lot to do with DoLayout.

(In reply to comment #13) > Tabs expect their "deck" to stretch all of its children to fit... That should only be an issue for GetMinSize and GetPrefSize, not DoLayout, which is what should be at issue here.

Actually, comment 18 isn't quite right, since it's the base class call (which ends up calling nsStackLayout::Layout) that probably does all the work. I'll try to put together a patch.

This patch fixes the problem, although I still need to add dirty bits to avoid adding a resize when switching tabs and the deck hasn't resized. It's also a little ugly...

Two more things to fix: * nsStackLayout::DoLayout and nsDeckLayout::DoLayout can share code (called, say, nsStackLayout::LayoutChild * this causes problems when there are two tabs open and the *first* one is closed -- the second one doesn't show when it's the only one left.

This fixes the three issues mentioned (or should, at least -- I should test that it's doing what I expect with some printfs).

With the latest patch I seem to get a small regression (though I could be wrong since my build is a bit messed up): In the "View page info" window the forms tab and media tab both have their list minimized to height 0 which shouldn't be the case. Other than that it seems to work fine.

Page Info looks fine to me, but the Select Profile dialog doesn't :-(

(The printfs showed that it wasn't doing what I expected.)

Stupid questions day again.. I tried to apply the patch but it failed. Where do I get /cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml ? I don't have rev 1.35 on disk, and LXR doesn't reveal it either.

it's a firebird specific file. if you want to find it in LXR, use /mozilla/ instead of /seamonkey/. but if you watn to just compile mozilla, not firebird or thunderbird, you can ignore it.

Thanks. Omitted that part of the patch, but building crashes: nsDeckFrame.cpp:58:26: nsDeckLayout.h: No such file or directory

The patch creates two new files. You need to touch them before patch will fill in their contents.

dbaron, will this make it into 1.7b? IHMO this is too good to miss the 1.7 boat.

Comment on attachment 140628 [details] [diff] [review] patch So I think there should still be a simpler way -- something should just suppress resizes when the root view is hidden. This could just be the view manager, which is what fires the normal case of resize reflows -- and since it's also responsible for resizing the widgets, which is I think what fires onresize events, it probably wouldn't break onresize events either.

Sure, that'd work.

*** Bug 245126 has been marked as a duplicate of this bug. ***

btw, will a fix for this bug also (greatly?) improve the speed of opening multiple tabs?

I couldn't get the view thing to work.

Some thoughts, starting from bug 253773 comment 6: * nsDocShell::GetVisibility and nsDocShell::SetVisibility are asymmetric. We need something that is symmetric that the tabbrowser can set. * There's no way currently to get a notification when visibility is changed, since it's done by the deck (which nsDocShell::GetVisibility discovers)

This was the last state of my view patch.

This actually works. It also seems to change the feel of page loading a bit, although I'm not sure exactly why. It also makes us use a gray background for plaintext documents rather than synthesizing a white one.

... same, plus header dependency reduction that makes it clear this is the only reason view depends on content/layout

[Comments on the risky version] > It also makes us use a gray background for plaintext documents rather than > synthesizing a white one. I think that linking up all the view trees is stopping the background color preference from working, around here: http://lxr.mozilla.org/mozilla/source/layout/html/style/src/nsCSSRendering.cpp#2728 You're just seeing straight through to the chrome. (Hmm, would look cool to put a Firefox logo in there or something :-) ). Not sure what the best way is to fix that. Maybe change the way the background color works; instead of doing it here, you could style the background of the <browser> element and it would shine through. Whatever we do, we should check that scrolling is still using bitblitting! > It also seems to change the feel of page loading a bit, In what way? if (containerView) { // see if the containerView has already been hooked into a foreign view manager hierarchy // if it has, then we have to hook into the hierarchy too otherwise bad things will happen. nsIViewManager* containerVM = containerView->GetViewManager(); nsIView* pView = containerView; do { pView = pView->GetParent(); } while (pView && pView->GetViewManager() == containerVM); You can delete all this. + // A rect storing the size for a resize that we delayed until the root + // view becomes visible again. + nsSize mDelayedResize; It's not a rect :-) +static PRBool IsViewVisible(nsView *view) +{ + for (; view; view = view->GetParent()) { + if (view->GetVisibility() == nsViewVisibility_kHide) + return PR_FALSE; Hidden views should not have regular view children. Therefore I think this can be optimized to something like this: + for (; view; view = view->GetViewManager()->GetRootView()->GetParent()) { + if (view->GetVisibility() == nsViewVisibility_kHide) + return PR_FALSE; The non-risky version is rather grotesque ... I'd prefer to take the risk. I've always wanted to fix this, anyway.

Note that linking the view trees is necessary to allow XUL content to stack properly over XUL <iframe> or <browser> elements.

(removes some extraneous changes I'd made that are of questionable value)

(In reply to comment #43) > Hidden views should not have regular view children. Therefore I think this can > be optimized to something like this: For decks they do: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/xul/base/src/nsDeckFrame.cpp&rev=1.68&mark=139-161#139 > The non-risky version is rather grotesque ... I'd prefer to take the risk. > I've always wanted to fix this, anyway. It looks like a good bit more work is needed, and this may be wanted on the aviary branch (esp. for bug 253773, not that I can test that).

fix comments

ick. Well, I think there are two options 1) land this patch only on branch and do the view hookup approach on trunk 2) move the root visibility check into a new method on nsIViewObserver

Actually, I'm noticing some problems even with the less risky patch...

So the basic problem here is that we need a better way of being notified when views become visible, and that probably depends on linking the trees. Paint isn't sufficient since views with size of 0x0 aren't going to be painted. I could add yet another hack in the other direction, but I'm not even sure how to do it, since I'd need to be able to find *child* view managers from within the ancestor view tree.

In the future, I'd like to use the code here to change the semantics of view visibility a little (make it officially acceptable for a hidden view to have visible children -- which makes the children hidden anyway), which requires changing many callers of GetVisibility to call IsVisible and then allows the removal of code in nsViewManager::SetViewVisibility and nsDeckFrame::HideBox/ShowBox. I should probably file a separate bug on that. This still requires fixing at least the color issue for the view tree linking.

Although with my fix to DocumentViewerImpl in InitPresentationStuff that's in attachment 160801 [details] [diff] [review], I think comment 50 isn't true anymore (it was a guess at the problems I was seeing that turned out to be at least partially wrong) so I could go back to the safer approach.

FWIW, the "less risky version"s have in common that when switching to a tab, you see a flash of the old position before the new position paints. But the latest "less risky" patch does actually work pretty well, and doesn't break anything that I know of.

Comment on attachment 161002 [details] [diff] [review] approach 3 (less risky version) (Though I am wondering why I didn't continue with the nsDeckFrame approach (#2). Then again, this version, plus nsDocShell visibility fixes, would probably be more useful to embedders doing tab-like interfaces.)

Hrm. This actually leaves a gray space at the bottom after closing a tab when two tabs were open (presumably the height of the tab bar).

That was a one-line fix to nsViewManager::SetWindowDimensions (we need to clear since we could get a resize between being changed to visible and being painted, and that resize could then be undone).

It looks like when you detect the window being unhidden in the NS_PAINT event, we're going to reflow (causing a bunch of invalidation), do the Refresh, and then those invalidates are going to do cause the window to be repainted again right away, to no avail. Right? Maybe after doing the DoSetWindowDimensions in the paint handler, you should just call UpdateView() to get the whole window to be repainted soon, then break out without calling Refresh, on the grounds that it's going to be called again right away anyway.

(In reply to comment #58) > It looks like when you detect the window being unhidden in the NS_PAINT event, > we're going to reflow (causing a bunch of invalidation), do the Refresh, and > then those invalidates are going to do cause the window to be repainted again > right away, to no avail. Right? Hrm. I would have hoped that doing the refresh would clear the invalidates, but I guess not. > Maybe after doing the DoSetWindowDimensions in the paint handler, you should > just call UpdateView() to get the whole window to be repainted soon, then break > out without calling Refresh, on the grounds that it's going to be called again > right away anyway. I tried that -- it doesn't seem to make any difference regarding whether I see a flash of the old position when switching tabs -- it happens some of the time but not others (and depends on how I'm switching tabs -- keyboard or mouse). But it could be making a difference for whether there's an additional paint afterwards.

Do you want to revise the patch?

I'm not sure. I don't fully understand its current behavior, and I'd need a chunk of time free from other interruptions (which certainly isn't going to happen tomorrow) to debug what it's doing now.

Comment on attachment 161208 [details] [diff] [review] approach 3 (less risky version) I'll clear the review request until this is settled.

This just does what roc suggested in comment 58. I don't think I'm going to figure out anything else low enough risk in time.

I suspect this caused a bit of a Tp regression ... probably the loop to find the visiblity of a view.

Backed out for now. I could change it (even more of a hack) to do the IsViewVisible test only if the root view has no parent (i.e., we're the toplevel content tree, not a frame), which would mean the loop would never loop.

That backout didn't seem to affect the Tp numbers at all.. Could the original rise have been from bug 69070 instead?

Yeah, it seems to be, and jst seems to have improved the situation. Relanded - checked in to trunk a second time, 2004-10-14 14:51 -0700 (although some parts stayed in from 2004-10-13 14:59 / 15:05/08 -0700).

Comment on attachment 161968 [details] [diff] [review] approach 3 (less risky version) a=brendan@mozilla.org for aviary and 1.7 branches. /be

Fix checked in to AVIARY_1_0_20040515_BRANCH, 2004-10-15 16:58 -0700. Fix checked in to MOZILLA_1_7_BRANCH, 2004-10-15 17:00 -0700.

*** Bug 253773 has been marked as a duplicate of this bug. ***

*** Bug 258917 has been marked as a duplicate of this bug. ***

I've tried this and it certainly makes windows with large numbers of tabs resize (and switch) faster. However, there is an important bug: when the browser size is *decreased*, switching to other tabs will not resize the content accordingly. Increasing the width seems to work fine. I haven't read anything to suggest this was expected or adequate. To reproduce: 0) open a browser window that's 75% the width of the screen with two tabs, both looking at http://www.google.com/. 1) Verify that the page content is centred on both tabs (no difference between them). 2) Resize the browser to half its width. 3) Compare the two tabs: the active tab should appear correct, but the other should have the Google logo distinctly off-centre Note that if you'd increased the browser width in step two, both tabs would look correct in step three. The failure to resize is on switching the tabs: resizing from 75% to 25% of the screen then to 50% before switching tabs has the same problem as resizing 75% -> 50% and switching (as you'd expect). (I had another issue after installing where opening tabs would leave their content blank, but I need to investigate further and do not blame this change yet: it could be due to browser extensions or because I first started the browser as a peon after installing as an administrator.) Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041016 Firefox/1.0

I suspect this bug of causing the crashing and freezing of firefox since the October 16th nightly builds if you try to click on a link from a page that either has the popup blocker or blocked install bar displayed. Thiese crashes/freezes did not occur with the Ocober 15th nightly, but did occur with the tinderbox build from 7:27 PM Mozilla time. These http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=AviaryBranchTinderbox&branch=AVIARY_1_0_20040515_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-10-15+04%3A00&maxdate=2004-10-15+18%3A00&cvsroot=%2Fcvsroot are the checkins made between the 2 builds. Although there is a chekcin effecting popups, this would appear to be the only chekcin that would effect both the popup blocker and the blocked install notificatin bars. It appears that when you click on the link, the bar disappears and the page reflows and that is when the hang or crash occurs.

William, see bug 264683

(In reply to comment #72) > I've tried this and it certainly makes windows with large numbers of tabs resize > (and switch) faster. However, there is an important bug: when the browser size > is *decreased*, switching to other tabs will not resize the content accordingly. I am also experiencing this behavior. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041018 Firefox/1.0

adding the blocking + flag so if this gets reopened it will appear on radar...

(In reply to comment #72) > (and switch) faster. However, there is an important bug: when the browser size > is *decreased*, switching to other tabs will not resize the content accordingly. > Increasing the width seems to work fine. I haven't read anything to suggest this > was expected or adequate. I certainly don't see this. Is the display corrected if you minimize and restore the Window or if you drag another window in front of it?

In Linux the display is corrected (reflowed or repainted?) if I move the mouse out of the browser window.

What if you remove it entirely without readding (since what HideBox and ShowBox do should cause a repaint)?

This is a screenshot of the expected result from my testcase in comment 72. (In reply to comment #77) > Is the display corrected if you minimize and restore the Window or if you drag > another window in front of it? No... it doesn't seem to be a simple deferred paint problem. Minimising and restoring, dragging other windows in front, or bringing a maximised window to the foreground has no affect. Maximising, as other resizing, obviously does. Since you cannot reproduce this, I should mention my configuration: This is my home development machine, running XP SP2 with themes and full-window dragging enabled. It's a slightly aging dual processor 2800+ Athlon MP with an ATI 9800XT and 1GB RAM.

(In reply to comment #81) > No... it doesn't seem to be a simple deferred paint problem. Minimising and > restoring, dragging other windows in front, or bringing a maximised window to > the foreground has no affect. Maximising, as other resizing, obviously does. I can confirm this on Windows 2000 SP4, Celeron 1.8 GHz, Intel OnBoard Graphic, 1 GB RAM, Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.3) Gecko/20041018 Firefox/1.0

I think this is the right fix for the Windows problems.

Comment on attachment 162612 [details] [diff] [review] fix Windows problems Fix checked in to trunk, 2004-10-19 15:04 -0700. Fix checked in to AVIARY_1_0_20040515_BRANCH, 2004-10-19 15:24 -0700. Fix checked in to MOZILLA_1_7_BRANCH, 2004-10-19 15:24 -0700.

(In reply to comment #83) > Created an attachment (id=162612) > fix Windows problems > > I think this is the right fix for the Windows problems. I've just tried it, and it seems to work fine with this change. Thanks!