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

Jacky Jiang
Reported 2012-02-03 10:37:55 PST
Upstream BlackBerry WebCoreSupport ClientExtension and GeolocationControllerClientBlackBerry classes.
Attachments
Patch (13.86 KB, patch)
2012-02-03 11:19 PST, Jacky Jiang 		no flags
DetailsFormatted DiffDiff
Patch (13.74 KB, patch)
2012-02-03 13:09 PST, Jacky Jiang 		no flags
DetailsFormatted DiffDiff
Patch (13.70 KB, patch)
2012-02-03 14:56 PST, Jacky Jiang 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Jacky Jiang
Comment 1 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.
Rob Buis
Comment 2 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.
Jacky Jiang
Comment 3 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.
Rob Buis
Comment 4 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?
Jacky Jiang
Comment 5 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!
Jacky Jiang
Comment 6 2012-02-03 14:56:26 PST
Created attachment 125427 [details] Patch Update the patch according to the early return suggestion.
Rob Buis
Comment 7 2012-02-03 14:59:33 PST
Comment on attachment 125427 [details] Patch Looks good.
WebKit Review Bot
Comment 8 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>
WebKit Review Bot
Comment 9 2012-02-04 01:24:30 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.