Bug 209666 – Remove synchronous termination of service workers

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

Remove synchronous termination of service workers

[youenn fablet]

Created attachment 394890 [details]
Patch

[youenn fablet]

Created attachment 394908 [details]
Patch

[youenn fablet]

<rdar://problem/59318239>

[Chris Dumez]

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.

[youenn fablet]

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

[Chris Dumez]

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

[youenn fablet]

(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.

[Chris Dumez]

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.

[youenn fablet]

Created attachment 394924 [details]
Patch

[youenn fablet]

Ping review.

[Chris Dumez]

Comment on attachment 394924 [details]
Patch

r=me

[EWS]

Patch 394924 does not build

[youenn fablet]

Created attachment 395256 [details]
Patch for landing

[EWS]

Committed r259383: <https://trac.webkit.org/changeset/259383>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395256 [details].