Bug 179270 – Implement ServiceWorkerRegistration.update()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Implement ServiceWorkerRegistration.update():
- https://w3c.github.io/ServiceWorker/#service-worker-registration-update

[Chris Dumez]

Created attachment 325978 [details]
WIP Patch

[Chris Dumez]

Created attachment 326005 [details]
Patch

[Chris Dumez]

Created attachment 326009 [details]
Patch

[Chris Dumez]

Created attachment 326011 [details]
Patch

[youenn fablet]

Comment on attachment 326011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326011&action=review

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:171
> +    if (!context || !context->sessionID().isValid()) {

I wonder why we check for context->sessionID() in ServiceWorkerContainer but not in other classes like ServiceWorkerRegistration.
If sessionID is not valid, this should probably error in some code below where the sessionID is actually used, no?
Maybe we can remove that check here.

Also, updateRegistration is called by ServiceWorkerRegistration::update which is also checking for its scriptExecutionContext.
Maybe we should pass the necessary parameters to ServiceWorkerContainer::updateRegistration so that it does not require to use its own context.
That may mean more or less directly exposing scheduleJob.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:235
> +            // sure that ServiceWorkerRegistration objects stays alive as long as their SWServerRegistration on server side.

Agreed, lifetime of the registration should be governed by the StorageProcess basically.
Also, currently ServiceWorkerContainer is tied to navigator which may not always be right, since a service worker client predates it.
Service worker clients should probably have a ServiceWorkerRegistration and not only a ServiceWorker.
Let's continue refining the model in future patches.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:282
> +                registration->waiting()->setState(ServiceWorker::State::Installed);

I know this is code that will go away at some point.
It seems though like we could just do a single function call that would do all these state changes at once.
Might depend on how much this would diverge from matching the spec line by line.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:286
> +                    registration->active()->setState(ServiceWorker::State::Activating);

Ditto.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:311
> +    RefPtr<ServiceWorkerRegistration> registration = m_registrations.get(data.key);

Could be moved closer to registration first use.

> Source/WebCore/workers/service/ServiceWorkerJob.cpp:107
> +        promise().reject(Exception { SecurityError, ASCIILiteral("MIME Type is not a JavaScript MIME type") });

ServiceWorkerJob model is more something like "store the promise and let my client reject/resolve it".
Can we try keeping that model? Otherwise, we would ideally be resolving the promise and unrefing it.
Would something like calling jobFailedLoadingScript and then jobFailedWithException work?

> Source/WebCore/workers/service/ServiceWorkerRegistration.cpp:90
> +    return m_activeWorker.get();

Seems like a good idea to introduce these 3 workers so as to match the spec more easily.

> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:72
> +        finishCurrentJob();

I know this is the style of SWServerJobQueue but the code is done in a way that makes these comments unnecessary (which is good!).
Ditto below. Since we are trying to make our implementation match the spec line by line more or less, do we need these comments?

> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:240
>      auto* newestWorker = registration->getNewestWorker();

If newest worker is null, we are probably in a bad state.
Should we debug assert it?

[Chris Dumez]

Comment on attachment 326011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326011&action=review

>> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:235
>> +            // sure that ServiceWorkerRegistration objects stays alive as long as their SWServerRegistration on server side.
> 
> Agreed, lifetime of the registration should be governed by the StorageProcess basically.
> Also, currently ServiceWorkerContainer is tied to navigator which may not always be right, since a service worker client predates it.
> Service worker clients should probably have a ServiceWorkerRegistration and not only a ServiceWorker.
> Let's continue refining the model in future patches.

YEs, we may want to simply expose scheduleJob() at some point.

>> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:282
>> +                registration->waiting()->setState(ServiceWorker::State::Installed);
> 
> I know this is code that will go away at some point.
> It seems though like we could just do a single function call that would do all these state changes at once.
> Might depend on how much this would diverge from matching the spec line by line.

I think Brady is about to implement this properly so I did not worry too much about simplifying the code. In the spec, the update registration state and update worker state steps are separate so I think the current code matches the spec more closely for now.

>> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:311
>> +    RefPtr<ServiceWorkerRegistration> registration = m_registrations.get(data.key);
> 
> Could be moved closer to registration first use.

ok.

>> Source/WebCore/workers/service/ServiceWorkerJob.cpp:107
>> +        promise().reject(Exception { SecurityError, ASCIILiteral("MIME Type is not a JavaScript MIME type") });
> 
> ServiceWorkerJob model is more something like "store the promise and let my client reject/resolve it".
> Can we try keeping that model? Otherwise, we would ideally be resolving the promise and unrefing it.
> Would something like calling jobFailedLoadingScript and then jobFailedWithException work?

We'd need to pass an exception instead of a ResourceError. I'll see if it is a trivial change to make.

>> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:240
>>      auto* newestWorker = registration->getNewestWorker();
> 
> If newest worker is null, we are probably in a bad state.
> Should we debug assert it?

Not yet. getNewestWorker() always returns null on server side currently. I believe Brady is working on this.

[Chris Dumez]

Created attachment 326071 [details]
Patch

[Chris Dumez]

Created attachment 326072 [details]
Patch

[Chris Dumez]

Comment on attachment 326072 [details]
Patch

Clearing flags on attachment: 326072

Committed r224469: <https://trac.webkit.org/changeset/224469>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35567176>