Bug 223183 – Add a new delegate for geolocation permission

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[youenn fablet]

Add a new delegate for geolocation permission

[youenn fablet]

Created attachment 423178 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/75430600>

[youenn fablet]

Created attachment 423183 [details]
Patch

[youenn fablet]

Created attachment 423185 [details]
Patch

[Eric Carlson]

Comment on attachment 423185 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:415
> +        [(id <WKUIDelegatePrivate>)delegate _webView:m_uiDelegate->m_webView.get().get() requestGeolocationPermissionForOrigin:wrapper(API::SecurityOrigin::create(securityOrigin.get())) initiatedByFrame:wrapper(API::FrameInfo::create(FrameInfoData { frameInfo }, &page)) decisionHandler:makeBlockPtr([completionHandler = std::exchange(completionHandler, nullptr), securityOrigin = securityOrigin->data(), checker = WTFMove(checker), page = makeWeakPtr(page)] (_WKPermissionDecision decision) mutable {

This is *much* harder to read and understand than the one you updated for `UIClient::decidePolicyForUserMediaPermissionRequest`, so it might be worth using a local variable or two to aid readability.

[youenn fablet]

(In reply to Eric Carlson from comment #5)
> Comment on attachment 423185 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423185&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:415
> > +        [(id <WKUIDelegatePrivate>)delegate _webView:m_uiDelegate->m_webView.get().get() requestGeolocationPermissionForOrigin:wrapper(API::SecurityOrigin::create(securityOrigin.get())) initiatedByFrame:wrapper(API::FrameInfo::create(FrameInfoData { frameInfo }, &page)) decisionHandler:makeBlockPtr([completionHandler = std::exchange(completionHandler, nullptr), securityOrigin = securityOrigin->data(), checker = WTFMove(checker), page = makeWeakPtr(page)] (_WKPermissionDecision decision) mutable {
> 
> This is *much* harder to read and understand than the one you updated for
> `UIClient::decidePolicyForUserMediaPermissionRequest`, so it might be worth
> using a local variable or two to aid readability.

OK, will add a local variable for the decisionHandler

[youenn fablet]

Created attachment 423211 [details]
Patch for landing

[youenn fablet]

Comment on attachment 423211 [details]
Patch for landing

Win test failure is unrelated

[EWS]

Committed r274469: <https://commits.webkit.org/r274469>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423211 [details].

[Darryl Pogue]

Comment on attachment 423211 [details]
Patch for landing

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

> Source/WebKit/UIProcess/Cocoa/MediaPermissionUtilities.mm:139
> +        return [NSString stringWithFormat:WEB_UI_NSSTRING(@"Allow â%@â to use your current location?", @"Message for geolocation prompt"), visibleDomain(origin.host)];

Is there a reason not to use visibleOrigin here to show the app name for apps that embed WKWebView with custom schemes?

[youenn fablet]

(In reply to Darryl Pogue from comment #10)
> Comment on attachment 423211 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423211&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/MediaPermissionUtilities.mm:139
> > +        return [NSString stringWithFormat:WEB_UI_NSSTRING(@"Allow â%@â to use your current location?", @"Message for geolocation prompt"), visibleDomain(origin.host)];
> 
> Is there a reason not to use visibleOrigin here to show the app name for
> apps that embed WKWebView with custom schemes?

No, we should probably update it accordingly.