L10nNotification will be a Gaia wrapper on regular Notification API that will allow us to use WebL10n API for Notifications.

Stas, Kevin, I'm looking for feedback here. The idea here is to: - make it easier to create localizable notifications in Gaia - remove the risk of running it before l10n is ready - remove a lot of mozL10n.get uses in the wild

Etienne, you seem to be one of the contributors to Notification Helper. How is it related to Notification API? Why does it use mozNotification? I think I'd like to refactor NotificationHelper to support titleL10n/bodyL10n. Should I also update it to use Notification API?

One thing I see I missed is that titleL10n and bodyL10n should be allowed to be {raw: 'string'} in line with https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#Writing_APIs_that_operate_on_L10nIDs I'll update that in the next patch.

Also, wrt. NotificationHelper. I see it hold references to Notifications. That could allow us to retranslate Notifications that are alive if the app that fired them is still alive.

Comment on attachment 8521885 [details] [diff] [review] l10nnot.patch Review of attachment 8521885 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for flagging me on this. I agree that it would be good to abstract our string handling for notifications somewhere, I'm not sure if this is the right place though. I think introducing yet another file and utility for notifications at this time is not ideal. My desire here would be to revamp notification_helper.js, either refactoring the send method, or potentially creating a new method which takes l10n args. I do like that you are using promises here, I think we should do something similar to that as well. Also flagging Etienne for feedback here.

Comment on attachment 8521885 [details] [diff] [review] l10nnot.patch Review of attachment 8521885 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Kevin Grandon :kgrandon (In Europe/Conf until 11/24) from comment #5) > Comment on attachment 8521885 [details] [diff] [review] > l10nnot.patch > > Review of attachment 8521885 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thank you for flagging me on this. I agree that it would be good to abstract > our string handling for notifications somewhere, I'm not sure if this is the > right place though. I think introducing yet another file and utility for > notifications at this time is not ideal. > > My desire here would be to revamp notification_helper.js, either refactoring > the send method, or potentially creating a new method which takes l10n args. > I do like that you are using promises here, I think we should do something > similar to that as well. > +1 The NotificationHelper was initially a workaround for a bug in the old notification API. We shouldn't use this API anymore (looks like we barely do). So I'd love it if we could remove the old and add the new l10n features rather than making another pile :)

(In reply to Kevin Grandon :kgrandon (In Europe/Conf until 11/24) from comment #5) > My desire here would be to revamp notification_helper.js, either refactoring > the send method, or potentially creating a new method which takes l10n args. From my perspective, I feel that designing separate API for l10n and non-l10n results in developers having to make a choice each time and later binding them or writing logic that makes switching between them hard. Designing polymorphic API [0] results in developers easily transitioning between {raw: "raw strong"} and {id: "l10nid", args: {}}, and using l10nId as default string param cues devs that this is the default approach we'd like them to take. [0] https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#Writing_APIs_that_operate_on_L10nIDs

(In reply to Etienne Segonzac (:etienne) from comment #6) > +1 > The NotificationHelper was initially a workaround for a bug in the old > notification API. > We shouldn't use this API anymore (looks like we barely do). Oh, awesome! > So I'd love it if we could remove the old and add the new l10n features > rather than making another pile :) So, should I just refactor NotificationHelper to use Notification API, promise and l10n API? Stats: "new Notification" - 73 uses "NotificationHelper.send" - 13 uses My only concern is that it seems that you guys perceive attaching one more js file as a burden (although we concat them and minify). Would you prefer such wrapper to live inside l10n.js?

NI Etienne about my questions :)

(In reply to Zibi Braniecki [:gandalf] from comment #8) > So, should I just refactor NotificationHelper to use Notification API, > promise and l10n API? That's the ideal way forward imo :) > > Stats: > "new Notification" - 73 uses > "NotificationHelper.send" - 13 uses > > My only concern is that it seems that you guys perceive attaching one more > js file as a burden (although we concat them and minify). Would you prefer > such wrapper to live inside l10n.js? I don't think it's a performance issue. It's more about "code browsability" when you want to debug a notification-related issue. The current NotificationHelper is pretty confusing in this regard right now :)

Ok, so my current approach is to modify NotificationHelper and its uses. The new NotificationHelper has getIconURI method and l10n method which takes titleL10n and options and returns a promise. I will move all uses of NotificationHelper.send to use NotificationHelper.l10n and then we can start tackling raw Notification calls to move them to use NotificationHelper.l10n in a follow up bug.

Etienne, can you review this? I'll ask for review from all affect apps owners/peers, but first wanted to get your r+ on the update to NotificationHelper class. The tests are passing except of one test in system (desktop_notifications) that tests a behavior of mozNotification that I think works differently in W3C Notification - the test expects that the notification will disappear when the app is closed. I guess I will want to remove that test if the app owner/peer agrees.

Comment on attachment 8531049 [details] pull request Looks good to me, but I would personally prefer to leave the method as .send().

Comment on attachment 8531049 [details] pull request Looking good! Just need the method name changed. Flagging Salva for the cost control part, and flagging Stas (who already started) more formally ;)

Comment on attachment 8531049 [details] pull request :mhenretty - On top of your verbal go-ahead, can I get a formal r+ on removal of that obsolete test? :)

Comment on attachment 8531049 [details] pull request Thanks for flagging me here, Etienne. r=me.

Comment on attachment 8531049 [details] pull request r=me on the removal of the deprecated test.

Comment on attachment 8531049 [details] pull request Marina, do you mind to review the part of receiving the low / zero balance notifications as you have working SIMs, please?

Comment on attachment 8531049 [details] pull request LGTM but please wait for Marina feedback before landing. Thank you.

Comment on attachment 8531049 [details] pull request It works fine, Thanks!

Thanks all! Commit: https://github.com/mozilla-b2g/gaia/commit/0833002fdf6a33d44590d6b8c728a03f7eccd977 Merge: https://github.com/mozilla-b2g/gaia/commit/256ffaa7ae85bc95cae269482fae7314fdbf2cc6

Added to l10n best practices: https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#Notification_API