Bug 179338 - [Service Workers] Add support for "install" event
Summary: [Service Workers] Add support for "install" event
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-06 12:46 PST by Chris Dumez
Modified: 2017-11-15 12:10 PST (History)
5 users (show)

See Also:


Attachments
WIP Patch (29.01 KB, patch)
2017-11-06 14:05 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (37.52 KB, patch)
2017-11-06 16:51 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (38.18 KB, patch)
2017-11-06 16:55 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (55.12 KB, patch)
2017-11-06 19:24 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (56.50 KB, patch)
2017-11-06 19:44 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (56.30 KB, patch)
2017-11-06 19:55 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (56.50 KB, patch)
2017-11-06 20:00 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (61.93 KB, patch)
2017-11-06 22:07 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-elcapitan (1.79 MB, application/zip)
2017-11-06 23:26 PST, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (1.08 MB, application/zip)
2017-11-07 06:39 PST, Build Bot
no flags Details
Patch (66.06 KB, patch)
2017-11-07 09:45 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (54.72 KB, patch)
2017-11-07 12:07 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-11-06 12:46:15 PST
Add support for "install" event in service workers.
Comment 1 Chris Dumez 2017-11-06 14:05:28 PST
Created attachment 326156 [details]
WIP Patch
Comment 2 Chris Dumez 2017-11-06 16:51:11 PST
Created attachment 326170 [details]
WIP Patch
Comment 3 Chris Dumez 2017-11-06 16:55:50 PST
Created attachment 326171 [details]
WIP Patch
Comment 4 Chris Dumez 2017-11-06 19:24:21 PST
Created attachment 326182 [details]
Patch
Comment 5 Chris Dumez 2017-11-06 19:44:55 PST
Created attachment 326185 [details]
Patch
Comment 6 Chris Dumez 2017-11-06 19:55:58 PST
Created attachment 326188 [details]
Patch
Comment 7 Chris Dumez 2017-11-06 20:00:43 PST
Created attachment 326189 [details]
Patch
Comment 8 Brady Eidson 2017-11-06 22:01:38 PST
Comment on attachment 326189 [details]
Patch

At a glance on iPhone where I can never effectively review patches, you have some non ascii characters
Comment 9 Chris Dumez 2017-11-06 22:07:09 PST
Created attachment 326193 [details]
Patch
Comment 10 Build Bot 2017-11-06 23:26:28 PST
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
Comment 11 Build Bot 2017-11-06 23:26:29 PST
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
Comment 12 Build Bot 2017-11-07 06:38:58 PST
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
Comment 13 Build Bot 2017-11-07 06:39:00 PST
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
Comment 14 Chris Dumez 2017-11-07 09:45:05 PST
Created attachment 326216 [details]
Patch
Comment 15 Brady Eidson 2017-11-07 09:57:41 PST
Comment on attachment 326216 [details]
Patch

This all seems fine once EWS is happy
Comment 16 Chris Dumez 2017-11-07 09:58:18 PST
(In reply to Brady Eidson from comment #15)
> Comment on attachment 326216 [details]
> Patch
> 
> This all seems fine once EWS is happy

r=you?
Comment 17 youenn fablet 2017-11-07 11:42:03 PST
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
Comment 18 Chris Dumez 2017-11-07 11:47:57 PST
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.
Comment 19 Chris Dumez 2017-11-07 11:55:48 PST
(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.
Comment 20 Chris Dumez 2017-11-07 12:07:46 PST
Created attachment 326235 [details]
Patch
Comment 21 Chris Dumez 2017-11-07 12:12:03 PST
Comment on attachment 326235 [details]
Patch

Clearing flags on attachment: 326235

Committed r224542: <https://trac.webkit.org/changeset/224542>
Comment 22 Chris Dumez 2017-11-07 12:12:05 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Radar WebKit Bug Importer 2017-11-15 12:10:09 PST
<rdar://problem/35567067>