Bug 68135 – [chromium] CCLayerTreeHost shouldn't be RefCounted

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[James Robinson]

[chromium] CCLayerTreeHost shouldn't be RefCounted

[James Robinson]

Created attachment 107431 [details]
just FYI, webgl printing is broken

[Adrienne Walker]

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/

[Antoine Labour]

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

[James Robinson]

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

[Stephen Chenney]

https://code.google.com/p/chromium/issues/detail?id=230626