Bug 71517 - RenderLayer::styleChanged invalidates the GraphicsLayer needlessly
Summary: RenderLayer::styleChanged invalidates the GraphicsLayer needlessly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL: http://jsbin.com/ulofav
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-03 16:18 PDT by Julien Chaffraix
Modified: 2011-11-04 11:53 PDT (History)
4 users (show)

See Also:


Attachments
Proposed change: remove the invalidation code so that we rely on repainting properly. (2.07 KB, patch)
2011-11-03 16:30 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (2.05 KB, patch)
2011-11-04 11:00 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2011-11-03 16:18:29 PDT
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.
Comment 1 Julien Chaffraix 2011-11-03 16:30:22 PDT
Created attachment 113573 [details]
Proposed change: remove the invalidation code so that we rely on repainting properly.
Comment 2 Darin Adler 2011-11-03 17:26:22 PDT
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.
Comment 3 Simon Fraser (smfr) 2011-11-03 17:31:59 PDT
It's going to be hard to find out whether this causes painting regressions.
Comment 4 Julien Chaffraix 2011-11-03 17:50:31 PDT
(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 5 Simon Fraser (smfr) 2011-11-03 18:51:25 PDT
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?
Comment 6 Julien Chaffraix 2011-11-03 19:51:49 PDT
(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.
Comment 7 Julien Chaffraix 2011-11-04 11:00:25 PDT
Created attachment 113681 [details]
Patch for landing
Comment 8 Julien Chaffraix 2011-11-04 11:06:30 PDT
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 9 WebKit Review Bot 2011-11-04 11:53:11 PDT
Comment on attachment 113681 [details]
Patch for landing

Clearing flags on attachment: 113681

Committed r99307: <http://trac.webkit.org/changeset/99307>
Comment 10 WebKit Review Bot 2011-11-04 11:53:16 PDT
All reviewed patches have been landed.  Closing bug.