Bug 72041 – [chromium] Pipe compositor begin/end frame up to WebWidgetClient

• Component: New Bugs
• Status: RESOLVED FIXED

[Adrienne Walker]

[chromium] Pipe compositor begin/end frame up to WebWidgetClient

[Adrienne Walker]

Created attachment 114541 [details]
Patch

[Adrienne Walker]

This corresponds to a Chromium change here: http://codereview.chromium.org/8528006

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[James Robinson]

There seem to be merge conflicts here

[Adrienne Walker]

Created attachment 114546 [details]
Rebaselined

[James Robinson]

Comment on attachment 114546 [details]
Rebaselined

Looks good, need Darin to approve the WebWidgetClient changes

[Darin Fisher (:fishd, Google)]

Comment on attachment 114546 [details]
Rebaselined

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

> Source/WebKit/chromium/public/WebWidgetClient.h:63
> +    virtual void didBeginFrame() { }

the term "Frame" is a bit overloaded.  see WebFrame.  WebView extends from WebWidget.  WebView has a tree of frames.  See how this can get confusing?  Maybe we can try to stick to using terms involving variations of the word "compositor"?

[Nat Duca]

Comment on attachment 114546 [details]
Rebaselined

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

(In reply to comment #7)
> the term "Frame" is a bit overloaded.  see WebFrame.  WebView extends from WebWidget.  WebView has a tree of frames.  See how this can get confusing?  Maybe we can try to stick to using terms involving variations of the word "compositor"?

Can we do CompositorFrame? We're gonna kill ourselves if we try to not use the word frame, since that's the word we use in graphics... long term, maybe what we should do is remove all this stuff from the WebWidget API and instead have WebWidget have a WebLayerTreeView* accessor that gives access to WebViewImpl::m_layerTreeHost. That'd avoid this better...

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

Struggling with the renaming to didFinishFrame here (sorry to be 'that guy'!). We have a finishAllRendering and this definitely isn't related to that. Also it should be clear what was done --- its not that the GPU has finished the work [e.g. swap-ack driven], just that its been submitted. That's why it was so pedantically named as it was before.

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:307
> +        m_layerTreeHost->didFinishFrame();

Ooh, does this mean that we can stop the WebGraphicsContext from calling didSwap/etc on the RenderView? That's causing me some headaches right now..

> Source/WebKit/chromium/src/WebViewImpl.cpp:2783
> +    m_client->didFinishFrame();

Do we need a null-check? I forget...

[Adrienne Walker]

Created attachment 114769 [details]
Rename functions, pipe swapbuffers properly

[Nat Duca]

Comment on attachment 114769 [details]
Rename functions, pipe swapbuffers properly

Awesome. Jamesr or darin for official ceremony.

[James Robinson]

Comment on attachment 114769 [details]
Rename functions, pipe swapbuffers properly

R=me on everything but WebWidgetClient.h, please wait for Darin on that.

[WebKit Review Bot]

Comment on attachment 114769 [details]
Rename functions, pipe swapbuffers properly

Attachment 114769 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10460592

[Adrienne Walker]

Created attachment 114840 [details]
Now with compiling tests

[Darin Fisher (:fishd, Google)]

Comment on attachment 114840 [details]
Now with compiling tests

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

> Source/WebKit/chromium/public/WebWidgetClient.h:63
> +    virtual void didCommitAndDrawCompositorFrame() { }

nit: please add some documentation explaining what these events are telling the embedder.

> Source/WebKit/chromium/public/WebWidgetClient.h:64
> +    virtual void didCompleteSwapbuffers() { }

nit: should it be SwapBuffers... with a capital B?

[Adrienne Walker]

Created attachment 114999 [details]
Address nits

[Darin Fisher (:fishd, Google)]

Comment on attachment 114999 [details]
Address nits

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

> Source/WebKit/chromium/public/WebWidgetClient.h:66
> +    // Called for compositing mode when swapbuffers has been posted in the GPU

nit: insert new line above the start of a comment block.

[James Robinson]

Comment on attachment 114999 [details]
Address nits

R=me

[Adrienne Walker]

Committed r100199: <http://trac.webkit.org/changeset/100199>