Closed Bug 1254291 Opened 4 years ago Closed 4 years ago

Add schema for the notifications API

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.2 - Apr 4
Tracking Status
firefox48 --- fixed

People

(Reporter: wbamberg, Assigned: bsilverberg)

References

Details

(Whiteboard: [notifications])

Attachments

(3 files)

Attached file notify-bug.xpi
chrome.notifications.create takes 3 arguments:
* ID (optional)
* notificationOptions (mandatory)
* callback (optional)

If you omit ID, but provide callback, then the notification displayed is empty.

I've attached a WebExtension that demonstrates this.
This is because we don't have a schema for the notifications API yet. I see we don't have a bug for it either.
Summary: notifications are empty when ID is omitted and callback is supplied → Add schema for the notifications API
Whiteboard: [notifications]
Flags: blocking-webextensions?
Assignee: nobody → amckay
I'm stealing this from Andy.
Assignee: amckay → bob.silverberg
Status: NEW → ASSIGNED
Blocks: 1213455
Also update ext-notifications.js to use a Map instead of a Set

Note that I am not sure whether the schema has been implemented exactly as we'd want. There are, for example, options in NotificationOptions that we will never use/support, so I'm not sure how to document that.

Review commit: https://reviewboard.mozilla.org/r/39783/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39783/
Attachment #8730258 - Flags: review?(kmaglione+bmo)
Flags: blocking-webextensions? → blocking-webextensions+
Comment on attachment 8730258 [details]
MozReview Request: Bug 1254291 - Add schema for the notifications API, r?kmag

https://reviewboard.mozilla.org/r/39783/#review36607

::: toolkit/components/extensions/ext-notifications.js:88
(Diff revision 1)
> -        } else {
> -          [notificationId, options, callback] = args;
> -        }
> -
>          if (!notificationId) {
>            notificationId = nextId++;

We need to make this a string, since we're enforcing the type now.

::: toolkit/components/extensions/ext-notifications.js:94
(Diff revision 1)
>          }
>  
>          // FIXME: Lots of options still aren't supported, especially
>          // buttons.
>          let notification = new Notification(extension, notificationId, options);
> -        notificationsMap.get(extension).add(notification);
> +        notificationsMap.get(extension).set(notificationId, notification);

What happens when a notification with this ID already exists?

::: toolkit/components/extensions/ext-notifications.js:109
(Diff revision 1)
> -
> -        return context.wrapPromise(Promise.resolve(cleared), callback);
>        },
>  
> -      getAll: function(callback) {
> -        let notifications = notificationsMap.get(extension);
> +      getAll: function() {
> +        let result = Array.from(notificationsMap.get(extension).values(), notification => notification.id);

`let result = Array.from(notificationMap.keys())`

Although, annoyingly, see bug 1254359.

::: toolkit/components/extensions/schemas/notifications.json:45
(Diff revision 1)
> +      },
> +      {
> +        "id": "NotificationOptions",
> +        "type": "object",
> +        "properties": {
> +          "type": {

This needs to be optional when used with `.update()`. Same for several of the other properties below.

::: toolkit/components/extensions/schemas/notifications.json:74
(Diff revision 1)
> +            "type": "string"
> +          },
> +          "priority": {
> +            "optional": true,
> +            "description": "Priority ranges from -2 to 2. -2 is lowest priority. 2 is highest. Zero is default.",
> +            "type": "number"

`"minimum": -2, "maximum": 2`

::: toolkit/components/extensions/schemas/notifications.json:95
(Diff revision 1)
> +            "items": { "$ref": "NotificationItem" }
> +          },
> +          "progress": {
> +            "optional": true,
> +            "description": "Current progress ranges from 0 to 100.",
> +            "type": "number"

`"minimum": 0, "maximum": 100`

::: toolkit/components/extensions/schemas/notifications.json:124
(Diff revision 1)
> +            "name": "options",
> +            "description": "Contents of the notification."
> +          },
> +          {
> +            "type": "function",
> +            "name": "callback",

`"optional": true`

::: toolkit/components/extensions/schemas/notifications.json:154
(Diff revision 1)
> +            "name": "options",
> +            "description": "Contents of the notification to update to."
> +          },
> +          {
> +            "type": "function",
> +            "name": "callback",

`"optional": true`

::: toolkit/components/extensions/schemas/notifications.json:178
(Diff revision 1)
> +            "name": "notificationId",
> +            "description": "The id of the notification to be updated."
> +          },
> +          {
> +            "type": "function",
> +            "name": "callback",

`"optional": true`
Attachment #8730258 - Flags: review?(kmaglione+bmo)
Attachment #8730258 - Flags: review?(kmaglione+bmo)
Comment on attachment 8730258 [details]
MozReview Request: Bug 1254291 - Add schema for the notifications API, r?kmag

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

> What happens when a notification with this ID already exists?

The docs say that if an id is passed which already exists then the existing notification should be cleared [1], so I updated the code to do that. I should also update the test for onClosed, which is in another patch, to test creating a notification with the same id as an existing one, and I will do that in the other patch.

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/notifications/create#Parameters
Attachment #8730258 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8730258 [details]
MozReview Request: Bug 1254291 - Add schema for the notifications API, r?kmag

https://reviewboard.mozilla.org/r/39783/#review36927
Keywords: checkin-needed
Thanks for the heads up Wes. I am currently looking into this.
Flags: needinfo?(bob.silverberg)
FTR, the leaks produced (which I can reproduce locally with a debug build) are:

     |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
     |                                      | Per-Inst   Leaked|   Total      Rem|
   0 |TOTAL                                 |       34      416| 1223625        5|
 349 |Mutex                                 |       32       32|    1077        1|
 690 |imgLoader                             |      336      336|       3        1|
 691 |imgMemoryReporter                     |       32       32|       1        1|
1159 |nsStringBuffer                        |        8        8|  105412        1|
1199 |nsTArray_base                         |        8        8|  286137        1|

I was able to get rid of the leaks by changing the test code, but I'm not sure that the real issue is being addressed. 

The leak seems to be generated when we pass an actual URL into the `iconUrl` property of NotificationOptions, which has become mandatory with the new schema. If I pass a value of "" into that property the leaks go away, but doing that doesn't really accurately test the API as we would expect a real URL to be passed.

I am currently passing `browser.runtime.getURL("data/a.png")` and that causes leaks, as does simply `browser.runtime.getURL("/")`. I'm not sure if these leaks are test-only artifacts or whether they represent real leaks that would be experienced by a user if they pass a real URL into the iconUrl property of NotificationOptions. In any case it seems like we should be able to run a test with an actual URL.

I'm also not sure whether the problem results from my use of browser.runtime.getUrl, and maybe generating a url by some other method might resolve the issue.

Kris, what advice do you have about what we can do to address these leaks, other than simply changing the tests to pass "" into iconUrl?
Flags: needinfo?(kmaglione+bmo)
Can you build a simple test case that just uses the alert service directly and replicates the leak?  That would pinpoint the issue there, then you could file a separate bug on that component, leave "" in the extension test for the time being and make a note to come back and restore the URL when the underlying bug is fixed.
Attached file cclogs from test run
Here are the cclogs from running the test which generates the leaks.
Attachment #8731647 - Flags: feedback?(kmaglione+bmo)
Sorry, I forgot to comment on this last week.

The CC logs weren't helpful, but after looking through the notification center code, I'm pretty sure this is happening because the image loader is being cached, and not cleared in time for the leak checks at shutdown.
Flags: needinfo?(kmaglione+bmo)
Attachment #8731647 - Flags: feedback?(kmaglione+bmo)
Thanks for the update Kris, although I'm not sure where that leaves this patch. Is there something we can do in the test code to avoid the leak check failure? If we're sure this isn't an issue is there a way to flag the leak as expected?
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8730258 [details]
MozReview Request: Bug 1254291 - Add schema for the notifications API, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39783/diff/2-3/
Flags: needinfo?(kmaglione+bmo)
Blocks: 1260528
No longer blocks: 1260528
I believe the leaks have been addressed.
Keywords: checkin-needed
Blocks: 1190324
Iteration: --- → 48.2 - Apr 4
https://hg.mozilla.org/mozilla-central/rev/adc196371da9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.