Closed Bug 288117 Opened 15 years ago Closed 15 years ago

Eliminate nsScrollBoxFrame


(Core :: Layout, defect)

Not set





(Reporter: roc, Assigned: roc)




(6 files, 1 obsolete file)

I'm working on eliminating nsScrollBoxFrame so that the scrolled frame is a
direct child of the nsHTML/XULScrollFrame. I'll have a patch soon. The frame and
view structure looks like this:

  <scrollbar 1>
  <scrollbar 2>
  <the scrolled frame>

The scrollbars and scrollcorner might not be present, e.g. if the style is

The scroll frame always has a view, as does the scrolled frame. In between is an
anonymous view, the view implementing nsIScrollableView (nsScrollPortView). This
view is positioned and sized to where the nsScrollBoxFrame used to be.

Apart from trimming down the code and making a shallower frame tree, this will
make it possible for nsHTMLScrollFrame to stop being a box frame and directly
reflow its child using normal reflow rules.
Is this going to cause problems for scrollbars in XUL?
This step won't, it shouldn't change the reflow logic in visible ways.

The next step won't either because nsXULScrollFrame (which wraps all XUL boxes)
won't be modified.
Attached patch fix (obsolete) — Splinter Review
The fix is mostly not very difficult. nsScrollBoxFrame is a fairly thin wrapper
around the scrolled frame, and it was always guaranteed to have no borders,
margins or padding, so in most places we can replace access to the scrollbox
with direct access to the scrolled frame. The biggest chunk of logic that
needed to be preserved was copying nsScrollBoxFrame::DoLayout to

I'm changing scrolling=no iframes to get a real nsHTMLScrollFrame ... there is
no choice since there are no nsScrollBoxFrames anymore. David did this before
and saw a Tp problem and had to back it out. I'm hoping that there's enough
gain from removing the extra layer of frames that that is no longer a problem.

I'm forcing view creation for the scrollframe to simplify the logic inside the
scrollframe and also ensure that view reparenting still works (otherwise we'd
have problems when looking for views to reparent, we wouldn't find the
scrollframe's interior view). I've created a new nsIFrame API "NeedsView" for
this that we can use for other things.

I was a bit concerned that adding this non-leaf anonymous view might cause
problems for code that calculates coordinates by traversing the frame ancestors
and adding up view and frame positions. Things seem to be mostly OK, probably
because once upon a time "native" scrollframes had similar anonymous views. I
did have to fix nsContainerFrame::PositionFrameView and
nsFrame::GetContentAndOffsetsFromPoint. We do *really* need to get rid of all
uses of GetOffsetFromView and GetOriginFromViewOffset, every time I look at
them I barf.

I've tested the usual suspects --- scrolling menus, comboboxes, autocomplete,
listboxes, nested iframes, scrolling="no", general browsing, and it currently
all seems to work very well. The patch did expose a transient garbage painting
issue for which I have a separate fix in the view manager (bug 288222). There
is of course the big question of what this will do to Tp. Even if this hurts Tp
a bit for some reason, I think making the HTMLScrollFrame not be a box and be
smarter will get us more back.

After this, nsScrollBoxFrame.cpp still has the autorepeatbutton code, which
really should live somewhere else. I will file a followup bug to clean that up.
Attachment #178999 - Flags: superreview?(dbaron)
Attachment #178999 - Flags: review?(dbaron)
Oh, I tested composer as well.
Depends on: 288222
Also, I noticed that if the scrolledframe is overflowing, we fire an 'overflow'
event every time we reflow the scrolled frame. Is this really necessary? It
seems like it could be hurting performance.
Attached patch oops, right fixSplinter Review
That was, of course, the wrong patch. This is the right patch.
Attachment #178999 - Attachment is obsolete: true
Attachment #179000 - Flags: superreview?(dbaron)
Attachment #179000 - Flags: review?(dbaron)
Attachment #178999 - Flags: superreview?(dbaron)
Attachment #178999 - Flags: review?(dbaron)
Comment on attachment 179000 [details] [diff] [review]
oops, right fix

Call nsIFrame::NeedsView ClassNeedsView or ClassAlwaysNeedsView instead, and
Attachment #179000 - Flags: superreview?(dbaron)
Attachment #179000 - Flags: superreview+
Attachment #179000 - Flags: review?(dbaron)
Attachment #179000 - Flags: review+
checked in
Closed: 15 years ago
Resolution: --- → FIXED
Blocks: 260652
No real Tp impact. Maybe a slight improvement on pawn (Camino). Too bad btek is
Depends on: 288451
I backed this out to figure out whether it was the cause of the performance
regressions during the tinderbox outage.
I got good performance data from the backout (it's still backed out, not sure
whether it should reland given the crash regressions):

 * 1.5% Tp regression on btek
 * 1.5% Tp regression on luna
 * 6% Txul regression on luna
 * 1.5% Ts regression on luna
 * no significant Tp change on monkey
 * 11% Txul **improvement** on monkey
I talked to dbaron on IRC and I think we agreed to reland this once the
regressions are fixed.
Depends on: 288459
Resolution: FIXED → ---
Note the bug 288459 layout regression too.
Here's the fix for those regressions. It also fixes a few regression-y problems
I found along the way.

nsCSSFrameConstructor was setting the viewport child list twice in some cases.
The solution is to just make FinishBuildingScrollFrame happen symmetrically
with BeginBuildingScrollFrame (both simply "if (isScrollable)"). This was
causing an assertion failure but no actual bug as far as I can tell.

nsListBoxBodyFrame had a few places where it assumed it was the grandchild of
the scrollframe. I fixed those to use appropriate nsLayoutUtils functions.
nsListBoxObject had a similar issue which caused GetListBoxBody to always fail,
which was the real cause of this bug (too bad it only manifested in such a
subtle way).

I also checked the code for trees, and tested them, and they seem to be OK.

DocShell::SetCanvasHasFocus assumed that the canvas frame always has a view,
which isn't true anymore. So we just refresh all views (not much worse than
refreshing just the canvas viwe).
Attachment #179243 - Flags: superreview?(dbaron)
Attachment #179243 - Flags: review?(dbaron)
Attachment #179243 - Flags: superreview?(dbaron)
Attachment #179243 - Flags: superreview+
Attachment #179243 - Flags: review?(dbaron)
Attachment #179243 - Flags: review+
Checked in the original patch and the regression fix.
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
I have found a bug related to this check-in.
I use Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050404
Firefox/1.0+, Linux ocean Clobber release Started 04/03 22:25, finished 04/04 01:55.

As visible on the two screenshots, the list layout has changed.
I attached a testcase (HTML and CSS).
Depends on: 288949
I filled bug 288949.
(In reply to comment #7)
=> Call nsIFrame::NeedsView ClassNeedsView or ClassAlwaysNeedsView instead, and
> r+sr=dbaron.

I forgot to change the name back again when I relanded, sorry... 

This seems to have broken copy and paste from non-urlbar parts of the browser in
OS/2 using MB2-MB1 (press MB2 and hold while then pressing MB1) to paste and MB1
highlight - MB2 (Highlight with mb1 and hold MB1 down while then pressing MB2)
copy.  It works fine in URL bar but in this form for instance I can't copy or
paste and I couldn't copy the description below for another.
MB1 (ussually left and MB2 right)
Depends on: 293453
Target Milestone: --- → mozilla1.8beta2
Depends on: 374327
You need to log in before you can comment on or make changes to this bug.