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
Adrienne Walker
2011-01-18 16:15:26 PST
Created attachment 79650 [details]
Patch
This bug was causing weird repainting issues on youtube when viewing hardware accelerated video. See: http://crbug.com/69692 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?
(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. (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? (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). (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. Created attachment 79654 [details]
Manual test
I've attached the manual test I created.
cmarrin likes compositor bugs. :) (In reply to comment #9) > cmarrin likes compositor bugs. :) This is a chromium compositor bug. 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.
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. OK, just promise not to break it :) Comment on attachment 79650 [details] Patch Clearing flags on attachment: 79650 Committed r76437: <http://trac.webkit.org/changeset/76437> All reviewed patches have been landed. Closing bug. |