[webkit-reviews] review denied: [Bug 178882] Implement Service Worker Matching Registration algorithm : [Attachment 325460] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied youenn fablet <youennf at gmail.com>'s
request for review:
Bug 178882: Implement Service Worker Matching Registration algorithm
https://bugs.webkit.org/show_bug.cgi?id=178882

Attachment 325460: Patch

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




--- Comment #23 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 325460
  --> https://bugs.webkit.org/attachment.cgi?id=325460
Patch

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

> Source/WebCore/workers/service/server/SWServer.cpp:87
> +void SWServer::lastTaskBeforeClearing()

Do we really need method for this. Cannot we merely use a lambda to construct
the task? This would solve the fact that I don't like this method name :P

> Source/WebCore/workers/service/server/SWServer.cpp:230
> +    if (m_isClearing)

This looks wrong. If we:
1. Clear()
2. Append a RegisterTask

I would expected the registration to succeed but in your case, it will be
ignored.

> Source/WebCore/workers/service/server/SWServer.cpp:288
> +    return selectedRegistration &&
!selectedRegistration->isUninstallingFlagSet() ? selectedRegistration :
nullptr;

As mentioned in an earlier iteration, we should make
SWServerRegistration::isEmpty() public and make sure we do not return an empty
registration. An empty registration in our implementation is equivalent to a
null registration in the spec.

Also, I think we should do the uninstalling flag and isEmpty() checks inside
the loop to skip them. Otherwise, we may overlook a registration that matches
but has a shorter URL than a registration that is uninstalled.