Closed Bug 1130686 Opened 5 years ago Closed 5 years ago

Implement Service Worker Client.focus()

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bkelly, Assigned: catalinb)

References

Details

Attachments

(3 files, 6 obsolete files)

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: ServiceWorkers-v1
No longer blocks: ServiceWorkers
Flags: needinfo?(catalin.badea392)
(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)
Blocks: 1144660
Attachment #8579359 - Flags: review?(nsm.nikhil)
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)
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: nobody → catalin.badea392
Attachment #8579359 - Attachment is obsolete: true
Attached patch Implement client.focus. (obsolete) — Splinter Review
Attachment #8579360 - Attachment is obsolete: true
Blocks: 1130687
Attachment #8585627 - Flags: review?(nsm.nikhil)
Attachment #8587003 - Flags: review?(nsm.nikhil)
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+
Attached patch Implement client.focus. (obsolete) — Splinter Review
Attachment #8585627 - Attachment is obsolete: true
Attachment #8587003 - Attachment is obsolete: true
Attachment #8585624 - Attachment is obsolete: true
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+
Attachment #8589051 - Attachment is obsolete: true
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+
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
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.