[webkit-reviews] review denied: [Bug 93010] [chromium] Add WebExternalTextureProvider; an alternate means for providing textures to a TextureLayerChromium : [Attachment 168092] 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 168092: Patch
https://bugs.webkit.org/attachment.cgi?id=168092&action=review

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


We need some tests for this - take a look at
Tools/DumpRenderTree/chromium/TestWebPlugin.cpp for one way to approach tests
in layout tests.

> Source/Platform/chromium/public/WebExternalTextureLayer.h:84
> +    virtual void setTextureProvider(WebExternalTextureProvider*) { };

no ;

> Source/Platform/chromium/public/WebExternalTextureProvider.h:8
> + *	  * Redistributions of source code must retain the above copyright

2-clause is preferred for new files. See Source/WebCore/LICENSE-APPLE

> Source/Platform/chromium/public/WebExternalTextureProvider.h:52
> +	   // Notifies the provider's client that a call to getCurrentFrame()
will return new data.

I don't see any getCurrentFrame() - what is this comment referring to?

> Source/Platform/chromium/public/WebExternalTextureProvider.h:56
> +	   // Insert a synchronization point in the graphics command stream
> +	   virtual unsigned insertSyncPoint() = 0; 

What does this mean and what is the return value? The inserted sync point?

> Source/Platform/chromium/public/WebExternalTextureProvider.h:59
> +    virtual void setTextureProviderClient(Client*) = 0;

This could use some documentation - what are the threading requirements of this
call? How often can it be called?

> Source/Platform/chromium/public/WebExternalTextureProvider.h:64
> +    virtual bool premultipliedAlpha() const = 0;
> +    virtual bool flipped() const = 0;
> +    virtual WebFloatRect uvRect() const = 0;

Why are these all pull?  Is it expected that these could change from frame to
frame and that the user of this provider has to check their value every time it
presents?

> Source/WebKit/chromium/public/WebPluginContainer.h:75
> +    virtual void setBackingTextureProvider(WebExternalTextureProvider*) = 0;


Please separate this with blank lines from adjacent functions.	Also say
something about how this interacts with setBackingTexture() /
setBackingIOSurfaceId().  I believe they're all mutually exclusive but having
that explicit here would be easier for callers.

> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:401
> +    if (m_textureProvider == provider)

Do we really want to support setting different providers over the same plugin's
lifetime?  That seems a bit tricksy to manage on the impl side and not
something that we need for video layers currently