[webkit-reviews] review granted: [Bug 170362] Add SPI for handling geolocation authorization requests : [Attachment 306764] Patch v2.1

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted David Quesada
<david_quesada at apple.com>'s request for review:
Bug 170362: Add SPI for handling geolocation authorization requests
https://bugs.webkit.org/show_bug.cgi?id=170362

Attachment 306764: Patch v2.1

https://bugs.webkit.org/attachment.cgi?id=306764&action=review




--- Comment #15 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 306764
  --> https://bugs.webkit.org/attachment.cgi?id=306764
Patch v2.1

View in context: https://bugs.webkit.org/attachment.cgi?id=306764&action=review

LGTM except the memory leak.

> Source/WebKit2/UIProcess/ios/WKGeolocationProviderIOS.mm:92
> +    RetainPtr<id<_WKGeolocationCoreLocationProvider>> _coreLocationProvider;

I'm not sure if the id is necessary here.  If it is, we seem to have a space
after id elsewhere in WebKit.

> Source/WebKit2/UIProcess/ios/WKGeolocationProviderIOS.mm:269
> +    _provider = [[WebGeolocationCoreLocationProvider alloc]
initWithListener:self];

We need an adoptNS or this is a memory leak.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/Geolocation.mm:42
> +SOFT_LINK_FRAMEWORK(CoreLocation)
> +SOFT_LINK_CLASS(CoreLocation, CLLocation)

Is there a reason we're soft linking?