[webkit-reviews] review denied: [Bug 60817] [chromium] Composited rendering mode for plugins : [Attachment 93529] 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 93529: Patch
https://bugs.webkit.org/attachment.cgi?id=93529&action=review

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

What sort of compositing will happen within the plugin layer? Currently it
seems that it's just a set of textures with no actual properties on them.  Will
it get more complicated in the future?

Please fix the many style problems.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:350
> +	   renderSurfaceLayer->drawsContent();

this seems to call a function returning a bool and ignore the result.  is this
a mismerge?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:358
> -	   if (!renderSurface->m_layerList.size())
> +	   if (!renderSurface->m_layerList.size()) {
>	       continue;
> +	   }

nack, one-line conditionals shouldn't have braces

> Source/WebCore/platform/graphics/chromium/PluginLayerCompositorChromium.h:73
> +    explicit PluginLayerCompositorChromium();

you don't need explicit for a no-arg c'tor

please declare the (virtual) destructor and define it in the .cpp. otherwise
the very large Vector d'tor will be inlined into the header

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

> +    nonCopyingSort(m_layers.begin(), m_layers.end(), zIndexCompare);

I don't fully understand this step - the plugin provides a list of textures in
order along with the z indices.  Why doesn't it just provide the textures in
the order it wants to render them in?

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

> +void
CCPluginLayerCompositorImpl::setTextureProperties(PluginLayerCompositorChromium
::Type type, const Vector<unsigned>& textures)

this takes a Vector<> but only sets properties on the first member of the
vector.  Should it just take an unsigned as the second parameter?

>
Source/WebCore/platform/graphics/chromium/cc/CCPluginLayerCompositorImpl.cpp:10
9
> +void CCPluginLayerCompositorImpl::drawLayer3D(const
PluginLayerCompositorChromium::Layer& layer)

what does "3D" mean here?

>
Source/WebCore/platform/graphics/chromium/cc/CCPluginLayerCompositorImpl.cpp:12
9
> +    // FIXME: Share this rendering code with CCVideoLayerImpl.

can you just do this now instead of leaving it as a FIXME?

> Source/WebKit/chromium/public/WebPluginLayerCompositor.h:57
> +    virtual void setBackingTextures(int layer, const std::vector<unsigned>&
textures) = 0;

we don't do this in the WebKit API - take a look at WebVector

> Source/WebKit/chromium/src/WebPluginLayerCompositorImpl.cpp:2
> + * Copyright (C) 2009 Google Inc. All rights reserved.

2009? 2011

> Source/WebKit/chromium/src/WebPluginLayerCompositorImpl.cpp:16
> + *	  * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *	  * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *	  * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.

this is the wrong copyright header.  PluginLayerCompositorChromium.cpp has the
correct one

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

wrong header