[Webkit-unassigned] [Bug 210422] [GTK][WPE] Add API to expose UIClient::requestStorageAccessConfirm

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=210422

--- Comment #8 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 401725
  --> https://bugs.webkit.org/attachment.cgi?id=401725
Patch

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

>> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:33
>> + * WebKitUserMediaPermissionRequest represents a request for
> 
> WebKitWebsiteDataPermissionRequest

Oops.

>> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:34
>> + * permission to allow a third-party domain access cookies and website data.
> 
> access to cookies and website data
> 
> Although, I'm not actually certain it *does* allow a third-party domain to access website data other than cookies. Are you sure about this? Firefox allows this according to https://developer.mozilla.org/en-US/docs/Web/API/Storage_Access_API, but its documentation implies that WebKit does not. John would know exactly how this works.

I used the safari dialog message as a reference, see https://webkit.org/wp-content/uploads/storage-access-prompt.png

>> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:81
>> +    webkitWebsiteDataAccessPermissionRequestDeny(WEBKIT_PERMISSION_REQUEST(object));
> 
> So I see this is copied from our various other permission request classes, but all those classes have a bool madeDecision member variable to track whether a permission request has previously been completed or not and only call the CompletionHandler if so. You don't have that here, so it's going to call the CompletionHandler again unconditionally. If you try this patch in a debug build, I think it's going to assert, because CompletionHandler is designed to assert when called twice.

We don't need an extra variable for that in this case because completion handler is false once it has been called, and I'm checking it's true before calling it in both Allow and Deny.

>> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:125
>> + * Since: 2.30
> 
> Hm, I assumed that the domain requesting storage access only gets access to its *own* cookies (and maybe also other website data), not access to the first-party domain's data. Does it really get access to the data of the first-party domain? John?
> 
> Please investigate before landing; we need to be sure about this before we land this API.

Again I used the safari dialog as reference.

>> Source/WebKit/UIProcess/API/gtk/WebKitWebsiteDataAccessPermissionRequest.h:56
>> +};
> 
> Idle brainstorming: now that GTK 4 is upon us, it might be a good idea to figure out if it's possible to hide these class structs from the GTK 4 API. There's no reason for this type to be derivable, and no value in exposing class structs for non-derivable types. Problem is we can't use #if USE(GTK4) in the header files, so we'd need a third copy of all the public header files... possibly not worth it.

We are already adding another header for the cases where gtk4 needs a different API. I don't think it's worth using a different header for all classes in gtk4 just to me them final.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200614/491fc722/attachment.htm>