[webkit-reviews] review denied: [Bug 75732] [chromium] Add WebSolidColorLayer interface to draw non-textured color layers from Aura. : [Attachment 123145] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied W. James MacLean
<wjmaclean at chromium.org>'s request for review:
Bug 75732: [chromium] Add WebSolidColorLayer interface to draw non-textured
color layers from Aura.
https://bugs.webkit.org/show_bug.cgi?id=75732

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

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


> Source/WebCore/platform/graphics/chromium/SolidColorLayerChromium.cpp:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

copyright headers throughout this patch are inconsistent. for all new files
they should say 2012 and be 2-clause, not 3-clause

> Source/WebCore/platform/graphics/chromium/SolidColorLayerChromium.h:44
> +class SolidColorLayerChromium : public LayerChromium {

it's strange that there's no color parameter anywhere in this class. can you at
least document how you specify the color (guessing it's by setBackgroundColor()
on the base class)?

> Source/WebCore/platform/graphics/chromium/SolidColorLayerChromium.h:47
> +    static PassRefPtr<SolidColorLayerChromium> create(CCLayerDelegate*);

this class doesn't need a delegate

> Source/WebKit/chromium/public/platform/WebSolidColorLayer.h:40
> +// This class represents a layer that renders a solid color that is
generated
> +// externally (not managed by the WebLayerTreeView).
> +// When in single-thread mode, this means during
WebLayerTreeView::composite().
> +// When using the threaded compositor, this can mean at an arbitrary time
until
> +// the WebLayerTreeView is destroyed.

whaaaat? why is the lifecycle different for this layer type?

> Source/WebKit/chromium/public/platform/WebSolidColorLayer.h:52
> +    WebSolidColorLayer() { } 
> +    WebSolidColorLayer(const WebSolidColorLayer& layer) : WebLayer(layer) {
}
> +    virtual ~WebSolidColorLayer() { } 
> +    WebSolidColorLayer& operator=(const WebSolidColorLayer& layer)
> +    {
> +	   WebLayer::assign(layer);
> +	   return *this;
> +    }

do you need all these? why not just the create()?

> Source/WebKit/chromium/public/platform/WebSolidColorLayer.h:54
> +    WEBKIT_EXPORT void setBackgroundColor(const WebColor&);

do we need to be able to change the color after the layer is constructed, or
can it be a construction-time invariant?

> Source/WebKit/chromium/public/platform/WebSolidColorLayer.h:60
> +#if WEBKIT_IMPLEMENTATION
> +    WebSolidColorLayer(const WTF::PassRefPtr<WebSolidColorLayerImpl>&);
> +    WebSolidColorLayer& operator=(const
WTF::PassRefPtr<WebSolidColorLayerImpl>&);
> +    operator WTF::PassRefPtr<WebSolidColorLayerImpl>() const;
> +#endif

do you need these?

> Source/WebKit/chromium/src/WebSolidColorLayerImpl.cpp:51
> +void WebSolidColorLayerImpl::paintContents(GraphicsContext&, const IntRect&)

> +{
> +}

you don't need this

> Source/WebKit/chromium/src/WebSolidColorLayerImpl.h:34
> +class WebSolidColorLayerImpl : public WebCore::SolidColorLayerChromium,
public WebCore::CCLayerDelegate {

no need to be a delegate