Implement Service Worker Client.focus()

RESOLVED FIXED in Firefox 40

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bkelly, Assigned: catalinb)

Tracking

unspecified
mozilla40
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

3 years ago
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?
Blocks: 1059784
No longer blocks: 903441
Flags: needinfo?(catalin.badea392)
(Assignee)

Comment 2

3 years ago
(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.
Flags: needinfo?(catalin.badea392)
(Assignee)

Comment 3

3 years ago
Created attachment 8579359 [details] [diff] [review]
Refactor PromiseHolder in the service worker clients code.

Updated

3 years ago
Blocks: 1144660
(Assignee)

Comment 4

3 years ago
Created attachment 8579360 [details] [diff] [review]
Implement service worker client.focus.
(Assignee)

Updated

3 years ago
Attachment #8579359 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 5

3 years ago
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?
Attachment #8579360 - Flags: feedback?(nsm.nikhil)
Attachment #8579359 - Flags: review?(nsm.nikhil) → review+
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.
Attachment #8579360 - Flags: feedback?(nsm.nikhil)
(Assignee)

Updated

3 years ago
Assignee: nobody → catalin.badea392
(Assignee)

Comment 7

3 years ago
Created attachment 8585624 [details] [diff] [review]
Refactor PromiseHolder in the service worker clients code.
Attachment #8579359 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
Created attachment 8585627 [details] [diff] [review]
Implement client.focus.
Attachment #8579360 - Attachment is obsolete: true

Updated

3 years ago
Blocks: 1130687
(Assignee)

Comment 9

3 years ago
Created attachment 8587003 [details] [diff] [review]
Add test for service worker client.focus.
(Assignee)

Updated

3 years ago
Attachment #8585627 - Flags: review?(nsm.nikhil)
(Assignee)

Updated

3 years ago
Attachment #8587003 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 10

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec943c57d96e
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.
Attachment #8585627 - Flags: review?(nsm.nikhil) → review+
Attachment #8587003 - Flags: review?(nsm.nikhil) → review+
(Assignee)

Comment 12

3 years ago
Created attachment 8589050 [details] [diff] [review]
Refactor PromiseHolder in the service worker clients code.
(Assignee)

Comment 13

3 years ago
Created attachment 8589051 [details] [diff] [review]
Implement client.focus.
Attachment #8585627 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
Created attachment 8589053 [details] [diff] [review]
Add test for service worker client.focus.
Attachment #8587003 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8585624 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8589051 - Flags: review?(amarchesini)
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);
}
Attachment #8589051 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 16

3 years ago
Created attachment 8589090 [details] [diff] [review]
Implement client.focus.
Attachment #8589051 - Attachment is obsolete: true
(Assignee)

Comment 17

3 years ago
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
Attachment #8589050 - Flags: checkin+
(Assignee)

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/18ae121e24dc
Attachment #8589053 - Flags: checkin+
(Assignee)

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6057e430332b
Attachment #8589090 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/20932983712e
https://hg.mozilla.org/mozilla-central/rev/18ae121e24dc
https://hg.mozilla.org/mozilla-central/rev/6057e430332b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.