| Summary: | Remove synchronous termination of service workers | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||
| Component: | Service Workers | Assignee: | youenn fablet <youennf> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | achristensen, cdumez, ggaren, webkit-bug-importer | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Local Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
youenn fablet
2020-03-27 11:06:22 PDT
Created attachment 394890 [details]
Patch
Created attachment 394908 [details]
Patch
Comment on attachment 394908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394908&action=review > Source/WebCore/ChangeLog:8 > + Instead of supporting sycnhronous IPC to terminate a service worker, SWServerWorker will asynchronously ask for the service worker to terminate. Typo: sycnhronous > Source/WebCore/workers/service/WorkerSWClientConnection.cpp:161 > + notImplemented(); What's this? > Source/WebCore/workers/service/server/SWServerWorker.cpp:126 > +void SWServerWorker::terminateCompleted() startTermination -> terminationCompleted would be more consistent. > > Source/WebCore/workers/service/WorkerSWClientConnection.cpp:161
> > + notImplemented();
>
> What's this?
We are not calling terminate from a worker but only from a document and for testing purposes.
Plan is to add support for it should we need to call this for testing from workers.
(In reply to youenn fablet from comment #5) > > > Source/WebCore/workers/service/WorkerSWClientConnection.cpp:161 > > > + notImplemented(); > > > > What's this? > > We are not calling terminate from a worker but only from a document and for > testing purposes. > Plan is to add support for it should we need to call this for testing from > workers. It looks like something that used to work and no longer does, aka a regression. How hard would it be to keep it working? (In reply to Chris Dumez from comment #6) > (In reply to youenn fablet from comment #5) > > > > Source/WebCore/workers/service/WorkerSWClientConnection.cpp:161 > > > > + notImplemented(); > > > > > > What's this? > > > > We are not calling terminate from a worker but only from a document and for > > testing purposes. > > Plan is to add support for it should we need to call this for testing from > > workers. > > It looks like something that used to work and no longer does, aka a > regression. How hard would it be to keep it working? We could and it would be fairly simple (although we would add a Vector<callback>) but I do not think this is worth it. Another option is to move this API to ServiceWorkerProvider so that only Documents would do that. I would tend to go this way in that case. Comment on attachment 394908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394908&action=review >> Source/WebCore/workers/service/WorkerSWClientConnection.cpp:161 >> + notImplemented(); > > What's this? This looks bad. I think we should either make it work or drop this code (assuming it is not currently useful). Going from something that works to something notImplemented() does not seem like the right thing to do. Created attachment 394924 [details]
Patch
Ping review. Comment on attachment 394924 [details]
Patch
r=me
Patch 394924 does not build Created attachment 395256 [details]
Patch for landing
Committed r259383: <https://trac.webkit.org/changeset/259383> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395256 [details]. |