288888 - Don't create scrollbars for scrolling="no" IFRAMEs (fix perf regression)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

I have a hunch that the performance regression when creating a full-fledged
nsHTMLScrollFrame for scrolling="no" IFRAMEs is because we create anonymous
scrollbar content where we didn't before. Indeed, we are creating anonymous
scrollbars for such IFRAMEs. You'd think the call to GetScrollbarStyles in
CreateAnonymousContent would pick up the docshell's scrollbar preferences and
return HIDDEN so that we don't create the scrollbars at all. Unfortunately in
that call the scrollframe has not yet been added to the viewport's child list so
when we check that the scrollframe is the first child of the viewport, it fails.
Instead we can just record explicitly whether we're the viewport scrollframe and
avoid peeking at the frame hierarchy, and we no longer create scrollbars for
those IFRAMEs. I think this will fix the performance regression.

Comment 1

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

Attachment: fix

The big comment explains what's happening. Because HTML/BODY overflow
propagation hasn't happened yet, the GetScrollbarStyles call is really only
checking the container's scroll preferences (which is what we want, since we
want dynamic changes to propagated overflow to work without reconstructing the
viewport frame, otherwise we'd need more annoying detection/propagation logic).
Dynamic changes to the scrolling attribute won't work, but then again, they
never have.

Comment 2

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

checked in. It looks like this has indeed resolved the perf regression.

Comment 3

[Steve England [:stevee]]

roc, one of your checking has fixed a long-time bug when rendering
http://www.shacknews.com/
On previous builds the text inside the 'latest interesting comments' rendered
incorrectly until i changed font size. now it renders properly first time. just
a fyi.

Comment 4

[Wayne Woods]

The fix for this couldn't be responsible for bug 289766, could it? The timing is
right, as is the scrolling="no" requirement. Just checking.

Comment 5

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

I'd be surprised. This patch only changes whether we create scrollbars and
should not have any effect on how content areas get painted.

Comment 6

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

Then again, I've been surprised before. Someone could verify by backing out this
patch and seeing if it fixes the regression.

Comment 7

[Wayne Woods]

Is anyone able to create a build with the patch backed-out? I'm happy to test it
to see if it caused the bug I mentioned in comment 4, but I have no way of
managing a cvs on my Mac (including the know-how) to create the build myself.
Thanks :)