1066385 - [System][Notifications] Add gaia support for behavior dict

Status: RESOLVED FIXED

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

Description

[Robert Bindar]

We need to add support in Gaia in order for apps to use the "behavior" dict.

Comment 1

[Robert Bindar]

Attachment: gaia_modes_dict

I think we need to land these changes all at once because otherwise we would have to firstly merge the SystemApp changes and disable the failing tests and then merge the apps updates and enable the tests again. If you're not ok with it let me know, I'll file some other bugs for that.

Comment 2

[James Burke [:jrburke]]

Comment on attachment 8488372 [details] [review]
gaia_modes_dict

The email change looks fine to me. After this lands, I will likely open a ticket that is email-specific to have our ux folks go over all the email notification scenarios to see if they want to adjust any of the behaviors, but this seems great as a baseline. Thank you!

Comment 3

[Robert Bindar]

Thanks James, there are more to come! About the behavior you guys discussed on Bug 1042361, it is more likely a B2G bug, from the spec perspective that's how the default behavior should be when replacing, no disrupting, we just need to edit it to be more clear on this matter.

Comment 4

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

Comment on attachment 8488372 [details] [review]
gaia_modes_dict

Looking good Robert!

Ok, next thing we need is integration tests. Let's have a new integration test file in the system app. We need at least two tests for each new behavior: true and false for the boolean ones, and pattern and no patter for the vibration one.

Comment 5

[Aus Lacroix [:aus]]

Comment on attachment 8488372 [details] [review]
gaia_modes_dict

lgtm. minor nit for code style in github.

Comment 6

[Robert Bindar]

I fixed the nits and added a couple of integration tests.

Comment 7

[Anthony Ricaud (:rik)]

I think this is a great opportunity to get rid of the noNotify/SILENT_APPLICATIONS/isResending logic. But that could be done in another patch I guess (involves changing call sites).

With the current patch, applications can't say "don't vibrate nor ring". It should be possible to pass empty options.

(In reply to Michael Henretty [:mhenretty] from comment #4)
> We need at least two tests for each new
> behavior: true and false for the boolean ones, and pattern and no patter for
> the vibration one.
I'd suggest three :) No pattern, empty pattern, pattern for vibration. No sound, empty sound, sound for sounds.

Comment 8

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

(In reply to Anthony Ricaud (:rik) from comment #7)
> I think this is a great opportunity to get rid of the
> noNotify/SILENT_APPLICATIONS/isResending logic. But that could be done in
> another patch I guess (involves changing call sites).
> 
> With the current patch, applications can't say "don't vibrate nor ring". It
> should be possible to pass empty options.

This is intentional. We did it this way because there were concerns in the whatwg mailing list that if we allowed users to disable vibration through this API, they might get a completely silent notification without meaning to. So for example, a user might want to disable vibration but play a sound. If the phone volume was turned all the way down they would get a silent notification without meaning to. To be clear, we fought for the ability to disable vibration, be we ultimately lost that battle for now.

See this thread for more info:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2014-August/297575.html

Comment 9

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

Comment on attachment 8488372 [details] [review]
gaia_modes_dict

Great job Robert! I left a couple of comments on github about the tests, but other than that I think we are good to go!


There are, however, a couple of weird issues with notification sounds, but I think we can fix these incrementally in follow ups:

- First, we don't wait for the soundFile to load before displaying the notification. This can be weird UX when loading a large sound file from the network (ie. the toaster displays, and then a moment later the sound is played).

- When a sound is longer than two seconds, we cut it off abruptly. It sounds like a record skipping. I think having the cutoff is a good idea, but the sound should fade out.

- If a sound file doesn't load for some reason, we fail silently. Would be nice to have some sort of log explaining what happened.

Please file follow-ups for these :)

Comment 10

[Anthony Ricaud (:rik)]

(In reply to Michael Henretty [:mhenretty] from comment #8)
> (In reply to Anthony Ricaud (:rik) from comment #7)
> > I think this is a great opportunity to get rid of the
> > noNotify/SILENT_APPLICATIONS/isResending logic. But that could be done in
> > another patch I guess (involves changing call sites).
> > 
> > With the current patch, applications can't say "don't vibrate nor ring". It
> > should be possible to pass empty options.
> 
> This is intentional. We did it this way because there were concerns in the
> whatwg mailing list that if we allowed users to disable vibration through
> this API, they might get a completely silent notification without meaning
> to. So for example, a user might want to disable vibration but play a sound.
> If the phone volume was turned all the way down they would get a silent
> notification without meaning to. To be clear, we fought for the ability to
> disable vibration, be we ultimately lost that battle for now.
> 
> See this thread for more info:
> http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2014-August/297575.html
Fair enough. We can't let applications opt out of one of them but can we let them opt out of vibration and sound at the same time? This is the use case we want for bug 1069981 and bug 1073020.

Comment 11

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

(In reply to Anthony Ricaud (:rik) from comment #10)
> Fair enough. We can't let applications opt out of one of them but can we let
> them opt out of vibration and sound at the same time? This is the use case
> we want for bug 1069981 and bug 1073020.

Good idea. I just spoke to Jonas about this, and filed bug 1073717 to add the "silent" behavior.

Comment 12

[Robert Bindar]

I fixed the nits, waiting for the try run.
Michael can you please

Comment 13

[Robert Bindar]

Sorry, commenting failure :). I'll checkin-needed this when the try run is done.

Comment 14

[Carsten Book [:Tomcat]]

master: https://github.com/mozilla-b2g/gaia/commit/b528ba93ced4a8f643462de266d1753f9bc053a5