Bug 64224 – Move updateLayers call to WebViewImpl::layout so that we do not trigger redundant draws

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

[John Bates]

Add popPendingUpdate method to chromium WebWidgetClient.

[John Bates]

Created attachment 100198 [details]
Patch

[John Bates]

Associated with chromium patch:
http://codereview.chromium.org/7327030/

Fixed bug that caused canvas updates to trigger two compositor paints. LayerRendererChromium::updateLayers was triggering a scheduleComposite because the paint aggregator in RenderWidget was emptied beforehand. This change (in combination with the chromium-side patch) delays the paint aggregator emptying until after updateLayers and before paint. 

This patch must land before the chromium patch above.

[James Robinson]

popPendingUpdate() is not a very enlightening API name.  I've looked at the chromium side and I'm still not completely sure what it is intended to mean.

[John Bates]

Created attachment 100401 [details]
Patch

[James Robinson]

Comment on attachment 100401 [details]
Patch

Wait, it's not valid to call LayerRendererChromium::updateLayers() at this point.  You want to just call WebViewImpl's updateLayers(), which does a different thing, and remove that call from LayerRendererChromium.  Leave doComposite() alone.

Also, this bug needs to be retitled.

[John Bates]

Created attachment 100536 [details]
Patch

[James Robinson]

Comment on attachment 100536 [details]
Patch

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

Closer!  Please also rename this bug, preferably to describe the problem this patch fixes (double frames on canvas).

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:97
> -void CCLayerTreeHost::updateLayers()
> +void CCLayerTreeHost::syncCompositingLayers()
>  {
> -    m_client->updateLayers();
> +    m_client->syncCompositingLayers();
>  }

delete

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:44
> +    virtual void syncCompositingLayers() = 0;

Just delete this (and the corresponding entry in CCLayerTreeHost), there aren't any callers of this any more.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:65
> +    void syncCompositingLayers();

delete

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:153
>      m_commitPending = false;
>      {
> -        TRACE_EVENT("CCLayerTreeHost::updateLayers", this, 0);
> -        m_layerTreeHost->updateLayers();
> +        TRACE_EVENT("CCLayerTreeHost::syncCompositingLayers", this, 0);
> +        m_layerTreeHost->syncCompositingLayers();
>      }

You can delete this section completely, animateAndLayout() takes care of this now (since it calls WebViewImpl::layout())

[John Bates]

Created attachment 100547 [details]
Patch

[James Robinson]

Comment on attachment 100547 [details]
Patch

Cool.  One more nit to consider before landing:  Could you add a TRACE_EVENT() around syncCompositingLayers()?

[John Bates]

Created attachment 100549 [details]
Patch

[John Bates]

(In reply to comment #9)
> (From update of attachment 100547 [details])
> Cool.  One more nit to consider before landing:  Could you add a TRACE_EVENT() around syncCompositingLayers()?

Done.

[James Robinson]

Comment on attachment 100549 [details]
Patch

Flags away!

[WebKit Review Bot]

Comment on attachment 100549 [details]
Patch

Clearing flags on attachment: 100549

Committed r90850: <http://trac.webkit.org/changeset/90850>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.