navigator.serviceWorker.controller does not track underlying state

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
a month ago

People

(Reporter: nsm, Assigned: nsm)

Tracking

33 Branch
mozilla39
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

ServiceWorkerRegistration objects track the intenral ServiceWorkerState and update their ServiceWorkers accordingly and also fire events on them. .controller does not.
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.
Attachment #8565235 - Flags: review?(amarchesini)
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
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.
Attachment #8565235 - Flags: review?(amarchesini) → review+
I tried including this in the build, but it needs to be rebased since persistent registrations landed.
Sorry, I already did the builds.  I'll include this in the next build if it hasn't landed yet.
https://hg.mozilla.org/mozilla-central/rev/489a21a96e5d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.