Closed Bug 288888 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(1 file)

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.
Attached patch fixSplinter Review
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.
Attachment #179517 - Flags: superreview?(dbaron)
Attachment #179517 - Flags: review?(dbaron)
Attachment #179517 - Flags: superreview?(dbaron)
Attachment #179517 - Flags: superreview+
Attachment #179517 - Flags: review?(dbaron)
Attachment #179517 - Flags: review+
checked in. It looks like this has indeed resolved the perf regression.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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.
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.
I'd be surprised. This patch only changes whether we create scrollbars and
should not have any effect on how content areas get painted.
Then again, I've been surprised before. Someone could verify by backing out this
patch and seeing if it fixes the regression.
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 :)
Duplicate of this bug: 100547
You need to log in before you can comment on or make changes to this bug.