[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 #110268|review?                     |review+
               Flag|                            |




--- Comment #16 from Martin Robinson <mrobinson at webkit.org>  2011-10-08 08:36:00 PST ---
(From update of attachment 110268)
View in context: https://bugs.webkit.org/attachment.cgi?id=110268&action=review

This looks great! I realized that since we need this code for WebKit2 also we'll have to share it anyway. We can tackle that later though. It only requires hoisting the code out into another class. Please fix the style nits before landing.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:126
> +    static void setMockGeolocationPermission(WebKitWebView*, bool);
> +    static void setMockGeolocationPosition(WebKitWebView*, double, double, double);
> +    static void setMockGeolocationError(WebKitWebView*, int, const gchar*);

You should include the parameter names for types that are generic like int, double, and char*.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:33
> +void getPositionCallback(GeocluePosition *position, GeocluePositionFields fields, int timestamp, double latitude, double longitude, double altitude, GeoclueAccuracy* accuracy, GError* error, GeolocationClient* client)

The asterisk in GeocluePosition *position is in the wrong place.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:56
> +    , m_latitude(0.0)
> +    , m_longitude(0.0)
> +    , m_altitude(0.0)
> +    , m_accuracy(0.0)
> +    , m_altitudeAccuracy(0.0)

Here you should use 0 instead of 0.0.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:75
> +        errorOccured("Could not connect to location provider.");

I think this string should be prefixed by an underscore so that it can be localized.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:82
> +                                                 false, GEOCLUE_RESOURCE_ALL, &error.outPtr())) {

The indentation looks slightly off here.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:94
> +    geoclue_position_get_position_async(m_geocluePosition.get(), (GeocluePositionCallback)getPositionCallback, this);

Here you should use reinterpret_cast.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:165
> +                                                          true, m_altitude, true, m_altitudeAccuracy, false, 0.0, false, 0.0);

The 0.0 here should just be 0.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.h:27
> +

No newline necessary here.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1103
>      DumpRenderTreeSupportGtk::setDumpRenderTreeModeEnabled(true);
>  
> +    WebKitWebView* view = WEBKIT_WEB_VIEW(self_scrolling_webkit_web_view_new());

I think it's also important to put a little comment here like "It's important to create the WebvView after entering DumpRenderTree mode."

> Tools/Scripts/build-webkit:159
> +      define => "ENABLE_CLIENT_BASED_GEOLOCATION", default => (isAppleWebKit() || isGtk()), value => \$clientBasedGeolocationSupport },

Hopefully this doesn't lead to a warning about an unknown parameter. If it does, it could mean our bots always build with warnings.

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