814074 - Send a system message to the application when the application is not running anymore but waits for a notification

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-0.1

When an application use the desktop notification API but is closed (because of an OOM or by a user action) the notification stays and clicking on it does nothing.

This affects the dialer and soon the sms application.

This patch use the support provided by bug 778668 to know what is the source of the notification and use system message to open it if needed, sending the title/text of the notification to the application in order to let it reposition the UI to the right state.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Since this blocks a blocker and also fix the the broken notification with the dialer i believe this is a blocker.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

This patch is on top of the patch from fabrice in bug 778668. If the original application has been killed/closed it send a system message to the application (if this one has registered to receive them) with the content of the title and text of the notification to let it a chance to resume the UI in a correct state.

Asking r? to fabrice for the shell.js change.

Asking r? to cjones for the ContentParent.cpp

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 684384 [details] [diff] [review]
Patch

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

::: b2g/chrome/content/shell.js
@@ +696,5 @@
> +          target = messages[i]["notification"];
> +          break;
> +        }
> +      }
> +

You also need to look for system messages in entry_points there.

@@ +697,5 @@
> +          break;
> +        }
> +      }
> +
> +      // The application does not expect any system messages for notications,

nit: s/notications/notifications

@@ +704,5 @@
> +        return;
> +
> +      gSystemMessenger.sendMessage("notification",
> +        { title: listener.title, body: listener.text },
> +        Services.io.newURI(origin + target, null, null),

We must resolve the uri properly here, with newURI(target, null, origin)

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 684384 [details] [diff] [review]
Patch

>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
>index 8dc7f8a..26af2e7 100644
>--- a/dom/ipc/ContentParent.cpp
>+++ b/dom/ipc/ContentParent.cpp
>@@ -1022,7 +1022,7 @@ ContentParent::Observe(nsISupports* aSubject,
>     }
> 
>     if (!mIsAlive || !mSubprocess)
>-        return NS_OK;
>+        return NS_ERROR_NOT_AVAILABLE;

What's the motivation for this change?

With that settled, fabrice's review is sufficient for this.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> Comment on attachment 684384 [details] [diff] [review]
> Patch
> 
> >diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
> >index 8dc7f8a..26af2e7 100644
> >--- a/dom/ipc/ContentParent.cpp
> >+++ b/dom/ipc/ContentParent.cpp
> >@@ -1022,7 +1022,7 @@ ContentParent::Observe(nsISupports* aSubject,
> >     }
> > 
> >     if (!mIsAlive || !mSubprocess)
> >-        return NS_OK;
> >+        return NS_ERROR_NOT_AVAILABLE;
> 
> What's the motivation for this change?
> 

I need a way to know if the process that create the notification has died or not. If there an other way I would be fine with it.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 684384 [details] [diff] [review]
Patch

OK, that seems fine in this context.  Please push to try to ensure this doesn't trip observer service notifications to remote content.

Comment 8

[JP Rosevear [:jpr]]

Vivien, can this patch land now?

Comment 9

[Tony Chung [:tchung]]

smoketest blocker.   it prevents user from tapping their sms message from notification to launch into SMS app.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to JP Rosevear [:jpr] from comment #8)
> Vivien, can this patch land now?

No. The underlying code has changed. This patch needs a new version. Working on it.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v2

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v3

Comment 13

[[:fabrice] Fabrice Desré]

Comment on attachment 687011 [details] [diff] [review]
Patch v3

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

I'd like to take another look with the manifest helper.

::: b2g/chrome/content/shell.js
@@ +641,5 @@
> +    } else if (uid.startsWith("alert")) {
> +      try {
> +        listener.observer.observe(null, topic, listener.cookie);
> +      } catch (e) {
> +      } 

Nit: trailing whitespace :P

@@ +683,5 @@
> +              continue;
> +
> +            for (let i = 0; i < messages.length; i++) {
> +              let message = messages[i];
> +              if (message == "notification") {

s/==/===

@@ +684,5 @@
> +
> +            for (let i = 0; i < messages.length; i++) {
> +              let message = messages[i];
> +              if (message == "notification") {
> +                target = app.launch_path;

That doesn't work because you fail to take l10n into account. You should use the ManifestHelper form AppsUtils.jsm, and resolve uris using https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#363

@@ +699,5 @@
> +      // If there is any notification listener, let's map it on
> +      // the listener so it will be used in when it will be time to
> +      // wake up the application.
> +      if (target) {
> +        listener.target = app.origin + target;

Use Services.io.newURI(target, null, app.origin).spec in a try{} catch(){} or resolveURI from the manifest helper.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v4

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 687038 [details] [diff] [review]
Patch v4

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

r- because we need to get this entry points thing right.

::: b2g/chrome/content/shell.js
@@ +661,5 @@
> +
> +    let app = DOMApplicationRegistry.getAppByManifestURL(listener.manifestURL);
> +    DOMApplicationRegistry.getManifestFor(app.origin, function(manifest) {
> +      let helper = new ManifestHelper(manifest, app.origin);
> +      var getEntryPointFor = function(messages) {

nit s/var/let

@@ +687,5 @@
> +      if (!entryPoints)
> +        return;
> +
> +      // XXX This code would take the last one declared. What else can be done
> +      // at this stage?

don't we want to check that the url that launched the notification is the same as the entry point url?

::: dom/apps/src/AppsUtils.jsm
@@ +293,5 @@
>      return this._localeProp("launch_path");
>    },
>  
> +  get entry_points() {
> +    return this._localeProp("entry_points");

entry points are not under a locale, this is the other way around. That also means that fullLaunchPath is buggy, and works in gaia because we use the same url for all languages.

For instance the manifest of the communications app looks like:
"entry_points": {
    "dialer": {
      "launch_path": "/dialer/index.html#keyboard-view",
      "name": "Phone",
      "icons": {
        "120": "/dialer/style/icons/Dialer.png",
        "60": "/dialer/style/icons/60/Dialer.png"
      },
      "locales": {
        "ar": {
          "name": "هاتف",
          "description": "Gaia هاتف"
        },
        "en-US": {
          "name": "Phone",
          "description": "Gaia Phone"
        },
        "fr": {
          "name": "téléphone",
          "description": "téléphone Gaia"
        },

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v5

Let's get rid of entry points for now. I have opened a followup.

Comment 17

[[:fabrice] Fabrice Desré]

Comment on attachment 687053 [details] [diff] [review]
Patch v5

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

r=me with nits addressed

::: b2g/chrome/content/shell.js
@@ +661,5 @@
> +
> +    let app = DOMApplicationRegistry.getAppByManifestURL(listener.manifestURL);
> +    DOMApplicationRegistry.getManifestFor(app.origin, function(manifest) {
> +      let helper = new ManifestHelper(manifest, app.origin);
> +      let getEntryPointFor = function(messages) {

nit: rename this function since it doesn't deal with entry points anymore.

::: dom/apps/src/AppsUtils.jsm
@@ +295,5 @@
>  
> +  get entry_points() {
> +    return this._localeProp("entry_points");
> +  },
> +

nit: remove that for now.

Comment 18

[Dylan Oliver [:doliver]]

Raised priority for smoketest blocker.

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 687053 [details] [diff] [review]
Patch v5

Carrying r+ed.

http://hg.mozilla.org/integration/mozilla-inbound/rev/7aa3201571dd

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7aa3201571dd

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2efa0233605d
https://hg.mozilla.org/releases/mozilla-beta/rev/50c5c2ddff7d

Comment 22

[Marcia Knous [:marcia]]

Tony advised commenting on this bug - using:

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/059a7e0badf7
Gaia   4164424049f9caa4e6b26aefe3a4fa7421451b2b
BuildID 20130217070201
Version 18.0

I am seeing an issue where when I send SMS messages to my phone while the screen is locked, when I open the screen the SMS app never launches - it just sits on the homescreen. I will open a new bug now to track this.