[webkit-reviews] review granted: [Bug 64970] [Gtk] Support for client-based geolocation : [Attachment 110268] Updated patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Oct 8 08:36:00 PDT 2011

Martin Robinson <mrobinson at webkit.org> has granted Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 64970: [Gtk] Support for client-based geolocation

Attachment 110268: Updated patch

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=110268&action=review

This looks great! I realized that since we need this code for WebKit2 also
we'll have to share it anyway. We can tackle that later though. It only
requires hoisting the code out into another class. Please fix the style nits
before landing.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:126
> +    static void setMockGeolocationPermission(WebKitWebView*, bool);
> +    static void setMockGeolocationPosition(WebKitWebView*, double, double,
> +    static void setMockGeolocationError(WebKitWebView*, int, const gchar*);

You should include the parameter names for types that are generic like int,
double, and char*.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:33
> +void getPositionCallback(GeocluePosition *position, GeocluePositionFields
fields, int timestamp, double latitude, double longitude, double altitude,
GeoclueAccuracy* accuracy, GError* error, GeolocationClient* client)

The asterisk in GeocluePosition *position is in the wrong place.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:56
> +    , m_latitude(0.0)
> +    , m_longitude(0.0)
> +    , m_altitude(0.0)
> +    , m_accuracy(0.0)
> +    , m_altitudeAccuracy(0.0)

Here you should use 0 instead of 0.0.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:75
> +	   errorOccured("Could not connect to location provider.");

I think this string should be prefixed by an underscore so that it can be

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:82
> +						    false,
GEOCLUE_RESOURCE_ALL, &error.outPtr())) {

The indentation looks slightly off here.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:94
> +    geoclue_position_get_position_async(m_geocluePosition.get(),
(GeocluePositionCallback)getPositionCallback, this);

Here you should use reinterpret_cast.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:165
> +							     true, m_altitude,
true, m_altitudeAccuracy, false, 0.0, false, 0.0);

The 0.0 here should just be 0.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.h:27
> +

No newline necessary here.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1103
>      DumpRenderTreeSupportGtk::setDumpRenderTreeModeEnabled(true);
> +    WebKitWebView* view =

I think it's also important to put a little comment here like "It's important
to create the WebvView after entering DumpRenderTree mode."

> Tools/Scripts/build-webkit:159
> +	 define => "ENABLE_CLIENT_BASED_GEOLOCATION", default =>
(isAppleWebKit() || isGtk()), value => \$clientBasedGeolocationSupport },

Hopefully this doesn't lead to a warning about an unknown parameter. If it
does, it could mean our bots always build with warnings.

More information about the webkit-reviews mailing list