Bug 70067

Created attachment 110931 [details] Patch

Comment on attachment 110931 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110931&action=review > Source/WebCore/rendering/RenderLayerCompositor.cpp:115 > + Frame* frame = m_renderView->frameView()->frame(); > + Page* page = frame ? frame->page() : 0; > + if (page) > + m_chromeClient = page->chrome()->client(); Is it OK to cache the chrome client? What happens when the compositor's Frame goes into the page cache? > Source/WebCore/rendering/RenderLayerCompositor.cpp:1314 > + || (m_chromeClient && m_chromeClient->requiresCompositingLayerForObject(renderer, &m_compositingDependsOnGeometry)); I'm a bit worried about the perf impact of this. To be clear, I'm fine allowing position:fixed to create compositing layers in some cases, but the specifics of when position:fixed causes composting should be hidden in client code. In iOS, RLC stores a cache of position:fixed layers, and tells ChromeClient when something goes into and out of that cache. Moving those layers around on scrolling is handled outside of WebCore, by poking things under GraphicsLayer.

(In reply to comment #2) > (From update of attachment 110931 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=110931&action=review > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:115 > > + Frame* frame = m_renderView->frameView()->frame(); > > + Page* page = frame ? frame->page() : 0; > > + if (page) > > + m_chromeClient = page->chrome()->client(); > > Is it OK to cache the chrome client? What happens when the compositor's Frame goes into the page cache? Thanks for flagging that. The page cache code is new to me but from an inspection it doesn't look the Frame's Page and the Pages's Chrome change when the Frame goes in and out of the cache. Is there reason to think otherwise? > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1314 > > + || (m_chromeClient && m_chromeClient->requiresCompositingLayerForObject(renderer, &m_compositingDependsOnGeometry)); > > I'm a bit worried about the perf impact of this. To be clear, I'm fine allowing position:fixed to create compositing layers in some cases, but the specifics of when position:fixed causes composting should be hidden in client code. > I started by adding a specific method on the ChromeClient just for dealing with fixed position but it occurred to me that it would be more flexible to have a generic hook like the one above. It can be used in the future to experiment with other implementation specific compositing triggers (e.g. maybe one for overflow divs) without poking again into RLC or needing to further extend ChromeClient. Regarding performance, are you worried about the addition of the virtual call into ChromeClient? > In iOS, RLC stores a cache of position:fixed layers, and tells ChromeClient when something goes into and out of that cache. Moving those layers around on scrolling is handled outside of WebCore, by poking things under GraphicsLayer. That's great to know, thanks. Are you opposed to adding layout-related information to GraphicsLayers? It seems that it could be generally useful.

Comment on attachment 110931 [details] Patch Attachment 110931 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/10073819

Comment on attachment 110931 [details] Patch Attachment 110931 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/10073822

Simon, how would you like to proceed with this patch? Are you opposed to making virtual method calls to client code? Would you prefer to have a flag that lets the RLC know that the client should be asked whether to create a layer so that the virtual calls aren't made on platforms that don't use this functionality?

Closing as client-triggered compositor triggers aren't needed anymore.

Comment on attachment 110931 [details] Patch Cleared review? from attachment 110931 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).