Summary: | [BlackBerry] Upstream BlackBerry WebCoreSupport ClientExtension and GeolocationControllerClientBlackBerry classes | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jacky Jiang <jkjiang> | ||||||||
Component: | New Bugs | Assignee: | 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
Jacky Jiang
2012-02-03 10:37:55 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 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. 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 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 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! Created attachment 125427 [details]
Patch
Update the patch according to the early return suggestion.
Comment on attachment 125427 [details]
Patch
Looks good.
Comment on attachment 125427 [details] Patch Clearing flags on attachment: 125427 Committed r106731: <http://trac.webkit.org/changeset/106731> All reviewed patches have been landed. Closing bug. |