[webkit-reviews] review denied: [Bug 77751] [BlackBerry] Upstream BlackBerry WebCoreSupport ClientExtension and GeolocationControllerClientBlackBerry classes : [Attachment 125369] Patch

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied Jacky Jiang <zkjiang008 at gmail.com>'s
request for review:
Bug 77751: [BlackBerry] Upstream BlackBerry WebCoreSupport ClientExtension and
GeolocationControllerClientBlackBerry classes
https://bugs.webkit.org/show_bug.cgi?id=77751

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

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=125369&action=review


Some things to check and improve still.

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


These all probably need 2012 Copyright if they are changed in 2012.

> Source/WebKit/blackberry/WebCoreSupport/ClientExtension.cpp:30
> +#include <wtf/OwnArrayPtr.h>

Is this needed?

> Source/WebKit/blackberry/WebCoreSupport/ClientExtension.cpp:35
> +using namespace std;

Are all of these needed?

> Source/WebKit/blackberry/WebCoreSupport/ClientExtension.cpp:55
> +	   string retVal = client->invokeClientJavaScriptCallback(strArgs,
argumentCount).utf8();

You should be able to combine the below if and retVal.

>
Source/WebKit/blackberry/WebCoreSupport/GeolocationControllerClientBlackBerry.c
pp:26
> +#include "GeolocationPosition.h"

Can this be removed?

>
Source/WebKit/blackberry/WebCoreSupport/GeolocationControllerClientBlackBerry.c
pp:27
> +#include "NotImplemented.h"

Can this be removed?

>
Source/WebKit/blackberry/WebCoreSupport/GeolocationControllerClientBlackBerry.h
:30
> +class GeolocationControllerClientBlackBerry : public
WebCore::GeolocationClient, public BlackBerry::Platform::GeoTrackerListener {

You don't need WebCore:: since it is in the WebCore namespace.

>
Source/WebKit/blackberry/WebCoreSupport/GeolocationControllerClientBlackBerry.h
:37
> +    virtual WebCore::GeolocationPosition* lastPosition();

Ditto.