Bug 203144 - Move service worker registration matching for navigation loads to network process
Summary: Move service worker registration matching for navigation loads to network pro...
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:
 
Reported: 2019-10-18 01:10 PDT by youenn fablet
Modified: 2019-10-21 18:35 PDT (History)
6 users (show)

See Also:


Attachments
Patch (56.10 KB, patch)
2019-10-18 02:37 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (56.18 KB, patch)
2019-10-18 03:20 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Win fix (56.17 KB, patch)
2019-10-18 06:44 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (53.48 KB, patch)
2019-10-21 15:46 PDT, Alex Christensen
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 2019-10-18 01:10:38 PDT
Move service worker registration matching for navigation loads to network process
Comment 1 youenn fablet 2019-10-18 02:37:17 PDT
Created attachment 381288 [details]
Patch
Comment 2 youenn fablet 2019-10-18 03:20:06 PDT
Created attachment 381293 [details]
Patch
Comment 3 youenn fablet 2019-10-18 06:44:42 PDT
Created attachment 381299 [details]
Win fix
Comment 4 youenn fablet 2019-10-21 05:41:31 PDT
Ping review.

Mac-wk1 tests are not about service workers ( fast/scrolling/latching/scroll-select-bottom-test.html  and  fast/events/resize-subframe-in-rendering-update.html ), so I doubt this is a regression.
Perl bot is failing test262 tests which should not be affected by this patch either.
Comment 5 youenn fablet 2019-10-21 05:42:14 PDT
Also previous patch was passing in WK1 and webkitperl and the changes were #ifdef for WIN compilation.
Comment 6 Chris Dumez 2019-10-21 08:23:37 PDT
Comment on attachment 381299 [details]
Win fix

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

> Source/WebCore/ChangeLog:8
> +        For regular loads, we no longer match service worker registration explicitly.

"explicitly"

> Source/WebCore/ChangeLog:10
> +        We still need to explicitely match registrations in those two cases:

"explicitely"

-> Make up your mind :P

> Source/WebCore/workers/service/server/SWServer.cpp:144
> +    m_importCompletedCallbacks.append(WTFMove(callback));

I think we should ASSERT(!m_importCompleted);

> Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp:191
> +void WebSWClientConnection::documentIsControlled(DocumentIdentifier documentIdentifier, ServiceWorkerRegistrationData&& data, CompletionHandler<void(bool)>&& completionHandler)

The naming is bad IMO. It looks like a getter but is really a setter. I'd suggest something like setDocumentIsControlled() or controlDocument().
Comment 7 Alex Christensen 2019-10-21 15:46:13 PDT
Created attachment 381469 [details]
Patch
Comment 8 Alex Christensen 2019-10-21 18:34:53 PDT
http://trac.webkit.org/r251409
Comment 9 Radar WebKit Bug Importer 2019-10-21 18:35:18 PDT
<rdar://problem/56485381>