1103560 - Use one system message handler for notification for System app

Status: RESOLVED FIXED

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

Description

[:gerard-majax]

We cannot have multiple system message handler for the "notification" message in the system app.

Currently, this is pre-empted by Find My Device. What we should do is:
 - have only one entry point, like |navigator.mozSetMessageHandler('notification')|
 - dispatch events based on either mozbehavior dict/content or based on the tag

This is needed to fix bug 1084247.

Comment 1

[:gerard-majax]

We should be able to do this using Service.register/Service.request

Comment 2

[:gerard-majax]

Attachment: Gaia PR

Comment 3

[:gerard-majax]

Alive just confirmed to me on IRC that this is clearly not a dupe of bug 931339.

Comment 4

[:gerard-majax]

Updated the PR

Comment 5

[:gerard-majax]

I'm being blocked for doing notification closing.

Comment 6

[:gerard-majax]

Comment on attachment 8528264 [details] [review]
Gaia PR

Michael, I would like your feedback on a couple of points:
 - regarding the way I'm storing the information to match the system app component user of the notification, i.e., adding a "systemMessageTarget" property to the data object in the notification
 - regarding the way I'm closing the notification, by directly calling NotificationScreen.

Specifically, on this last point, Notification.get() notifications do not have the id (it's not part of the spec), so we indeed have no clean way to distinguish each one, hence why I'm using NotificationScreen. That's one of my interrogations on this topic. The other one is where we should be doing this closing: I was thinking that, maybe, we would rather NotificationScreen.removeNotification() (or anything else) in the NotificationsSystemMessage object, prior or after publishing the event.

My last point of feedback is regarding any kind of races we may encounter: do you see anything that may go wrong?

This PR is WIP in the sense that there's no test yet, and probably a lot of existing ones are broken. But this is already working as expected with the screenshot case.

The ultimate goal is to migrate all Notification API users in the System App to follow this approach and indeed each one would get the ability to properly handle system message, if they need it.

Comment 7

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

Comment on attachment 8528264 [details] [review]
Gaia PR

(In reply to Alexandre LISSY :gerard-majax from comment #6)
>  - regarding the way I'm storing the information to match the system app
> component user of the notification, i.e., adding a "systemMessageTarget"
> property to the data object in the notification

I think this works fine. One thing that seems kind of clunky to me is using Services.request to create a new type of window event, and then register to listen to the event immediately after making the service request. Instead, we should explicitly register for the system message. So, with your use case, screenshot.js could do something like:

Service.request('handleNotificationSystemMessage', 'screenshot', this);
or...
Service.request('handleNotificationSystemMessage', 'screenshot', handler);

Then, your notifications_system_message.js module can explicitly call into the module handler rather than dispatching events on the window object. This seems to fall more in line with how Services.request works in other places (like with 'registerHierarchy').

>  - regarding the way I'm closing the notification, by directly calling
> NotificationScreen.

I don't think we should do this. Rather than giving special abilities to system app code (like calling directly into NotificationScreen), we should be leveraging the Notification API as much as possible. Managing pseudo-notifications like Download notifications causes all sorts of edge cases. Also, calling NotificationScreen.removeNotification doesn't remove the notification from the database, just the UI, so we are in a sense creating a zombie notification.

I see a couple of things we can do here. 1.) Call Notification.get() and iterate through all the notifications until we find one that has a body that matches the clicked notification, and then close that. Or 2.) repeat the body in the notification tag, and then fetch that tag. In either case, system app should have to do the same thing other apps have to do in this case (ie. use the API), and not call directly into other system modules. For reference, email does this: https://github.com/mozilla-b2g/gaia/blob/b9219822ce1d5f8cc28ed2749560872bb463cd94/apps/email/js/app_messages.js#L85

Or if we want to get really fancy we could fix bug 966481.

> The other one is where we should be doing
> this closing: I was thinking that, maybe, we would rather
> NotificationScreen.removeNotification() (or anything else) in the
> NotificationsSystemMessage object, prior or after publishing the event.

I think we should still leave it up to the handler whether or not to close the notification. So, we should think of screenshot.js in this case of a separate app that decides whether or not to close it's notification on click.

> My last point of feedback is regarding any kind of races we may encounter:
> do you see anything that may go wrong?

Not at first glance. Gecko makes sure systemMessages will be delivered after the handler is registered, and similarly the system app queues Service requests that happen before the associated Service register call. I think we are good on races.

> The ultimate goal is to migrate all Notification API users in the System App
> to follow this approach and indeed each one would get the ability to
> properly handle system message, if they need it.

+1000

Comment 8

[:gerard-majax]

(In reply to Michael Henretty [:mhenretty] from comment #7)
> Comment on attachment 8528264 [details] [review]
> WIP Gaia PR
> 
> (In reply to Alexandre LISSY :gerard-majax from comment #6)
> >  - regarding the way I'm storing the information to match the system app
> > component user of the notification, i.e., adding a "systemMessageTarget"
> > property to the data object in the notification
> 
> I think this works fine. One thing that seems kind of clunky to me is using
> Services.request to create a new type of window event, and then register to
> listen to the event immediately after making the service request. Instead,
> we should explicitly register for the system message. So, with your use
> case, screenshot.js could do something like:
> 
> Service.request('handleNotificationSystemMessage', 'screenshot', this);

I've implemented it.

> or...
> Service.request('handleNotificationSystemMessage', 'screenshot', handler);
> 
> Then, your notifications_system_message.js module can explicitly call into
> the module handler rather than dispatching events on the window object. This
> seems to fall more in line with how Services.request works in other places
> (like with 'registerHierarchy').
> 
> >  - regarding the way I'm closing the notification, by directly calling
> > NotificationScreen.
> 
> I don't think we should do this. Rather than giving special abilities to
> system app code (like calling directly into NotificationScreen), we should
> be leveraging the Notification API as much as possible. Managing
> pseudo-notifications like Download notifications causes all sorts of edge
> cases. Also, calling NotificationScreen.removeNotification doesn't remove
> the notification from the database, just the UI, so we are in a sense
> creating a zombie notification.

100% agree, but that was just for the PoC for now :)

> 
> I see a couple of things we can do here. 1.) Call Notification.get() and
> iterate through all the notifications until we find one that has a body that
> matches the clicked notification, and then close that. Or 2.) repeat the
> body in the notification tag, and then fetch that tag. In either case,
> system app should have to do the same thing other apps have to do in this
> case (ie. use the API), and not call directly into other system modules. For
> reference, email does this:
> https://github.com/mozilla-b2g/gaia/blob/
> b9219822ce1d5f8cc28ed2749560872bb463cd94/apps/email/js/app_messages.js#L85

I'm not a big fan of using the tag for something that is not really a tag. We now have data payload, so maybe we can use it here: save an id when we generate the notification and use it for matching after.

> 
> Or if we want to get really fancy we could fix bug 966481.

Oh yes this one. .. I'm not sure how hard it is to fix this one.

> 
> > The other one is where we should be doing
> > this closing: I was thinking that, maybe, we would rather
> > NotificationScreen.removeNotification() (or anything else) in the
> > NotificationsSystemMessage object, prior or after publishing the event.
> 
> I think we should still leave it up to the handler whether or not to close
> the notification. So, we should think of screenshot.js in this case of a
> separate app that decides whether or not to close it's notification on click.

Which was my feeling, good to see it's shared. The logic for closing itself ca, however, be exposed in this module.

We could even have a service exposed, so that we could:

> Service.request('closeSystemMessageNotification', ID);

And then in the implementation of closeSystemMessageNotification we would do the Notification.get() logic.

> 
> > My last point of feedback is regarding any kind of races we may encounter:
> > do you see anything that may go wrong?
> 
> Not at first glance. Gecko makes sure systemMessages will be delivered after
> the handler is registered, and similarly the system app queues Service
> requests that happen before the associated Service register call. I think we
> are good on races.
> 
> > The ultimate goal is to migrate all Notification API users in the System App
> > to follow this approach and indeed each one would get the ability to
> > properly handle system message, if they need it.
> 
> +1000

BTW, PR updated :)

Comment 9

[:gerard-majax]

Comment on attachment 8528264 [details] [review]
Gaia PR

That's fixing the feedback you gave earlier. I've also covered screenshot_test.js with what was lacking.

This looks more mature, now. If you don't have any big point against, then I'll continue with tests for the new module and proceed with its tests, and migrate the rest of the users of Notification to this.

Comment 10

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

Comment on attachment 8528264 [details] [review]
Gaia PR

I don't think we should do this:

Service.request('closeSystemMessageNotification', message.data.systemId);

Requiring a special data field called 'systemId' for all system notifications seems unnecessarily rigid and obscure. Again I think the best approach is leaving it up to the module to uniquely identify it's own notifications and close the appropriate one when handling system messages. This falls more in line with how the notification system message works for other apps (like email), and I think the system app should follow the same principals.

More specifically, for the case of screenshot notifications I think we should use the filename (maybe with a `screenshot_` prefix) as the tag which we then  use to close the appropriate notification when handling the system message. Uniquely identifying notifications is what the tag is for, so this is indeed an appropriate use of the tag attribute.

Comment 11

[:gerard-majax]

Ok. Going back to using a tag (screenshot:timestamp), and data.systemMessageTarget.
We need to find a way to handle previously saved notifications.

Comment 12

[:gerard-majax]

Handling of old notifications that do not have the systemMessageTarget has a proposal now.
We may also want to queue messages: system message may be triggered while the module has not been started yet.

Comment 13

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #12)
> Handling of old notifications that do not have the systemMessageTarget has a
> proposal now.
> We may also want to queue messages: system message may be triggered while
> the module has not been started yet.

Or we just leave it this way: until there is a module to handle it, then nothing will happen and the user can still tap on it.

Comment 14

[:gerard-majax]

Michael, do you mind having another look to give feedback ? I've followed your advice:
 - data.systemMessageTarget for the module responsible
 - using a tag, in the form 'module:ID'
 - closing on the module side, with tags

I've also had to add handling of update cases: resent notification that do not have the data.systemMessageTarget field set, nor a tag. For this case, the proposal is that the user module (here, screenshot) provides a way to match the system message notification. This way, when the system message handler is called, and if we don't have data.systemMessageTarget, we ask knowns module if one can handle the payload, and if it does, we deletage him.

If you see a better way to handle this, feel free to suggest :). It pushes a bit some constraints on the module itself, but I hope we have enough liberty in the current implementation:
 (1) define a field that we should have to trigger the matching,
 (2) define a function that we will call to test if there is a match

Maybe we should just pass the whole system notification message payload for step (2).

Comment 15

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

(In reply to Alexandre LISSY :gerard-majax from comment #14)
> I've also had to add handling of update cases: resent notification that do
> not have the data.systemMessageTarget field set, nor a tag. For this case,
> the proposal is that the user module (here, screenshot) provides a way to
> match the system message notification. This way, when the system message
> handler is called, and if we don't have data.systemMessageTarget, we ask
> knowns module if one can handle the payload, and if it does, we deletage him.

This is pretty neat and robust Alex. But ultimately I think it is introducing a lot complexity for handling legacy notifications which will just go away soon after upgrade anyway. Personally I think that when notifications_system_message.js cannot find a systemTarget for a notification, it should just close that notification (by doing a Notication.get() and then matching on every field) and console.warn that we closed an unhandled notification system message. Maybe this is a bit heavy-handed, but to me the goal of this bug is to fix system message notifications going forward, and not have to jump through too many hoops to fix existing notifications which will quickly die after upgrade anyway.

But I leave the decision up to you. If you want to do this, my suggestion would be to just have the compatibility logic directly in findMatchingCompatibility (ie. that function knows that a message.body of 'screenshotXXXXX.png' is a systemMessageTarget of 'screenshot'). I know this is less modular than your solution, but it avoids the complexity of modules having to register a compatibility field and function for a dying edge case. I think the code is easier to read/understand if the compatibility table is all in one place. Again though, I agree your solution is the most robust. But notifications come and go all the time and so rigid correctness can be sacrificed sometimes in favor of simplifying the code path. That is my perspective anyway, but the choice is yours :)

Comment 16

[:gerard-majax]

I have rethought about it over the weekend, and I agree with the over-engineering part.

Comment 17

[:gerard-majax]

Comment on attachment 8528264 [details] [review]
Gaia PR

Michael, do you mind doing another pass of feedback ? We should be good for everything.

Comment 18

[:gerard-majax]

This will impact Find My Device

Comment 19

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

Comment on attachment 8528264 [details] [review]
Gaia PR

Love all the tests for this!

Comment 20

[:gerard-majax]

Thanks, I'll resume my work on this now that we have an agreement.

Comment 21

[:gerard-majax]

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=8750469e942e
Green locally, fixes all the current known uses: Screenshot, FindMyDevice. I need to make sure everything is okay with find my device, though.

Comment 22

[:gerard-majax]

Tested on my Mulet build, it looks good.

Comment 23

[:gerard-majax]

Comment on attachment 8528264 [details] [review]
Gaia PR

I've asked each of you for:
 - Michael, for the Notification-related part. You already gave f+ so it should be quite straight forward, there was not a lot of changes
 - Tim, as being a system peer and for the screenshot part, since I see you did a lot of reviews on it
 - Guilherme, for the FindMyDevice part

Thanks for reviewing!

Comment 24

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8528264 [details] [review]
Gaia PR

I made a first pass asking questions to make sure I understand the change. If they're too stupid, then I probably don't understand it; feel free to ping me to discuss :)

Also, thanks for adding tests for our notifications!

Comment 25

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

Comment on attachment 8528264 [details] [review]
Gaia PR

Good job!

Comment 26

[:gerard-majax]

Guiherme, I don't know why but I don't see your comments on the parts of code they are intended to be. This makes me hardly able to read them :(.

You were right on the two first ones, I have been too fast on hitting r? :)

Comment 27

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8528264 [details] [review]
Gaia PR

I don't understand this part of System enough to review the patch.

Comment 28

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8528264 [details] [review]
Gaia PR

Well done! Some nits and I expect the usage of this service is documented in the user story of the bug as well as in the code :)

Comment 29

[:gerard-majax]

Thanks alive, I've put some comments, can you have a look and tell me if it's enough ?

Comment 30

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alexandre LISSY :gerard-majax from comment #29)
> Thanks alive, I've put some comments, can you have a look and tell me if
> it's enough ?

Nice :)

Comment 31

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8528264 [details] [review]
Gaia PR

Looks good to me, I only had a few nits about the tests. The only reason why I'm not giving the r+ yet is that I don't totally understand why we need to use requireApp() in the tests now. It would be nice if we could avoid that and require() everything in one place.

Hopefully the GitHub comments went through this time, but feel free to ping if they didn't.

Comment 32

[:gerard-majax]

Comment on attachment 8528264 [details] [review]
Gaia PR

Final round, everything should be addressed

Comment 33

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8528264 [details] [review]
Gaia PR

Thanks!

Comment 34

[:gerard-majax]

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=ac7a64136d88
All green after retriggers.

Comment 35

[:gerard-majax]

Latest repush was just to update the r=...

Comment 36

[:gerard-majax]

https://github.com/mozilla-b2g/gaia/commit/3a4971da3b9ccb91474f36a413990484694f6623

Comment 37

[(PTO June 19-July 7) Ryan VanderMeulen]

Blocks a blocker (bug 1084247).

Comment 38

[Gregor Wagner [:gwagner]]

It has been on master for a while and we need it to fix a blocker.

Comment 39

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

Comment on attachment 8528264 [details] [review]
Gaia PR

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): not a regression.

[User impact] if declined:
Bug 1084247 will happen, which means when rebooting the phone, certain notifications initiated by the system (like screenshots) won't do anything when clicking on them.

[Testing completed]:
Manually tested, and many many new automated tests.

[Risk to taking this patch] (and alternatives if risky):
This is not a small patch. However, it has been baking on master for a couple of months now, and it has a lot of tests. I would say the risk is small at this point. There are no alternatives to fixing bug 1084247.

[String changes made]: none.

Comment 40

[(PTO June 19-July 7) Ryan VanderMeulen]

v2.2: https://github.com/mozilla-b2g/gaia/commit/50bb7ec7221d3333f679ee9b321615154f6cb230