[webkit-reviews] review denied: [Bug 59831] [chromium] WebView support for async compositing : [Attachment 91987] Typo.

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Nat Duca <nduca at chromium.org>'s
request for review:
Bug 59831: [chromium] WebView support for async compositing
https://bugs.webkit.org/show_bug.cgi?id=59831

Attachment 91987: Typo.
https://bugs.webkit.org/attachment.cgi?id=91987&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=91987&action=review

I'm not sure we actually need all this.  It seems that the callback exists just
to call a function on WebViewClient, which in Chrome is the RenderView - but we
can (and do) have a pointer to the RenderView in
WebGraphicsContext3DCommandBufferImpl::OnSwapBuffers().  What's the motivation
for plumbing these calls through the WebViewImpl?  Do you plan to do additional
work when these callbacks are invoked?

> Source/WebKit/chromium/ChangeLog:7
> +

there should be some description here of what is going on

> Source/WebKit/chromium/public/WebViewClient.h:325
> +    // Informs the browser that a compositing call completed.
> +    virtual void didComposite() { }
> +
> +    // Informs the browser that compositing failed, usually due to lost
context.
> +    virtual void compositeAborted() { }

Compositor WebGraphicsContext3DCommandBufferImpls can already get a pointer to
the associated RenderView directly.  Do we need to add new API for this or can
it just make calls through that pointer directly to the RenderView (which is a
subclass of RenderWidget)

> Source/WebKit/chromium/src/WebViewImpl.cpp:2587
> +   
GraphicsContext3DInternal::extractWebGraphicsContext3D(context)->setSwapBuffers
CompleteCallbackCHROMIUM(0);

We need to null this pointer out when the WebViewImpl is destroyed and when the
context is reset as well since WebViewImpl does not have exclusive ownership of
the compositor GraphicsContext3D, it only has a reference to it.  Overall this
feels a little fragile - it seems really easy to have a
WebGraphicsContext3DCommandBufferImpl that has a pointer to a stale
swapbuffers_complete_callback_.