Bug 180360 – Support container.getRegistration() / getRegistrations() inside service workers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Support container.getRegistration() / getRegistrations() inside service workers.

[Chris Dumez]

Created attachment 328367 [details]
WIP Patch

[Chris Dumez]

Created attachment 328373 [details]
WIP Patch

[Chris Dumez]

Created attachment 328374 [details]
WIP Patch

[Chris Dumez]

Created attachment 328379 [details]
WIP Patch

[EWS Watchlist]

Comment on attachment 328379 [details]
WIP Patch

Attachment 328379 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5491816

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/skip-waiting-without-using-registration.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/activation.https.html

[EWS Watchlist]

Created attachment 328389 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 328379 [details]
WIP Patch

Attachment 328379 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5491833

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/skip-waiting-without-using-registration.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/activation.https.html

[EWS Watchlist]

Created attachment 328391 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 328395 [details]
Patch

[Chris Dumez]

Created attachment 328396 [details]
Patch

[Chris Dumez]

Created attachment 328397 [details]
Patch

[Chris Dumez]

Comment on attachment 328397 [details]
Patch

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

> LayoutTests/imported/w3c/ChangeLog:16
> +        a cleanup step to unregister. The test now passes all checks.

PR: https://github.com/w3c/web-platform-tests/pull/8559

[youenn fablet]

Comment on attachment 328397 [details]
Patch

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

> Source/WebCore/dom/ScriptExecutionContext.cpp:588
> +void ScriptExecutionContext::postTaskTo(const DocumentOrWorkerIdentifier& contextIdentifier, WTF::Function<void(ScriptExecutionContext&)>&& task)

I am always wondering whether to use Identifier or const Identifier& in such cases.
I have difficulties keeping it consistent. Since it is a Variant, it is better to pass it by ref?

> Source/WebCore/dom/ScriptExecutionContext.cpp:592
> +    switchOn(contextIdentifier, [&](DocumentIdentifier identifier) {

space between ] and ( is apparently our style.

> Source/WebCore/dom/ScriptExecutionContext.cpp:596
> +        document->postTask([task = WTFMove(task)](ScriptExecutionContext& scope) {

auto& here and in other various places below ?

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:232
> +    Ref<SecurityOrigin> topOrigin = context->topOrigin();

This may end up topOrigin to be destroyed on the main thread. Shouldn't we isolate it?
Ditto below probably.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:320
> +    }

You can probably write it as:
auto registrations = WTF::map(WTFMove(registrations), [this] (auto&& registrationData) {
    return ServiceWorkerRegistration::getOrCreate(*scriptExecutionContext(), *this, WTFMove(registrationData));
});

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:545
> +    m_pendingGetRegistrationsRequests.clear();

I would try to use one map that keeps DeferredPromise instead of typed DOMPromiseDeferred.

> Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp:140
> +    ASSERT(isMainThread());

We do not do that often when receiving IPC messages.
Should we try to do that more aggressively? Or should these assertions be moved to the lambdas instead?

[Chris Dumez]

Comment on attachment 328397 [details]
Patch

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

>> Source/WebCore/dom/ScriptExecutionContext.cpp:588
>> +void ScriptExecutionContext::postTaskTo(const DocumentOrWorkerIdentifier& contextIdentifier, WTF::Function<void(ScriptExecutionContext&)>&& task)
> 
> I am always wondering whether to use Identifier or const Identifier& in such cases.
> I have difficulties keeping it consistent. Since it is a Variant, it is better to pass it by ref?

I am not sure in the case of a Variant so I figured better safe than sorry.

>> Source/WebCore/dom/ScriptExecutionContext.cpp:592
>> +    switchOn(contextIdentifier, [&](DocumentIdentifier identifier) {
> 
> space between ] and ( is apparently our style.

Uh? ok.

>> Source/WebCore/dom/ScriptExecutionContext.cpp:596
>> +        document->postTask([task = WTFMove(task)](ScriptExecutionContext& scope) {
> 
> auto& here and in other various places below ?

ok.

>> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:232
>> +    Ref<SecurityOrigin> topOrigin = context->topOrigin();
> 
> This may end up topOrigin to be destroyed on the main thread. Shouldn't we isolate it?
> Ditto below probably.

Will isolateCopy().

>> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:320
>> +    }
> 
> You can probably write it as:
> auto registrations = WTF::map(WTFMove(registrations), [this] (auto&& registrationData) {
>     return ServiceWorkerRegistration::getOrCreate(*scriptExecutionContext(), *this, WTFMove(registrationData));
> });

Ok, wasn't familiar with WTF::map()

>> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:545
>> +    m_pendingGetRegistrationsRequests.clear();
> 
> I would try to use one map that keeps DeferredPromise instead of typed DOMPromiseDeferred.

Will look into this.

[Chris Dumez]

Created attachment 328409 [details]
Patch

[youenn fablet]

Comment on attachment 328409 [details]
Patch

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

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:292
> +    Ref<SecurityOrigin> topOrigin = context->topOrigin();

No need for this variable and refing topOrigin probably as well.

[Chris Dumez]

Created attachment 328411 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 328411 [details]
Patch

Rejecting attachment 328411 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 328411, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
..99b2274  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 225506 = 6dd634539ab447fd1507bbe274df2c73431ab7b7
r225507 = 99b227420d9a3b88cc44f866efee9ec3cb1c6da1
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Total errors found: 0 in 0 files

Full output: http://webkit-queues.webkit.org/results/5494450

[Chris Dumez]

Comment on attachment 328411 [details]
Patch

Clearing flags on attachment: 328411

Committed r225513: <https://trac.webkit.org/changeset/225513>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35846638>