[webkit-reviews] review granted: [Bug 238400] Support ServiceWorkerClients.openWindow : [Attachment 455888] PFR v2

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Brady Eidson
<beidson at apple.com>'s request for review:
Bug 238400: Support ServiceWorkerClients.openWindow
https://bugs.webkit.org/show_bug.cgi?id=238400

Attachment 455888: PFR v2

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




--- Comment #16 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 455888
  --> https://bugs.webkit.org/attachment.cgi?id=455888
PFR v2

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

>
LayoutTests/http/tests/workers/service/resources/shownotification-openwindow-wo
rker.js:16
> +    await messageClients("gotUserGestureFail");

I was initially expecting this test to fail since ServiceWorkerInternals is
making sure to disable user gesture
But ServiceWorkerInternals is currently injected after the initial run of the
worker script I believe.
Which makes the test pass!
When we start injecting ServiceWorkerInternals sooner, we might have to update
the test to add a call to enforce the user gesture for this particular test.

> Source/WebCore/workers/service/ServiceWorkerClients.cpp:143
> +			   promise->resolveWithJSValue(JSC::jsNull());

If there is no page identifier, it probably means the navigation failed and the
promise should reject, as per
https://w3c.github.io/ServiceWorker/#clients-openwindow step 7.2.
Can we add a test where the navigation fails?

> Source/WebCore/workers/service/ServiceWorkerClients.cpp:147
> +		   matchWindowWithPageIdentifier(serviceWorkerIdentifier,
*pageIdentifier, [promiseIdentifier] (auto& scope,
std::optional<ServiceWorkerClientData> clientData) mutable {

The work to identify the corresponding service worker can be done in network
process, that would probably more efficient.
openWindow could take a completion handler taking something like
Expected/ExceptionOr<std::optional<ServiceWorkerClientData>> to handle
failure/null/client cases.

>
Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:1
75
> +	   callback(WTFMove(pageIdentifier));

We could introduce a dedicated std::optional<ServiceWorkerClientData>
SWServer::serviceWorkerClientFromPageIdentifier(PageIdentifier) and use it here
to do the translation.
If there is no PageId, send back an exception.
If service worker is third party, return std::nullopt.
That would be functionally equivalent to matchWindowWithPageIdentifier, but a
bit more efficient and ServiceWorkerClients code would look closer to the spec.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:60
> +    explicit WebsiteDataStoreClient(WKWebsiteDataStore *dataStore,
id<_WKWebsiteDataStoreDelegate> delegate)

s/explicit//