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

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #103182|review?                     |review-
               Flag|                            |




--- Comment #11 from Martin Robinson <mrobinson at webkit.org>  2011-10-06 22:00:12 PST ---
(From update of attachment 103182)
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?

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