Bug 64224 - Move updateLayers call to WebViewImpl::layout so that we do not trigger redundant draws
Summary: Move updateLayers call to WebViewImpl::layout so that we do not trigger redun...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Bates
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-08 18:42 PDT by John Bates
Modified: 2011-07-12 14:23 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.29 KB, patch)
2011-07-08 18:46 PDT, John Bates
no flags Details | Formatted Diff | Diff
Patch (1.76 KB, patch)
2011-07-11 18:13 PDT, John Bates
no flags Details | Formatted Diff | Diff
Patch (7.84 KB, patch)
2011-07-12 11:57 PDT, John Bates
no flags Details | Formatted Diff | Diff
Patch (7.93 KB, patch)
2011-07-12 12:59 PDT, John Bates
no flags Details | Formatted Diff | Diff
Patch (8.08 KB, patch)
2011-07-12 13:05 PDT, John Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Bates 2011-07-08 18:42:21 PDT
Add popPendingUpdate method to chromium WebWidgetClient.
Comment 1 John Bates 2011-07-08 18:46:46 PDT
Created attachment 100198 [details]
Patch
Comment 2 John Bates 2011-07-08 19:05:54 PDT
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.
Comment 3 James Robinson 2011-07-11 14:15:29 PDT
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.
Comment 4 John Bates 2011-07-11 18:13:54 PDT
Created attachment 100401 [details]
Patch
Comment 5 James Robinson 2011-07-11 18:16:04 PDT
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.
Comment 6 John Bates 2011-07-12 11:57:42 PDT
Created attachment 100536 [details]
Patch
Comment 7 James Robinson 2011-07-12 12:24:08 PDT
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())
Comment 8 John Bates 2011-07-12 12:59:00 PDT
Created attachment 100547 [details]
Patch
Comment 9 James Robinson 2011-07-12 13:00:56 PDT
Comment on attachment 100547 [details]
Patch

Cool.  One more nit to consider before landing:  Could you add a TRACE_EVENT() around syncCompositingLayers()?
Comment 10 John Bates 2011-07-12 13:05:08 PDT
Created attachment 100549 [details]
Patch
Comment 11 John Bates 2011-07-12 13:06:41 PDT
(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.
Comment 12 James Robinson 2011-07-12 13:39:42 PDT
Comment on attachment 100549 [details]
Patch

Flags away!
Comment 13 WebKit Review Bot 2011-07-12 14:22:57 PDT
Comment on attachment 100549 [details]
Patch

Clearing flags on attachment: 100549

Committed r90850: <http://trac.webkit.org/changeset/90850>
Comment 14 WebKit Review Bot 2011-07-12 14:23:02 PDT
All reviewed patches have been landed.  Closing bug.