[webkit-reviews] review denied: [Bug 64970] [Gtk] Support for client-based geolocation : [Attachment 103182] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 6 22:00:12 PDT 2011
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 103182: Patch
https://bugs.webkit.org/attachment.cgi?id=103182&action=review
------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103182&action=review
Sorry for letting this sit so long. I didn't really understand what was going
on here at first. In general it looks good to me. I think that if this is a
feature-preserving replacement for the old system, we should just remove the
old system completely.
> Source/WebCore/platform/gtk/GeolocationServiceGtk.cpp:22
> -#if ENABLE(GEOLOCATION)
> +#if ENABLE(GEOLOCATION) && !ENABLE(CLIENT_BASED_GEOLOCATION)
Is client-based geolocation is a full replacement for this, why not remove this
file and make it always on? Is there any loss in functionality?
> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:2
> + * Copyright (C) 2011 Zan Dobersek <zandobersek at gmail.com>
You need to preserve the copyright from
Source/WebCore/platform/gtk/GeolocationServiceGtk.cpp.
> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:74
> + GOwnPtr<GError> error;
Please move this down to where you first use it.
> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:97
> + }
> +
> + GeoclueAccuracyLevel accuracyLevel = m_enableHighAccuracy ?
GEOCLUE_ACCURACY_LEVEL_DETAILED : GEOCLUE_ACCURACY_LEVEL_LOCALITY;
> + gboolean result = geoclue_master_client_set_requirements(client.get(),
accuracyLevel, 0,
> + false,
GEOCLUE_RESOURCE_ALL, &error.outPtr());
> +
> + if (!result) {
> + errorOccured(error->message);
> + return;
> + }
> +
> + m_geocluePosition =
adoptGRef(geoclue_master_client_create_position(client.get(),
&error.outPtr()));
> + if (!m_geocluePosition) {
> + errorOccured(error->message);
> + return;
> + }
> +
There's lots of extra newlines in the method. Do you think you can clump
related ideas together a bit more?
> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:125
> + if (m_isUpdating) {
Please use an early return here.
> Tools/ChangeLog:13
> + (createWebView): Create web view after declaring DumpRenderTree
mode.
Do you mind explaining why this is important here?
> configure.ac:627
> + [enable support for client-based geolocation
[default=yes]]),
> + [],[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?
More information about the webkit-reviews
mailing list