Bug 52681

Summary: [chromium] Fix compositor repaints for offscreen fixed elements
Product: WebKit Reporter: Adrienne Walker <enne>
Component: WebKit Misc.Assignee: Adrienne Walker <enne>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, commit-queue, enne, eric, fishd, jamesr, kbr, vangelis
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Manual test none

Description Adrienne Walker 2011-01-18 16:15:26 PST
[chromium] Fix compositor repaints for offscreen fixed elements
Comment 1 Adrienne Walker 2011-01-20 14:14:50 PST
Created attachment 79650 [details]
Patch
Comment 2 Adrienne Walker 2011-01-20 14:16:48 PST
This bug was causing weird repainting issues on youtube when viewing hardware accelerated video.

See: http://crbug.com/69692
Comment 3 James Robinson 2011-01-20 14:24:04 PST
Comment on attachment 79650 [details]
Patch

Can we at least add a test somewhere that would demonstrate the issue, even if it doesn't show the issue in our current test harness?
Comment 4 Adrienne Walker 2011-01-20 14:27:05 PST
(In reply to comment #3)
> (From update of attachment 79650 [details])
> Can we at least add a test somewhere that would demonstrate the issue, even if it doesn't show the issue in our current test harness?

Sure.  I'll admit that I'm reluctant to add a manual test that will almost certainly gather copious amounts of dust, but if you tell me where it should go, I'd be glad to add it to this patch.
Comment 5 Vangelis Kokkevis 2011-01-20 14:29:06 PST
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 79650 [details] [details])
> > Can we at least add a test somewhere that would demonstrate the issue, even if it doesn't show the issue in our current test harness?
> 
> Sure.  I'll admit that I'm reluctant to add a manual test that will almost certainly gather copious amounts of dust, but if you tell me where it should go, I'd be glad to add it to this patch.

Is it not possible to have a pixel test for it and demonstrate it by scrolling the page in JS?
Comment 6 James Robinson 2011-01-20 14:40:10 PST
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 79650 [details] [details])
> > Can we at least add a test somewhere that would demonstrate the issue, even if it doesn't show the issue in our current test harness?
> 
> Sure.  I'll admit that I'm reluctant to add a manual test that will almost certainly gather copious amounts of dust, but if you tell me where it should go, I'd be glad to add it to this patch.

What does the manual test look like?  It's probably possible to coerce it into something that we can run in DRT (although it may be tricksy).
Comment 7 Adrienne Walker 2011-01-20 14:50:02 PST
(In reply to comment #5)

> Is it not possible to have a pixel test for it and demonstrate it by scrolling the page in JS?

I have a page that automatically scrolls and demonstrates this bug in the browser.  I've spent a good bit of time unsuccessfully trying to make it work in the test harness.

After some information from jamesr, I verified that the FrameView::scrollContentsFastPath code path that I modified doesn't get called in DumpRenderTree.  It seems to use FrameView::scrollContentsSlowPath, which always works because everything is invalidated.  So, this bug doesn't get triggered when using DumpRenderTree at this point.

I can file a bug about improving our test coverage, but I'd prefer to not shave that yak in this patch.
Comment 8 Adrienne Walker 2011-01-20 14:55:12 PST
Created attachment 79654 [details]
Manual test

I've attached the manual test I created.
Comment 9 Eric Seidel (no email) 2011-01-21 03:06:25 PST
cmarrin likes compositor bugs. :)
Comment 10 James Robinson 2011-01-21 09:28:31 PST
(In reply to comment #9)
> cmarrin likes compositor bugs. :)

This is a chromium compositor bug.
Comment 11 James Robinson 2011-01-21 13:44:33 PST
Comment on attachment 79650 [details]
Patch

R=me.  Please consider scripting that manual test and adding it to compositing/, even though it doesn't fail without this patch in chromium today.  At some point in the future the test code will get close enough to the real code for that test to be a real regression test.
Comment 12 Adrienne Walker 2011-01-21 13:58:34 PST
Thanks for the review.

(In reply to comment #11)
> (From update of attachment 79650 [details])
> R=me.  Please consider scripting that manual test and adding it to compositing/, even though it doesn't fail without this patch in chromium today.  At some point in the future the test code will get close enough to the real code for that test to be a real regression test.

Forgive me if I'm reluctant to add a test that doesn't actually test this regression.  Each additional test is a burden and this has no immediate benefit.

The bug tracker will keep this file around until we need it.  Also, if this issue ever regresses, we can reprioritize changing how we scroll when testing.
Comment 13 James Robinson 2011-01-21 14:38:11 PST
OK, just promise not to break it :)
Comment 14 WebKit Commit Bot 2011-01-22 02:00:21 PST
Comment on attachment 79650 [details]
Patch

Clearing flags on attachment: 79650

Committed r76437: <http://trac.webkit.org/changeset/76437>
Comment 15 WebKit Commit Bot 2011-01-22 02:00:27 PST
All reviewed patches have been landed.  Closing bug.