[webkit-reviews] review denied: [Bug 64970] [Gtk] Support for client-based geolocation : [Attachment 101634] Same patch but now with license headers in new files

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied 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 101634: Same patch but now with license headers in new files
https://bugs.webkit.org/attachment.cgi?id=101634&action=review

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


This patch looks pretty good in general, and unskips a ton of tests!  I don't
quite understand why we want to implement client-based geolocation without
exposing an API. Is  it necessary just to unskip the tests? Could we somehow
create mock locations by interfacing directly with geoclue? If so that would be
great, because we could test the geoclue stuff with layout tests.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:56
> +    if (m_geoclueClient)
> +	   g_object_unref(m_geoclueClient);
> +
> +    if (m_geocluePosition)
> +	   g_object_unref(m_geocluePosition);

It would be better to use GRefPtr instead of handing reference counts manually.


> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:69
> +    GeoclueMaster* master = geoclue_master_get_default();

Please use GRefPtr here with adoptGref.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:70
> +    GeoclueMasterClient* client = geoclue_master_create_client(master, 0,
0);

Ditto.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:162
> +						   GeocluePositionFields
fields,
> +						   int timestamp,
> +						   double latitude,
> +						   double longitude,
> +						   double altitude,
> +						   GeoclueAccuracy* accuracy,
> +						   GError* error,
> +						   GeolocationClient* that)
> +{

We don't typically add newlines to argument lists in function definitions. This
function can be static to the file.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:165
> +	   g_error_free(error);

Does the callback really need to free the GError?

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:186
> +void GeolocationClient::positionChanged(GeocluePosition*,
GeocluePositionFields fields, int timestamp, double latitude, double longitude,
double altitude, GeoclueAccuracy* accuracy, GeolocationClient* that)
> +{
> +    if (!(fields & GEOCLUE_POSITION_FIELDS_LATITUDE && fields &
GEOCLUE_POSITION_FIELDS_LONGITUDE)) {
> +	   that->errorOccured("Position could not be determined.");
> +	   return;
> +    }
> +
> +    that->m_timestamp = timestamp;
> +    that->m_latitude = latitude;
> +    that->m_longitude = longitude;
> +    that->m_altitude = altitude;
> +
> +    GeoclueAccuracyLevel level;
> +    geoclue_accuracy_get_details(accuracy, &level, &that->m_accuracy,
&that->m_altitudeAccuracy);
> +    that->updatePosition();
> +}

Shouldn't this be an instance method?

> Tools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp:495
> +    gchar* cMessage = JSStringCopyUTF8CString(message);
> +    DumpRenderTreeSupportGtk::setMockGeolocationError(view, code, cMessage);

> +    g_free(cMessage);
>  }

Could you use GOwnPtr here?