1369833 - Make `nsIAlertNotification` properties writable

Status: NEW

(Toolkit :: Alerts Service, enhancement)

Description

[Lina Butler [:lina]]

Making these writable is so much nicer than passing 13 arguments to `init`.

Comment 1

[Lina Butler [:lina]]

I meant to do this last summer, for bug 1369834. >.>

Comment 2

[Lina Butler [:lina]]

Attachment: Bug 1369833 - Make `nsIAlertNotification` properties writable.

This commit removes `nsIAlertNotification.init`, and makes alert
notifications' properties writable instead. This is a bit more
cumbersome to use from C++, but nicer from JS, and doesn't require
folks to memorize the argument order.

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

Comment 3

[Lina Butler [:lina]]

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

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8989278 [details]
Bug 1369833 - Make `nsIAlertNotification` properties writable.

https://reviewboard.mozilla.org/r/254334/#review262074

I get how it's hard to read callers (comments beside the arguments could help) and I agree that some optional features would maybe be good to move out of the constructor but I think this introduces new problems related to added mutability. e.g. what happens if a setter is called during a show? What about after it has been shown? We would ideally want to enforce defined behaviour there. Since this mostly cleaning up the internal API but leaving the external API ugly, I'm not sure this is overall worthwhile but maybe there is more benefits that I'm missing.

Personally I was thinking it would be nice to re-use the Notification webidl interface to replace some alert service XPIDL rather than having the WebIDL wrapping around the old alert service XPIDL API.

Comment 5

[Lina Butler [:lina]]

Comment on attachment 8989278 [details]
Bug 1369833 - Make `nsIAlertNotification` properties writable.

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #4)
> I get how it's hard to read callers (comments beside the arguments could
> help) and I agree that some optional features would maybe be good to move
> out of the constructor but I think this introduces new problems related to
> added mutability. e.g. what happens if a setter is called during a show?
> What about after it has been shown? We would ideally want to enforce defined
> behaviour there. Since this mostly cleaning up the internal API but leaving
> the external API ugly, I'm not sure this is overall worthwhile but maybe
> there is more benefits that I'm missing.

That makes sense. The setter API is nicer to use from JavaScript than C++, especially when you're only changing a handful of properties, so I think there's some benefit...especially if we can remove the old `nsIAlertsService.showAlertNotification` method that also takes all these arguments (bug 1236688). I think the mutability issue still exists today: you can call `init` on an `nsIAlertNotification` instance that you've passed to `showAlert`, and we'll (IIRC) ignore those changes.

> Personally I was thinking it would be nice to re-use the Notification webidl
> interface to replace some alert service XPIDL rather than having the WebIDL
> wrapping around the old alert service XPIDL API.

I think we looked into using `Notification` back in the day, too, before you could use WebIDL types in XPIDL. Constructing it from C++ is still a bit cumbersome; you need a global object, and there's a lot of permission checks and worker handling that we don't need.

Reusing `NotificationOptions` could work, though! WebIDL dictionaries are simpler to use from C++ and JS, and that leaves us with 6 params (the options, title, clickable flag, cookie, principal, and private browsing flag). We can probably ditch clickable and private browsing (every notification can be clickable, and we can get the private browsing ID from the principal), leaving us with 4 params. 3 if you omit the principal, which most chrome callers will do, 2 if you also omit the cookie.

WDYT? I might play around with this when I have spare cycles, but, if it doesn't seem worthwhile, I'm also happy to just close this. :-)

Comment 6

[Matthew N. [:MattN]]

(In reply to Lina Cambridge (she/her) [:lina] from comment #5)

Comment on attachment 8989278 [details]
Bug 1369833 - Make nsIAlertNotification properties writable.

Reusing NotificationOptions could work, though! WebIDL dictionaries are
simpler to use from C++ and JS, and that leaves us with 6 params (the
options, title, clickable flag, cookie, principal, and private browsing
flag). We can probably ditch clickable and private browsing (every
notification can be clickable, and we can get the private browsing ID from
the principal), leaving us with 4 params. 3 if you omit the principal, which
most chrome callers will do, 2 if you also omit the cookie.

WDYT? I might play around with this when I have spare cycles, but, if it
doesn't seem worthwhile, I'm also happy to just close this. :-)

That could be a good start. Since nobody's actively maintaining this code now though I'm reluctant to make big changes.