Bug 87603 – [BlackBerry] WebOverlay API

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Arvid Nilsson]

This new API makes it possible to leverage the awesomeness of the BlackBerry accelerated compositing implementation to draw, transform and fluidly animate overlays in the embedding library or application.

PR #156812

[Arvid Nilsson]

Please note that this has not been internally reviewed yet, so it needs actual review. And also, it has a dependency that needs to be landed first.

[Arvid Nilsson]

Created attachment 144249 [details]
Patch

[Antonio Gomes]

Comment on attachment 144249 [details]
Patch

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

A few nitpicks to fix before land, but it looks good overall. Please pre-fill Review by Antonio Gomes and set cq? only.

> Source/WebKit/blackberry/Api/WebAnimation.cpp:2
> + * Copyright (C) 2010, 2011, 2012 Research In Motion Limited. All rights reserved.

2012 only?

> Source/WebKit/blackberry/Api/WebAnimation.cpp:37
> +WebAnimation WebAnimation::fadeAnimation(const char* name, float from, float to, double duration)

would a WebString for 'name' be better, since ::name returns a WebString?

> Source/WebKit/blackberry/Api/WebOverlay.cpp:193
> +    // Must be called on UI thread

should we ASSERT here so we know mis uses of this API?

> Source/WebKit/blackberry/Api/WebOverlay.cpp:219
> +    // Page might have changed if we were removed from the page and added to
> +    // some other page

nit: misses a . at the end.

> Source/WebKit/blackberry/Api/WebOverlay.cpp:399
> +WebOverlayLayerCompositingThreadClient::~WebOverlayLayerCompositingThreadClient()
> +{
> +}

do we this needs this?

> Source/WebKit/blackberry/Api/WebOverlay.cpp:685
> +#else // USE(ACCELERATED_COMPOSITING)
> +namespace BlackBerry {
> +namespace WebKit {
> +
> +WebOverlay::WebOverlay()
> +{
> +}
> +
> +WebOverlay::~WebOverlay()
> +{
> +}
> +
> +Platform::FloatPoint WebOverlay::position() const
> +{
> +    return Platform::FloatPoint();
> +}

not that it is wrong, but usually class definitions are unique, and #if #else #endif goes within the method bodies.

> Source/WebKit/blackberry/Api/WebOverlay.h:62
> +    // The position of the layer (the location of its top-left corner in its parent)

nit: dot at the end

> Source/WebKit/blackberry/Api/WebOverlay.h:75
> +    // Whether the layer is scaled together with the web page

ditto

> Source/WebKit/blackberry/Api/WebOverlay.h:101
> +    // Will result in a future call to WebOverlayClient::drawContents, if the layer draws custom contents

ditto

> Source/WebKit/blackberry/Api/WebOverlayOverride.cpp:127
> +#else // USE(ACCELERATED_COMPOSITING)
> +namespace BlackBerry {
> +namespace WebKit {
> +
> +WebOverlayOverride::WebOverlayOverride(WebOverlayPrivate*)
> +{
> +}
> +
> +WebOverlayOverride::~WebOverlayOverride()
> +{
> +}
> +
> +void WebOverlayOverride::setPosition(const Platform::FloatPoint&)
> +{
> +}
> +
> +void WebOverlayOverride::setAnchorPoint(const Platform::FloatPoint&)
> +{
> +}
> +
> +void WebOverlayOverride::setSize(const Platform::FloatSize&)
> +{
> +}
> +
> +void WebOverlayOverride::setTransform(const Platform::TransformationMatrix&)
> +{
> +}
> +
> +void WebOverlayOverride::setOpacity(float)
> +{
> +}
> +
> +void WebOverlayOverride::addAnimation(const WebAnimation&)
> +{
> +}
> +
> +void WebOverlayOverride::removeAnimation(const WebString&)
> +{
> +}
> +
> +}
> +}
> +#endif // USE(ACCELERATED_COMPOSITING)

same as I said previously.

> Source/WebKit/blackberry/Api/WebPage.h:343
> +    // Adds an overlay that can be modified on the WebKit thread, and
> +    // whose attributes can be overridden on the compositing thread

misses a dot

> Source/WebKit/blackberry/Api/WebPage.h:347
> +    // Adds an overlay that can only be modified on the compositing thread

misses a dot

[Arvid Nilsson]

Thanks for the review!

The only point I disagree with is this - because WebPageCompositor.cpp already uses this strategy. If another strategy is desired, I think it should be some kind of WebPageCompositorNone.cpp and WebOverlayNone.cpp that the build system can use instead, when GLES2 and EGL are disabled. But I would prefer to.do that in another patch.


> Source/WebKit/blackberry/Api/WebOverlay.cpp:685
> +#else // USE(ACCELERATED_COMPOSITING)
> +namespace BlackBerry {
> +namespace WebKit {
> +
> +WebOverlay::WebOverlay()
> +{
> +}
> +
> +WebOverlay::~WebOverlay()
> +{
> +}
> +
> +Platform::FloatPoint WebOverlay::position() const
> +{
> +    return Platform::FloatPoint();
> +}

not that it is wrong, but usually class definitions are unique, and #if #else #endif goes within the method bodies.

[Antonio Gomes]

(In reply to comment #4)
> Thanks for the review!
> 
> The only point I disagree with is this - because WebPageCompositor.cpp already uses this strategy. If another strategy is desired, I think it should be some kind of WebPageCompositorNone.cpp and WebOverlayNone.cpp that the build system can use instead, when GLES2 and EGL are disabled. But I would prefer to.do that in another patch.
> 

No strong preference.

would you like to get this patch landed as is, and we keep the bug opened to address the nitpicks in a follow up?

[Arvid Nilsson]

(In reply to comment #5)
> (In reply to comment #4)
> > Thanks for the review!
> > 
> > The only point I disagree with is this - because WebPageCompositor.cpp already uses this strategy. If another strategy is desired, I think it should be some kind of WebPageCompositorNone.cpp and WebOverlayNone.cpp that the build system can use instead, when GLES2 and EGL are disabled. But I would prefer to.do that in another patch.
> > 
> 
> No strong preference.
> 
> would you like to get this patch landed as is, and we keep the bug opened to address the nitpicks in a follow up?

I'm uploading a new patch now, adressing all review comments except the #ifdef thing - the WebPageCompositor.cpp uses this strategy, we should change them both in a separate patch.

[Arvid Nilsson]

Created attachment 144399 [details]
Patch

[Rob Buis]

Comment on attachment 144399 [details]
Patch

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

Looks good to me, maybe someone can cq+ it for you or you can decide to fix the Copyright thing first.

> Source/WebKit/blackberry/Api/WebOverlay.cpp:2
> + * Copyright (C) 2010, 2011, 2012 Research In Motion Limited. All rights reserved.

I am guessing this should be 2012 only...

[Arvid Nilsson]

Created attachment 144475 [details]
Patch

[Arvid Nilsson]

Comment on attachment 144475 [details]
Patch

Fixed year of copyright notice as Rob pointed out, should be good for CQ+ now.

[Rob Buis]

Comment on attachment 144475 [details]
Patch

Ok.

[WebKit Review Bot]

Comment on attachment 144475 [details]
Patch

Clearing flags on attachment: 144475

Committed r118750: <http://trac.webkit.org/changeset/118750>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.