[webkit-reviews] review denied: [Bug 111097] [Chromium] Use Texture Mailboxes to Transport WebGL Buffers : [Attachment 193157] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Mar 14 15:49:09 PDT 2013


James Robinson <jamesr at chromium.org> has denied alexst at chromium.org's request
for review:
Bug 111097: [Chromium] Use Texture Mailboxes to Transport WebGL Buffers
https://bugs.webkit.org/show_bug.cgi?id=111097

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

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


I don't think it's a great idea to delete the texture based path in the same
patch as you add the mailbox based path.  There's no way to stage this across
repos and it makes the patch a bit harder to follow.  Also if there end up
being bugs in either the WebKit or the chromium side we'll just have to revert
the whole thing.  Instead, can you add the new path while leaving the old one
intact until we can switch over?

> Source/Platform/chromium/public/WebCompositorSupport.h:80
> +    virtual WebExternalTextureLayer*
createExternalTextureLayer(WebExternalTextureLayerClient* = 0, bool mailbox =
false) { return 0; }

bool parameters in multi-parameter APIs like this are generally a bad idea
since it's really hard to tell at callsites what the value means.  For this
one, it's pretty much impossible to guess what "createExternalTextureLayer(...,
true)" means at the callsite without opening this header up too.  Please use a
two-state enum if you want this to be conditional.

> Source/Platform/chromium/public/WebExternalTextureLayerClient.h:44
> +class Mailbox {

The WebKit API follows a one-class-per-header rule (which WebTextureUpdater is
actually breaking above).  It's also pretty weird to have implementation logic
in a public header.  Also, WebKit::Mailbox is a very generic namespace to be
grabbing in a public header.  If you need this logic to be part of the public
WebKit API, please put this in its own header and figure out where the
implementation should go.  In general the WebKit API for something like this
should just be a transport mechanism so I'm not sure why we need logic here.

> Source/Platform/chromium/public/WebExternalTextureLayerClient.h:52
> +	   for (int i = 0; i < (int)arraysize(name); ++i) {

we don't use c-style casts in WebKit anywhere. Why not just make the loop
variable a size_t (or whatever arraysize() wants)

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:220
> +

no blank line here.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.h:65
> +    virtual bool prepareMailbox(WebKit::Mailbox*) OVERRIDE;
> +    virtual void mailboxReleased(const WebKit::Mailbox&) OVERRIDE;

Why not just inline these definitions?	They're trivial


More information about the webkit-reviews mailing list