1190324 - Test coverage for notifications extension API

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Gabor Krizsanits (INACTIVE)]

https://wiki.mozilla.org/Browser_Extensions#List_of_supported_APIs

Comment 1

[Kris Maglione [:kmag]]

This is still missing coverage for:

* Notifications with an |iconUrl| property.
* Notification observers being fired with the "alertfinished" topic.
* The |create| API method with the 1-arg form or without a notification ID.
* The |clear| API method.
* The |onClosed| event.


https://people.mozilla.org/~kmaglione/webextension-test-coverage/toolkit/components/extensions/ext-notifications.js.html

Comment 2

[Bob Silverberg [:bsilverberg]]

I will work on this next.

Comment 3

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1190324 - Test coverage for notifications extension API, r?kmag

Add coverage:

* Notification observers being fired with the "alertfinished" topic.
* The |create| API method without a notification ID.
* The |clear| API method.
* The |onClosed| event.

Review commit: https://reviewboard.mozilla.org/r/40069/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40069/

Comment 4

[Bob Silverberg [:bsilverberg]]

Note that the coverage for Notifications with an |iconUrl| property is addressed in bug 1254291.

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8730650 [details]
MozReview Request: Bug 1190324 - Test coverage for notifications extension API, r?kmag

https://reviewboard.mozilla.org/r/40069/#review36617

::: toolkit/components/extensions/ext-notifications.js:89
(Diff revision 1)
>    return {
>      notifications: {
>        create: function(notificationId, options) {
>          if (!notificationId) {
>            notificationId = nextId++;
> +          notificationId = notificationId.toString();

`notificationId = String(nextId++)`

Comment 6

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8730650 [details]
MozReview Request: Bug 1190324 - Test coverage for notifications extension API, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40069/diff/1-2/

Comment 7

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/40069/#review36617

> `notificationId = String(nextId++)`

This is fixed now (in the commit for adding the schema), but I also added a new test that checks the logic for a pre-existing notification. You may want to review that before I request checkin.

Comment 8

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8730650 [details]
MozReview Request: Bug 1190324 - Test coverage for notifications extension API, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40069/diff/2-3/

Comment 9

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/40069/#review36937

::: toolkit/components/extensions/ext-notifications.js:88
(Diff revision 1)
>  extensions.registerSchemaAPI("notifications", "notifications", (extension, context) => {
>    return {
>      notifications: {
>        create: function(notificationId, options) {
>          if (!notificationId) {
>            notificationId = nextId++;

I think you may have rebased this in the wrong order.

Comment 10

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/40069/#review36937

> I think you may have rebased this in the wrong order.

I thought I might have done something wrong too, but that file isn't even touched by this patch, and this is meant to land on top of https://reviewboard.mozilla.org/r/39783/#review36927, so when that happens everything will be ok, won't it?

Comment 11

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de794e145da1

Comment 12

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8730650 [details]
MozReview Request: Bug 1190324 - Test coverage for notifications extension API, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40069/diff/3-4/

Comment 13

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ce97832e038

Comment 14

[Bob Silverberg [:bsilverberg]]

Please land *after* bug 1254291, for which I added checkin-needed about 10 hours ago.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5851bac416b3

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5851bac416b3