Summary: | [BlackBerry] WebOverlay API | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Arvid Nilsson <anilsson> | ||||||||
Component: | WebKit BlackBerry | Assignee: | Arvid Nilsson <anilsson> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | jpetsovits, kpiascik, manyoso, mifenton, mlattanzio, rakuco, rwlbuis, staikos, tonikitoo, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 87602 | ||||||||||
Bug Blocks: | 87604 | ||||||||||
Attachments: |
|
Description
Arvid Nilsson
2012-05-27 15:07:18 PDT
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. Created attachment 144249 [details]
Patch
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 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.
(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? (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. Created attachment 144399 [details]
Patch
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... Created attachment 144475 [details]
Patch
Comment on attachment 144475 [details]
Patch
Fixed year of copyright notice as Rob pointed out, should be good for CQ+ now.
Comment on attachment 144475 [details]
Patch
Ok.
Comment on attachment 144475 [details] Patch Clearing flags on attachment: 144475 Committed r118750: <http://trac.webkit.org/changeset/118750> All reviewed patches have been landed. Closing bug. |