Closed Bug 227361 Opened 21 years ago Closed 20 years ago

Don't reflow documents in background tabs until window resizing is complete

Categories

(SeaMonkey :: Tabbed Browser, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.7alpha

People

(Reporter: rp.moz, Assigned: dbaron)

References

Details

(Keywords: fixed-aviary1.0, fixed1.7.5, perf, Whiteboard: [patch])

Attachments

(4 files, 15 obsolete files)

20.72 KB, patch
roc
: review+
brendan
: approval1.7.5+
Details | Diff | Splinter Review
1.11 KB, patch
dbaron
: review+
bryner
: superreview+
Details | Diff | Splinter Review
42.53 KB, image/png
Details
1.53 KB, patch
roc
: review+
Details | Diff | Splinter Review
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.
OS: Windows 2000 → All
Hardware: PC → All
Clarifying summary: 'invisible tabs' confused me.
Summary: Don't reflow invisible tabs during window resizing → Don't reflow documents in background tabs until window resizing is complete
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.
Attached patch Patch 0.1 (obsolete) — Splinter Review
This is a very quick and dirty hack.  But something like this could be
possible?
Attached patch V. 0.2 (obsolete) — Splinter Review
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.
Attachment #138078 - Attachment is obsolete: true
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....)
Attached patch v. 0.3 (obsolete) — Splinter Review
Function and variable names changed.
Fewer function calls when resize reflow is disabled.
Attachment #138121 - Attachment is obsolete: true
Blocks: 203448
BTW, Opera and Konqueror seem to process the resize reflow when switching a tab, 
or at least the resize event is dispatched then. 
Attachment #138177 - Flags: review?(dbaron)
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.
Attached patch work in progress (obsolete) — Splinter Review
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...
Assignee: tabbed-browser → dbaron
Attachment #138177 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [patch]
Target Milestone: --- → mozilla1.7alpha
Attachment #138177 - Flags: review?(dbaron) → review-
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.
Attached patch patch (obsolete) — Splinter Review
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.)
Attached patch patch (obsolete) — Splinter Review
Attachment #140134 - Attachment is obsolete: true
Attachment #140138 - Attachment is obsolete: true
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.
Attachment #140628 - Attachment is obsolete: true
*** 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)
Attached patch approach 3 (failed, so far) (obsolete) — Splinter Review
This was the last state of my view patch.
Attached patch approach 3 (obsolete) — Splinter Review
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.
Attachment #160369 - Attachment is obsolete: true
Attached patch approach 3 (less risky version) (obsolete) — Splinter Review
... 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.
Attached patch approach 3 (less risky version) (obsolete) — Splinter Review
(removes some extraneous changes I'd made that are of questionable value)
Attachment #160403 - Attachment is obsolete: true
(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).
Attached patch approach 3 (less risky version) (obsolete) — Splinter Review
fix comments
Attachment #160405 - Attachment is obsolete: true
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.
Attached patch approach 3 (obsolete) — Splinter Review
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.)
Attachment #161002 - Flags: superreview?(roc)
Attachment #161002 - Flags: review?(roc)
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).
Attached patch approach 3 (less risky version) (obsolete) — Splinter Review
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).
Attachment #161002 - Attachment is obsolete: true
Attachment #161002 - Flags: superreview?(roc)
Attachment #161002 - Flags: review?(roc)
Attachment #161208 - Flags: superreview?(roc)
Attachment #161208 - Flags: review?(roc)
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.
Attachment #161208 - Flags: superreview?(roc)
Attachment #161208 - Flags: review?(roc)
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.
Attachment #161208 - Attachment is obsolete: true
Attachment #161968 - Flags: superreview?(roc)
Attachment #161968 - Flags: review?(roc)
Attachment #161968 - Flags: superreview?(roc)
Attachment #161968 - Flags: superreview+
Attachment #161968 - Flags: review?(roc)
Attachment #161968 - Flags: review+
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?
Attachment #161968 - Flags: approval1.7.x?
Attachment #161968 - Flags: approval-aviary?
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).
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment on attachment 161968 [details] [diff] [review]
approach 3 (less risky version)

a=brendan@mozilla.org for aviary and 1.7 branches.

/be
Attachment #161968 - Flags: approval1.7.x?
Attachment #161968 - Flags: approval1.7.x+
Attachment #161968 - Flags: approval-aviary?
Attachment #161968 - Flags: approval-aviary+
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...
Flags: blocking-aviary1.0+
(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.
Attachment #162503 - Flags: superreview?(bryner)
Attachment #162503 - Flags: review+
Attachment #162503 - Flags: superreview?(bryner) → superreview+
(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.
Attachment #162612 - Flags: superreview?(roc)
Attachment #162612 - Flags: review?(roc)
Attachment #162612 - Flags: superreview?(roc)
Attachment #162612 - Flags: superreview+
Attachment #162612 - Flags: review?(roc)
Attachment #162612 - Flags: review+
Attachment #162612 - Flags: approval1.7.x+
Attachment #162612 - Flags: approval-aviary+
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!
Depends on: 285445
Depends on: 280777
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: