[Webkit-unassigned] [Bug 64970] [Gtk] Support for client-based geolocation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=64970


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #101634|review?                     |review-
               Flag|                            |




--- Comment #7 from Martin Robinson <mrobinson at webkit.org>  2011-08-05 09:43:14 PST ---
(From update of attachment 101634)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.