[webkit-reviews] review granted: [Bug 87603] [BlackBerry] WebOverlay API : [Attachment 144249] Patch

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo at webkit.org> has granted Arvid Nilsson
<anilsson at rim.com>'s request for review:
Bug 87603: [BlackBerry] WebOverlay API
https://bugs.webkit.org/show_bug.cgi?id=87603

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

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
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::~WebOverlayLayerCompositingThreadClien
t()
> +{
> +}

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