Bug 238738 – Implement ServiceWorkerWindowClient.navigate

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

Implement ServiceWorkerWindowClient.navigate

[youenn fablet]

Created attachment 456577 [details]
WIP

[youenn fablet]

Created attachment 456601 [details]
WIP

[youenn fablet]

Created attachment 456695 [details]
Patch

[youenn fablet]

Created attachment 456696 [details]
Patch

[youenn fablet]

@kal, any idea why gtk tests are failing? The bots do not provide the diff results sadly.

[youenn fablet]

Created attachment 456702 [details]
Patch

[Chris Dumez]

Comment on attachment 456702 [details]
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.

[youenn fablet]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 456702 [details]
> 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().

Right.

> > 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.

OK

> > 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.

Yes, same assertion as added in openWindow patch that introduces WorkerGlobalScope::clientOrigin. will add it.

> > 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.

OK

> > 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?

I reverted the change but I am not quite sure here.
Isn't it that if we do using namespace WebCore inside the namespace WebKit, we are good with unified builds?

> > 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);`

I thought using std::exchange was usually better whenever we reuse variables (here m_navigateCallback).
Given these are completion handlers, this does not change anything in practice.

[youenn fablet]

Created attachment 456793 [details]
Patch

[Carlos Garcia Campos]

hmm, I can't see the diff in the test results, it seems there's a bug in the generated html for test results.

[youenn fablet]

Created attachment 456797 [details]
Patch for landing

[youenn fablet]

(In reply to Carlos Garcia Campos from comment #10)
> hmm, I can't see the diff in the test results, it seems there's a bug in the
> generated html for test results.

I marked the tests as failing for now in glib.

[EWS]

Committed r292459 (249310@main): <https://commits.webkit.org/249310@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456797 [details].

[Radar WebKit Bug Importer]

<rdar://problem/91346404>

[Diego Pino]

There are only 2 tests filed under this bug (both in GLIB TestExpectations):

```
LayoutTests/platform/glib/TestExpectations:1987:webkit.org/b/238738 http/wpt/service-workers/navigate-iframes-window-client.https.html [ Failure ]
LayoutTests/platform/glib/TestExpectations:1988:webkit.org/b/238738 imported/w3c/web-platform-tests/service-workers/service-worker/windowclient-navigate.https.html [ Failure ]
```

Test 'http/wpt/service-workers/navigate-iframes-window-client.https.html' was fixed by 296976@main:

  - https://results.webkit.org/?platform=GTK&platform=WPE&&suite=layout-tests&test=http%2Fwpt%2Fservice-workers%2Fnavigate-iframes-window-client.https.html

Perhaps is worth to close this bug and file 'imported/w3c/web-platform-tests/service-workers/service-worker/windowclient-navigate.https.html' under a different bug. Notice this test is marked as flaky for iOS and Mac, although results.webkit.org show this test is consistently failing under those platforms:

https://results.webkit.org/?platform=GTK&platform=WPE&platform=ios&platform=mac&&suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fservice-worker%2Fwindowclient-navigate.https.html&limit=4000

[Diego Pino]

I just noticed this bug was actually already closed. Ideally, test failures in TestExpectations should not point to closed bugs. So I just going to go ahead and file 'imported/w3c/web-platform-tests/service-workers/service-worker/windowclient-navigate.https.html' under a new bug for GLIB ports.