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

[bugzilla-daemon at webkit.org]

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





--- Comment #8 from Zan Dobersek <zandobersek at gmail.com>  2011-08-07 09:18:30 PST ---
(From update of attachment 101634)
View in context: https://bugs.webkit.org/attachment.cgi?id=101634&action=review

>> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:56
>> +        g_object_unref(m_geocluePosition);
> 
> It would be better to use GRefPtr instead of handing reference counts manually.

True, that'd look much nicer.

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

Will be used for both GeoclueMaster and GeoclueMasterClient.

>> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:162
>> +{
> 
> We don't typically add newlines to argument lists in function definitions. This function can be static to the file.

Noted.

>> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:165
>> +        g_error_free(error);
> 
> Does the callback really need to free the GError?

The error originates from the provider GeoClue is currently using, and looking at GeoClue code, the error is not freed anywhere. Because of that I think it would be appropriate that we free it.

>> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:186
>> +}
> 
> Shouldn't this be an instance method?

And instance method will be made. Also, because of that, a new callback will be added, positionChangedCallback, for the 'position-changed' signal. The callback will then call the GeolocationClient's positionChanged method in the same way the getPositionCallback callback does.

>> Tools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp:495
>>  }
> 
> Could you use GOwnPtr here?

Will be used.

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