Bug 179396

Created attachment 326273 [details] WIP Patch

Comment on attachment 326273 [details] WIP Patch Attachment 326273 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5140403 New failing tests: imported/w3c/web-platform-tests/service-workers/service-worker/windowclient-navigate.https.html imported/w3c/web-platform-tests/service-workers/service-worker/oninstall-script-error.https.html imported/w3c/web-platform-tests/service-workers/service-worker/register-wait-forever-in-install-worker.https.html

Created attachment 326275 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 326273 [details] WIP Patch Attachment 326273 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5140439 New failing tests: imported/w3c/web-platform-tests/streams/readable-streams/count-queuing-strategy-integration.serviceworker.https.html imported/w3c/web-platform-tests/service-workers/service-worker/oninstall-script-error.https.html imported/w3c/web-platform-tests/service-workers/service-worker/register-wait-forever-in-install-worker.https.html imported/w3c/web-platform-tests/service-workers/service-worker/windowclient-navigate.https.html

Created attachment 326279 [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 326283 [details] WIP Patch

Created attachment 326288 [details] WIP Patch

Attachment 326288 [details] did not pass style-queue: ERROR: Source/WebCore/workers/service/ServiceWorkerContainer.h:75: The parameter name "registration" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/workers/service/ServiceWorkerContainer.h:76: The parameter name "registration" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 326289 [details] WIP Patch

Created attachment 326290 [details] WIP Patch

Created attachment 326292 [details] WIP Patch

Created attachment 326294 [details] WIP Patch

Created attachment 326295 [details] Patch

Created attachment 326296 [details] Patch

Comment on attachment 326296 [details] Patch Clearing flags on attachment: 326296 Committed r224584: <https://trac.webkit.org/changeset/224584>

All reviewed patches have been landed. Closing bug.

Comment on attachment 326296 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326296&action=review > Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:126 > + installEvent->whenAllExtendLifetimePromisesAreSettled([serviceWorkerIdentifier](HashSet<Ref<DOMPromise>>&& extendLifetimePromises) { Removing a promise from the hashset as soon as promise is settled seems simpler and is more efficient. I am not sure why we want to keep references for all promises. In the particular install case, we could instead have a callback taking an allResolved/rejected flag. Would that work with all other cases?

Ah I arrived too late there... If the callback(allresolved/rejected) works, I would much prefer this model.

(In reply to youenn fablet from comment #18) > Ah I arrived too late there... > If the callback(allresolved/rejected) works, I would much prefer this model. We can simplify later on. The current design is very flexible since it provides all the promises to the caller. This also matches the spec very closely. I do not really buy the 'more efficient' argument since I do not have to remove anything from the HashSet and I only keep the promises alive until I call the "all promises resolved" callback.

(In reply to Chris Dumez from comment #19) > (In reply to youenn fablet from comment #18) > > Ah I arrived too late there... > > If the callback(allresolved/rejected) works, I would much prefer this model. > > We can simplify later on. The current design is very flexible since it > provides all the promises to the caller. This also matches the spec very > closely. > > I do not really buy the 'more efficient' argument since I do not have to > remove anything from the HashSet and I only keep the promises alive until I > call the "all promises resolved" callback. Note that I am not against the AllResolved/Rejected idea. I just do not think this has to be done now. There will be more users of waitUntil() and I'd rather we simplify the design once all are implemented (correctly).

"more efficient" is in the sense that before the patch the promise is collectable as soon as settled. AFAIUI, with this patch, this will not happen until all related promises are settled. This does not seem like an improvement to me. If there is an actual use case for checking each individual promise result, this patch is fine. My guess is that rejected/resolved is probably sufficient.

(In reply to Chris Dumez from comment #20) > (In reply to Chris Dumez from comment #19) > > (In reply to youenn fablet from comment #18) > > > Ah I arrived too late there... > > > If the callback(allresolved/rejected) works, I would much prefer this model. > > > > We can simplify later on. The current design is very flexible since it > > provides all the promises to the caller. This also matches the spec very > > closely. > > > > I do not really buy the 'more efficient' argument since I do not have to > > remove anything from the HashSet and I only keep the promises alive until I > > call the "all promises resolved" callback. > > Note that I am not against the AllResolved/Rejected idea. I just do not > think this has to be done now. There will be more users of waitUntil() and > I'd rather we simplify the design once all are implemented (correctly). I'll let you know once we implement the other call sites properly.

<rdar://problem/35567002>