1369833 - Make `nsIAlertNotification` easier to init

Status: RESOLVED FIXED

(Toolkit :: Alerts Service, enhancement)

Description

[Lina Butler (ex-Mozilla)]

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

Comment 1

[Lina Butler (ex-Mozilla)]

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

Comment 2

[Lina Butler (ex-Mozilla)]

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 (ex-Mozilla)]

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 (ex-Mozilla)]

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.

Comment 7

[Kagami Rosylight [:saschanaz] (they/them)]

JS can just provide a general object like this:

// this works!
var svc = Cc["@mozilla.org/alerts-service;1"].getService(Ci.nsIAlertsService);
var alert = {
  name: "name",
  imageURL: "",
  title: "title",
  text: "message",
  textClickable: true,
  cookie: "test-id",
  dir: "",
  lang: "",
  data: "test-id",
  principal: null,
  URI: null,
  inPrivateBrowsing: false,
  actionable: true,
  silent: false,
  vibrate: [],
  actions: [],
  source: "",
  opaqueLaunchData: "",
  loadImage: (timeout, listener, userData) => listener.onImageMissing(userData)
}
svc.showAlert(alert, console.log)

The problem is that not all members are supposed to be caller provided. Something like this would be nice:

interface nsIAlertAction : nsISupports {
  // ...

  void wrap(nsIAlertAction aAction);
};

var svc = Cc["@mozilla.org/alerts-service;1"].getService(Ci.nsIAlertsService);
var alert = Cc["@mozilla.org/alert-notification;1"].createInstance(Ci.nsIAlertNotification);
// Missing attributes will get default values
alert.wrap({
  name: "name",
  title: "title",
  text: "message",
  textClickable: true,
});
svc.showAlert(alert, console.log)

Comment 8

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1369833 - Part 1: Add a way to init with an object r=nalexander,nrishel,tschuster

Comment 9

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1369833 - Part 2: Use initWithObject instead of showAlertNotification for WebExtensions r=#extension-reviewers

Comment 10

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1369833 - Part 3: Remove unused showAlertNotification in dom/notification/test r=asuth

Comment 11

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1369833 - Part 4: Use initWithObject instead of showAlertNotification in browser/ r=#fx-desktop

Comment 12

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1369833 - Part 5: Remove showAlertNotification r=nalexander,nrishel

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/5e7addaf7736
https://hg.mozilla.org/integration/autoland/rev/7f41ead4c9db
Part 1: Add a way to init with an object r=nalexander,tschuster
https://github.com/mozilla-firefox/firefox/commit/e36669b3f824
https://hg.mozilla.org/integration/autoland/rev/002a8aeb403b
Part 2: Use initWithObject instead of showAlertNotification for WebExtensions r=extension-reviewers,willdurand
https://github.com/mozilla-firefox/firefox/commit/153cd95271ed
https://hg.mozilla.org/integration/autoland/rev/a210a9dde103
Part 3: Remove unused showAlertNotification in dom/notification/test r=asuth
https://github.com/mozilla-firefox/firefox/commit/9ee3a6db7fb6
https://hg.mozilla.org/integration/autoland/rev/bdcbf49a5e8c
Part 4: Use initWithObject instead of showAlertNotification in browser/ r=firefox-desktop-core-reviewers ,Gijs
https://github.com/mozilla-firefox/firefox/commit/446248523d95
https://hg.mozilla.org/integration/autoland/rev/2be19dbf9b01
Part 5: Remove showAlertNotification r=nalexander,nrishel,geckoview-reviewers,win-reviewers,gstoll

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d5e3e7cab7ce
https://hg.mozilla.org/integration/autoland/rev/a7679d64fafd
Revert "Bug 1369833 - Part 5: Remove showAlertNotification r=nalexander,nrishel,geckoview-reviewers,win-reviewers,gstoll" for causing bc failures @browser_device_connected.js.

Comment 15

[agoloman]

Backed out for causing bc failures @browser_device_connected.js.

• Backout link
• Push with failures
• Failure Log

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/eaa8ef331365
https://hg.mozilla.org/integration/autoland/rev/8f349df6d9dc
Part 1: Add a way to init with an object r=nalexander,tschuster
https://github.com/mozilla-firefox/firefox/commit/45387a99a5f0
https://hg.mozilla.org/integration/autoland/rev/0fa7b9d4643a
Part 2: Use initWithObject instead of showAlertNotification for WebExtensions r=extension-reviewers,willdurand
https://github.com/mozilla-firefox/firefox/commit/2f67a4b53a71
https://hg.mozilla.org/integration/autoland/rev/4ab7a7d3decd
Part 3: Remove unused showAlertNotification in dom/notification/test r=asuth
https://github.com/mozilla-firefox/firefox/commit/e9c5c6ef4e55
https://hg.mozilla.org/integration/autoland/rev/ae669e66bfd0
Part 4: Use initWithObject instead of showAlertNotification in browser/ r=firefox-desktop-core-reviewers ,Gijs
https://github.com/mozilla-firefox/firefox/commit/305692f25236
https://hg.mozilla.org/integration/autoland/rev/7ce16044c7d6
Part 5: Remove showAlertNotification r=nalexander,nrishel,geckoview-reviewers,win-reviewers,gstoll

Comment 17

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/8f349df6d9dc
https://hg.mozilla.org/mozilla-central/rev/0fa7b9d4643a
https://hg.mozilla.org/mozilla-central/rev/4ab7a7d3decd
https://hg.mozilla.org/mozilla-central/rev/ae669e66bfd0
https://hg.mozilla.org/mozilla-central/rev/7ce16044c7d6