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

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

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

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.

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.

> 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 on attachment 153134 [details] [diff] [review] Updated to review comments roc, jst, would you review?

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

Checked in.

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.

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.

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