[webkit-reviews] review denied: [Bug 83877] [GTK][WK2] Implement geolocation provider for the GTK port : [Attachment 145056] Patch proposal

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Mario Sanchez Prada
<msanchez at igalia.com>'s request for review:
Bug 83877: [GTK][WK2] Implement geolocation provider for the GTK port
https://bugs.webkit.org/show_bug.cgi?id=83877

Attachment 145056: Patch proposal
https://bugs.webkit.org/attachment.cgi?id=145056&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=145056&action=review


Looks good, though I've listed a few issues below and it looks like the build
failed.

> Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationProvider.cpp:71
> +	   WKGeolocationManagerRef
wkGeolocationManager(WKContextGetGeolocationManager(m_wkContext.get()));
> +	   WKGeolocationPositionRef
wkGeolocationPosition(WKGeolocationPositionCreate(timestamp, latitude,
longitude, accuracy));

I think you are leaking the position here. These are just pointers, so you
should use the assignment style, not the constructor style:

WKGeolocationManagerRef wkGeolocationManager =
WKContextGetGeolocationManager(m_wkContext.get());

> Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationProvider.cpp:80
> +	   WKGeolocationManagerRef
wkGeolocationManager(WKContextGetGeolocationManager(m_wkContext.get()));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationProvider.cpp:108
> +    static GeolocationProviderClientInfo clientInfo;

I don't think you want to make this static. Theoretically there may be more
than one WebKitWebContext.