Clean up Geolocation request handling

[Alex Christensen]

Clean up Geolocation request handling

[Alex Christensen]

Created attachment 320170 [details]
Patch

[Alex Christensen]

Created attachment 320178 [details]
Patch

[Build Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Alex Christensen]

Created attachment 320179 [details]
Patch

[Alex Christensen]

Created attachment 320181 [details]
Patch

[Tim Horton]

Comment on attachment 320181 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:5827
> +    completionHandler = m_uiClient->decidePolicyForGeolocationPermissionRequest(*this, *frame, origin.get(), WTFMove(completionHandler));
> +#if PLATFORM(IOS)
> +    if (completionHandler)
> +        completionHandler = m_pageClient.decidePolicyForGeolocationPermissionRequest(*frame, origin.get(), WTFMove(completionHandler));
> +#endif
> +    if (completionHandler)
> +        completionHandler(false);

This looks SO weird, but you explained it. Maybe a comment?

[Alex Christensen]

Created attachment 320186 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r221764

[Darin Adler]

Comment on attachment 320186 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        Instead of passing a GeolocationPermissionRequestProxy& to each client and returning a bool if the request will be handled,
> +        pass a Function<void(bool)>&& completionHandler and return the function if it won't be handled.
> +        This allows to temporarily support our UIClient/PageClient hybrid design while moving towards a sane design.

Why "return the function if it won’t be handled”?

Seems like we could just have it "not null out the function if it won’t be handled" or "not move the function if it won’t be handled", which is the same thing. The return value seems unneeded.

[Darin Adler]

Comment on attachment 320186 [details]
Patch

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

>> Source/WebKit/ChangeLog:10
>> +        This allows to temporarily support our UIClient/PageClient hybrid design while moving towards a sane design.
> 
> Why "return the function if it won’t be handled”?
> 
> Seems like we could just have it "not null out the function if it won’t be handled" or "not move the function if it won’t be handled", which is the same thing. The return value seems unneeded.

Then I think the argument turns into a:

    Function<void(bool)>&

Because it’s not safe to pass in a new object and then not look at it afterward. Anyway, that’s better than both taking and returning a Function I think.

[Radar WebKit Bug Importer]

<rdar://problem/34693823>