1194078 - client.focus() should focus the app in firefox OS

Status: RESOLVED WONTFIX

(Core :: DOM: Push Subscriptions, defect)

Description

[Robert Bindar]

Calling client.focus() from within a serviceworker should foreground the caller app.

Comment 1

[Robert Bindar]

Attachment: client_focus_b2g

This patch sends a message to the parent process saying that it wants to foreground the app with a particular appID, the parent process then uses the service created in bug 1189091 to send an event to the system app foregrounds the app eventually.

I tested it on both b2g and b2g-desktop and it works as expected.

Comment 2

[Robert Bindar]

Attachment: client_focus_b2g

Sorry, I forgot to check "patch".

Comment 3

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

Comment on attachment 8647345 [details] [diff] [review]
client_focus_b2g

Review of attachment 8647345 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/content.js
@@ +1237,5 @@
>      text = text.replace(middleRE, " ");
>      return text.replace(endRE, "");
>    }
>  };
> +pageInfoListener.init(this);

Some newline change?

::: dom/interfaces/base/nsIPushProxyHelper.idl
@@ +40,5 @@
>  
>      void forwardMessageToParentSWM(in DOMString aMessageName,
>                                     in jsval aMessage);
> +
> +    void forwardSWFocusClientEvent(in unsigned long aAppID);

In light of this function, the interface definitely needs rename to something not push specific. It seems to be turning into an aggregation of Service Worker related process finding utilities.

::: dom/workers/PServiceWorkerManager.ipdl
@@ +20,5 @@
>    Register(ServiceWorkerRegistrationData data);
>  
>    Unregister(PrincipalInfo principalInfo, nsString scope);
>  
> +  ServiceWorkerFocusClient(PrincipalInfo principalInfo, uint32_t appId);

You should be able to get the appId from the principal info when it is deserialized to the principal in the implementation. No need for this second param.

::: dom/workers/PushProxyHelper.js
@@ +110,5 @@
>        debug("sending push message to child");
>        aChildMM.sendAsyncMessage(aMessageName, aMessage);
>    },
>  
> +  wakeUpApp: function(aAppID, aShowApp) {

At the minimum add a object literal param like:

  wakeUpApp(id, { show: true })

and document it at the function declaration [0].

Also, if wakeUpApp is meant to be used internally only, prefix with a '_'.

[0]: http://wiki.qt.io/API_Design_Principles#The_Boolean_Parameter_Trap

::: dom/workers/ServiceWorkerManager.cpp
@@ +5182,5 @@
>  {
>    return ++gServiceWorkerInfoCurrentID;
>  }
>  
> +

Nit: extra newline

::: dom/workers/ServiceWorkerManagerParent.cpp
@@ +104,5 @@
>  
> +class DispatchFocusClientCallback final : public nsRunnable
> +{
> +public:
> +  DispatchFocusClientCallback(const PrincipalInfo& aPrincipalInfo,

Please make the parameter ordering consistent with CheckPrincipalWithCallbackRunnable.

@@ +115,5 @@
> +    AssertIsInMainProcess();
> +    AssertIsOnBackgroundThread();
> +  }
> +
> +  NS_IMETHODIMP

NS_IMETHOD when it is in the declaration, also annotate with `override`. Could you make the same changes to CheckPrincipalWithCallbackRunnable while you are here? Thanks!

::: dom/workers/ServiceWorkerWindowClient.cpp
@@ +90,5 @@
>      nsGlobalWindow* window = nsGlobalWindow::GetInnerWindowWithId(mWindowId);
>      UniquePtr<ServiceWorkerClientInfo> clientInfo;
>  
>      if (window) {
>        nsContentUtils::DispatchChromeEvent(window->GetExtantDoc(), window->GetOuterWindow(), NS_LITERAL_STRING("DOMServiceWorkerFocusClient"), true, true);

This should not be called on b2g so please macro it out.

@@ +91,5 @@
>      UniquePtr<ServiceWorkerClientInfo> clientInfo;
>  
>      if (window) {
>        nsContentUtils::DispatchChromeEvent(window->GetExtantDoc(), window->GetOuterWindow(), NS_LITERAL_STRING("DOMServiceWorkerFocusClient"), true, true);
>        clientInfo.reset(new ServiceWorkerClientInfo(window->GetDocument(),

Can't do this before success.

@@ +100,5 @@
> +      nsRefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
> +
> +      PrincipalInfo principalInfo;
> +      if (NS_WARN_IF(NS_FAILED(PrincipalToPrincipalInfo(principal, &principalInfo)))) {
> +        return NS_ERROR_FAILURE;

if (NS_SUCCEEDED(PrincipalToPrincipalInfo...)) {
  send IPDL message
  initialize clientInfo.
}

Otherwise keep clientInfo uninitialized and the call to DispatchResult() will ensure the Promise is rejected.

@@ +102,5 @@
> +      PrincipalInfo principalInfo;
> +      if (NS_WARN_IF(NS_FAILED(PrincipalToPrincipalInfo(principal, &principalInfo)))) {
> +        return NS_ERROR_FAILURE;
> +      }
> +      nsRefPtr<ServiceWorkerManagerChild> actor = swm->GetBackgroundActor();

I'd rather have swm->FocusClient(...) that internally uses the actor than exposing the actor to other consumers.

@@ +162,5 @@
>      }
>  
>      nsRefPtr<ClientFocusRunnable> r = new ClientFocusRunnable(mWindowId,
>                                                                promiseProxy);
> +

Nit: newline.

Comment 4

[Robert Bindar]

Thanks Nikhil for the quick review! I'm glad you're ok with this solution. I'll fix the nits and upload a new patch soon :)

Comment 5

[Robert Bindar]

Attachment: MozReview Request: Bug 1194078: make client.focus() work on b2g r=nsm

Bug 1194078: make client.focus() work on b2g

Comment 6

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

Comment on attachment 8647830 [details]
MozReview Request: Bug 1194078: make client.focus() work on b2g r=nsm

https://reviewboard.mozilla.org/r/16063/#review14441

Ship It!

::: dom/interfaces/base/nsIPushIPCHelper.idl:13
(Diff revision 1)
>  interface nsIPushIPCHelper : nsISupports

I think you mean ServiceWorkerIPCHelper.

::: dom/workers/PushIPCHelper.js:109
(Diff revision 1)
>        if (!aChildMM && aMessageName === "pushsubscriptionchange") {

Why shouldn't the app be woken up for pushsubscriptionchange? It seems like an important message for the app to receive.

::: dom/workers/PushIPCHelper.js:123
(Diff revision 1)
> -  _wakeUpApp: function(aAppID) {
> +  // aShowOptions is {showApp: bool_value, onlyShowApp: bool_value}

Nit: s/bool_value/boolean

::: dom/workers/PushIPCHelper.js:124
(Diff revision 1)
> +  // showApp:     This flag indicates the app must be brought to the
> +  //              foreground.
> +  // onlyShowApp: This flag indicates the app must be *only* brought to
> +  //              the foreground without loading to handle messages.

Seems better to ask them to look at the canonical documentation in https://dxr.mozilla.org/mozilla-central/source/dom/messages/interfaces/nsISystemMessageGlue.idl#18

::: dom/workers/ServiceWorkerManagerParent.cpp:136
(Diff revision 1)
> +    helper->SendClientFocusEvent(principal->GetAppId());

You have to check the app id for NO_APP_ID or UNKNOWN_APP_ID, unless the SystemApp will do that for you.

Comment 7

[Robert Bindar]

Comment on attachment 8647830 [details]
MozReview Request: Bug 1194078: make client.focus() work on b2g r=nsm

Bug 1194078: make client.focus() work on b2g r=nsm

Comment 8

[Robert Bindar]

I fixed, all the issues. Checking appID == UNKNOWN | NO_APP_ID is done in ServiceWorkerIPCHelper.
Waiting for bug 1189091 to land.