[webkit-reviews] review granted: [Bug 179270] Implement ServiceWorkerRegistration.update() : [Attachment 326011] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 179270: Implement ServiceWorkerRegistration.update()
https://bugs.webkit.org/show_bug.cgi?id=179270

Attachment 326011: Patch

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




--- Comment #5 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 326011
  --> https://bugs.webkit.org/attachment.cgi?id=326011
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?