Bug 77751

Summary: [BlackBerry] Upstream BlackBerry WebCoreSupport ClientExtension and GeolocationControllerClientBlackBerry classes
Product: WebKit Reporter: Jacky Jiang <jkjiang>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dbates, manyoso, rwlbuis, staikos, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 73144    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Jacky Jiang 2012-02-03 10:37:55 PST
Upstream BlackBerry WebCoreSupport ClientExtension and GeolocationControllerClientBlackBerry classes.
Comment 1 Jacky Jiang 2012-02-03 11:19:44 PST
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.
Comment 2 Rob Buis 2012-02-03 11:53:14 PST
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.
Comment 3 Jacky Jiang 2012-02-03 13:09:32 PST
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.
Comment 4 Rob Buis 2012-02-03 14:24:35 PST
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?
Comment 5 Jacky Jiang 2012-02-03 14:30:36 PST
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!
Comment 6 Jacky Jiang 2012-02-03 14:56:26 PST
Created attachment 125427 [details]
Patch

Update the patch according to the early return suggestion.
Comment 7 Rob Buis 2012-02-03 14:59:33 PST
Comment on attachment 125427 [details]
Patch

Looks good.
Comment 8 WebKit Review Bot 2012-02-04 01:24:25 PST
Comment on attachment 125427 [details]
Patch

Clearing flags on attachment: 125427

Committed r106731: <http://trac.webkit.org/changeset/106731>
Comment 9 WebKit Review Bot 2012-02-04 01:24:30 PST
All reviewed patches have been landed.  Closing bug.