Fire updatefound event after resolving the registration promise and populate ServiceWorkerRegistration.installing to unblock some web-platform-tests.
Created attachment 325290 [details] Patch
Created attachment 325291 [details] Patch
Created attachment 325293 [details] Patch
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
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
Created attachment 325297 [details] Patch
Created attachment 325299 [details] Patch
Created attachment 325303 [details] Patch
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
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
Created attachment 325309 [details] Patch
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
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
Created attachment 325318 [details] Patch
Created attachment 325376 [details] Patch
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.
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.
Created attachment 325379 [details] Patch
https://bugs.webkit.org/show_bug.cgi?id=179018 will allow us to unskip even more tests.
Created attachment 325381 [details] Patch
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
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
Created attachment 325389 [details] Patch
Created attachment 325403 [details] Patch
Created attachment 325409 [details] Patch
Comment on attachment 325409 [details] Patch Clearing flags on attachment: 325409 Committed r224218: <https://trac.webkit.org/changeset/224218>
All reviewed patches have been landed. Closing bug.
> >> 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.
<rdar://problem/35567842>