Closed Bug 1095109 Opened 6 years ago Closed 6 years ago

Update NotificationHelper to use L10n API and new W3C Notification API

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

()

Details

Attachments

(1 file, 1 obsolete file)

L10nNotification will be a Gaia wrapper on regular Notification API that will allow us to use WebL10n API for Notifications.
Attached patch l10nnot.patch (obsolete) — Splinter Review
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
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Attachment #8521885 - Flags: feedback?(stas)
Attachment #8521885 - Flags: feedback?(kgrandon)
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.
Attachment #8521885 - Flags: feedback?(kgrandon) → feedback?(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 :)
Attachment #8521885 - Flags: feedback?(etienne)
(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?
Attachment #8521885 - Flags: feedback?(stas)
NI Etienne about my questions :)
Flags: needinfo?(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 :)
Flags: needinfo?(etienne)
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.
Attached file 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.
Attachment #8521885 - Attachment is obsolete: true
Attachment #8531049 - Flags: review?(etienne)
Attachment #8531049 - Flags: feedback?(kgrandon)
Comment on attachment 8531049 [details] [review]
pull request

Looks good to me, but I would personally prefer to leave the method as .send().
Attachment #8531049 - Flags: feedback?(kgrandon)
Blocks: 952453
Summary: Add L10nNotification API → Update NotificationHelper to use L10n API and new W3C Notification API
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 ;)
Attachment #8531049 - Flags: review?(stas)
Attachment #8531049 - Flags: review?(salva)
Attachment #8531049 - Flags: review?(etienne)
Attachment #8531049 - Flags: review+
Duplicate of this bug: 938541
Duplicate of this bug: 948449
Duplicate of this bug: 948349
Duplicate of this bug: 948338
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? :)
Attachment #8531049 - Flags: review?(mhenretty)
Comment on attachment 8531049 [details] [review]
pull request

Thanks for flagging me here, Etienne.  r=me.
Attachment #8531049 - Flags: review?(stas) → review+
Comment on attachment 8531049 [details] [review]
pull request

r=me on the removal of the deprecated test.
Attachment #8531049 - Flags: review?(mhenretty) → review+
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?
Attachment #8531049 - Flags: feedback?(marina.rodriguez.iglesias)
Comment on attachment 8531049 [details] [review]
pull request

LGTM but please wait for Marina feedback before landing. Thank you.
Attachment #8531049 - Flags: review?(salva) → review+
Comment on attachment 8531049 [details] [review]
pull request

It works fine, Thanks!
Attachment #8531049 - Flags: feedback?(marina.rodriguez.iglesias) → feedback+
Duplicate of this bug: 1025628
Duplicate of this bug: 1025629
You need to log in before you can comment on or make changes to this bug.