[webkit-reviews] review denied: [Bug 93010] [chromium] Add WebExternalTextureProvider; an alternate means for providing textures to a TextureLayerChromium : [Attachment 168708] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied alexst at chromium.org's request
for review:
Bug 93010: [chromium] Add WebExternalTextureProvider; an alternate means for
providing textures to a TextureLayerChromium
https://bugs.webkit.org/show_bug.cgi?id=93010

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

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


Needs a bit more polish.

> Source/Platform/chromium/public/WebExternalTextureLayer.h:83
> +    // TODO: make pure virtual once the chromium impl is changed and deps
roll

WebKit spells this FIXME, not TODO.

I would put this next to setTextureId() and document how this interacts with
setTextureId() and WebExternalTextureLayerClient.  Needing 3 different ways to
provide a texture is kind of unfortunate :/

Do we need a way to set this after the fact or could this be another ::create()
override? The advantage of having this be ::create() is that you don't have to
worry about the provider changing after creation

> Source/Platform/chromium/public/WebExternalTextureProvider.h:33
> +class WebGraphicsContext3D;

this appears to be unused

> Source/Platform/chromium/public/WebExternalTextureProvider.h:52
> +	   virtual void didUpdatePremultipliedAlpha(bool) = 0;
> +	   virtual void didUpdateFlipped(bool) = 0;
> +	   virtual void didUpdateUVRect(const WebFloatRect&) = 0;

Do these mean that the various state bits on the WebExternalTextureLayer are
not used?  Do you really need this level of granularity on properties for your
use case?  Please document

> Source/Platform/chromium/public/WebExternalTextureProvider.h:58
> +    // This is called on impl thread and happens during pushProperties

The "happens during pushProperties" part is not really useful to people reading
this header since it's not part of the public API.  Much more useful would be
knowing how often this is called (once) and what the lifetime of the parameter
is.

nit: end comments with a period

> Source/WebKit/chromium/src/WebPluginContainerImpl.h:106
> +    // This is mutually exclusive with setBackingTextureId and
setBackingIOSurfaceId

This is not the most useful place to document this since it's the
implementation, not the public API.

Comments are supposed to be sentences and thus end with a period.