I don't know if we can optimize the way we resolve the pending entry promises. Maybe you have a better idea.

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.

(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?

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.

> 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?

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.

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.

Needs rebasing.

Andrea, does this have the same problem as 1058043? Could you fix it?

I uploaded a patch on bug 1058043. It's in your review queue.

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.

See https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerContainer/ready for the doc on this.