[webkit-reviews] review denied: [Bug 92685] [BlackBerry] Enable CSS Filter Effects : [Attachment 155548] Patch

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo at webkit.org> has denied Joshua Netterfield
<jnetterfield at rim.com>'s request for review:
Bug 92685: [BlackBerry] Enable CSS Filter Effects
https://bugs.webkit.org/show_bug.cgi?id=92685

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

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=155548&action=review


nice!

please fix the style nits and minor issues. r-'ing since it needs another
iteration to fix EFL anyways.

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.h:83
> +    virtual bool setFilters(const FilterOperations &);

the 'bool' return type here is very unclear. I imagine this class implements
this method which is inherited from someone else?

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:215
> +    bool filterOperationsChanged() { return m_filterOperationsChanged; }

const

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:218
> +    WTF::Vector<RefPtr<LayerFilterRendererAction> > filterActions() { return
m_filterActions; }

const

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:266
> +    bool m_filterOperationsChanged;

you should manually default initialize it in the ctor.

> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.cpp:70
> +    default:
> +	   return -1;
> +    }

does it cover all cases of the enum? if you ASSERT_NOT_REACHED here, please.

> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.cpp:89
> +Uniform1f::Uniform1f(int c_location, float c_val) : Uniformf(c_location),
m_val(c_val)

wrong style, new line here.

> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.cpp:138
> +{
> +
> +}

unneeded extra line.

> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.cpp:142
> +LayerFilterRendererAction::~LayerFilterRendererAction()
> +{
> +}

omit this. let the compiler private one to us.