Bug 179270 - Implement ServiceWorkerRegistration.update()
Summary: Implement ServiceWorkerRegistration.update()
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: https://w3c.github.io/ServiceWorker/#...
Keywords: InRadar
Depends on: 179253
Blocks:
  Show dependency treegraph
 
Reported: 2017-11-03 15:19 PDT by Chris Dumez
Modified: 2017-11-15 12:14 PST (History)
4 users (show)

See Also:


Attachments
WIP Patch (33.00 KB, patch)
2017-11-03 15:50 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (36.96 KB, patch)
2017-11-03 19:58 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (38.41 KB, patch)
2017-11-03 20:40 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (38.94 KB, patch)
2017-11-03 20:53 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (40.64 KB, patch)
2017-11-05 12:00 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (41.12 KB, patch)
2017-11-05 12:01 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-03 15:19:42 PDT
Implement ServiceWorkerRegistration.update():
- https://w3c.github.io/ServiceWorker/#service-worker-registration-update
Comment 1 Chris Dumez 2017-11-03 15:50:15 PDT
Created attachment 325978 [details]
WIP Patch
Comment 2 Chris Dumez 2017-11-03 19:58:05 PDT
Created attachment 326005 [details]
Patch
Comment 3 Chris Dumez 2017-11-03 20:40:02 PDT
Created attachment 326009 [details]
Patch
Comment 4 Chris Dumez 2017-11-03 20:53:15 PDT
Created attachment 326011 [details]
Patch
Comment 5 youenn fablet 2017-11-05 11:39:19 PST
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?
Comment 6 Chris Dumez 2017-11-05 11:52:17 PST
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.
Comment 7 Chris Dumez 2017-11-05 12:00:16 PST
Created attachment 326071 [details]
Patch
Comment 8 Chris Dumez 2017-11-05 12:01:35 PST
Created attachment 326072 [details]
Patch
Comment 9 Chris Dumez 2017-11-05 12:18:23 PST
Comment on attachment 326072 [details]
Patch

Clearing flags on attachment: 326072

Committed r224469: <https://trac.webkit.org/changeset/224469>
Comment 10 Chris Dumez 2017-11-05 12:18:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2017-11-15 12:14:10 PST
<rdar://problem/35567176>