[webkit-reviews] review granted: [Bug 179791] [Service Worker] Implement "Try Clear Registration" algorithm : [Attachment 327109] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Nov 16 15:51:22 PST 2017
youenn fablet <youennf at gmail.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 179791: [Service Worker] Implement "Try Clear Registration" algorithm
https://bugs.webkit.org/show_bug.cgi?id=179791
Attachment 327109: Patch
https://bugs.webkit.org/attachment.cgi?id=327109&action=review
--- Comment #5 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 327109
--> https://bugs.webkit.org/attachment.cgi?id=327109
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=327109&action=review
> Source/WebCore/dom/ScriptExecutionContext.cpp:550
> + uint64_t contextIdentifier = is<Document>(*this) ?
downcast<Document>(*this).identifier() : 0;
It might be better to exit early if we are not a Document.
I am not sure calling ServiceWorkerProvider::singleton() will work properly on
worker thread.
> Source/WebCore/workers/service/server/SWClientConnection.h:75
> + virtual void
serviceWorkerStartedControllingClient(ServiceWorkerIdentifier, uint64_t
scriptExecutionContextIdentifier) = 0;
Would be nice to have a ScriptExcutionContextIdentifier at some point :)
> Source/WebCore/workers/service/server/SWServer.cpp:333
> + auto* registration =
m_registrations.get(serviceWorker->registrationKey());
Are there cases where we have a worker but no longer a registration?
> Source/WebCore/workers/service/server/SWServerRegistration.cpp:156
> + auto it =
m_clientsUsingRegistration.find(clientIdentifier.serverConnectionIdentifier);
iterator
> Source/WebCore/workers/service/server/SWServerRegistration.cpp:158
> + return;
Should we assert that it is not the end?
> Source/WebCore/workers/service/server/SWServerRegistration.cpp:160
> + it->value.remove(clientIdentifier.scriptExecutionContextIdentifier);
Should we assert remove returned true?
We could also do below test only if remove returned true.
>
LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/clie
nts-matchall-client-types.https-expected.txt:4
> +FAIL Verify matchAll() with {window, sharedworker, worker} client types
promise_test: Unhandled rejection with value: object "Error: wait_for_state
must be passed a ServiceWorker"
Test probably flaky or maybe regressing?
More information about the webkit-reviews
mailing list