[webkit-reviews] review granted: [Bug 210422] [GTK][WPE] Add API to expose UIClient::requestStorageAccessConfirm : [Attachment 401725] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at gnome.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 210422: [GTK][WPE] Add API to expose UIClient::requestStorageAccessConfirm
https://bugs.webkit.org/show_bug.cgi?id=210422

Attachment 401725: Patch

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




--- Comment #6 from Michael Catanzaro <mcatanzaro at gnome.org> ---
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

I think it would be pretty hard to create an API test for this. Manual testing
should suffice. (Bonus points for Epiphany implementation.) Do we know of any
existing websites that make use of the storage access API with either Safari or
Firefox?

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

WebKitWebsiteDataPermissionRequest

>
Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:3
4
> + * 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.

>
Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:8
1
> +    // Default behaviour when no decision has been made is denying the
request.
> +   
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.

>
Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:1
25
> +/**
> + * webkit_website_data_access_permission_request_get_current_domain:
> + * @request: a #WebKitWebsiteDataAccessPermissionRequest
> + *
> + * Get the domain from which the requesting domain wants to access cookies
and website data while browsing.
> + *
> + * Returns: the current domain name
> + *
> + * 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.

> Source/WebKit/UIProcess/API/gtk/WebKitWebsiteDataAccessPermissionRequest.h:56
> +struct _WebKitWebsiteDataAccessPermissionRequestClass {
> +    GObjectClass parent_class;
> +
> +    void (*_webkit_reserved0) (void);
> +    void (*_webkit_reserved1) (void);
> +    void (*_webkit_reserved2) (void);
> +    void (*_webkit_reserved3) (void);
> +};

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.