Bug 92685 – [BlackBerry] Enable CSS Filter Effects

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Joshua Netterfield]

BlackBerry does not currently support hardware-accelerated CSS filter effects.

This bug covers neither CSS reference (SVG) filters, nor CSS custom (shader) filters.

[Joshua Netterfield]

Created attachment 155548 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 155548 [details]
Patch

Attachment 155548 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13386881

[Antonio Gomes]

Comment on attachment 155548 [details]
Patch

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.

[Joshua Netterfield]

Created attachment 155565 [details]
Patch

[Joshua Netterfield]

Please disregard the previous patch.

[Joshua Netterfield]

Created attachment 155567 [details]
Patch

[Joshua Netterfield]

Created attachment 155578 [details]
Patch

[Joshua Netterfield]

Got rid of WTF:: prefixes.

[Rob Buis]

Comment on attachment 155578 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155578&action=review

Looks good, one more iteration needed.

> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.cpp:371
> +    for (int i = 0;i < LayerData::NumberOfCSSFilterShaders; ++i)

Add a space after i = 0;

> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.h:98
> +        // see the ping-pong note in LayerFilterRenderer::applyActions

Not completely WebKit style comments.

> Source/WebCore/platform/graphics/blackberry/LayerWebKitThread.h:102
> +    void setFilters(const FilterOperations& filters) { m_filters = filters; m_filtersChanged = 1; setNeedsCommit(); }

Inconsistent use of m_filtersChanged, setting to 1 and false.

> ChangeLog:11
> +        * Source/cmakeconfig.h.cmake: Acknowldge CSS filter effects

Typo Acknowldge

[Arvid Nilsson]

Comment on attachment 155578 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155578&action=review

> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.cpp:76
> +    , m_refCount(1)

Now that Uniformf is RefCounted, you can remove m_refCount =)

[Joshua Netterfield]

Created attachment 155589 [details]
Patch

[Rob Buis]

Comment on attachment 155589 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155589&action=review

> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.h:100
> +    bool shouldPushSnapshot() { return m_pushSnapshot; }

Could be const.

> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.h:101
> +    void setPushSnapshot() { m_pushSnapshot = 1; }

Better use true.

> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.h:103
> +    bool shouldPopSnapshot() { return m_popSnapshot; }

Ditto.

> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.h:104
> +    void setPopSnapshot() { m_popSnapshot = 1; }

Etcetera.

[Joshua Netterfield]

Created attachment 155596 [details]
Patch

[Rob Buis]

Comment on attachment 155596 [details]
Patch

Good work!

[WebKit Review Bot]

Comment on attachment 155596 [details]
Patch

Clearing flags on attachment: 155596

Committed r124242: <http://trac.webkit.org/changeset/124242>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.