Implement Client.focus() discussed here: https://github.com/slightlyoff/ServiceWorker/issues/602

Catalin, this seems very useful. is this on your radar and can it be done in time for v1?

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #1) > Catalin, this seems very useful. is this on your radar and can it be done in > time for v1? Yes, I'll look into it. Currently working on the changes from matchAll.

Comment on attachment 8579360 [details] [diff] [review] Implement service worker client.focus. I'm not sure how to reliably test this from a mochitest, my guess is that I should write a browser test for it. What do you think?

Comment on attachment 8579360 [details] [diff] [review] Implement service worker client.focus. Review of attachment 8579360 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerWindowClient.cpp @@ +3,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this file, > * You can obtain one at http://mozilla.org/MPL/2.0/. > */ > > +#include "mozilla/dom/PromiseWorkerProxy.h" ServiceWorkerWindowClient.h - winner due to being this file's corresponding header. m/d/ClientBinding.h m/d/PromiseWorkerProxy.h @@ +21,5 @@ > +namespace { > + > +// We need to manually dispatch runnables back to the service worker because > +// we might not have a window to create the proxied promise on the main thread. > +class ReleasePromiseRunnable MOZ_FINAL : public MainThreadWorkerControlRunnable Promise.cpp has in anon namespace a class that does exactly this. Could you move it's declaration to PromiseWorkerProxy.h (with usage comments. Also update the last section of PromiseWorkerProxy's doc to remind users that dispatching worker runnable can fail in which case they should use the control runnable) and update this and Promise.cpp to use it? @@ +49,5 @@ > + } > +}; > + > +// Passing a null clientInfo will reject the promise with InvalidAccessError. > +class ResolveOrRejectPromiseRunnable MOZ_FINAL : public WorkerRunnable s/MOZ_FINAL/final since bug 1145631. @@ +66,5 @@ > + AssertIsOnMainThread(); > + } > + > + bool > + WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) Nit: add override at the end. @@ +89,5 @@ > + return true; > + } > +}; > + > +class ClientFocusRunnable MOZ_FINAL : public nsRunnable Nit: final. @@ +104,5 @@ > + MOZ_ASSERT(mPromiseProxy->GetWorkerPromise()); > + } > + > + NS_IMETHOD > + Run() Nit: override @@ +108,5 @@ > + Run() > + { > + AssertIsOnMainThread(); > + nsGlobalWindow* window = nsGlobalWindow::GetOuterWindowWithId(mWindowId); > + nsAutoPtr<ServiceWorkerClientInfo> clientInfo; Please use the new mozilla::UniquePtr<> and Move(). It has clearer and stronger semantics. Thanks! @@ +120,5 @@ > + //FIXME(catalinb): Bug 1144660 - check if we are allowed to focus here. > + ErrorResult result; > + window->Focus(result); > + if (NS_WARN_IF(result.Failed())) { > + return result.ErrorCode(); If this is hit, DispatchResult will never get called. The spec seems to say that we should always resolve the promise with a client even if the focus fails, so may be just ignore the result? @@ +125,5 @@ > + } > + clientInfo = new ServiceWorkerClientInfo(window->GetDocument()); > + DispatchResult(clientInfo); > + > + return NS_OK; From what I understand of this function, instead of early returning NS_ERROR_FAILURE, always return NS_OK (since the runnable return doesn't affect operation here). UniquePtr<SWClientInfo> clientInfo; if (window) { // focusing code clientInfo.reset(new ...); } DispatchResult(Move(clientInfo)); return NS_OK; @@ +178,5 @@ > + } > + > + nsRefPtr<ClientFocusRunnable> r = new ClientFocusRunnable(GetWindowId(), > + promiseProxy); > + nsresult rv = NS_DispatchToMainThread(r); aRv = ... ErrorResult has an overload accepting nsresult. @@ +180,5 @@ > + nsRefPtr<ClientFocusRunnable> r = new ClientFocusRunnable(GetWindowId(), > + promiseProxy); > + nsresult rv = NS_DispatchToMainThread(r); > + > + if (NS_WARN_IF(NS_FAILED(rv))) { MOZ_ALWAYS_TRUE(!aRv.Failed()) since main thread dispatch can never fail on a worker.

Comment on attachment 8585627 [details] [diff] [review] Implement client.focus. Review of attachment 8585627 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerClient.h @@ +83,5 @@ > JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override; > > protected: > + uint64_t > + GetWindowId() const Just WindowId() since it can't fail. ::: dom/workers/ServiceWorkerWindowClient.cpp @@ +53,5 @@ > + } else { > + promise->MaybeReject(NS_ERROR_DOM_INVALID_ACCESS_ERR); > + } > + > + // release the reference on the worker thread. Nit: Capitalize Release. @@ +67,5 @@ > + nsRefPtr<PromiseWorkerProxy> mPromiseProxy; > + > +public: > + ClientFocusRunnable(uint64_t aWindowId, PromiseWorkerProxy* aPromiseProxy) > + : mWindowId(aWindowId), Nit: comma on next line. @@ +103,5 @@ > + nsRefPtr<ResolveOrRejectPromiseRunnable> resolveRunnable = > + new ResolveOrRejectPromiseRunnable(workerPrivate, mPromiseProxy, > + Move(aClientInfo)); > + > + AutoSafeJSContext cx; AutoJSAPI jsapi; jsapi.Init(); JSContext* cx = jsapi.cx(); will give you the context that you want. @@ +107,5 @@ > + AutoSafeJSContext cx; > + if (!resolveRunnable->Dispatch(cx)) { > + nsRefPtr<PromiseWorkerProxyControlRunnable> controlRunnable = > + new PromiseWorkerProxyControlRunnable(workerPrivate, mPromiseProxy); > + nsresult rv = workerPrivate->DispatchControlRunnable(controlRunnable); Use the controlRunnable->Dispatch(cx) form.

Comment on attachment 8589051 [details] [diff] [review] Implement client.focus. Review of attachment 8589051 [details] [diff] [review]: ----------------------------------------------------------------- Can you add some tests? ::: dom/workers/ServiceWorkerWindowClient.cpp @@ +104,5 @@ > + new ResolveOrRejectPromiseRunnable(workerPrivate, mPromiseProxy, > + Move(aClientInfo)); > + > + AutoJSAPI jsapi; > + jsapi.Init(); you can use the window here. @@ +140,5 @@ > + // Don't dispatch if adding the worker feature failed. > + return promise.forget(); > + } > + > + nsRefPtr<ClientFocusRunnable> r = new ClientFocusRunnable(WindowId(), mWindowId, @@ +143,5 @@ > + > + nsRefPtr<ClientFocusRunnable> r = new ClientFocusRunnable(WindowId(), > + promiseProxy); > + aRv = NS_DispatchToMainThread(r); > + MOZ_ALWAYS_TRUE(!aRv.Failed()); In theory this can failed. I talked with smaug about this and yes, let's stop doing this MOZ_ALWAYS_TRUE with NS_DispatchToMainThread(). Just do: nsresult rv = NS_DispatchToMainThread(r); if (NS_WARN_IF(NS_FAILED(rv))) { promise->MaybeReject(rv); }

Comment on attachment 8589050 [details] [diff] [review] Refactor PromiseHolder in the service worker clients code. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/20932983712e

Comment on attachment 8589053 [details] [diff] [review] Add test for service worker client.focus. https://hg.mozilla.org/integration/mozilla-inbound/rev/18ae121e24dc

Comment on attachment 8589090 [details] [diff] [review] Implement client.focus. https://hg.mozilla.org/integration/mozilla-inbound/rev/6057e430332b

https://hg.mozilla.org/mozilla-central/rev/20932983712e https://hg.mozilla.org/mozilla-central/rev/18ae121e24dc https://hg.mozilla.org/mozilla-central/rev/6057e430332b