Closed Bug 1025077 Opened 6 years ago Closed 6 years ago

Implement ServiceWorkerContainer.ready

Categories

(Core :: DOM: Workers, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: nsm, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 3 obsolete files)

Assignee: nobody → amarchesini
Attached patch ready.patch (obsolete) — Splinter Review
I don't know if we can optimize the way we resolve the pending entry promises.
Maybe you have a better idea.
Attachment #8477332 - Flags: review?(nsm.nikhil)
Comment on attachment 8477332 [details] [diff] [review]
ready.patch

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

This patch does not deal with the active worker changing after a Promise has already been vended to a ServiceWorkerContainer. I think you'll need to keep a reference to the ServiceWorkerContainers like before to make that work :(
Please also add a test case for the above case with the new patch (if it isn't possible to test due to API limitations like lack of replace(), file a followup)

Could you also file a followup to see if we can get away with ServiceWorkerManager only keeping references to ServiceWorkerContainers and the Containers dealing with ServiceWorkerRegistration IDL objects? ServiceWorkerManager will only deal with ServiceWorkerRegistrationInfos, not with ServiceWorkerRegistrationInfos.

I haven't looked at the rest of the code closely after spotting the issue above, but it looks good cursorily.

::: dom/interfaces/base/nsIServiceWorkerManager.idl
@@ +12,4 @@
>  interface nsIServiceWorkerManager : nsISupports
>  {
>    // Returns a Promise
>    nsISupports register(in nsIDOMWindow aWindow, in DOMString aScope, in DOMString aScriptURI);

This part will conflict with a patch i landed for Bug 1057135, so you will have to upload a new patch for checkin (or just push to m-i yourself). Thanks!

::: dom/workers/ServiceWorkerContainer.cpp
@@ +141,5 @@
>    MOZ_ASSERT(ret);
>    return ret.forget();
>  }
>  
> +Promise*

Why is this a rawptr instead of creating a copy of the refptr and forgetting it?
If it's a valid reason, please add a comment.

::: dom/workers/ServiceWorkerManager.cpp
@@ +753,5 @@
> +    nsCOMPtr<nsIURI> docURI = doc->GetDocumentURI();
> +    if (!docURI) {
> +      mPromise->MaybeReject(NS_ERROR_UNEXPECTED);
> +      return NS_OK;
> +    }

The above checks can be moved into ::GetReadyPromise.
Attachment #8477332 - Flags: review?(nsm.nikhil) → review-
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #2)
> Comment on attachment 8477332 [details] [diff] [review]
> ready.patch
> 
> Review of attachment 8477332 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch does not deal with the active worker changing after a Promise has
> already been vended to a ServiceWorkerContainer. I think you'll need to keep

No and I didn't find this in the spec. 'ready' is an attribute, is not a method.
When the promise is sent to the Container and it's resolved, we cannot change its value.
Is this what you meant?
Flags: needinfo?(nsm.nikhil)
I should've linked to the spec issue https://github.com/slightlyoff/ServiceWorker/issues/223
See @coonsta's last comment.

The thing is it deals with fairly edge case-y things, and it is very wonky behaviour from an attribute (sigh!). I have added a comment to the issue. I guess meanwhile we can stick with this and file followups. I'll do a full review right now.
Flags: needinfo?(nsm.nikhil)
> The thing is it deals with fairly edge case-y things, and it is very wonky
> behaviour from an attribute (sigh!).

I think 'ready' is misleading.
To me 'ready' should be a boolean. 'whenReady' is a better name because it makes sense that is a promise. But again, it should return a boolean.

What we want here is a callback that is executed any time a ServiceWorker is active. A promise is not good for this because it has just 1 resolve function.
Comment on attachment 8477332 [details] [diff] [review]
ready.patch

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

The rest of it looks good, but please answer the rawptr questions above.

::: dom/workers/ServiceWorkerContainer.cpp
@@ +29,5 @@
>  NS_IMPL_ADDREF_INHERITED(ServiceWorkerContainer, DOMEventTargetHelper)
>  NS_IMPL_RELEASE_INHERITED(ServiceWorkerContainer, DOMEventTargetHelper)
>  
>  NS_IMPL_CYCLE_COLLECTION_INHERITED(ServiceWorkerContainer, DOMEventTargetHelper,
> +                                   mControllerWorker, mReadyPromise)

Shouldn't mWindow also be in here since we hold a COMPtr? I might be wrong.

::: dom/workers/ServiceWorkerManager.cpp
@@ +802,5 @@
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);
> +  if (!window) {
> +    return NS_ERROR_FAILURE;
> +  }
> +

Can you add an assertion mPendingReadyPromises contains window.

::: dom/workers/test/serviceworkers/test_installation_simple.html
@@ +150,5 @@
> +    document.body.appendChild(frame);
> +
> +    var channel = new MessageChannel();
> +    frame.addEventListener('load', function() {
> +      frame.contentWindow.postMessage('your port!', '*', [channel.port2]);

very anthropocentric ;)
(In reply to Andrea Marchesini (:baku) from comment #5)
> > The thing is it deals with fairly edge case-y things, and it is very wonky
> > behaviour from an attribute (sigh!).
> 
> I think 'ready' is misleading.
> To me 'ready' should be a boolean. 'whenReady' is a better name because it
> makes sense that is a promise. But again, it should return a boolean.
> 
> What we want here is a callback that is executed any time a ServiceWorker is
> active. A promise is not good for this because it has just 1 resolve
> function.

Can you put this in the spec issue?
> The rest of it looks good, but please answer the rawptr questions above.

Because mReadyPromise is an internal value already refcounted.
For instance, GetConsole does the same, but GetLocation does not in WorkerScope...
I think there is no need to create an extra object just to return an already_AddRefed for an internal value.

I asked roc an opinion:

<baku> do we have any preference between returning it with:
<baku> Something* fnc() { return mSomething; }
<baku> or 
<baku> already_AddRefed<Something> fnc() { nsRefPtr<Something> a = mSomething; return a.forget(); }
<baku> ?
<roc> the former

> >  NS_IMPL_CYCLE_COLLECTION_INHERITED(ServiceWorkerContainer, DOMEventTargetHelper,
> > +                                   mControllerWorker, mReadyPromise)

Correct. We should use GetOwner() maybe... follow up?
Attached patch ready.patch (obsolete) — Splinter Review
Actually we don't need to hold a reference to mWindow.
This patch uses GetOwner() everywhere and overwrite DisconnectFromOwner() to unregister ServiceWorkerContainer from the ServiceWorkerManager.

The reason why we need the window is because the Promise and the ServiceWorkerRegistration objects need the window in the constructor.
Attachment #8477332 - Attachment is obsolete: true
Attachment #8478273 - Flags: review?(nsm.nikhil)
Attachment #8478273 - Flags: feedback?(bugs)
Comment on attachment 8478273 [details] [diff] [review]
ready.patch

Does RemoveReadyPromise do anything interesting with the Window object?
I hope it doesn't for example take a strong ref, since 
DisconnectFromOwner can be called when the Window is about to be deleted.

But assuming RemoveReadyPromise just removes, fine.
Attachment #8478273 - Flags: feedback?(bugs) → feedback+
Attachment #8478273 - Flags: review?(nsm.nikhil) → review+
Attached patch ready.patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=42b606e75e62
Attachment #8478273 - Attachment is obsolete: true
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
Attached patch ready.patchSplinter Review
Attachment #8478500 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e48a1782de89
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Andrea, does this have the same problem as 1058043? Could you fix it?
Flags: needinfo?(amarchesini)
I uploaded a patch on bug 1058043. It's in your review queue.
Flags: needinfo?(amarchesini)
Attached patch container.patchSplinter Review
This patch has been reviewed by nsm in Bug 1058043
We decided to move this patch here because it was more related to this bug than the other one.

https://hg.mozilla.org/integration/mozilla-inbound/rev/aed61458371b
Bug reopened.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/aed61458371b
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.