Bug 45845 – [chromium] WebViewImpl should not destroy accelerated compositor when compositing is not needed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Vangelis Kokkevis]

Currently the LayerRendererChromium created for the WebViewImpl gets destroyed when compositing is no longer needed for the page.  Pages that turn compositing on and off regularly (maps.google.com is one of them when you zoom the map) suffer as every time the compositor is initialized it has to create its shader programs, VBOs etc.  There's really little overhead keeping the compositor around even if it doesn't get used.

[Vangelis Kokkevis]

Created attachment 67744 [details]
proposed patch

[James Robinson]

Comment on attachment 67744 [details]
proposed patch

Looks like a great optimization but I think the logic is off.

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

> WebKit/chromium/src/WebViewImpl.cpp:317
> +    m_layerRenderer.clear();
You don't have to do this. m_layerRenderer is an OwnPtr() and will delete what it points to when it is destroyed.

> WebKit/chromium/src/WebViewImpl.cpp:2313
>          m_layerRenderer = LayerRendererChromium::create(getOnscreenGLES2Context());
>          if (m_layerRenderer) {
>              m_isAcceleratedCompositingActive = true;
> +            m_compositorCreationFailed = false;
I don't quite understand - if someone calls setIsAcceleratedCompositing(false) then setIsAcceleratedCompositing(true), then on the second call the assignment to m_layerRenderer will still destroy the old LayerRendererChromium and then create a new one.  Did you mean to null-check m_layerRenderer before creating a new one?

[Vangelis Kokkevis]

(In reply to comment #2)
> (From update of attachment 67744 [details])
> Looks like a great optimization but I think the logic is off.

You are right!!

> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=67744&action=prettypatch
> 
> > WebKit/chromium/src/WebViewImpl.cpp:317
> > +    m_layerRenderer.clear();
> You don't have to do this. m_layerRenderer is an OwnPtr() and will delete what it points to when it is destroyed.

The problem I was trying to solve here is that the layerRenderer needs to be destroyed before the gles2Context which is also owned by the WebView as it needs to clean up after itself.  We could change the ownership of the context but it's not worth doing it now as kbr's upcoming change removed the GLES2Context class altogether.

> 
> > WebKit/chromium/src/WebViewImpl.cpp:2313
> >          m_layerRenderer = LayerRendererChromium::create(getOnscreenGLES2Context());
> >          if (m_layerRenderer) {
> >              m_isAcceleratedCompositingActive = true;
> > +            m_compositorCreationFailed = false;
> I don't quite understand - if someone calls setIsAcceleratedCompositing(false) then setIsAcceleratedCompositing(true), then on the second call the assignment to m_layerRenderer will still destroy the old LayerRendererChromium and then create a new one.  Did you mean to null-check m_layerRenderer before creating a new one?

Yikes, thanks for catching this one... You are right, a test for null is required. 

I'll hold off on this CL until kbr checks in his changes.

[Vangelis Kokkevis]

Created attachment 69075 [details]
patch fixing previous issues

[Kenneth Russell]

Comment on attachment 69075 [details]
patch fixing previous issues

Basically looks fine. WebKit style prefers early returns, so it might be better to restructure things to peel off cases one at a time and return. For example,

if (!active) {
    m_isAcceleratedCompositingActive = false;
    return;
}

if (m_layerRenderer) {
    m_isAcceleratedCompositingActive = true;
    return;
}

and so on.

[Vangelis Kokkevis]

Committed r68606: <http://trac.webkit.org/changeset/68606>