[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