1001278 - [Tarako] "notification" system message does not fire when app is running (background or foreground)

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Michael Henretty [:mikehenrty][:mhenretty]]

STR:

1.) Remove notification click handler here
https://github.com/mozilla-b2g/gaia/blob/32b171e12d6e72598ddfa9137b401a27c0b66a2e/apps/sms/js/activity_handler.js#L426
2.) Send SMS to Tarako
3.) Click notification toaster (must do this fast because memory pump will kill sms eventually)

Actual Result:
desktop-notification-click event is fired, but app remains in the background.

Expected Result:
App is brought to the foreground, since the notification system message handler does this here:
https://github.com/mozilla-b2g/gaia/blob/32b171e12d6e72598ddfa9137b401a27c0b66a2e/apps/sms/js/activity_handler.js#L475

To further test this, putting logs in the onNotification handler will show that this handler doesn't get called when the app is already running.

Comment 1

[Michael Henretty [:mikehenrty][:mhenretty]]

Fabrice, can this be fixed by uplifting bug 988787?

Comment 2

[[:fabrice] Fabrice Desré]

Note, to reproduce, launch the sms app on the tarako first.

bug 988787 didn't help here unfortunately. Gene, any idea?

Comment 3

[Michael Henretty [:mikehenrty][:mhenretty]]

More precise STR:

1.) Open SMS app, and stay on the main thread page
2.) Add log line in onNotification system message handler here:
https://github.com/mozilla-b2g/gaia/blob/32b171e12d6e72598ddfa9137b401a27c0b66a2e/apps/sms/js/activity_handler.js#L459
3.) Re-flash and send SMS to Tarako
4.) Click on the toaster

Expected Result: see log from system message handler

Actual Result: no logs

Comment 4

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

noming for backlog.. unless I can automatically backlog this stuff?

Comment 5

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #4)
> noming for backlog.. unless I can automatically backlog this stuff?

The only thing I would add here is that if we can fix this it would make fixing bug 995907 (1.3t+) much easier and less hacky. I'm not making it block while I investigate workarounds in that bug. But if we can fix this quickly I would love to rely on it for bug 995907.

Comment 6

[Jason Smith [:jsmith]]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #4)
> noming for backlog.. unless I can automatically backlog this stuff?

Yeah, you can automatically backlog it.

Comment 7

[Michael Henretty [:mikehenrty][:mhenretty]]

Crap, I think this is invalid (ie. intentional behavior). It looks like for desktop-notification-click events we only send the system message if the app is not yet launched [1]. That is unfortunate for bug 995907.

1.) http://hg.mozilla.org/mozilla-central/file/0e91262606a6/b2g/chrome/content/shell.js#l826

Comment 8

[Julien Wajsberg [:julienw]]

I'd like to investigate if we can change this behavior if we have different pages as launch_path value and system message's URL or notification's href value.

Comment 9

[Julien Wajsberg [:julienw]]

The current behavior makes it necessary to register the system message handler in all pages even if one of these pages is specified in manifest.webapp for this system message. This is really wrong IMO.

A simple command line:
  find apps -name manifest.webapp -exec grep -A 5 '"messages" *:' {} \+

shows me that only costcontrol has a different message handler page than the main page. Reading their code, I see they do something weird to workaround the issue: [1].

[1] https://github.com/mozilla-b2g/gaia/blob/4457f14677f3a5ef626b7905dca584681401acc3/apps/costcontrol/js/widget.js#L67-L79

Comment 10

[Gene Lian (I already quit Mozilla)]

(In reply to Michael Henretty [:mhenretty] from comment #1)
> Fabrice, can this be fixed by uplifting bug 988787?

No, bug 988787 has nothing to do with this bug. That bug is about when to release the CPU wake lock. This bug is related to whether the system message is delivered or not.

Comment 11

[Gene Lian (I already quit Mozilla)]

Sorry, I'm a bit out of this thread. It seems this bug:

  "notification" system message does not fire when app is running (background or foreground)

is caused by comment #9:

  "The current behavior makes it necessary to register the system message handler in all pages even if one of these pages is specified in manifest.webapp for this system message."

But I didn't figure out this well. I'd like to clarify more about this. In the current system message behaviour, we have to claim the message's launch path in the manifest, like:

  "messages": [
     { "sms-received": "/index.html" },
     { "notification": "/index.html" }
  ],

so that the platform can know which page it needs to launch if the app's page (index.html) is not running. Also, the JS codes in that launch page (index.html) have to register for a system message handler to handle that system message.

So, the full conditions for successfully handling system message is:

  (1) manifest has to claim the system message's launch path
  (2) that page has to register for a system handler via mozSetMessageHandler(...)

Based on this, what's the issue here?

Comment 12

[Julien Wajsberg [:julienw]]

Sorry, I was especially confusing in comment 9, because I was talking about system messages and not notifications, which is the goal of this bug (that's why I should not work on Saturdays). My comment really belonged in another bug (which is maybe not open yet).

So, this bug is about firing the notification system message if a notification has no onclick handler but the app is launched.

Is this intended to work like this?

Comment 13

[Gene Lian (I already quit Mozilla)]

We have to talk to Vivien about this notification design. In [1], it only fires system message when the app is not running?

[1] http://hg.mozilla.org/mozilla-central/file/0e91262606a6/b2g/chrome/content/shell.js#l826

Comment 14

[Julien Wajsberg [:julienw]]

Discussed with Vivien, we don't send system messages if the Notification object that was instanciated is still alive, and we don't want to change this.

Comment 15

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Julien Wajsberg [:julienw] from comment #14)
> Discussed with Vivien, we don't send system messages if the Notification
> object that was instanciated is still alive, and we don't want to change
> this.

Out of curiosity, why don't we want to change this? Is it just that apps already rely on not receiving a system message for notification events when running?

Comment 16

[Julien Wajsberg [:julienw]]

Vivien will probably answer better than I could here :)

Comment 17

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

(In reply to Michael Henretty [:mhenretty] from comment #15)
> (In reply to Julien Wajsberg [:julienw] from comment #14)
> > Discussed with Vivien, we don't send system messages if the Notification
> > object that was instanciated is still alive, and we don't want to change
> > this.
> 
> Out of curiosity, why don't we want to change this? Is it just that apps
> already rely on not receiving a system message for notification events when
> running?

Well it's mostly because of the way Web Notifications are spec'ed right ? System messages is not part of any spec, and instead of hacking something that tieds those 2 specs (notifcations & system messages) together I feel like we should improve the Web Notifications API to cover the case where the app is killed instead of degrading the case where the app is alive!

That said, I could be wrong. Let needinfo? Ehsan and Anne to see:
 1. If they are aware of this issue at all
 2. If they have any other opinions

Comment 18

[Anne (:annevk)]

I have no knowledge of how we souped the notifications API and system messages together. If you could briefly explain the problem I might be able to help. I cannot make much from this bug.

Comment 19

[Anne (:annevk)]

After a brief chat with Julien (#webapi) my suggestion would be to always dispatch a system message for notification clicks. (I suspect the web-equivalent of this will be a service worker opt-in of sorts.)

Comment 20

[(no longer active)]

Sorry, I can't make much of this bug either.  Can someone please post a summary of what the issue mentioned in comment 17 is?

Comment 21

[Julien Wajsberg [:julienw]]

Basically we wanted to receive a system message instead of the click event handler when the user clicks a notification. Thi sway we can have a unique cpde path to handle this click event.

Ehsan, hope this makes it clearer. NI you so that you see the answer.

Anne's proposal makes sense to me. Should we reopen the bug then?

Comment 22

[(no longer active)]

Sure, sounds good.

Comment 23

[BMO Automation]

Firefox OS is not being worked on