1448012 - Initial ServiceWorkerContainer refactor and ready promise rewrite

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

This bug covers some initial patches I wrote for bug 1436812.  They are stand alone and can land now before I finish the rest of that bug.  Moving here so we can land them.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add ServiceWorkerContainer::Create() factory method. r=asuth

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Eagerly create the ServiceWorkerContainer.controller. r=asuth

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add new ServiceWorkerContainer.ready mechanism based on MozPromise. r=asuth

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Remove stale service worker ready promise code. r=asuth

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8961410 [details] [diff] [review]
P1 Add ServiceWorkerContainer::Create() factory method. r=asuth

This patch is just a small refactor to change ServiceWorkerContainer to use a factory method for creation.  We will use this in the future to select the kind of inner object (same-process vs remote) based on a pref.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8961411 [details] [diff] [review]
P2 Eagerly create the ServiceWorkerContainer.controller. r=asuth

This patch makes the container eagerly allocate the controller ServiceWorker binding object.  This is necessary for a few reasons:

1. We need it available synchonously for the .controller getter.
2. Once we are remote we cannot just ask the SWM for it on demand.
3. We want to make sure the binding object gets updates if the state of the ServiceWorker changes, etc.  The easiest way to do this is to create the binding object and let it request these updates from the parent process.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8961412 [details] [diff] [review]
P3 Add new ServiceWorkerContainer.ready mechanism based on MozPromise. r=asuth

This patch re-writes our ready promise so that ServiceWorkerManager exposes a MozPromise.  The ServiceWorkerContainer is then responsible for storing the exposed Promise.

I did not try to change anything based on the spec issues I filed.  I just tried to match existing behavior.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8961414 [details] [diff] [review]
P4 Remove stale service worker ready promise code. r=asuth

This patch removes some stale code from the legacy ready promise implementation.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8961411 [details] [diff] [review]
P2 Eagerly create the ServiceWorkerContainer.controller. r=asuth

Review of attachment 8961411 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the excellent review comment describing the rationale here.  The same way one might comment "RAII" as a shorthand rationale, it'd be neat if we had an acronym like "EBIPS" for "Eager Binding Instantiation for Purposes of Subscriptions" that we could comment wherever we do that.  It's obviously a higher meta-level thing, so having a paragraph everywhere gets messy, but having something weird that one can search with searchfox to figure out what's going on might be handy.

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8961412 [details] [diff] [review]
P3 Add new ServiceWorkerContainer.ready mechanism based on MozPromise. r=asuth

Review of attachment 8961412 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/serviceworkers/ServiceWorkerContainer.cpp
@@ +316,2 @@
>  
> +  swm->OnReady(clientInfo.ref())->Then(

This is a phenomenally cool clean-up that makes a complicated process much easier to understand.  Thank you!

::: dom/serviceworkers/ServiceWorkerManager.cpp
@@ +1482,5 @@
>        iter.Remove();
>      }
>    }
> +
> +  nsTArray<UniquePtr<PendingReadyData>> pendingReadyList;

Nice idiom cleanup here!  (From mutating a hashtable to immutably-ish filtering a list, both here and in the next hunk.)

::: dom/serviceworkers/ServiceWorkerManager.h
@@ +324,5 @@
>    void
>    WorkerIsIdle(ServiceWorkerInfo* aWorker);
>  
> +  RefPtr<ServiceWorkerRegistrationPromise>
> +  OnReady(const ClientInfo& aClientInfo);

nit: This is a confusing/inconsistent name choice.  In most of the codebase, OnFoo methods are expected to be invoked when the "Foo" event happens, not in order to produce a promise that will be resolved when "Foo" event happens.  While the type-signature clears things up a lot, it is ambiguous in the face of ExtendableEvent which is event dispatch with a returned promise that has the opposite semantic intent.

I'd suggest switching to WhenReady() which I think still fits with its usage in ServiceWorkerContainer in a literate fashion.  (It's also consistent with the evolutionary stages of JS promises where `when(mayNotBePromiseValue, thenedMethod)` was a normalizing helper.  Although that should hopefully be information consigned to history.)

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrew Sutherland [:asuth] from comment #10)
> nit: This is a confusing/inconsistent name choice.  In most of the codebase,
> OnFoo methods are expected to be invoked when the "Foo" event happens, not
> in order to produce a promise that will be resolved when "Foo" event
> happens.  While the type-signature clears things up a lot, it is ambiguous
> in the face of ExtendableEvent which is event dispatch with a returned
> promise that has the opposite semantic intent.
> 
> I'd suggest switching to WhenReady() which I think still fits with its usage
> in ServiceWorkerContainer in a literate fashion.  (It's also consistent with
> the evolutionary stages of JS promises where `when(mayNotBePromiseValue,
> thenedMethod)` was a normalizing helper.  Although that should hopefully be
> information consigned to history.)

But I already snuck a case in so I have a precedent!  See ClientHandle::OnDetach().

https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/dom/clients/manager/ClientHandle.cpp#196

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

But you also have ClientHandle::OnShutdownThing() too.. which resolves the promise returned by OnDetach()...
https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/dom/clients/manager/ClientHandle.cpp#68

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add new ServiceWorkerContainer.ready mechanism based on MozPromise. r=asuth

s/OnReady/WhenReady/

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Remove stale service worker ready promise code. r=asuth

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a606511084d
P1 Add ServiceWorkerContainer::Create() factory method. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/28e5672119bf
P2 Eagerly create the ServiceWorkerContainer.controller. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/98b4c2899b36
P3 Add new ServiceWorkerContainer.ready mechanism based on MozPromise. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb59e1c7bc4f
P4 Remove stale service worker ready promise code. r=asuth

Comment 16

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/3a606511084d
https://hg.mozilla.org/mozilla-central/rev/28e5672119bf
https://hg.mozilla.org/mozilla-central/rev/98b4c2899b36
https://hg.mozilla.org/mozilla-central/rev/fb59e1c7bc4f