Bug 52681 – [chromium] Fix compositor repaints for offscreen fixed elements

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Adrienne Walker]

[chromium] Fix compositor repaints for offscreen fixed elements

[Adrienne Walker]

Created attachment 79650 [details]
Patch

[Adrienne Walker]

This bug was causing weird repainting issues on youtube when viewing hardware accelerated video.

See: http://crbug.com/69692

[James Robinson]

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?

[Adrienne Walker]

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

[Vangelis Kokkevis]

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

[James Robinson]

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

[Adrienne Walker]

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

[Adrienne Walker]

Created attachment 79654 [details]
Manual test

I've attached the manual test I created.

[Eric Seidel (no email)]

cmarrin likes compositor bugs. :)

[James Robinson]

(In reply to comment #9)
> cmarrin likes compositor bugs. :)

This is a chromium compositor bug.

[James Robinson]

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.

[Adrienne Walker]

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.

[James Robinson]

OK, just promise not to break it :)

[WebKit Commit Bot]

Comment on attachment 79650 [details]
Patch

Clearing flags on attachment: 79650

Committed r76437: <http://trac.webkit.org/changeset/76437>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.