[webkit-reviews] review granted: [Bug 203144] Move service worker registration matching for navigation loads to network process : [Attachment 381299] Win fix

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 203144: Move service worker registration matching for navigation loads to
network process
https://bugs.webkit.org/show_bug.cgi?id=203144

Attachment 381299: Win fix

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




--- Comment #6 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 381299
  --> https://bugs.webkit.org/attachment.cgi?id=381299
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().