Following on the work in bug 1462772 we need to also automatically fire the updatefound event when the ServiceWorkerRegistration state is updated appropriately. Specifically, we want to fire it when a new installing worker is seen.

Comment on attachment 8988563 [details] [diff] [review] P1 Automatically fire updatefound events when the ServiceWorkerRegistration gets a new installing worker. r=mrbkap Blake, as I discussed in bug 1462772 I want to move the logic that triggers the updatefound event into ServiceWorkerRegistration. This allows us to make sure it fires when the registration properties are in the correct state. This patch implements this feature noting when the registration sees a new installing worker. The spec has the event firing from a task queued in step 7 here: https://w3c.github.io/ServiceWorker/#installation-algorithm This takes place after step 4 queues a task (inside Update Worker State) to update the registration property. The patch works by tracking the id of the installing worker. There is a scheduled id and then the actual dispatched id. A runnable is used to perform the actual event dispatch based on the scheduled id. If the state changes again before actually dispatching the event from the runnable, then we dispatch the event immediately before overwriting the installing property.

Comment on attachment 8988564 [details] [diff] [review] P2 Delay dispatching updatefound events until any pending update() promises resolve. r=mrbkap You may have noticed in the previous patch description that between queuing the task to update the installing property in step 4 and queuing the task to fire updatefound in step 7 here: https://w3c.github.io/ServiceWorker/#installation-algorithm There is step 6 to queue a task to resolve any update() promise attached to this update job. The previous patch breaks the requirement that the promise resolve before the updatefound event fires. Usually the promise will resolve before the scheduled runnable fires, but there is no guarantee. This patch fixes the ordering with the update job promise by delaying the updatefound event as long as their is an update() promise pending. Note, this is safe even if code calls update() in a loop. Every time the installing worker changes before the last updatefound has been fired the registration will fire the updatefound immediately. So the updatefound events will still fire in this situation of "always has a list of pending update promises". I'll admit that I don't love this approach. I did not see a great alternative, however. We need to have coherent state updates and event dispatching even if changes occur before our binding object has attached its actor. We also need to support updatefound even if there is no update() promise when an update automatically occurs. Slightly delaying the updatefound seemed the best approach.

Comment on attachment 8988567 [details] [diff] [review] P3 Remove the old updatefound infrastructure in favor of always updating the registration state and auto-dispatch. r=mrbkap This patch removes the old infrastructure for triggering updatefound events directly in the ServiceWorkerUpdateJob. It also makes the worker-thread registration impl call ServiceWorkerRegistration::UpdateState(). Previously it did not have to do this because we don't expose any ServiceWorker objects on workers yet. So the registration properties are always null. We do support updatefound event on workers, though. So removing the old scheme requires making the worker thread impl us the UpdateState() approach.

Comment on attachment 8988564 [details] [diff] [review] P2 Delay dispatching updatefound events until any pending update() promises resolve. r=mrbkap Review of attachment 8988564 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/serviceworkers/ServiceWorkerRegistration.cpp @@ +215,5 @@ > return; > } > outer->MaybeResolve(ref); > + }, [outer, self] (ErrorResult& aRv) { > + auto scopeExit = MakeScopeExit([&] { self->UpdatePromiseSettled(); }); I guess this is C++'s version of the "single entry single exit" rule.

Comment on attachment 8988567 [details] [diff] [review] P3 Remove the old updatefound infrastructure in favor of always updating the registration state and auto-dispatch. r=mrbkap Review of attachment 8988567 [details] [diff] [review]: ----------------------------------------------------------------- This actually seems much simpler to me.

Try build in comment 3.

Patch failed to apply: mozilla@ubuntu:~/mozilla-unified$ hg qpush -a applying bug1471929_p1_autoupdate.patch patching file dom/serviceworkers/ServiceWorkerRegistration.cpp Hunk #2 FAILED at 332 1 out of 2 hunks FAILED -- saving rejects to file dom/serviceworkers/ServiceWork erRegistration.cpp.rej patching file dom/serviceworkers/ServiceWorkerRegistration.h Hunk #1 FAILED at 125 1 out of 1 hunks FAILED -- saving rejects to file dom/serviceworkers/ServiceWork erRegistration.h.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh bug1471929_p1_autoupdate.patch

There is a dependency between the bugs I marked for checkin-needed. I guess they will have to land manually.

The conflict here was due to the change to the AutoTArray initializer list in a previous bug. It only seems to affect the P1 patch.

Rebased

Pushed by ben@wanderview.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5166edd661b1 P1 Automatically fire updatefound events when the ServiceWorkerRegistration gets a new installing worker. r=mrbkap https://hg.mozilla.org/integration/mozilla-inbound/rev/e818f2182882 P2 Delay dispatching updatefound events until any pending update() promises resolve. r=mrbkap https://hg.mozilla.org/integration/mozilla-inbound/rev/e7eb79224276 P3 Remove the old updatefound infrastructure in favor of always updating the registration state and auto-dispatch. r=mrbkap

https://hg.mozilla.org/mozilla-central/rev/5166edd661b1 https://hg.mozilla.org/mozilla-central/rev/e818f2182882 https://hg.mozilla.org/mozilla-central/rev/e7eb79224276