[webkit-reviews] review denied: [Bug 107776] [chromium] Provide compositor offscreen context through the WebLayerTreeViewClient interface : [Attachment 186516] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Dana Jansens
<danakj at chromium.org>'s request for review:
Bug 107776: [chromium] Provide compositor offscreen context through the
WebLayerTreeViewClient interface
https://bugs.webkit.org/show_bug.cgi?id=107776

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

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


> Source/Platform/chromium/public/WebGraphicsContext3D.h:475
> +    WEBKIT_EXPORT GrGLInterface* createGrGLInterface();

this is weird to me. WebGraphicsContext3D is mostly a pure virtual interface
that is implemented by the embedder and provided to WebKit, not the other way
around.  For WEBKIT_EXPORT to make sense the definition of this function would
have to exist inside WebKit.dll and the caller to this function would have to
be outside.  Is that the structure you're trying to end up with?

> Source/Platform/chromium/public/WebLayerTreeViewClient.h:76
> +    virtual WebKit::WebGraphicsContext3D*
createOrGetOffscreenContext3dForMainThread() { return 0; }

Normally "create..." functions return pointers to objects that are then owned
by the caller.	That isn't the case for these, since  it's owned by the other
side of the interface.	I think they should be named something other than
'create...' and you should document the ownership rules in the header.

It's also a bit weird to me that these are on WebLayerTreeViewClient since they
don't have anything to do with the specific WebLayerTreeView instance