Bug 77751 – [BlackBerry] Upstream BlackBerry WebCoreSupport ClientExtension and GeolocationControllerClientBlackBerry classes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jacky Jiang]

Upstream BlackBerry WebCoreSupport ClientExtension and GeolocationControllerClientBlackBerry classes.

[Jacky Jiang]

Created attachment 125369 [details]
Patch

Note: this patch includes the self review diff with the local repo, which is in the local repo webkit/b3011d25da9ab740bac3eed65c94021b14576499.

[Rob Buis]

Comment on attachment 125369 [details]
Patch

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.cpp:26
> +#include "GeolocationPosition.h"

Can this be removed?

> Source/WebKit/blackberry/WebCoreSupport/GeolocationControllerClientBlackBerry.cpp: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.

[Jacky Jiang]

Created attachment 125393 [details]
Patch

Updated the patch according to Rob's review except combining retVal and removing some of the using namespace in ClientExtension.cpp.

[Rob Buis]

Comment on attachment 125393 [details]
Patch

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

This is almost perfect, just needs the early return style.

> Source/WebKit/blackberry/WebCoreSupport/ClientExtension.cpp:39
> +    JSValueRef jsRetVal = JSValueMakeUndefined(ctx);

This code should look better with early return if argumentCount <= 0, which is webkit style, can you do that?

[Jacky Jiang]

Comment on attachment 125393 [details]
Patch

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

>> Source/WebKit/blackberry/WebCoreSupport/ClientExtension.cpp:39
>> +    JSValueRef jsRetVal = JSValueMakeUndefined(ctx);
> 
> This code should look better with early return if argumentCount <= 0, which is webkit style, can you do that?

Ok, I will do it, thanks!

[Jacky Jiang]

Created attachment 125427 [details]
Patch

Update the patch according to the early return suggestion.

[Rob Buis]

Comment on attachment 125427 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 125427 [details]
Patch

Clearing flags on attachment: 125427

Committed r106731: <http://trac.webkit.org/changeset/106731>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.