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

Mon Jun 20 20:08:31 PDT 2011

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 97198: Patch
https://bugs.webkit.org/attachment.cgi?id=97198&action=review

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

> Source/WebCore/ChangeLog:17
> +	   Actual rendering of these layers happen in CCPluginCompositingLayer,
it communicates with PluginComopsitingLayerChromium through a vector of
SublayerProperties.

typo: Comopsiting

> Source/WebCore/ChangeLog:20
> +	   No new tests since this patch is not fully functionally yet and
requires Chromium side changes.

This is bad

>
Source/WebCore/platform/graphics/chromium/PluginCompositingLayerChromium.cpp:62

> +    virtual bool updateContent(VideoFrameChromium* frame)
> +    {
> +	   m_client->onUpdateContentComplete(frame);
> +	   return true;
> +    }

what is the return value supposed to indicate here? is it useful?

>
Source/WebCore/platform/graphics/chromium/PluginCompositingLayerChromium.cpp:75

> +	   PluginCompositingLayerChromium::SublayerProperties prop;
> +	   prop.rect = rect();
> +	   prop.type = type();
> +	   prop.textures = textures();

seems like SubLayerProperties() should have a constructor to initialize its
fields. that'll be easier to maintain when properties are added

>
Source/WebCore/platform/graphics/chromium/PluginCompositingLayerChromium.cpp:81

> +    Vector<VideoLayerChromium::Texture> m_textures;

the fact that this has a Vector<> of VideoLayerChromium::Textures indicates
that there's a design problem

>
Source/WebCore/platform/graphics/chromium/PluginCompositingLayerChromium.cpp:13
3
> +    // FIXME: Create a specialized implementation based on type.

What does this mean?

>
Source/WebCore/platform/graphics/chromium/PluginCompositingLayerChromium.cpp:13
4
> +    m_sublayers.append(adoptPtr(new SublayerImpl(type, client)));

nit: if SublayerImpl is always stored in an OwnPtr<>, then slightly better is
having SublayerImpl expose a create() function that returns a PassOwnPtr<>
instead of having to call adoptPtr() yourself

>
Source/WebCore/platform/graphics/chromium/PluginCompositingLayerChromium.cpp:14
1
> +void PluginCompositingLayerChromium::commitContent()
> +{
> +    // FIXME: Implement.
> +    notImplemented();

what is this supposed to do?

> Source/WebCore/platform/graphics/chromium/PluginCompositingLayerChromium.h:47

> +    public:
> +	   virtual ~Client();
> +	   virtual void onCommitContentComplete() = 0;

since this class has a trivial d'tor (no members, no superclass) just put the
body of the d'tor here. is it valid to destroy a
PluginCompositingLayerChromium::Client via a
PluginCompositingLayerChromium::Client* or should the d'tor be protected?

> Source/WebCore/platform/graphics/chromium/PluginCompositingLayerChromium.h:60

> +    struct SublayerProperties {
> +	   IntRect rect;
> +	   PluginCompositingLayerChromium::SublayerType type;
> +	   Vector<VideoLayerChromium::Texture> textures;
> +    };

why are these not just members of PluginCompositingLayerChromium::Sublayer?

> Source/WebCore/platform/graphics/chromium/PluginCompositingLayerChromium.h:76

> +    class Sublayer {
> +    public:
> +	   virtual ~Sublayer();
> +
> +	   virtual void initialize() = 0;
> +	   virtual bool updateContent(VideoFrameChromium*) = 0;
> +
> +	   virtual void setRect(const IntRect&) = 0;
> +	   virtual const IntRect& rect() const = 0;
> +
> +	   virtual SublayerType type() const = 0;
> +
> +	   virtual const Vector<VideoLayerChromium::Texture>& textures() const
= 0;
> +	   virtual SublayerProperties properties() const = 0;
> +    };

There's only one implementation of PluginCompositingLayerChromium::SubLayer, so
these should all be non-virtual.  Virtual doesn't make any sense, pure virtual
even less so.

> Source/WebCore/platform/graphics/chromium/PluginCompositingLayerChromium.h:80

> +    public:
> +	   virtual ~SublayerClient();

same comments apply here that applied to Client

>
Source/WebCore/platform/graphics/chromium/PluginCompositingLayerChromium.h:100
> +    PluginCompositingLayerChromium(Client*);

need explicit for this c'tor

>
Source/WebCore/platform/graphics/chromium/cc/CCPluginCompositingLayerImpl.cpp:6
1
> +	   default:
> +	       break;

leave out the default: clause here so the compiler can warn us if the switch is
missing a valid enum value

>
Source/WebCore/platform/graphics/chromium/cc/CCPluginCompositingLayerImpl.cpp:8
0
> +    CCVideoLayerImpl::drawRGBA(layerRenderer(), drawTransform(), bounds(),
opacity(), properties.textures[0]);

it's really weird that this calls into functions declared on CCVideoLayerImpl.

>
Source/WebCore/platform/graphics/chromium/cc/CCPluginCompositingLayerImpl.h:53
> +    explicit CCPluginCompositingLayerImpl(LayerChromium*, int id);

nit: no explicit for 2-arg c'tors

>
Source/WebCore/platform/graphics/chromium/cc/CCPluginCompositingLayerImpl.h:58
> +    Vector<PluginCompositingLayerChromium::SublayerProperties>
m_sublayerProperties;

we don't share property bag values for LayerChromium/CCLayerImpl types.

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.h:67
> +    static void drawYUV(LayerRendererChromium*, const TransformationMatrix&,
const IntSize&, float opacity,
> +			   const float* colorConversion, const float*
colorAdjust,
> +			   const VideoLayerChromium::Texture& yTexture,
> +			   const VideoLayerChromium::Texture& uTexture,
> +			   const VideoLayerChromium::Texture& vTexture);
> +    static void drawRGBA(LayerRendererChromium*, const
TransformationMatrix&, const IntSize&, float opacity,
> +			    const VideoLayerChromium::Texture&);
> +    static const float* getYUV2RGB() { return yuv2RGB; }
> +    static const float* getYUVAdjust() { return yuvAdjust; }

please post this refactoring as a separate patch

> Source/WebKit/chromium/public/WebPluginContainer.h:74
> +    // Returns a WebPluginLayerCompositor object if successful.
> +    // Returns 0 if a backing texture has already been assigned to this
> +    // container.
> +    virtual WebPluginLayerCompositor*
enterCompositedMode(WebPluginLayerCompositorClient*) { return 0; }

the comment doesn't match what the default impl appears to do. should this be
pure virtual if we can't implement it usefully in this header?

who calls this function and when is it called?

> Source/WebKit/chromium/public/WebPluginContainer.h:81
> +    // After entering composited mode and plugin layers are added this
method
> +    // is needed to make the new structure to take effect.

This is not quite English. Can you rewrite this in an active voice and try to
explain what this function does a little better?

> Source/WebKit/chromium/public/WebPluginLayer.h:40
> +public:
> +    virtual ~WebPluginLayerClient() { }

does/should anyone destroy a WebPluginLayerClient via a WebPluginLayerClient*
(as opposed to a more specific type)?  typically the answer is no for client
interfaces and the d'tor should be protected

> Source/WebKit/chromium/public/WebPluginLayer.h:45
> +class WebPluginLayer {

This doesn't seem like a WebPluginLayer as it doesn't map 1:1 with a WebPlugin.
 See comments below.

> Source/WebKit/chromium/public/WebPluginLayerCompositor.h:41
> +class WebPluginLayerCompositor {

I think this class structure is confusing.  There's no compositor here, just
one layer that happens to have some internal structure.  There should be one
WebPluginLayer per plugin and its internal structure should be composed of some
appropriately-named sublayer type.

> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:333
> +    if (m_platformLayer->textureId())

When is this function called? Are you sure m_platformLayer will never be NULL
when this function is called?

> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:349
> +    // TODO: Clear the tree structure of the LayerChromium.

in WebKit this is a FIXME

> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:484
> +void WebPluginContainerImpl::commitLayerStructure()
> +{
> +    m_element->setNeedsStyleRecalc(WebCore::SyntheticStyleChange);
> +}

the name of this function and its implementation do not agree.	what is
supposed to happen when this function is called and how does style
recalculation factor in? are you trying to change the LayerChromium hierarchy?

> Source/WebKit/chromium/src/WebPluginLayerImpl.cpp:33
> +// static

We typically don't do these sorts of comments in WebKit.  It's pretty obvious
that ::create() functions are static

> Source/WebKit/chromium/src/WebPluginLayerImpl.h:73
> +    WebCore::PluginCompositingLayerChromium::Sublayer* m_layer;

what's the ownership and lifetime relationship between a WebPluginLayerImpl and
its associated WebCore::PluginCompositingLayerChromium::Sublayer?  The raw
pointer is suspicious.	Should Sublayer be ref counted, for example?