1131882 - navigator.serviceWorker.controller does not track underlying state

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

ServiceWorkerRegistration objects track the intenral ServiceWorkerState and update their ServiceWorkers accordingly and also fire events on them. .controller does not.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Associate ServiceWorkers with underlying ServiceWorkerInfo

This allows controller to track state too, and provides better encapsulation.
1) Move SW setup to SWM. RuntimeService now only creates the underlying SharedWorker.
2) Require a SWInfo to create a SW. The SW holds a refptr to the info.

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8565235 [details] [diff] [review]
Associate ServiceWorkers with underlying ServiceWorkerInfo

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

::: dom/workers/ServiceWorker.cpp
@@ +78,5 @@
>  
>  void
> +ServiceWorker::GetScriptURL(nsString& aURL) const
> +{
> +  aURL = NS_ConvertUTF8toUTF16(mInfo->ScriptSpec());

ConvertUTF8toUTF16(mInfo->ScriptSpec(), aURL);

@@ +109,5 @@
> +  nsCOMPtr<nsIRunnable> r =
> +    NS_NewRunnableMethodWithArg<ServiceWorkerState>(this,
> +                                                    &ServiceWorker::DispatchStateChange,
> +                                                    aState);
> +  NS_DispatchToMainThread(r);

MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DipsatchToMainThread(r)));

::: dom/workers/ServiceWorker.h
@@ +17,5 @@
>  
>  class Promise;
>  
>  namespace workers {
>  

class ServiceWorkerManager; <- this is needed for the friend stmt.

::: dom/workers/ServiceWorkerManager.cpp
@@ +630,5 @@
>      NS_DispatchToMainThread(upr);
>  
>      nsRefPtr<ServiceWorker> serviceWorker;
>      nsresult rv =
> +      swm->CreateServiceWorker(mRegistration->mInstallingWorker,

this is changed in my latest patch. can you rebase this patch on top of mine?

@@ +2446,5 @@
> +  for (uint32_t i = 0; i < mInstances.Length(); ++i) {
> +    mInstances[i]->QueueStateChangeEvent(mState);
> +  }
> +}
> +

extra line :)

::: dom/workers/ServiceWorkerManager.h
@@ +215,5 @@
>    const ServiceWorkerRegistrationInfo* mRegistration;
>    nsCString mScriptSpec;
>    ServiceWorkerState mState;
> +  // There is a high chance of there being at least one ServiceWorker
> +  // associated with this all the time.

Write that we have a raw pointer because the unregister is called on the DTOR.

@@ +264,5 @@
>    // Only used to set initial state when loading from disk!
>    void
>    SetActivateStateUncheckedWithoutEvent(ServiceWorkerState aState)
>    {
>      mState = aState;

Would be nice to assert this.. some how.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I tried including this in the build, but it needs to be rebased since persistent registrations landed.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc13de45e2ab

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Associate ServiceWorkers with underlying ServiceWorkerInfo

Ben, updated patch.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Sorry, I already did the builds.  I'll include this in the next build if it hasn't landed yet.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd7993ab879a

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/489a21a96e5d

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/489a21a96e5d