| Summary: | RenderLayer::styleChanged invalidates the GraphicsLayer needlessly | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> | ||||||
| Component: | Layout and Rendering | Assignee: | Julien Chaffraix <jchaffraix> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | enne, jamesr, simon.fraser, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| URL: | http://jsbin.com/ulofav | ||||||||
| Attachments: |
|
||||||||
Created attachment 113573 [details]
Proposed change: remove the invalidation code so that we rely on repainting properly.
Comment on attachment 113573 [details] Proposed change: remove the invalidation code so that we rely on repainting properly. View in context: https://bugs.webkit.org/attachment.cgi?id=113573&action=review > Source/WebCore/ChangeLog:8 > + Refactoring covered by existing tests. I don’t think this meets the definition of “refactoring”. The claim here is that we are removing unneeded code. It's going to be hard to find out whether this causes painting regressions. (In reply to comment #3) > It's going to be hard to find out whether this causes painting regressions. What is sure is that we are over-invalidating and this is hurting us (see http://code.google.com/p/chromium/issues/detail?id=99814 for an example where we repaint each frame for no good reason). This patch is maybe going too far (on purpose) and the scope can be reduced for safety. It may also be a good experiment to assess and extend our repaint coverage. Comment on attachment 113573 [details]
Proposed change: remove the invalidation code so that we rely on repainting properly.
I think it's OK to check this in, but we should be on the lookout for repainting regressions. Do all the pixel tests pass?
(In reply to comment #5) > (From update of attachment 113573 [details]) > I think it's OK to check this in, but we should be on the lookout for repainting regressions. Do all the pixel tests pass? Yes, all the pixel tests passed on Chromium Linux (the EWS confirmed that I did not miss some tests). I have tested also a bit on Mac but I have run into other issues at the time (what I got looked fine but it was noisy). I have to redo the ChangeLog to account for my sloppy use of the word 'refactoring' so I will re-run the tests on Mac before landing. Created attachment 113681 [details]
Patch for landing
There was still a lot of noise when running the tests on Mac. Most of them are because NRWT does not pick up the custom scrollbars or some missing rebaselines. One or 2 were looking weird but did not look like repainting issue. I will watch the bots and see what they think. Comment on attachment 113681 [details] Patch for landing Clearing flags on attachment: 113681 Committed r99307: <http://trac.webkit.org/changeset/99307> All reviewed patches have been landed. Closing bug. |
Currently we invalidate the whole GraphicsLayer with: void RenderLayer::styleChanged(StyleDifference diff, const RenderStyle* oldStyle) ... if (m_backing && diff >= StyleDifferenceRepaint) m_backing->setContentsNeedDisplay(); This does not seem to take into account that the repainting logic will invalidate the needed part of the layer.