1095109 - Update NotificationHelper to use L10n API and new W3C Notification API

Status: RESOLVED FIXED

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

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: l10nnot.patch

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

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

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?

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

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 5

[Kevin Grandon :kgrandon]

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 6

[Etienne Segonzac (:etienne)]

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 :)

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

(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

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

(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?

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

NI Etienne about my questions :)

Comment 10

[Etienne Segonzac (:etienne)]

(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 :)

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: pull request

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 13

[Kevin Grandon :kgrandon]

Comment on attachment 8531049 [details] [review]
pull request

Looks good to me, but I would personally prefer to leave the method as .send().

Comment 14

[Etienne Segonzac (:etienne)]

Comment on attachment 8531049 [details] [review]
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 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8531049 [details] [review]
pull request

:mhenretty - On top of your verbal go-ahead, can I get a formal r+ on removal of that obsolete test? :)

Comment 20

[Staś Małolepszy :stas]

Comment on attachment 8531049 [details] [review]
pull request

Thanks for flagging me here, Etienne.  r=me.

Comment 21

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

Comment on attachment 8531049 [details] [review]
pull request

r=me on the removal of the deprecated test.

Comment 22

[Salvador de la Puente González [:salva]]

Comment on attachment 8531049 [details] [review]
pull request

Marina, do you mind to review the part of receiving the low / zero balance notifications as you have working SIMs, please?

Comment 23

[Salvador de la Puente González [:salva]]

Comment on attachment 8531049 [details] [review]
pull request

LGTM but please wait for Marina feedback before landing. Thank you.

Comment 24

[Marina Rodríguez [:mai]]

Comment on attachment 8531049 [details] [review]
pull request

It works fine, Thanks!

Comment 25

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thanks all!

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

Comment 26

[Zibi Braniecki [:zbraniecki][:gandalf]]

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