[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