Bug 238738 - Implement ServiceWorkerWindowClient.navigate
Summary: Implement ServiceWorkerWindowClient.navigate
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks: 238800
  Show dependency treegraph
 
Reported: 2022-04-04 07:53 PDT by youenn fablet
Modified: 2022-04-06 04:19 PDT (History)
7 users (show)

See Also:


Attachments
WIP (51.47 KB, patch)
2022-04-04 08:55 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
WIP (51.77 KB, patch)
2022-04-04 11:50 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (71.81 KB, patch)
2022-04-05 05:49 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (71.53 KB, patch)
2022-04-05 06:01 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (70.00 KB, patch)
2022-04-05 08:39 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (64.94 KB, patch)
2022-04-06 01:57 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (70.23 KB, patch)
2022-04-06 03:12 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2022-04-04 07:53:25 PDT
Implement ServiceWorkerWindowClient.navigate
Comment 1 youenn fablet 2022-04-04 08:55:43 PDT
Created attachment 456577 [details]
WIP
Comment 2 youenn fablet 2022-04-04 11:50:36 PDT
Created attachment 456601 [details]
WIP
Comment 3 youenn fablet 2022-04-05 05:49:52 PDT
Created attachment 456695 [details]
Patch
Comment 4 youenn fablet 2022-04-05 06:01:40 PDT
Created attachment 456696 [details]
Patch
Comment 5 youenn fablet 2022-04-05 08:07:14 PDT
@kal, any idea why gtk tests are failing? The bots do not provide the diff results sadly.
Comment 6 youenn fablet 2022-04-05 08:39:19 PDT
Created attachment 456702 [details]
Patch
Comment 7 Chris Dumez 2022-04-05 09:22:51 PDT
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.
Comment 8 youenn fablet 2022-04-06 01:03:34 PDT
(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.
Comment 9 youenn fablet 2022-04-06 01:57:06 PDT
Created attachment 456793 [details]
Patch
Comment 10 Carlos Garcia Campos 2022-04-06 01:57:14 PDT
hmm, I can't see the diff in the test results, it seems there's a bug in the generated html for test results.
Comment 11 youenn fablet 2022-04-06 03:12:22 PDT
Created attachment 456797 [details]
Patch for landing
Comment 12 youenn fablet 2022-04-06 03:13:23 PDT
(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.
Comment 13 EWS 2022-04-06 04:18:37 PDT
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].
Comment 14 Radar WebKit Bug Importer 2022-04-06 04:19:15 PDT
<rdar://problem/91346404>