288117 - Eliminate nsScrollBoxFrame

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

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:

nsHTML/XULScrollFrame
  <scrollbar 1>
  <scrollbar 2>
  <scrollcorner>
  <the scrolled frame>

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

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is this going to cause problems for scrollbars in XUL?

Comment 2

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

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.

Comment 3

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

Attachment: fix

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
nsGfxScrollFrameInner::LayoutScrollArea.

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.

Comment 4

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

Oh, I tested composer as well.

Comment 5

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

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.

Comment 6

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

Attachment: oops, right fix

That was, of course, the wrong patch. This is the right patch.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 179000 [details] [diff] [review]
oops, right fix

Call nsIFrame::NeedsView ClassNeedsView or ClassAlwaysNeedsView instead, and
r+sr=dbaron.

Comment 8

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

checked in

Comment 9

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

No real Tp impact. Maybe a slight improvement on pawn (Camino). Too bad btek is
offline.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I backed this out to figure out whether it was the cause of the performance
regressions during the tinderbox outage.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 12

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

I talked to dbaron on IRC and I think we agreed to reland this once the
regressions are fixed.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Note the bug 288459 layout regression too.

Comment 14

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

Attachment: fix for regressions

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

Comment 15

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

Checked in the original patch and the regression fix.

Comment 16

[Marian POPESCU]

Attachment: Please observe list appearence

Comment 17

[Marian POPESCU]

Attachment: The CSS file for the list layout error

Comment 18

[Marian POPESCU]

Attachment: This is the correct list layout

Comment 19

[Marian POPESCU]

Attachment: This is the incorrect list layout

Comment 20

[Marian POPESCU]

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

Comment 21

[Marian POPESCU]

I filled bug 288949.

Comment 22

[Simon Fraser [no longer active]]

This caused bug 289022.

Comment 23

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

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

Comment 24

[Andy Willis (abwillis)]

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)