[webkit-reviews] review granted: [Bug 179338] [Service Workers] Add support for "install" event : [Attachment 326216] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 179338: [Service Workers] Add support for "install" event
https://bugs.webkit.org/show_bug.cgi?id=179338

Attachment 326216: Patch

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




--- Comment #17 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 326216
  --> https://bugs.webkit.org/attachment.cgi?id=326216
Patch

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

> Source/WebCore/workers/service/InstallEvent.cpp:36
> +    : ExtendableEvent(eventNames().installEvent, false, false)

Maybe we should make ExtendableEvent constructor have default values for the
last two parameters?

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:276
> +	  
registration->dispatchEvent(Event::create(eventNames().updatefoundEvent, false,
false));

What if ServiceWorkerContainer is stopped at that point. Should we refrain from
dispatching the event?

> Source/WebCore/workers/service/server/SWClientConnection.cpp:161
> +    // FIXME: We should iterate over all service worker clients, not only
documents.

We have this FIXME elsewhere.
Maybe we should introduce something like SWClientConnection::iterateClients and
pass it a lambda so that we have just one FIXME?

>
LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/fore
ign-fetch-event.https-expected.txt:12
> +FAIL ForeignFetchEvent constructor with all init dict members Can't find
variable: ForeignFetchEvent

We should just skip this test for now