Jeff Muizelaar [:jrmuizel] Reporter
	Description • 15 years ago

I've put a test case up at:

http://people.mozilla.org/~jmuizelaar/reader/reader.htm

It is worse in both 3.6 and trunk than 3.5

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 1 • 15 years ago

What happens here is that the fixed-pos text "View settings..." overflows microscopically into the scrolling text below:

    Text 0x9dc2830(Text(0)) (180,192,5211,900)
    Clip 0x0() (180,1105,79920,37731)

The first display item is the text, the second one is the clip region. The clip region gets snapped to pixels so its actual y-coordinate is at 1080 appunits.

Then, when we're scrolling down by lines, we're repainting a single row of pixels at the top and a line's worth of pixels at the bottom. Thus the bounding rectangle for the intersection of the visible region and the box-shadow is really large, and we draw a really large box-shadow, which is slow.

I'm not quite sure what to do here. It's unfortunate that pixel-snapping the scroll area makes it intersect with the text, but if the text used a font with unusually large descenders, it would intersect anyway.

Perhaps we should just speed up box-shadow. One option would be to store a visible region for nsDisplayBoxShadow instead of just a rect, and for disconnected regions like this just paint each part independently. Or we could speed up the blurring, see bug 509052.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 2 • 15 years ago

This makes it pretty good.

	David Baron :dbaron:
	Comment 3 • 15 years ago

Comment on attachment 414439 [details] [diff] [review]
fix

>+  nsRect borderRect = nsRect(offset, mFrame->GetSize());

Probably better to write
nsRect borderRect(offset, mFrame->GetSize());

r=dbaron

	Jeff Muizelaar [:jrmuizel] Reporter
	Comment 4 • 15 years ago

Why did this get so much worse in the first place and why do 3.6 and trunk have such different behavior? I should also note, that the problems were also really only noticeable on win32 (win7)

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 5 • 15 years ago

This would have been a regression from bug 513082, which snapped the clip area to pixels during display list analysis for scrolling, thus making it appear to overlap the text.

Bug 513082 was itself a fix for problems introduced by bug 507334. And bug 507334 reworked some of our scrolling analysis to enable optimizations needed for the Fennec UI.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 6 • 15 years ago

While testing bug 531461 I noticed that ComputeDisjointRectangles was embarrassingly broken. In particular we failed to reset 'accumulated' to empty after using it, and if a subsequent region rect failed to be accumulated into the existing 'accumulated', we'd just ignore it.

	David Baron :dbaron:
	Comment 7 • 15 years ago

Comment on attachment 415067 [details] [diff] [review]
fix v2

This seems fine, although adding only the single accumulated.Empty() line to the previous patch also does (and perhaps faster).  Or is there a difference I'm missing?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 8 • 15 years ago

Just adding accumulated.Empty() is not enough. If we do that, then in any iteration where r is non-null and we flush 'accumulated', *r is ignored.

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 15 years ago

Pushed http://hg.mozilla.org/mozilla-central/rev/1a0f2d658a49

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 15 years ago

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/035882e47ab3

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 15 years ago

Pushed http://hg.mozilla.org/mozilla-central/rev/8741cf0d0621 and http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2cd073bb2314 to fix the reftest orange due to the test ref name being typoed.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 15 years ago

And the reftest reference itself had a spurious reftests-wait too.  Pushed http://hg.mozilla.org/mozilla-central/rev/54e9fe755ea4 and http://hg.mozilla.org/releases/mozilla-1.9.2/rev/86a1fb0acac3 to fix.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 13 • 15 years ago

Sorry :-( thanks for fixing that up.

	Bryan
	Comment 14 • 15 years ago

Is this the same issue that now appears on http://www.boygeniusreport.com/ ???  Scrolling is now really choppy and laggy and I don't believe it was this was some time ago.

BUILD: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091201 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20091201022333

~B

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 15 • 15 years ago

Can you file a new bug for that? (It seems fine to me on my latest Mac and Windows builds.)

	Jeff Muizelaar [:jrmuizel] Reporter
	Comment 16 • 15 years ago

This is still much worse than 3.5:

On this page (http://people.mozilla.org/~jmuizelaar/reader/reader-auto.htm) I get 

took 87371ms
5218avg:83.720774243005
max:115,465

with this build:

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091202 Minefield/3.7a1pre (.NET CLR 3.5.30729)

vs.

3.5 which gives:

took 87371ms
5218avg:83.720774243005
max:115,465

	Jeff Muizelaar [:jrmuizel] Reporter
	Comment 17 • 15 years ago

More recent nightlies have improved though. A build from 11/11 gives:

took 102318ms
5218avg:98.04331161364509
max:120,4855

	David Baron :dbaron:
	Comment 18 • 15 years ago

The two sets of numbers in comment 16 are the same.

	Jeff Muizelaar [:jrmuizel] Reporter
	Comment 19 • 15 years ago

Indeed, the actual numbers for 3.5 are:

took 16788ms
5218avg:16.08662322729015
max:108,0

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 20 • 15 years ago

I don't know why I didn't see this before, but the scrolled content is inside a table, for which we create an nsTableBorderBackground. This doesn't move when we scroll, so we think we need to repaint most of the window to re-composite the scrolled content over the background.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 21 • 15 years ago

OK, this takes the patch in bug 531461 a step further, by creating nsDisplayTableBorderBackground only if a table part actually has a border or background.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 22 • 15 years ago

Oops, we need to null-check currentItem in nsTableCellFrame now too.

	David Baron :dbaron:
	Comment 23 • 15 years ago

Comment on attachment 415797 [details] [diff] [review]
ignore tables v2

>+       aFrame->GetStyleDisplay()->mAppearance ||

This may as well be aFrame->IsThemed()

r=dbaron

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 24 • 15 years ago

I saw a crash in AnyTablePartHasBorderOrBackground. It may have been related to my build being incorrectly partially rebuilt, but I'm not sure, so I've pushed the build to the try-server and not checking in for now.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 25 • 15 years ago

OK, all tests passed on all three platforms, so I guess we should push this.

	David Baron :dbaron:
	Comment 26 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/18afa623a64f

	David Baron :dbaron:
	Comment 27 • 15 years ago

Hmmm. Casuing bustage because this:

../../../dist/include/nsStyleStruct.h:831: warning: inline function 'PRBool nsStyleBorder::IsBorderImageLoaded() const' used but never defined

leads to linkage errors.  I'll try moving the function to be non-inline.

	David Baron :dbaron:
	Comment 28 • 15 years ago

Bustage fix (moved back to non-inline):
http://hg.mozilla.org/mozilla-central/rev/ecb85a29af12

	David Baron :dbaron:
	Comment 29 • 15 years ago

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/dcdb623dd8d9

	Jeff Muizelaar [:jrmuizel] Reporter
	Comment 30 • 15 years ago

A more recent nightly gives:

took 13809ms
5218avg:13.232081257186662
max:1022,2905

which is even better than we had before (though the max number is unusually high).