1130686 - Implement Service Worker Client.focus()

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

Implement Client.focus() discussed here:

  https://github.com/slightlyoff/ServiceWorker/issues/602

Comment 1

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

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

Comment 2

[Cătălin Badea (:catalinb)]

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

[Cătălin Badea (:catalinb)]

Attachment: Refactor PromiseHolder in the service worker clients code.

Comment 4

[Cătălin Badea (:catalinb)]

Attachment: Implement service worker client.focus.

Comment 5

[Cătălin Badea (:catalinb)]

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 6

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

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 7

[Cătălin Badea (:catalinb)]

Attachment: Refactor PromiseHolder in the service worker clients code.

Comment 8

[Cătălin Badea (:catalinb)]

Attachment: Implement client.focus.

Comment 9

[Cătălin Badea (:catalinb)]

Attachment: Add test for service worker client.focus.

Comment 10

[Cătălin Badea (:catalinb)]

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

Comment 11

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

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 12

[Cătălin Badea (:catalinb)]

Attachment: Refactor PromiseHolder in the service worker clients code.

Comment 13

[Cătălin Badea (:catalinb)]

Attachment: Implement client.focus.

Comment 14

[Cătălin Badea (:catalinb)]

Attachment: Add test for service worker client.focus.

Comment 15

[Andrea Marchesini [:baku]]

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 16

[Cătălin Badea (:catalinb)]

Attachment: Implement client.focus.

Comment 17

[Cătălin Badea (:catalinb)]

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 18

[Cătălin Badea (:catalinb)]

Comment on attachment 8589053 [details] [diff] [review]
Add test for service worker client.focus.

https://hg.mozilla.org/integration/mozilla-inbound/rev/18ae121e24dc

Comment 19

[Cătălin Badea (:catalinb)]

Comment on attachment 8589090 [details] [diff] [review]
Implement client.focus.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6057e430332b

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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