Closed Bug 530686 Opened 12 years ago Closed 12 years ago

Scrolling Google Reader has gotten much worse.

Categories

(Core :: Layout, defect, P2)

x86
Windows 7
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- final-fixed

People

(Reporter: jrmuizel, Assigned: roc)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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
Blocks: 527728
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.
Assignee: nobody → roc
Flags: blocking1.9.2?
Attached patch fix (obsolete) — Splinter Review
This makes it pretty good.
Attachment #414439 - Flags: review?(dbaron)
Whiteboard: [needs landing]
Whiteboard: [needs landing] → [needs review]
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
Attachment #414439 - Flags: review?(dbaron) → review+
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)
Whiteboard: [needs review] → [needs landing]
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.
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Attached patch fix v2Splinter Review
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.
Attachment #414439 - Attachment is obsolete: true
Attachment #415067 - Flags: review?(dbaron)
Whiteboard: [needs landing] → [needs review]
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?
Attachment #415067 - Flags: review?(dbaron) → review+
Whiteboard: [needs review] → [needs landing]
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.
Pushed http://hg.mozilla.org/mozilla-central/rev/1a0f2d658a49
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
Whiteboard: [needs landing]
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.
Sorry :-( thanks for fixing that up.
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
Can you file a new bug for that? (It seems fine to me on my latest Mac and Windows builds.)
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
More recent nightlies have improved though. A build from 11/11 gives:

took 102318ms
5218avg:98.04331161364509
max:120,4855
The two sets of numbers in comment 16 are the same.
Indeed, the actual numbers for 3.5 are:

took 16788ms
5218avg:16.08662322729015
max:108,0
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.
Attached patch ignore tables (obsolete) — Splinter Review
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.
Attachment #415792 - Flags: review?(dbaron)
Whiteboard: [needs review]
Attached patch ignore tables v2Splinter Review
Oops, we need to null-check currentItem in nsTableCellFrame now too.
Attachment #415792 - Attachment is obsolete: true
Attachment #415797 - Flags: review?(dbaron)
Attachment #415792 - Flags: review?(dbaron)
Comment on attachment 415797 [details] [diff] [review]
ignore tables v2

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

This may as well be aFrame->IsThemed()

r=dbaron
Attachment #415797 - Flags: review?(dbaron) → review+
Whiteboard: [needs review] → [needs landing]
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.
Whiteboard: [needs landing] → [needs analysis]
OK, all tests passed on all three platforms, so I guess we should push this.
Keywords: checkin-needed
Whiteboard: [needs analysis] → [needs landing]
Depends on: 531461
http://hg.mozilla.org/mozilla-central/rev/18afa623a64f
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 1.9.2 landing]
Target Milestone: --- → mozilla1.9.3a1
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.
Bustage fix (moved back to non-inline):
http://hg.mozilla.org/mozilla-central/rev/ecb85a29af12
Keywords: checkin-needed
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).
Status: RESOLVED → VERIFIED
Depends on: 541668
Depends on: 542620
You need to log in before you can comment on or make changes to this bug.