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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=64970

Attachment 110268: Updated patch
https://bugs.webkit.org/attachment.cgi?id=110268&action=review

------- 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,
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
localized.

> 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 =
WEBKIT_WEB_VIEW(self_scrolling_webkit_web_view_new());

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.