Bug 145554 – Calling FrameView::viewportContentsChanged() after style recalcs is too expensive

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

[Chris Dumez]

Calling FrameView::viewportContentsChanged() after style recalcs is too expensive and slows down scrolling on some sites. The reason will call FrameView::viewportContentsChanged() after style recalc is to resume animations in case a composited animation brings an element inside the viewport (as these don't necessarily cause a layout). However, it is very wasteful to cause viewportContentsChanged() for every style recalc as most of them cannot cause an element to be visible without causing a layout.

We should probably rely on compositing layers being updated after the style recalc instead.

Radar: <rdar://problem/21189478>

[Chris Dumez]

Created attachment 254075 [details]
Patch

[Darin Adler]

Comment on attachment 254075 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254075&action=review

> Source/WebCore/ChangeLog:22
> +        No new tests, already covered by:
> +        fast/images/animated-gif-webkit-transform.html

Unfortunate that there is no way to test the performance improvement and catch if we screw it up. We should think about a way to regression test this kind of change. I have a similar issue with my patch that cuts down on unnecessary layout when updating an image but not its dimensions.

[Chris Dumez]

Comment on attachment 254075 [details]
Patch

Simon, can you please take a quick look as well before I land?

[Simon Fraser (smfr)]

Comment on attachment 254075 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254075&action=review

> Source/WebCore/rendering/RenderLayerCompositor.h:124
> +    void didRecalcStyleWithNoPendingLayout(bool& updatedCompositedLayers);

I think it would be nicer if this were just the return value, with a comment saying what the return value means.

[Chris Dumez]

Created attachment 254080 [details]
Patch

[Chris Dumez]

(In reply to comment #4)
> Comment on attachment 254075 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254075&action=review
> 
> > Source/WebCore/rendering/RenderLayerCompositor.h:124
> > +    void didRecalcStyleWithNoPendingLayout(bool& updatedCompositedLayers);
> 
> I think it would be nicer if this were just the return value, with a comment
> saying what the return value means.

Ok, done.

[WebKit Commit Bot]

Comment on attachment 254080 [details]
Patch

Clearing flags on attachment: 254080

Committed r185118: <http://trac.webkit.org/changeset/185118>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.