[webkit-reviews] review denied: [Bug 179480] matchRegistration does not need to go to StorageProcess if no service worker is registered : [Attachment 326765] 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 179480: matchRegistration does not need to go to StorageProcess if no
service worker is registered
https://bugs.webkit.org/show_bug.cgi?id=179480

Attachment 326765: Patch

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




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

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

> Source/WebCore/workers/service/server/SWServer.cpp:87
> +    if (!m_registrations.set(key, WTFMove(registration)).isNewEntry)

How can it not be a new registration? I think this should be an assertion, not
an if check.

> Source/WebCore/workers/service/server/SWServer.cpp:91
> +    ++m_originCounts.ensure(topOrigin->toString(), [&] {

Can we move the originCount to the ServiceWorkerOriginStore for better
encapsulation? The server should not have to deal with counts.

> Source/WebCore/workers/service/server/SWServer.cpp:100
> +    if (!m_registrations.remove(key))

I don't think this should happen. Should probably be an assertion instead of an
if check.

> Source/WebCore/workers/service/server/SWServer.cpp:120
> +    m_originStore->clear();

I think moving the originStore to SWServer is indeed a good idea.

> LayoutTests/imported/w3c/ChangeLog:8
> +	   Updating tests as otherwise they would fail trying to access to
document.body which would be null.

How is this related to your code change? You code change is not supposed to
cause web-facing behavior change AFAICT. Therefore, no test update should be
needed.