[webkit-reviews] review denied: [Bug 60817] [chromium] Composited rendering mode for plugins : [Attachment 93849] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Hin-Chung Lam
<hclam at google.com>'s request for review:
Bug 60817: [chromium] Composited rendering mode for plugins
https://bugs.webkit.org/show_bug.cgi?id=60817

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

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

Exposing the compositor context to the outside world will not fly, relatively
soon we'll be manipulating this context from another thread so there is no way
that we can expose it out through the WebKit API.  Is the issue that the plugin
needs some context in order to upload its resources into?  If that's all you
need, then either the plugin should get its own context or it should use a
utility context (like canvas does and content layers soon will), not the
compositor context.  I'd like to see how that stuff works on the chromium impl
side.  It needs to be the sole responsibility of the compositor to decide when
to update textures within the compositor context and latch on them, etc.

There's a good bit of cruft to clean up here as well.

>
Source/WebCore/platform/graphics/chromium/PluginLayerCompositorChromium.cpp:69
> +    if (layer < 0 || static_cast<size_t>(layer) >= m_layers.size())
> +	   return;

this is weird. we normally don't do error checking like this when the caller is
internal (not from javascript or anything like that). are there callers to this
function that you do not trust?

Vector::operator[] has an ASSERT() on the parameter that will trip in debug.

>
Source/WebCore/platform/graphics/chromium/PluginLayerCompositorChromium.cpp:76
> +    if (layer < 0 || static_cast<size_t>(layer) >= m_layers.size())
> +	   return;

also weird (although i think this function can just be deleted)

> Source/WebCore/platform/graphics/chromium/PluginLayerCompositorChromium.h:57
> +	   int zIndex;

this seems unused.  unless there's a good reason to have it, i'd suggest
getting rid of zIndex throughout this patch

>
Source/WebCore/platform/graphics/chromium/cc/CCPluginLayerCompositorImpl.cpp:37

> +#include <wtf/text/WTFString.h>

is this #include used?

>
Source/WebCore/platform/graphics/chromium/cc/CCPluginLayerCompositorImpl.cpp:72

> +void CCPluginLayerCompositorImpl::setLayers(const
Vector<PluginLayerCompositorChromium::Layer>& plugniLayers)

typo: plugni

>
Source/WebCore/platform/graphics/chromium/cc/CCPluginLayerCompositorImpl.cpp:80

> +void
CCPluginLayerCompositorImpl::setTextureProperties(PluginLayerCompositorChromium
::Type layerType, const Vector<VideoLayerChromium::Texture>& textures)

this still feels like the wrong signature.  do you plan to set properties on
YUV layers?  if not, then this should just take the texture ID and the caller
should be checking the layer type

>
Source/WebCore/platform/graphics/chromium/cc/CCPluginLayerCompositorImpl.cpp:10
4
> +    // FIXME: Use the bounds in layer.

this fixme doesn't make any sense - the "Layer" here does not have any bounds

>
Source/WebCore/platform/graphics/chromium/cc/CCPluginLayerCompositorImpl.cpp:11
0
> +    // FIXME: Use the bounds in layer.

this fixme is also nonsense

> Source/WebCore/platform/graphics/chromium/cc/CCPluginLayerCompositorImpl.h:47

> +    typedef ProgramBinding<VertexShaderPosTex,
FragmentShaderRGBATexFlipAlpha> Program;

this is unused, remove

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

wrong license header

> Source/WebKit/chromium/public/WebPluginLayerCompositor.h:56
> +	   WebSize size;
> +	   WebSize visibleSize;

these both seem unused - is the plan to use these in some capacity in the
future? i'd prefer you not add unused code

> Source/WebKit/chromium/public/WebPluginLayerCompositor.h:71
> +    // Make changes of the plugin content visiable.

typo: visible.	this name/comment seem misleading - calling this function has
no immediate effect, it just enqueues a composite to happen at some point in
the future.

> Source/WebKit/chromium/public/WebPluginLayerCompositor.h:79
> +    // Return the graphics context used by the compositor.
> +    virtual WebGraphicsContext3D* graphicsContext3D() const = 0;

woah....this is very dodgy.  we don't want to expose the compositor's context
out to the world in general!  what are you planning to do with this context? 
do you need the compositor's context in particular?