Scrollbars should be reflow roots so that when we do a reflow to move the slider, it doesn't need to go through the whole document. This is a simple fix, and gives a big performance improvement on a marquee testcase for the marquee XBL in http://bugscape.netscape.com/show_bug.cgi?id=17067 (which works by manipulating .scrollTop and .scrollLeft on something with -moz-scrollbars-none). There are also a bunch of other ways this could be sped up, a bunch of which we should do, since they affect different types of "DHTML" testcases. I'll attach the simple testcase, although it requires the presence of the marquee XBL.

This is the tastcase, which requires the above-mentioned XBL. The problems are most easily visible with double-buffering turned off or with paint flashing in the debug|events prefs panel. We do lots of painting, and paint some things at the wrong position (I think because the floater movement that repositions views is also repositioning *widgets*, which forces immediate repainting), because: * there's an attribute changed on the attribute for the slider's position, with aNotify set to true even though the scrollbar isn't visible. * This posts a reflow command, and we reflow from the top to move the slider (which is hidden). * the floater code does its "pref-size" reflow (since we don't have separate GetMinSize and GetPrefSize) at a different position from its final one, so we end up with a lot more invalidates than we really need. (2 problems here: the use of the different position and the underlying architectural problem that we shouldn't be using Reflow (which does things like invalidation and view positioning) for this type of calculation)

This patch fixes the problem at one of the simple points where it can be fixed, by making scrollbars reflow roots so we don't reflow from the top.

getting on adt's radar

This approach would actually be even faster, except it doesn't help, since XBL clobbers the |aNotify| parameter in the following manner: #14 0x41ca76a1 in nsXULElement::SetAttr (this=0x8228d18, aNameSpaceID=0, aName=0x80d2938, aValue=@0xbfffd8c4, aNotify=1) at /home/dbaron/builds/clean/mozilla/content/xul/content/src/nsXULElement.cpp:2746 #15 0x41d376c0 in nsXBLPrototypeBinding::AttributeChanged (this=0x81e1760, aAttribute=0x80d2938, aNameSpaceID=0, aRemoveFlag=0, aChangedElement=0x8222fa8, aAnonymousContent=0x8228a80) at /home/dbaron/builds/clean/mozilla/content/xbl/src/nsXBLPrototypeBinding.cpp:603 #16 0x41d30bdd in nsXBLBinding::AttributeChanged (this=0x82289d0, aAttribute=0x80d2938, aNameSpaceID=0, aRemoveFlag=0) at /home/dbaron/builds/clean/mozilla/content/xbl/src/nsXBLBinding.cpp:1028 #17 0x41ca716d in nsXULElement::SetAttr (this=0x8222fa8, aNodeInfo=0x8202528, aValue=@0xbfffdd64, aNotify=0) at /home/dbaron/builds/clean/mozilla/content/xul/content/src/nsXULElement.cpp:2689 #18 0x41ca76a1 in nsXULElement::SetAttr (this=0x8222fa8, aNameSpaceID=0, aName=0x80d2938, aValue=@0xbfffdd64, aNotify=0) at /home/dbaron/builds/clean/mozilla/content/xul/content/src/nsXULElement.cpp:2746 #19 0x407c720d in nsGfxScrollFrameInner::SetAttribute (this=0x8222e48, aBox=0x8227f94, aAtom=0x80d2938, aSize=190, aReflow=0) at /home/dbaron/builds/clean/mozilla/layout/html/base/src/nsGfxScrollFrame.cpp:1532

Comment on attachment 90649 [details] [diff] [review] approach 1, v. 1: patch to make scrollbars reflow roots sr=kin@netscape.com

Comment on attachment 90659 [details] [diff] [review] approach 2, v. 1: patch to set |aNotify = PR_FALSE| when scrollbars collapsed This patch shouldn't be on this bug (it will get too confusing too fast), so I created bug 156547 for it.

Comment on attachment 90649 [details] [diff] [review] approach 1, v. 1: patch to make scrollbars reflow roots r=waterson

Fix checked in, 2002-07-09 19:17 PDT.

Did this get checked into the branch or trunk?

trunk

petersen: chris, can you verify this fix on the trunk tomorrow, so that we can get it on the branch asap. thanks!

Created attachment of test results using 7/10 trunk build. Flickering image problem fixed,CPU load reduced considerably.

verified on trunk

FYI, this caused bug 156985.

Adding adt1.0.1 on behalf of the adt. Please get drivers approval and check the fix into the Mozilla 1.0 branch.

Removing "+", as this caused a regression (bug 156985) on the trunk

So I actually don't think the regression should block checkin to the branch, since the regression is fixed by a patch that already landed on the branch but was not landed on the trunk. See bug 156985 comment 3 for details.

Adding back the "+", based on Comment #18 From David Baron. Thanks Dave! :-)

Comment on attachment 90649 [details] [diff] [review] approach 1, v. 1: patch to make scrollbars reflow roots a=chofmann for 1.0.1. add the fixed1.0.1 keyword after checking into the branch

OK, I really need to stop assuming that the branch and trunk are equivalent. While the patch works fine on the branch, once I pulled doron's marquee patch over to my branch tree to test the performance (which is a pain, since there really is no *patch*, and it requires rewriting the build goop myself, and differently for the branch than for the trunk), I found that this patch alone doesn't fix the performance problem. It also needs the changes kin made in revision 1.55 of nsBox.cpp, which are part of attachment 87306 [details] [diff] [review] of bug 141900. So, any thoughts on moving these changes over to the branch?

The checkin for this bug appears to have caused a regression where opening a new window with a blank page as the home page doesn't actually reflow the about:blank document, so the content area remains grey instead of white. I think this is also causing incorrect focus behavior when the first document is loaded (it should focus the page, but doesn't).

filed the last observation as bug 157487

FYI, attachment 87306 [details] [diff] [review] *can* be landed on the MOZILLA_1_0_BRANCH *independently* from attachment 87307 [details] [diff] [review] which is the other patch required to totally fix bug 141900. I wouldn't recommend landing attachment 87307 [details] [diff] [review] until I come up with a fix for bug 151882, unless everyone can live with ocassional caret flashing. Fixing 151882 requires some re-working of the several points we turn off and on the caret during editing and painting. I've already started some of that work as part of bug 54153.

Removing approvals due to bug 157487, which seems different from the issue in comment 18.

adding adt1.0.1-. Let's get this in the next release.

Removing minus to renominate it for the 1.0 branch. David: What do we need to do to get this one landed and working on the 1.0 branch. Is it simply fixing bug 157487 (if yes, pls add bug 157487 as a dependency), and adding portions from attachment 87306 [details] [diff] [review] from bug 141900? Pls advise, as this could be a stopper for a major embedding customer.

--> Kevin for reassignment. cc'ing Chris Waterson.

Just to be clear, if you want the fix for this bug on the MOZILLA_1_0_BRANCH, you will have to take the following patches: attachment 90649 [details] [diff] [review] (from dbaron for bug 156522) attachment 87306 [details] [diff] [review] (from kin for bug bug 141900) attachment 91690 [details] [diff] [review] (from bzbarsky for bug 156985) Also, before this happens, someone will need to verify that bug 157487 was indeed fixed by bzbarsky's fix for bug 156985 (attachment 91690 [details] [diff] [review]) as dbaron suggests. I think these fixes are low-risk, but taking a conservative approach, you may want to let bzbarsky's fix bake another day since it just landed lastnight, though I doubt his fix will introduce any regressions. The other 2 patches have been baking on the trunk for a while.

I ran our XML test cases with the trunk build (2002-07-18-13) which included this patch. No new regressions were found based on my testing. Tested under Windows ME.

This patch has been nominated as a possible cause of 157837

amar is testing to verify that bug 157487 is fixed by bzbarsky's fix for bug 156985 (attachment 91690 [details] [diff] [review]).

sdagley now tells ME that bnesse has seen this problem on the branch, as well. hence, it appears that this bug was not the culprit for bug 157837. which leaves us with verification for 157487 as fixed by bug 156985 (attachment 91690 [details] [diff] [review]), so that we can have this one OK for our various branch approvers.

My Carbon Trunk debug build from July 7th with attachment 90649 [details] [diff] [review] applied does not show bug 157837.

a=chofmann for 1.0.1

marking as "mozilla1.0.1+" per Comment #35 From chris hofmann. adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch. pls check this in asap, then change "mozilla1.0.1+" to "fixed1.0.1". thanks!

Checked in attachment 90649 [details] [diff] [review] to the 1.0 branch

petersen: can you pls verify this on the branch tomorrow? if you could also run the XML test cases to see if we introduced new regressions. thanks!

bug 157987 nolonger blocks this bug. I verified bug 157987 after the patch for this bug 156522 was checked in. It works fine on both branch and trunk builds. The patch for this bug 156522 did not cause any regression.

No regressions found while executing our XML 1.0 test suite.

verified in 7/22 branch.

The auto width issue mentioned in comment 1 is covered by bug 136549, I think.