[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 #13 from Martin Robinson <mrobinson at webkit.org>  2011-10-07 12:46:04 PST ---
(From update of attachment 103182)
View in context: https://bugs.webkit.org/attachment.cgi?id=103182&action=review

>>> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:97
>>> +
>> 
>> There's lots of extra newlines in the method. Do you think you can clump related ideas together a bit more?
> 
> I can avoid assigning the result variable and check the return of geoclue_master_client_set_requiremenets directly. That saves two lines. Other than that, I don't think it's a good idea to try to push through when an error occurs.

Sorry. Here I meant there were a lot of empty lines. I think they could be reduced by grouping related concepts together. For instance, line 86 and 77, 99, 101 and 104. This is just a nit, but it's hard to keep track of methods that take up too much vertical space.

>>> configure.ac:627
>>> +              [],[enable_client_based_geolocation="yes"])
>> 
>> By enabling this by deafult don't you disable the normal Geolocation service for all builds? Wouldn't it be sufficient to enable it only via build-webkit?
> 
> Yes, enabling this by default disables the use of service-based geolocation. If there's no motivation to have client-based solution enabled in release versions, I can revert this change.
> 
> One more thing on the two different approaches of providing geolocation and their subsequent status after this bug is resolved. Both methods are (still) supported in WebCore, so I see no reason why a configuration option should not be available to decide on the approach to be used. But I'd like to see the client-based geolocation being set as default both in configure.ac and build-webkit - there are more tests that cover it and if it comes to enabling the feature in build-webkit but not in configure.ac, this would meant we're testing client-based geolocation but shipping service-based one. Not a good idea.

It sounds like this API is a solid replacement for old one. It also seems like the difference between the two is totally hidden from the user of the library -- it's an implementation detail. If that's the case perhaps we shouldn't even support the old method.

I also dislike the idea of having the same code (effectively) copied in two places. If you still think that keeping the old code around is a good idea, we should find some way to share the geoclue glue between the two implementations. From what you say and the amount of tests you are unskipping with this patch it seems like client-based geolocation is the way to go. Let me know if I'm misunderstanding things here.

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