[webkit-reviews] review granted: [Bug 238738] Implement ServiceWorkerWindowClient.navigate : [Attachment 456702] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 238738: Implement ServiceWorkerWindowClient.navigate
https://bugs.webkit.org/show_bug.cgi?id=238738

Attachment 456702: Patch

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




--- Comment #7 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 456702
  --> https://bugs.webkit.org/attachment.cgi?id=456702
Patch

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

r=me

> Source/WebCore/dom/Document.cpp:8790
> +    postTask([weakThis = WeakPtr { *this }, url, callback =
WTFMove(callback)](auto&) mutable {

This should likely be using the HTML event loop instead of the legacy
postTask().

> Source/WebCore/workers/service/ServiceWorkerWindowClient.cpp:79
> +	   promise->reject(Exception { TypeError, makeString("URL string ",
urlString, " cannot successfully be parsed"_s) });

You may or may not use _s for string literals inside makeString(). It doesn't
impact perf in any way as far as I know. However, you should be consistent.
Your first literal uses "" and the second one uses ""_s. Please align one way
or another.

> Source/WebCore/workers/service/ServiceWorkerWindowClient.cpp:107
> +#if ASSERT_ENABLED

Aren't you missing an assertion?

It is unclear to me what this code does unless you missed some kind of
assertion relying on the ClientOrigin.

> Source/WebCore/workers/service/server/SWServerWorker.cpp:427
> +    return registrationIdentifier && *registrationIdentifier ==
m_data.registrationIdentifier;

Can be written as `registrationIdentifier == m_data.registrationIdentifier`.

std::optional's operator == will do the right thing for you.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1807
> +void NetworkProcessProxy::openWindowFromServiceWorker(PAL::SessionID
sessionID, const String& urlString, const WebCore::SecurityOriginData&
serviceWorkerOrigin, CompletionHandler<void(std::optional<PageIdentifier>&&)>&&
callback)

I am a bit concerned you dropped WebCore:: from PageIdentifier but not from
SecurityOriginData.

Also, I understand we use the WebCore namespace in this file. However, my
understanding is that we were trying to be explicit about namespaces in new
code (I think because of Unified builds). Therefore, this change may be going
in the wrong direction?

> Source/WebKit/UIProcess/WebFrameProxy.cpp:87
> +std::optional<PageIdentifier> WebFrameProxy::pageIdentifier()

Why isn't this const?

> Source/WebKit/UIProcess/WebFrameProxy.cpp:91
> +    return m_page->webPageID();

nit: I would have used a ternary.

> Source/WebKit/UIProcess/WebFrameProxy.h:133
> +    void transferNavigationCallbackToFrame(WebFrameProxy& frame) {
std::exchange(frame.m_navigateCallback, WTFMove(m_navigateCallback)); }

This doesn't return anything so this shouldn't be using std::exchange(). Just
do:
`frame.m_navigateCallback = WTFMove(m_navigateCallback);`

> Source/WebKit/UIProcess/WebFrameProxy.h:138
> +    std::optional<WebCore::PageIdentifier> pageIdentifier();

Should be const.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6445
> +    auto* document = Document::allDocumentsMap().get(documentIdentifier);

To be safe, I'd use `RefPtr document` here.

Also, we could write this function with a little less lines:
```
#if ENABLE(SERVICE_WORKER)
    if (RefPtr document = Document::allDocumentsMap().get(documentIdentifier))
{
	document->navigateFromServiceWorker(url, WTFMove(callback));
	return;
    }
#else
    UNUSED_PARAM(documentIdentifier);
    UNUSED_PARAM(url);
#endif
    callback(false);
```

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:2999
> +"</script>"_s;

Thank you for using ASCIILiteral.