Bug 179302 - [Service Workers] Add proper implementation for 'updatefound' event
Summary: [Service Workers] Add proper implementation for 'updatefound' 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, WebExposed
Depends on:
Blocks:
 
Reported: 2017-11-05 14:34 PST by Chris Dumez
Modified: 2017-11-15 12:13 PST (History)
9 users (show)

See Also:


Attachments
Patch (17.67 KB, patch)
2017-11-05 15:11 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.19 MB, application/zip)
2017-11-05 16:13 PST, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (1014.65 KB, application/zip)
2017-11-05 16:34 PST, Build Bot
no flags Details
Patch (19.05 KB, patch)
2017-11-05 18:00 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (18.44 KB, patch)
2017-11-06 09:48 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (17.01 KB, patch)
2017-11-06 12:21 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-05 14:34:17 PST
Add proper implementation for 'updatefound' event:
- https://w3c.github.io/ServiceWorker/#install (step 7)
Comment 1 Chris Dumez 2017-11-05 15:11:28 PST
Created attachment 326078 [details]
Patch
Comment 2 Build Bot 2017-11-05 16:13:19 PST
Comment on attachment 326078 [details]
Patch

Attachment 326078 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5116687

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/multiple-register.https.html
Comment 3 Build Bot 2017-11-05 16:13:20 PST
Created attachment 326086 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 4 Build Bot 2017-11-05 16:34:43 PST
Comment on attachment 326078 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/multiple-register.https.html
Comment 5 Build Bot 2017-11-05 16:34:45 PST
Created attachment 326087 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 6 Chris Dumez 2017-11-05 18:00:02 PST
Created attachment 326093 [details]
Patch
Comment 7 Brady Eidson 2017-11-06 07:20:01 PST
I have this locally... along with a lot of other update/install related stuff.

The reason it hasn't landed is that it's impossible to reconcile the real stuff with the fake microtask

While your patch doesn't change test results, that's because it doesn't do other "real" stuff too"

Let's sync up this morning instead of landing this, please.
Comment 8 Brady Eidson 2017-11-06 07:23:06 PST
(In reply to Brady Eidson from comment #7)
> I have this locally... along with a lot of other update/install related
> stuff.
> 
> The reason it hasn't landed is that it's impossible to reconcile the real
> stuff with the fake microtask
> 
> While your patch doesn't change test results, that's because it doesn't do
> other "real" stuff too"
> 
> Let's sync up this morning instead of landing this, please.

Note - The other stuff I have is most of the rest of "Install", which is literally the only place where updatefound is fired, so let's reconcile all of Install first.
Comment 9 Brady Eidson 2017-11-06 07:29:55 PST
(In reply to Brady Eidson from comment #8)
> (In reply to Brady Eidson from comment #7)
> > I have this locally... along with a lot of other update/install related
> > stuff.
> > 
> > The reason it hasn't landed is that it's impossible to reconcile the real
> > stuff with the fake microtask
> > 
> > While your patch doesn't change test results, that's because it doesn't do
> > other "real" stuff too"
> > 
> > Let's sync up this morning instead of landing this, please.
> 
> Note - The other stuff I have is most of the rest of "Install", which is
> literally the only place where updatefound is fired, so let's reconcile all
> of Install first.

https://bugs.webkit.org/show_bug.cgi?id=179318 for reference.
Comment 10 Chris Dumez 2017-11-06 09:01:33 PST
This focuses on updatefound event, nothing else, as discussed during our previous sync meeting. It applies and passes all the tests. As far as I can tell, this is a step in the right direction. Not sure why we cannot land it.

It does not implement the update service worker state / update registration state algorithms which you were working on.
Comment 11 Chris Dumez 2017-11-06 09:42:24 PST
(In reply to Chris Dumez from comment #10)
> This focuses on updatefound event, nothing else, as discussed during our
> previous sync meeting. It applies and passes all the tests. As far as I can
> tell, this is a step in the right direction. Not sure why we cannot land it.
> 
> It does not implement the update service worker state / update registration
> state algorithms which you were working on.

As far as I can tell, I did not take a very different direction than you so I do not see the problem. I also add the install() function where you can add your other steps.

The difference between my patch and your are mostly that:
1. Mine if case of Youenn's changes that landed last Friday. You'd likely have to apply the same changes to your patch anyway. Not sure this is better than landing my patch and rebasing.
2. You are doing more things (update worker / registration state), which I expected.
Comment 12 Chris Dumez 2017-11-06 09:48:37 PST
Created attachment 326127 [details]
Patch
Comment 13 Chris Dumez 2017-11-06 12:21:03 PST
Created attachment 326149 [details]
Patch
Comment 14 Chris Dumez 2017-11-06 12:22:16 PST
Comment on attachment 326149 [details]
Patch

Clearing flags on attachment: 326149

Committed r224506: <https://trac.webkit.org/changeset/224506>
Comment 15 Chris Dumez 2017-11-06 12:22:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2017-11-15 12:13:29 PST
<rdar://problem/35567107>