156522 - scrollbars should be reflow roots

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase

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)

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: approach 1, v. 1: patch to make scrollbars reflow roots

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.

Comment 3

[Doron Rosenberg (IBM)]

getting on adt's radar

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: approach 2, v. 1: patch to set |aNotify = PR_FALSE| when scrollbars collapsed

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 5

[kinmoz]

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

sr=[e-mail]

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 7

[Chris Waterson]

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

r=waterson

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 9

[Doron Rosenberg (IBM)]

Did this get checked into the branch or trunk?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

trunk

Comment 11

[Jaime Rodriguez, Jr.]

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

Comment 12

[Carine Held]

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

Comment 13

[Carine Held]

Attachment: Test Results for flickering image and marquee problem.

Comment 14

[Doron Rosenberg (IBM)]

verified on trunk

Comment 15

[Johnny Stenback (:jst)]

FYI, this caused bug 156985.

Comment 16

[Paul Wyskoczka]

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

Comment 17

[Jaime Rodriguez, Jr.]

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

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 19

[Jaime Rodriguez, Jr.]

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

Comment 20

[chris hofmann]

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

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 22

[Brian Ryner (not reading)]

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).

Comment 23

[Brian Ryner (not reading)]

filed the last observation as bug 157487

Comment 24

[kinmoz]

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.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 26

[scottputterman]

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

Comment 27

[Jaime Rodriguez, Jr.]

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.

Comment 28

[Jaime Rodriguez, Jr.]

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

Comment 29

[kinmoz]

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.

Comment 30

[Chris Petersen]

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.

Comment 31

[Steve Dagley]

This patch has been nominated as a possible cause of 157837

Comment 32

[Jaime Rodriguez, Jr.]

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

Comment 33

[Jaime Rodriguez, Jr.]

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.

Comment 34

[Peter Lubczynski]

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

Comment 35

[chris hofmann]

a=chofmann for 1.0.1

Comment 36

[Jaime Rodriguez, Jr.]

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!

Comment 37

[Kevin McCluskey (gone)]

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

Comment 38

[Jaime Rodriguez, Jr.]

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!

Comment 39

[Amarendra Hanumanula]

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.

Comment 40

[Chris Petersen]

No regressions found while executing our XML 1.0 test suite.

Comment 41

[Chris Petersen]

verified in 7/22 branch.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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