Bug 178985 – [Service Workers] Fire updatefound event after resolving the registration promise

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Fire updatefound event after resolving the registration promise and populate ServiceWorkerRegistration.installing to unblock some web-platform-tests.

[Chris Dumez]

Created attachment 325290 [details]
Patch

[Chris Dumez]

Created attachment 325291 [details]
Patch

[Chris Dumez]

Created attachment 325293 [details]
Patch

[Build Bot]

Comment on attachment 325293 [details]
Patch

Attachment 325293 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5033790

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/websocket.https.html

[Build Bot]

Created attachment 325295 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 325297 [details]
Patch

[Chris Dumez]

Created attachment 325299 [details]
Patch

[Chris Dumez]

Created attachment 325303 [details]
Patch

[Build Bot]

Comment on attachment 325303 [details]
Patch

Attachment 325303 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5034825

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/unregister-controller.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/extendable-event-async-waituntil.https.html

[Build Bot]

Created attachment 325308 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 325309 [details]
Patch

[Build Bot]

Comment on attachment 325309 [details]
Patch

Attachment 325309 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5035365

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-frame-resource.https.html

[Build Bot]

Created attachment 325313 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 325318 [details]
Patch

[Chris Dumez]

Created attachment 325376 [details]
Patch

[youenn fablet]

Comment on attachment 325376 [details]
Patch

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

> Source/WebCore/workers/service/ServiceWorker.cpp:41
> +ServiceWorker::ServiceWorker(ScriptExecutionContext& context, uint64_t serviceWorkerIdentifier, const URL& scriptURL)

URL&& probably

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:77
>  ServiceWorker* ServiceWorkerContainer::controller() const

CallWith=ScriptExecutionContext might be better.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:216
> +        context->setActiveServiceWorker(ServiceWorker::create(*context, data.identifier, data.scriptURL));

We should probably not create a ServiceWorker here.
Probably we should get it from the selected ServiceWorkerRegistration that we create below.
I guess the above FIXME might cover this though.
Or maybe some refactoring would get us closer to the end target?

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:225
> +    // a few events to make it look like we are installing and activing the service worker.

Let's hope we can remove that code and FIXME soon.
But end result in terms of testing is great!

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:254
> +        context->setActiveServiceWorker(nullptr);

I would prefer clearActiveServiceWorker and setActiveServiceWorker take a Ref<>. Both might be inlined anyway.

> Source/WebCore/workers/service/ServiceWorkerRegistration.cpp:54
>      return nullptr;

Do we not do it in the other way usually by returning nullptr early?

> Source/WebCore/workers/service/ServiceWorkerRegistration.h:74
> +    RefPtr<ServiceWorker> m_serviceWorker;

Right now I guess it can it be a Ref<> but in the future it may not. Would use a Ref<> though if possible now.

[Chris Dumez]

Comment on attachment 325376 [details]
Patch

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

>> Source/WebCore/workers/service/ServiceWorker.cpp:41
>> +ServiceWorker::ServiceWorker(ScriptExecutionContext& context, uint64_t serviceWorkerIdentifier, const URL& scriptURL)
> 
> URL&& probably

My call site is not able to transfer ownership so it would not be helpful and it would make that call site looks slightly worse.

>> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:77
>>  ServiceWorker* ServiceWorkerContainer::controller() const
> 
> CallWith=ScriptExecutionContext might be better.

This would be a behavior change I believe. I want the script execution context of the ServiceWorkerContainer object, not the one of the caller.

>> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:216
>> +        context->setActiveServiceWorker(ServiceWorker::create(*context, data.identifier, data.scriptURL));
> 
> We should probably not create a ServiceWorker here.
> Probably we should get it from the selected ServiceWorkerRegistration that we create below.
> I guess the above FIXME might cover this though.
> Or maybe some refactoring would get us closer to the end target?

What do you mean? I need to see the selected / active service worker, like the previous code used to. do. It used to be just am identifier, now it is a ServiceWorker object.

>> Source/WebCore/workers/service/ServiceWorkerRegistration.cpp:54
>>      return nullptr;
> 
> Do we not do it in the other way usually by returning nullptr early?

Sure.

>> Source/WebCore/workers/service/ServiceWorkerRegistration.h:74
>> +    RefPtr<ServiceWorker> m_serviceWorker;
> 
> Right now I guess it can it be a Ref<> but in the future it may not. Would use a Ref<> though if possible now.

My thought also. Ok, I'll try Ref<> for now.

[Chris Dumez]

Created attachment 325379 [details]
Patch

[Chris Dumez]

https://bugs.webkit.org/show_bug.cgi?id=179018 will allow us to unskip even more tests.

[Chris Dumez]

Created attachment 325381 [details]
Patch

[Build Bot]

Comment on attachment 325381 [details]
Patch

Attachment 325381 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5043692

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-css-images.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/extendable-event-async-waituntil.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/multiple-register.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/unregister-then-register.https.html

[Build Bot]

Created attachment 325385 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 325389 [details]
Patch

[Chris Dumez]

Created attachment 325403 [details]
Patch

[Chris Dumez]

Created attachment 325409 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 325409 [details]
Patch

Clearing flags on attachment: 325409

Committed r224218: <https://trac.webkit.org/changeset/224218>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

> >> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:77
> >>  ServiceWorker* ServiceWorkerContainer::controller() const
> > 
> > CallWith=ScriptExecutionContext might be better.
> 
> This would be a behavior change I believe. I want the script execution
> context of the ServiceWorkerContainer object, not the one of the caller.

Is there a case where they would not be the same thing?

> >> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:216
> >> +        context->setActiveServiceWorker(ServiceWorker::create(*context, data.identifier, data.scriptURL));
> > 
> > We should probably not create a ServiceWorker here.
> > Probably we should get it from the selected ServiceWorkerRegistration that we create below.
> > I guess the above FIXME might cover this though.
> > Or maybe some refactoring would get us closer to the end target?
> 
> What do you mean? I need to see the selected / active service worker, like
> the previous code used to. do. It used to be just am identifier, now it is a
> ServiceWorker object.

As per the spec, a page selects a registration.
Then, the active worker of the selected registration becomes the page active worker.

It would therefore be closer to spec to do something like either have the selected registration retrieved from the ScriptExecutionContext or the selected registration to set the ServiceWorker of the ScriptExecutionContext when registration state is changing.

[Radar WebKit Bug Importer]

<rdar://problem/35567842>