[chromium] CCLayerTreeHost shouldn't be RefCounted
Created attachment 107431 [details] just FYI, webgl printing is broken
Comment on attachment 107431 [details] just FYI, webgl printing is broken View in context: https://bugs.webkit.org/attachment.cgi?id=107431&action=review > Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:-89 > - RenderLayerBacking* backing = static_cast<RenderLayerBacking*>(m_owner->client()); > - if (!backing || backing->paintingGoesToWindow()) > - return; > - Can you remind me why we had these checks originally, but why we don't need them now? > Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:-156 > -void VideoLayerChromium::setLayerTreeHost(CCLayerTreeHost* host) > -{ > - if (host && layerTreeHost() != host) { > - for (size_t i = 0; i < 3; ++i) { > - m_textures[i].m_visibleSize = IntSize(); > - m_textures[i].m_texture = ManagedTexture::create(host->contentsTextureManager()); > - } > - } We don't need this because we don't ever destroy the CCLayerTreeHost once we've created it? I guess I probably should have removed that earlier, then. > Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.cpp:127 > + if (CCLayerTreeHost::animating() && m_contextSupportsRateLimitingExtension && !m_rateLimitingTimer.isActive()) This static function call here has some code smell to it, but from comments elsewhere it looks like this is only temporary. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:235 > -void CCLayerTreeHost::clearRenderSurfacesRecursive(LayerChromium* layer) > +void CCLayerTreeHost::clearStuffRecursive(LayerChromium* layer) Why is the original name not valid? > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:576 > bool WebMediaPlayerClientImpl::acceleratedRenderingInUse() > { > - return m_videoLayer.get() && m_videoLayer->layerTreeHost(); > + return m_videoLayer; > } Given that we never clear the LayerTreeHost (or LayerRendererChromium in the past) from WebViewImpl, this makes the condition consistent on the first time a page goes composited before the LayerTreeHost gets created. However, my only confusion is why the check for the LTH/LRC was needed in the first place. > Source/WebKit/chromium/src/WebViewImpl.h:575 > - RefPtr<WebCore::CCLayerTreeHost> m_layerTreeHost; > + OwnPtr<WebCore::CCLayerTreeHost> m_layerTreeHost; \o/
(In reply to comment #2) > (From update of attachment 107431 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107431&action=review > > > Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:-89 > > - RenderLayerBacking* backing = static_cast<RenderLayerBacking*>(m_owner->client()); > > - if (!backing || backing->paintingGoesToWindow()) > > - return; > > - > > Can you remind me why we had these checks originally, but why we don't need them now? BTW they've already been removed by https://bug-67750-attachments.webkit.org/attachment.cgi?id=107377 (see comments in the bug). > > > Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:-156 > > -void VideoLayerChromium::setLayerTreeHost(CCLayerTreeHost* host) > > -{ > > - if (host && layerTreeHost() != host) { > > - for (size_t i = 0; i < 3; ++i) { > > - m_textures[i].m_visibleSize = IntSize(); > > - m_textures[i].m_texture = ManagedTexture::create(host->contentsTextureManager()); > > - } > > - } > > We don't need this because we don't ever destroy the CCLayerTreeHost once we've created it? I guess I probably should have removed that earlier, then. > > > Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.cpp:127 > > + if (CCLayerTreeHost::animating() && m_contextSupportsRateLimitingExtension && !m_rateLimitingTimer.isActive()) > > This static function call here has some code smell to it, but from comments elsewhere it looks like this is only temporary. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:235 > > -void CCLayerTreeHost::clearRenderSurfacesRecursive(LayerChromium* layer) > > +void CCLayerTreeHost::clearStuffRecursive(LayerChromium* layer) > > Why is the original name not valid? That function was removed by https://bug-67883-attachments.webkit.org/attachment.cgi?id=107434 anyway. If you rebase, you can trim a bunch of stuff! > > > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:576 > > bool WebMediaPlayerClientImpl::acceleratedRenderingInUse() > > { > > - return m_videoLayer.get() && m_videoLayer->layerTreeHost(); > > + return m_videoLayer; > > } > > Given that we never clear the LayerTreeHost (or LayerRendererChromium in the past) from WebViewImpl, this makes the condition consistent on the first time a page goes composited before the LayerTreeHost gets created. However, my only confusion is why the check for the LTH/LRC was needed in the first place. > > > Source/WebKit/chromium/src/WebViewImpl.h:575 > > - RefPtr<WebCore::CCLayerTreeHost> m_layerTreeHost; > > + OwnPtr<WebCore::CCLayerTreeHost> m_layerTreeHost; > > \o/
(In reply to comment #2) > > Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:-156 > > -void VideoLayerChromium::setLayerTreeHost(CCLayerTreeHost* host) > > -{ > > - if (host && layerTreeHost() != host) { > > - for (size_t i = 0; i < 3; ++i) { > > - m_textures[i].m_visibleSize = IntSize(); > > - m_textures[i].m_texture = ManagedTexture::create(host->contentsTextureManager()); > > - } > > - } > > We don't need this because we don't ever destroy the CCLayerTreeHost once we've created it? I guess I probably should have removed that earlier, then. It's not a straight delete - I added some code in reserveTexture() as well. > > > Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.cpp:127 > > + if (CCLayerTreeHost::animating() && m_contextSupportsRateLimitingExtension && !m_rateLimitingTimer.isActive()) > > This static function call here has some code smell to it, but from comments elsewhere it looks like this is only temporary. I actually think that this is permanent. "am I animating right now?" is a process-wide piece of state, so I think having a static getter is reasonable (we can only animate from one thread and even if we have a bunch of different WebViewImpls only one can animate at a time) > > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:576 > > bool WebMediaPlayerClientImpl::acceleratedRenderingInUse() > > { > > - return m_videoLayer.get() && m_videoLayer->layerTreeHost(); > > + return m_videoLayer; > > } > > Given that we never clear the LayerTreeHost (or LayerRendererChromium in the past) from WebViewImpl, this makes the condition consistent on the first time a page goes composited before the LayerTreeHost gets created. However, my only confusion is why the check for the LTH/LRC was needed in the first place. I have no idea actually - I've never really looked at WebMediaPlayerClientImpl before and most of the code in it seems really wrong. I only changed this because stuff didn't compile, and this change makes the tests continue to pass. I will take a more careful look at what this is trying to do before landing.
https://code.google.com/p/chromium/issues/detail?id=230626