Bug 71517 – RenderLayer::styleChanged invalidates the GraphicsLayer needlessly

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 113573 [details]
Proposed change: remove the invalidation code so that we rely on repainting properly.

[Darin Adler]

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.

[Simon Fraser (smfr)]

It's going to be hard to find out whether this causes painting regressions.

[Julien Chaffraix]

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

[Simon Fraser (smfr)]

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?

[Julien Chaffraix]

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

[Julien Chaffraix]

Created attachment 113681 [details]
Patch for landing

[Julien Chaffraix]

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.

[WebKit Review Bot]

Comment on attachment 113681 [details]
Patch for landing

Clearing flags on attachment: 113681

Committed r99307: <http://trac.webkit.org/changeset/99307>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.