Bug 179338 – [Service Workers] Add support for "install" event

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Add support for "install" event in service workers.

[Chris Dumez]

Created attachment 326156 [details]
WIP Patch

[Chris Dumez]

Created attachment 326170 [details]
WIP Patch

[Chris Dumez]

Created attachment 326171 [details]
WIP Patch

[Chris Dumez]

Created attachment 326182 [details]
Patch

[Chris Dumez]

Created attachment 326185 [details]
Patch

[Chris Dumez]

Created attachment 326188 [details]
Patch

[Chris Dumez]

Created attachment 326189 [details]
Patch

[Brady Eidson]

Comment on attachment 326189 [details]
Patch

At a glance on iPhone where I can never effectively review patches, you have some non ascii characters

[Chris Dumez]

Created attachment 326193 [details]
Patch

[Build Bot]

Comment on attachment 326193 [details]
Patch

Attachment 326193 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5131229

New failing tests:
http/tests/loading/preload-picture-type.html

[Build Bot]

Created attachment 326197 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 326193 [details]
Patch

Attachment 326193 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5134002

New failing tests:
imported/w3c/web-platform-tests/streams/readable-byte-streams/brand-checks.serviceworker.https.html
imported/w3c/web-platform-tests/streams/readable-streams/general.serviceworker.https.html

[Build Bot]

Created attachment 326207 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 326216 [details]
Patch

[Brady Eidson]

Comment on attachment 326216 [details]
Patch

This all seems fine once EWS is happy

[Chris Dumez]

(In reply to Brady Eidson from comment #15)
> Comment on attachment 326216 [details]
> Patch
> 
> This all seems fine once EWS is happy

r=you?

[youenn fablet]

Comment on attachment 326216 [details]
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/foreign-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

[Chris Dumez]

Comment on attachment 326216 [details]
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?

This matches the pattern in Event:
Event(const AtomicString& type, bool canBubble, bool cancelable);

>> 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?

I'll add the check to be safe. I was hoping scriptExecutionContext would not post the task in such case but looking at the code, it does not look like it.

>> 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?

Ok.

[Chris Dumez]

(In reply to Chris Dumez from comment #18)
> Comment on attachment 326216 [details]
> 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?
> 
> This matches the pattern in Event:
> Event(const AtomicString& type, bool canBubble, bool cancelable);
> 
> >> 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?
> 
> I'll add the check to be safe. I was hoping scriptExecutionContext would not
> post the task in such case but looking at the code, it does not look like it.
> 
> >> 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?
> 
> Ok.

Brady says we should drop the InstallEvent idl/class before landing as it is about to be removed from the spec.

[Chris Dumez]

Created attachment 326235 [details]
Patch

[Chris Dumez]

Comment on attachment 326235 [details]
Patch

Clearing flags on attachment: 326235

Committed r224542: <https://trac.webkit.org/changeset/224542>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35567067>