Summary: | [GTK] Add a new and reusable Geoclue-based geolocation provider in WebCore | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mario Sanchez Prada <mario> | ||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | mrobinson | ||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 83877, 87801 | ||||||||||
Attachments: |
|
Description
Mario Sanchez Prada
2012-05-29 17:51:32 PDT
Created attachment 144664 [details]
Patch proposal
Comment on attachment 144664 [details] Patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=144664&action=review I know this code has just been moved from wk layer, but I think we can take advantage to improve or fix it. Most of the geoclue api includes sync and async versions of the methods, and we are using sync in some cases and async in others, so I wonder whether we could make this fully non-blocking by always using the async methods. > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:81 > + GOwnPtr<GError> error; > + GRefPtr<GeoclueMasterClient> client = adoptGRef(geoclue_master_create_client(master.get(), 0, &error.outPtr())); > + if (!client) { > + errorOccured(error->message); > + return; > + } Could we use geoclue_master_create_client_async() instead? > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:88 > + GeoclueAccuracyLevel accuracyLevel = m_enableHighAccuracy ? GEOCLUE_ACCURACY_LEVEL_DETAILED : GEOCLUE_ACCURACY_LEVEL_LOCALITY; > + if (!geoclue_master_client_set_requirements(client.get(), accuracyLevel, 0, > + false, GEOCLUE_RESOURCE_ALL, &error.outPtr())) { > + errorOccured(error->message); > + return; > + } Could we use geoclue_master_client_set_requirements_async() instead?. We could move this to a helper since it's used twice in this file. > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:94 > + m_geocluePosition = adoptGRef(geoclue_master_client_create_position(client.get(), &error.outPtr())); > + if (!m_geocluePosition) { > + errorOccured(error->message); > + return; > + } geoclue_master_client_create_position_async()? > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:99 > + g_signal_connect(G_OBJECT(m_geocluePosition.get()), "position-changed", > + G_CALLBACK(positionChangedCallback), this); g_signal_connect expects a gpointer not a GObject so we don't need to cast here. This could probably be a single line, > Source/WebCore/platform/geoclue/GeolocationProviderGeoclueListener.h:26 > +#include <geoclue/geoclue-master.h> > +#include <geoclue/geoclue-position.h> Do you really need these headers here? Created attachment 145031 [details]
Patch proposal
New patch, addressing issues raised by Carlos
Comment on attachment 145031 [details] Patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=145031&action=review > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:39 > + g_error_free(error); We are freeing the error here but not in the other callbacks, it always looked weird to me that we would have to free the error in the callback, but I assumed geoclue worked this way. If this is the case we should free the error in all other callbacks too. > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:60 > + provider->setGeocluePosition(position); > + > + geoclue_position_get_position_async(position, reinterpret_cast<GeocluePositionCallback>(getPositionCallback), provider); > + g_signal_connect(position, "position-changed", G_CALLBACK(positionChangedCallback), provider); In these cases where we have to use a static callback I prefer to do the minimum things in the callback, what do you think about moving these two lines to setGeocluePosition? > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:82 > + provider->setGeoclueClient(client); > + provider->updateClientRequirements(); > + > + geoclue_master_client_create_position_async(client, createPositionCallback, provider); Same here, we could updateClientRequirements and create the position in setGeoclueClient > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:143 > + m_isUpdating = true; It looks a bit weird to set isUpdating here, is it a problem if it's set in startUpdating()? > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:172 > +void GeolocationProviderGeoclue::errorOccured(const char* message) > +{ > + m_listener->notifyErrorOcurred(message); > +} This is funny, one method is errorOccured (double c in ocurred and only one r) and the other is notifyErrorOcurred (only on c and double r), I'm not English expert, but google says the right word id occurred (double c and r) :-) Created attachment 145321 [details]
Patch proposal
New patch addressing issues raised by Carlos.
I also renamed GeolocationProviderGeoclueListener to GeolocationProviderGeoclueClient.
Comment on attachment 145321 [details] Patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=145321&action=review Please consider changing the return in geoclueClientSetRequirementsCallback before landing. > Source/WebCore/platform/geoclue/GeolocationProviderGeoclue.cpp:66 > + if (error) { > + provider->errorOccurred(error->message); > + g_error_free(error); > + return; > + } This could be an early return, or remove the return, because there's nothing to do if there isn't an error Committed r119249: <http://trac.webkit.org/changeset/119249> |