[webkit-reviews] review granted: [Bug 92890] [chromium] Expose CCGraphicsContext as WebCompositorOutputSurface : [Attachment 157140] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Nat Duca
<nduca at chromium.org>'s request for review:
Bug 92890: [chromium] Expose CCGraphicsContext as WebCompositorOutputSurface
https://bugs.webkit.org/show_bug.cgi?id=92890

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

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


R=me - needs some more polish in bits but is in good shape.

I'm curious what your longer-term plans are w.r.t. which type the compositor
implementation will depend on (left some comments inline).

> Source/Platform/chromium/public/WebLayerTreeViewClient.h:65
> +    virtual void didRebindGraphicsContext(bool success) { return; }

nit: don't really need the return and i'd omit the parameter name on "success"
since it's unused

> Source/Platform/chromium/public/WebLayerTreeViewClient.h:73
> +    // Signals a successful rebinding of the output surface (e.g. after a
lost
> +    // 3D context event).
> +    virtual void didRebindOutputSurface(bool success) = 0;

I realize this comment is just copy-paste, but reading it is seems wrong - this
signals a rebind, but not necessarily a successful one (hence the parameter).
Could maybe use a bit of wordsmithing

> Source/WebCore/platform/graphics/chromium/cc/CCGraphicsContext.h:37
> +// FIXME: rename fully to CCOutputSurface.

This comment seems to contradict the ChangeLog a bit - is the end state that
the compositor all depends on WebCompositorOutputSurface (or some other exposed
type) or that it depends on CCOutputSurface? Or perhaps does CCOutputSurface
become the exposed type eventually?

> Source/WebCore/platform/graphics/chromium/cc/CCProxy.h:37
> +#include <public/WebCompositorOutputSurface.h>
>  #include <wtf/Noncopyable.h>
>  #include <wtf/PassOwnPtr.h>
>  #include <wtf/PassRefPtr.h>
>  #include <wtf/Threading.h>
>  
> +namespace WebKit {
> +class WebCompositorOutputSurface;
> +}

Do you need the include or the forward decl at all? I can't tell why this class
was forward declaring CCGraphicsContext before

> Source/WebKit/chromium/src/WebViewImpl.cpp:3696
> +// Adapts a pure WebGraphicsContext3D into a WebCompositorOutputSurface
until
> +// downstream code can be updated to produce output surfaces directly.
> +class WebGraphicsContextToOutputSurfaceAdapter : public
WebCompositorOutputSurface {

I think if all you did in WebViewImpl was patch createOutputSurface() to defer
out to m_client->createOutputSurface() then the following would happen on
compositor startup:

WebLayerTreeView::initialize() will call into CC which will eventually call
back out to WebLayerTreeView::createOutputSurface()
WLTV::createOutputSurface() will call WebLayerTreeView::createOutputSurface()
  WLTVC is WebViewImpl, so its default implementation of createOutputSurface()
will return 0
WebLayerTreeView::createOutputSurface() will go down the fallback path and call
WebLayerTreeViewClient::createContext3D()
  WLTVC is WebViewImpl, so it'll return a valid context from
WebViewClient::createContext3D()
WebLayerTreeView::createOutputSurface() will create its adapter around that
context and away we go.

Once you've implemented WebLayerTreeViewClient::createOutputSurface() for aura
on the chromium side, the WLTV adapter won't be used any more for aura context.

Once you've implemented WebViewClient::createOutputSurface() for renderer
compositor instances, the WLTV adapter won't be used for those contexts

once both are done and rolled in the WLTV adapter can go away.

So basically this is a really long-winded way of saying I don't think you need
this second adapter :)

> Source/WebKit/chromium/src/WebViewImpl.cpp:-3727
> -    // If we've already created an onscreen context for this view, return
that.
> -    if (m_temporaryOnscreenGraphicsContext3D)
> -	   webContext = m_temporaryOnscreenGraphicsContext3D.release();

Glad to see this temporary context code die at last

> Source/WebKit/chromium/tests/FakeWebCompositorOutputSurface.h:36
> +    static inline PassOwnPtr<FakeWebCompositorOutputSurface>
create(PassOwnPtr<WebGraphicsContext3D> context3D)

does the "inline" here mean anything?

> Source/WebKit/chromium/tests/FakeWebCompositorOutputSurface.h:61
> +    virtual void sendFrameToParentCompositor(const WebCompositorFrame&
frame) OVERRIDE

omit parameter name

> Source/WebKit/chromium/tests/FakeWebCompositorOutputSurface.h:66
> +    FakeWebCompositorOutputSurface(PassOwnPtr<WebGraphicsContext3D>
context3D)

explicit

> Source/WebKit/chromium/tests/WebLayerTreeViewTest.cpp:53
> +    virtual WebGraphicsContext3D* createContext3D() OVERRIDE {
ASSERT_NOT_REACHED(); return 0; }
> +    virtual void didRebindGraphicsContext(bool success) OVERRIDE {
ASSERT_NOT_REACHED(); }

these already have default impls that do what you want, can you just delete
these overrides? one less thing to worry about when you remove the interface

> Source/WebKit/chromium/tests/WebLayerTreeViewTest.cpp:59
> +    virtual void didRebindOutputSurface(bool success) OVERRIDE { }

omit parameter name