245131 - [FIXr]Don't flush reflows on document in window when looking for scrollinfo for window

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

As I understand it, the scrollinfo for the root scrolling view in a window
doesn't really depend on the document that lives inside it.  So we should be
able to just flush reflows for the _parent_ when getting scrollinfo...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 149660 [details] [diff] [review]
Proposed patch

roc, what do you think?  Note that there're several callers of this
GetScrollInfo method, which get various data off the view.  I _think_ they're
all OK with this change...

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

For the callers ScrollTo, ScrollBy, ScrollByLines, ScrollByPages, don't we need
to flush layout of the current document before we scroll it? Otherwise the
document might be shorter than it's supposed to be, and the scroll would be
inappropriately clamped. OTOH I *don't* think we need to layout the parent for
these, except for ScrollByPages where we probably should.

For GetScrollMaxXY, we definitely should layout the current document to find out
what the correct values of its width and height are. We also need to layout the
parent.

And for GetScrollXY, we need to layout the current document in case its height
decreases, clamping the scroll position. We don't need to layout the parent.

So I don't see how we can avoid laying out the current document here...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.. yeah... laying out the current document is the problem.

Specifically, the DHTML testcase I'm looking at (bug 186442) uses some sort of
DHTML library that checks window.scrollX/window.scrollY to make sure that the
snoflakes stay in the "visible area".  It ends up getting these properties for
each snowflake.  So on that testcase, if we have 100 snowflakes we would end up
doing 100 separate reflows to reposition them (200, actually, since we end up
with separate reflows for setting .left and .top).  Further, the whole thing
defeats the lazy style reresolution optimization I've been working on.  So not
having to flush on each of those gets makes that testcase run easily twice as
fast, possibly 3-4 times (since a single reflow of multiple frames is much
faster than a whole bunch of reflows).

Note that in that testcase the scroll position is always 0... Can we at least
attempt to optimize GetScrollXY for cases where the scroll position obviously
can't decrease?  Can it decrease if it's already smaller than the window size? 
If not, it may be worth it to just special-case 0....

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Zero is definitely an easy case, and also extremely common, so it makes sense to
special case it.

I'm trying to think of a reasonable situation where layout can actually decrease
the document height or width, but I can't.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Well... layout of the _parent_ can certainly decrease the document width or height.

In any case, I guess what I'll do is hoist the flush out of GetScrollInfo to
callers.  Does that seem reasonable?  Then GetScrollXY can check whether it got
(0,0) and if not, flush and reget.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Well... layout of the _parent_ can certainly decrease the document width or
> height.

Sure, I meant layout of the child :-) There are some weird cases like a tiny
image which is smaller than the placeholder. Delayed stylesheet loading of
course (do we ever render these days before the stylesheet has loaded?).

> In any case, I guess what I'll do is hoist the flush out of GetScrollInfo to
> callers.  Does that seem reasonable?  Then GetScrollXY can check whether it
> got (0,0) and if not, flush and reget.

Sounds like a good plan.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to review comments

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 153134 [details] [diff] [review]
Updated to review comments

roc, jst, would you review?

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 153134 [details] [diff] [review]
Updated to review comments

sr=jst

Comment 11

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 12

[Joe Sabash [:JoeS1]]

We use heavy amounts of DHTML in our newsgroup.
This seems to have had a dramatic effect on performance (positive)see attachments
Analysis based on a branch build of Thunderbird prior to this patch vs
the latest trunk build.
Platform is Win98 1.4 gig P3-s
I am using this bug to request inclusion in Thunderbird branch builds,
and I assume Firefox would benefit as well.
If this is not an appropriate way to make this request..then sorry for the spam.

Comment 13

[Joe Sabash [:JoeS1]]

Attachment: branch build..no patch

Comment 14

[Boris Zbarsky [:bzbarsky]]

Someone would need to merge the patch to branch.  It depends on quite a number
of changes that have happened on the trunk.

And I'd like this to get some trunk testing before we even think of putting this
on branch.

Comment 15

[Joe Sabash [:JoeS1]]

Attachment: trunk with patch

Comment 16

[Joe Sabash [:JoeS1]]

http://bugzilla.mozilla.org/show_bug.cgi?id=232714
is one good example,also you might want to look through my MM tracking bug
http://bugzilla.mozilla.org/show_bug.cgi?id=240183
The newsgroup I referred to is 
snews://secnews.netscape.com.netscape.test.multimedia
port #563
Some old NC users, but they are coming along