Closed
Bug 1254291
Opened 8 years ago
Closed 8 years ago
Add schema for the notifications API
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox48 fixed)
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: wbamberg, Assigned: bsilverberg)
References
Details
(Whiteboard: [notifications])
Attachments
(3 files)
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.
Comment 1•8 years ago
|
||
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]
Updated•8 years ago
|
Flags: blocking-webextensions?
Updated•8 years ago
|
Assignee: nobody → amckay
Assignee | ||
Comment 2•8 years ago
|
||
I'm stealing this from Andy.
Assignee: amckay → bob.silverberg
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Flags: blocking-webextensions? → blocking-webextensions+
Comment 4•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8730258 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 5•8 years ago
|
||
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/
Assignee | ||
Comment 6•8 years ago
|
||
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
Updated•8 years ago
|
Attachment #8730258 -
Flags: review?(kmaglione+bmo) → review+
Comment 7•8 years ago
|
||
Comment on attachment 8730258 [details] MozReview Request: Bug 1254291 - Add schema for the notifications API, r?kmag https://reviewboard.mozilla.org/r/39783/#review36927
Assignee | ||
Comment 8•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6136878b54e
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
I had to back this out for apparently leaking some stuff: https://treeherder.mozilla.org/logviewer.html#?job_id=8051161&repo=fx-team https://hg.mozilla.org/integration/fx-team/rev/a4c6d6add2fc
Flags: needinfo?(bob.silverberg)
Assignee | ||
Comment 11•8 years ago
|
||
Thanks for the heads up Wes. I am currently looking into this.
Flags: needinfo?(bob.silverberg)
Assignee | ||
Comment 12•8 years ago
|
||
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)
Comment 13•8 years ago
|
||
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.
Assignee | ||
Comment 14•8 years ago
|
||
Here are the cclogs from running the test which generates the leaks.
Attachment #8731647 -
Flags: feedback?(kmaglione+bmo)
Comment 15•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8731647 -
Flags: feedback?(kmaglione+bmo)
Assignee | ||
Comment 16•8 years ago
|
||
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)
Assignee | ||
Comment 17•8 years ago
|
||
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/
Assignee | ||
Comment 18•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=57f88cec6f63
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Updated•8 years ago
|
Iteration: --- → 48.2 - Apr 4
Comment 20•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/adc196371da9
Keywords: checkin-needed
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/adc196371da9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•