1213455 - Complete the implementation of chrome.notifications

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Andy McKay]

The methods on the API are:

https://developer.chrome.com/extensions/notifications

Currently implemented:

create − chrome.notifications.create(string notificationId, NotificationOptions options, function callback)
clear − chrome.notifications.clear(string notificationId, function callback)
getAll − chrome.notifications.getAll(function callback)

onClosed

To be implemented:

Types

TemplateType
PermissionLevel
NotificationOptions

Methods

update − chrome.notifications.update(string notificationId, NotificationOptions options, function callback)
getPermissionLevel − chrome.notifications.getPermissionLevel(function callback)

Events

onClicked
onButtonClicked
onPermissionLevelChanged
onShowSettings

Comment 1

[Mindaugas <LA-MJ>]

Clear does not actually do anything for me on Linux. It fails silently.

Of course, the notification only lives for 10 seconds or so anyway.

Is that a desirable behavior?

Comment 2

[Stephan Sokolow]

(In reply to 4mr.minj from comment #1)
> Clear does not actually do anything for me on Linux. It fails silently.
> 
> Of course, the notification only lives for 10 seconds or so anyway.
> 
> Is that a desirable behavior?

That may be outside Firefox's control. If it's presenting them via the system notification API (most ideal), some notification hosts (eg. KDE's Plasma desktop) ignore requests by applications to dismiss notifications because it takes control away from the user.

Comment 3

[Bob Silverberg [:bsilverberg]]

(In reply to Andy McKay [:andym] from comment #0)
> 
> To be implemented:

I'm not sure that we actually want to implement all of these, as some don't make sense/apply:

> 
> Types
> 
> TemplateType

As we are using nsIAlertsService [1], I'm not sure that this applies. We can only create one type of notification, "basic", and this is documented on MDN [2]. Should we have code that only accepts "basic" for `TemplateType` in `NotificationOptions`, or should we just accept any value and ignore whatever is passed as we will always create a "basic" type?

> PermissionLevel

Does this apply? It sounds like it has to do with whether the user has granted permission to the extension to show notifications. Is that something we support or plan to support?

> NotificationOptions

I added this to the schema, but again some of the items documented for Chrome do not apply. Should we only add those items to the schema that apply, or should we add all of them but ignore any that we won't use (which would allow users to specify them without generating an error, but would have the downside of the behaviour the user requested not being produced)?

> 
> Methods
> 
> update − chrome.notifications.update(string notificationId,
> NotificationOptions options, function callback)

This we can and should do.

> getPermissionLevel − chrome.notifications.getPermissionLevel(function
> callback)

As with PermissionLevel above, does this apply to us? If so, how would this be implemented? Where would we store this PermissionLevel?

> 
> Events
> 
> onClicked

This has already been implemented via `ignoreEvent` but I think we can implement it with behaviour as an alert created via nsIAlertsService does support at `alertListener`.

> onButtonClicked

This does not apply and is currently implemented via `ignoreEvent`, which seems appropriate.

> onPermissionLevelChanged

As above, I’m not sure if this applies and if it doesn’t should it be marked `unsupported` or should it simply be ignored?

> onShowSettings

I’m not sure if this applies and if it doesn’t should it be marked `unsupported` or should it simply be ignored?

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIAlertsService
[2] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/Notifications/TemplateType

Comment 4

[Bob Silverberg [:bsilverberg]]

Actually, I'm not sure that `update` applies either. It doesn't look like there's a way to update an alert in place using nsIAlertsService. It seems like the only option would be to create a new notification with the updated values, which doesn't sound like the same thing as updating a notification in place. I'm not sure what we can do other than mark this as unsupported, unless we propose either changing nsIAlertsService to support this type of behaviour, or using something other than nsIAlertsService.

Comment 5

[Stephan Sokolow]

(In reply to Bob Silverberg [:bsilverberg] from comment #3)
> > onButtonClicked
> 
> This does not apply and is currently implemented via `ignoreEvent`, which
> seems appropriate.


Forgive my ignorance, but how does it not apply?

Adding the described "up to two buttons" to the notification popup is possible on Linux via the "actions" facility of the notification spec and it's an incredibly useful thing to support.

https://developer.gnome.org/notification-spec/

Comment 6

[Bob Silverberg [:bsilverberg]]

(In reply to Stephan Sokolow from comment #5)
> (In reply to Bob Silverberg [:bsilverberg] from comment #3)
> > > onButtonClicked
> > 
> > This does not apply and is currently implemented via `ignoreEvent`, which
> > seems appropriate.
> 
> 
> Forgive my ignorance, but how does it not apply?
> 
> Adding the described "up to two buttons" to the notification popup is
> possible on Linux via the "actions" facility of the notification spec and
> it's an incredibly useful thing to support.
> 
> https://developer.gnome.org/notification-spec/

My apologies, Stephan, I may not have been clear on what I meant. I was referring to whether it is something that we can currently implement with nsIAlertsService or not.

Thank you for pointing out that it is a desired feature and also how Linux supports it. It may be that it remains unsupported for the first release of web extensions, but is something we endeavour to support in the future.

Comment 7

[Bob Silverberg [:bsilverberg]]

Oh, I see there is already a bug opened (bug 1190681) to add button support, which is listed as blocking this bug, so it looks like it is something we plan to support.

Comment 8

[Bob Silverberg [:bsilverberg]]

As the completion of this bug depends on bug 1190681, I'm going to unassign myself for now. If I am going to work on some parts of this I will create sub-bugs for them. Sorry for all the noise on this bug, but the questions I listed above still do need to be answered.

Comment 9

[Kris Maglione [:kmag]]

(In reply to Bob Silverberg [:bsilverberg] from comment #3)
> > TemplateType
> 
> As we are using nsIAlertsService [1], I'm not sure that this applies. We can
> only create one type of notification, "basic", and this is documented on MDN
> [2]. Should we have code that only accepts "basic" for `TemplateType` in
> `NotificationOptions`, or should we just accept any value and ignore
> whatever is passed as we will always create a "basic" type?

We can add support for these to the alerts service without too much
difficulty.

> > PermissionLevel
> 
> Does this apply? It sounds like it has to do with whether the user has
> granted permission to the extension to show notifications. Is that something
> we support or plan to support?
>
> > getPermissionLevel − chrome.notifications.getPermissionLevel(function
> > callback)
> 
> As with PermissionLevel above, does this apply to us? If so, how would this
> be implemented? Where would we store this PermissionLevel?
> > onPermissionLevelChanged
> 
> As above, I’m not sure if this applies and if it doesn’t should it be marked
> `unsupported` or should it simply be ignored?

Yes, we should support these, at least as stubs, in case we decide to add more
support in the future.

> This has already been implemented via `ignoreEvent` but I think we can
> implement it with behaviour as an alert created via nsIAlertsService does
> support at `alertListener`.

Yes, we should support this.

> > onShowSettings
> 
> I’m not sure if this applies and if it doesn’t should it be marked
> `unsupported` or should it simply be ignored?

I don't see any reason to support this, at least for the foreseeable future.

Comment 10

[Andy McKay]

The only issue left is bug 1190681 and it feels like this tracker doesn't have much purpose anymore. Notifications work pretty well.