[webkit-reviews] review granted: [Bug 238503] ServiceWorkerClients.openWindow should not need to get all clients asynchronously to resolve its promise : [Attachment 456351] 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 238503: ServiceWorkerClients.openWindow should not need to get all clients
asynchronously to resolve its promise
https://bugs.webkit.org/show_bug.cgi?id=238503

Attachment 456351: Patch

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




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

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

r=me

> Source/WebCore/workers/service/server/SWServer.cpp:123
> +	   if (clientIterator->value->frameType ==
ServiceWorkerClientFrameType::TopLevel && clientIterator->value->pageIdentifier
&& *clientIterator->value->pageIdentifier == pageIdentifier)

`&& clientIterator->value->pageIdentifier &&
*clientIterator->value->pageIdentifier == pageIdentifier`
can be replaced with:
`&& clientIterator->value->pageIdentifier == pageIdentifier`

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

>
Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:2
02
> +   
m_connection.networkProcess().parentProcessConnection()->sendWithAsyncReply(Mes
sages::NetworkProcessProxy::OpenWindowFromServiceWorker {
m_connection.sessionID(), url.string(), worker->origin().clientOrigin },
WTFMove(innerCallback));

Seems we should just IPC the URL, not the URL as a String. Looks like we end up
needing a URL in the UIProcess eventually anyway.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:2970
> +static const char* ServiceWorkerWindowClientOpenWindowMain =

static constexpr auto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:3011
> +"});";

Needs a ""_s suffix (note that I am about to make the String(const char*)
constructor explicit to force people to use ASCIILiteral.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:3029
> +	       [preferences _setEnabled:NO forInternalDebugFeature:feature];

Seems you can break; after finding the setting you care about.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:3038
> +	   { "/", { ServiceWorkerWindowClientOpenWindowMain } },

"/"_s

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:3039
> +	   { "/sw.js", { {{ "Content-Type", "application/javascript" }},
ServiceWorkerWindowClientOpenWindowJS } }

Missing 3 _s suffixes.