1227300 - Move `nsIAlertsService::showAlertNotification` params to an XPCOM interface

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Lina Butler [:lina]]

This method takes 12 params, and makes it difficult to refactor into separate functions (see bug 1224785, where we asynchronously fetch the favicon, then display the notification). The signature is also duplicated in each of the backends, and IPDL.

Instead, let's add a separate interface that we can pass around.

Comment 1

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1227300, Part 1 - Add an alert notification component. r=MattN,wchen

Bug 1227300, Part 1 - Add `nsIAlert` interface. r?MattN,wchen

Comment 2

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1227300, Part 2 - Deprecate `showAlertNotification` in favor of `showAlert`. r?MattN,wchen

Bug 1227300, Part 2 - Deprecate `showAlertNotification` in favor of `showAlert`. r?MattN,wchen

Comment 3

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1227300, Part 3 - Implement `showAlert` for the OS X alerts backend. r?mstange

Bug 1227300, Part 3 - Implement `showAlert` for the OS X alerts backend. r?mstange

Comment 4

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1227300, Part 4 - Implement `showAlert` for the libnotify alerts backend. r?karlt

Bug 1227300, Part 4 - Implement `showAlert` for the libnotify alerts backend. r?karlt

Comment 5

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1227300, Part 5 - Implement `showAlert` for the B2G alerts backend. r?mhenretty

Bug 1227300, Part 5 - Implement `showAlert` for the B2G alerts backend. r?mhenretty

Comment 6

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1227300, Part 6 - Use `showAlert` to display web notifications. r?wchen

Bug 1227300, Part 6 - Use `showAlert` to display web notifications. r?wchen

Comment 7

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1227300, Part 7 - Update test interfaces.

Bug 1227300, Part 7 - Update test interfaces.

Comment 8

[Lina Butler [:lina]]

This works fine on OS X, but I can't get it to link on Ubuntu. First, when I import "nsAlert.h," I get the error, "Internal string headers are not available from external-linkage code". I found "nsStringAPI.h" is for external consumers, and added this to "nsAlert.h":

#ifdef MOZILLA_INTERNAL_API
#include "nsString.h"
#else
#include "nsStringAPI.h"
#endif

...Except I then get this error: https://gist.github.com/kitcambridge/fb954d6545a429237584

Is that because `nsAlert` is considered an internal API, but the Gnome bindings aren't?

Comment 9

[Karl Tomlinson (:karlt)]

libmozgnome.so was outside of libxul.so so that systems without gnome-specific dependent libraries could still run most of the app.

These days I don't think libmozgnome.so depends on anything that libxul.so doesn't already depend on, and so it need no longer be a separate library.  If it is merged into libxul, then it would no longer need to be restricted to XPCOM interfaces.

Comment 10

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8691174 [details]
MozReview Request: Bug 1227300, Part 5 - Implement `showAlert` for the B2G alerts backend. r?mhenretty

Seems pretty straightforward from the B2G side. Let's just make sure the Gij test suite passes with this change.

Comment 11

[Lina Butler [:lina]]

Comment on attachment 8691170 [details]
MozReview Request: Bug 1227300, Part 1 - Add an alert notification component. r=MattN,wchen

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

Comment 12

[Lina Butler [:lina]]

Comment on attachment 8691171 [details]
MozReview Request: Bug 1227300, Part 2 - Deprecate `showAlertNotification` in favor of `showAlert`. r?MattN,wchen

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

Comment 13

[Lina Butler [:lina]]

Comment on attachment 8691172 [details]
MozReview Request: Bug 1227300, Part 3 - Implement `showAlert` for the OS X alerts backend. r?mstange

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

Comment 14

[Lina Butler [:lina]]

Comment on attachment 8691173 [details]
MozReview Request: Bug 1227300, Part 4 - Implement `showAlert` for the libnotify alerts backend. r?karlt

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

Comment 15

[Lina Butler [:lina]]

Comment on attachment 8691174 [details]
MozReview Request: Bug 1227300, Part 5 - Implement `showAlert` for the B2G alerts backend. r?mhenretty

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

Comment 16

[Lina Butler [:lina]]

Comment on attachment 8691175 [details]
MozReview Request: Bug 1227300, Part 6 - Use `showAlert` to display web notifications. r?wchen

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

Comment 17

[Lina Butler [:lina]]

Comment on attachment 8691176 [details]
MozReview Request: Bug 1227300, Part 7 - Update test interfaces.

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

Comment 18

[Lina Butler [:lina]]

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

Comment 19

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/26089/#review23645

::: toolkit/components/alerts/nsIAlertsService.idl:14
(Diff revision 2)
> +  readonly attribute AString name;

Actually, I think these should have setters, with default empty values. That should make it easier to use `nsAlert` from C++ callers like https://dxr.mozilla.org/mozilla-central/rev/099f695d31326c39595264c34988a0f4b7cbc698/toolkit/components/downloads/nsDownloadManager.cpp#2758-2763.

Comment 20

[Lina Butler [:lina]]

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

Comment 21

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8691174 [details]
MozReview Request: Bug 1227300, Part 5 - Implement `showAlert` for the B2G alerts backend. r?mhenretty

same as comment 10.

Comment 22

[Markus Stange [:mstange]]

Comment on attachment 8691172 [details]
MozReview Request: Bug 1227300, Part 3 - Implement `showAlert` for the OS X alerts backend. r?mstange

https://reviewboard.mozilla.org/r/26093/#review23741

Comment 23

[William Chen [:wchen]]

Comment on attachment 8691170 [details]
MozReview Request: Bug 1227300, Part 1 - Add an alert notification component. r=MattN,wchen

https://reviewboard.mozilla.org/r/26089/#review24323

::: toolkit/components/alerts/moz.build:17
(Diff revision 2)
> +    'nsAlertIPCSerializer.h',

New files should go into the mozilla namespace and drop the "ns" prefix. The other thing that concerns me is that alert already means something else that has been long established in DOM (window.alert) and it seems very likely that it will cause confusion, although I don't have a good suggestion for another name right now. I understand that the other classes have already been misnamed but I feel that we should avoid going further in that direction if we can help it.

::: toolkit/components/alerts/nsAlertIPCSerializer.h:36
(Diff revision 2)
> +    NS_ENSURE_SUCCESS_VOID(aParam->GetName(name));

This will send a partially serialized object if any of the getters fail. We should get all the parameters first and abort if we failed to get any of them before we start writing the parameters. If we send half serialized objects, the process will be killed when it fails to deserialize and it's better if we catch this problem earlier during serialization.

::: toolkit/components/alerts/nsIAlertsService.idl:12
(Diff revision 2)
> +interface nsIAlert : nsISupports

Documentation for the relevant attributes should be copied over from nsIAlertsService

Comment 24

[William Chen [:wchen]]

Comment on attachment 8691171 [details]
MozReview Request: Bug 1227300, Part 2 - Deprecate `showAlertNotification` in favor of `showAlert`. r?MattN,wchen

https://reviewboard.mozilla.org/r/26091/#review24365

::: dom/ipc/ContentParent.h:783
(Diff revision 2)
> -    virtual bool RecvShowAlertNotification(const nsString& aImageUrl, const nsString& aTitle,
> +    virtual bool RecvShowAlert(const Alert& aAlert) override;

I'd rather keep the old name just so that it's more clear this has nothing to do with window.alert. If you can think of an alternate name that would be even better.

Comment 25

[William Chen [:wchen]]

Comment on attachment 8691175 [details]
MozReview Request: Bug 1227300, Part 6 - Use `showAlert` to display web notifications. r?wchen

https://reviewboard.mozilla.org/r/26099/#review24367

Comment 26

[William Chen [:wchen]]

Comment on attachment 8691175 [details]
MozReview Request: Bug 1227300, Part 6 - Use `showAlert` to display web notifications. r?wchen

https://reviewboard.mozilla.org/r/26099/#review24369

Comment 27

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/26091/#review24365

> I'd rather keep the old name just so that it's more clear this has nothing to do with window.alert. If you can think of an alternate name that would be even better.

Windows calls this `ToastNotification`, and OS X calls it `NSUserNotification`. I'm partial toward "toast," except the service is already called the "alerts service." I think it'd be more confusing if we had `nsIAlertsService` and `nsIToastNotification`, since it's not immediately clear they're related. Another option is `nsIAlertNotification`.

Comment 28

[Lina Butler [:lina]]

Comment on attachment 8691170 [details]
MozReview Request: Bug 1227300, Part 1 - Add an alert notification component. r=MattN,wchen

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

Comment 29

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1227300, Part 2 - Implement `showAlert`. r=MattN,wchen

Bug 1227300, Part 2 - Deprecate `showAlertNotification` in favor of `showAlert`. r=wchen?MattN

Comment 30

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1227300, Part 3 - Implement `showAlert` for the OS X alerts backend. r=mstange

Bug 1227300, Part 3 - Implement `showAlert` for the OS X alerts backend. r=mstange

Comment 31

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1227300, Part 4 - Implement `showAlert` for the libnotify alerts backend. r=karlt

Bug 1227300, Part 4 - Implement `showAlert` for the libnotify alerts backend. r?karlt

Comment 32

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1227300, Part 5 - Implement `showAlert` for the B2G alerts backend. r=mhenretty

Bug 1227300, Part 5 - Implement `showAlert` for the B2G alerts backend. r=mhenretty

Comment 33

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1227300, Part 6 - Use `showAlert` to display web notifications. r=wchen

Bug 1227300, Part 6 - Use `showAlert` to display web notifications. r=wchen

Comment 34

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1227300, Part 7 - Update test interfaces. a=testonly

Bug 1227300, Part 7 - Update test interfaces.

Comment 35

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/26089/#review23645

> Actually, I think these should have setters, with default empty values. That should make it easier to use `nsAlert` from C++ callers like https://dxr.mozilla.org/mozilla-central/rev/099f695d31326c39595264c34988a0f4b7cbc698/toolkit/components/downloads/nsDownloadManager.cpp#2758-2763.

On further thought, there are a few convenience methods we can add to this interface, like image loading. That's currently duplicated between the libnotify and OS X backends. I went to make it a proper XPCOM component, then realized it could just be implemented in JS.

Comment 36

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/26089/#review24323

> New files should go into the mozilla namespace and drop the "ns" prefix. The other thing that concerns me is that alert already means something else that has been long established in DOM (window.alert) and it seems very likely that it will cause confusion, although I don't have a good suggestion for another name right now. I understand that the other classes have already been misnamed but I feel that we should avoid going further in that direction if we can help it.

Name changed to `nsIAlertNotification`. I'd prefer if we didn't use an entirely different name; otherwise, it becomes hard to see that callers need to pass this interface to `showAlert`. Of course, we could rename the method, but the service is still called `nsIAlertsService`.

> This will send a partially serialized object if any of the getters fail. We should get all the parameters first and abort if we failed to get any of them before we start writing the parameters. If we send half serialized objects, the process will be killed when it fails to deserialize and it's better if we catch this problem earlier during serialization.

Changed to write an "isNull" flag if any of the getters fail.

Comment 37

[William Chen [:wchen]]

Comment on attachment 8691170 [details]
MozReview Request: Bug 1227300, Part 1 - Add an alert notification component. r=MattN,wchen

https://reviewboard.mozilla.org/r/26089/#review24671

::: toolkit/components/alerts/AlertIPCSerializer.h:31
(Diff revision 3)
> +      WriteParam(aMsg, isNull);

You need to update callees where we may receive an AlertNotification from IPC (RecvShowAlertNotification) and make sure we handle nullptr.

::: toolkit/components/alerts/AlertIPCSerializer.h:110
(Diff revision 3)
> +      WriteParam(aMsg, true);

So it looks like we write a null object if we fail to get any of the parameters. I think it would be more obvious if you moved the WriteParam out of this function and WriteParam right before the caller returns, and maybe a comment saying that if getting any parameters fail we treat it as a null object.

::: toolkit/components/alerts/AlertNotification.js:14
(Diff revision 3)
> +const ALERT_NOTIFICATION_CONTRACTID = "@mozilla.org/alert;1";

If we keep this JS component, how about we call the contract alertnotification?

::: toolkit/components/alerts/moz.build:16
(Diff revision 3)
> +    'AlertNotification.js',

Is there a reason why you converted this to javascript? I would prefer if it remains a simple CPP class because it makes debugging easier and avoids overhead.

Comment 38

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/26089/#review24671

> You need to update callees where we may receive an AlertNotification from IPC (RecvShowAlertNotification) and make sure we handle nullptr.

I added `NS_ENSURE_ARG(aAlert)` for all backends in part 2. AFAICT, those are the only callees.

> So it looks like we write a null object if we fail to get any of the parameters. I think it would be more obvious if you moved the WriteParam out of this function and WriteParam right before the caller returns, and maybe a comment saying that if getting any parameters fail we treat it as a null object.

Agreed; will do.

> Is there a reason why you converted this to javascript? I would prefer if it remains a simple CPP class because it makes debugging easier and avoids overhead.

There are a couple of reasons. Eventually, I'd like to add some methods (I mentioned image loading in comment 35, and bug 1206560 will add favicons)...which is why I made it a component instead of a class. At that point, the cycle collector macros and getters felt like a lot of boilerplate for what's really a property bag with some convenience methods. And, for images and favicons, I think it's clearer to write JS functions than C++ callback classes.

That said, I'm way more comfortable with JS, so of course I'm going to find it clearer. If you are unpersuaded, I can change it back. :-)

Comment 39

[Lina Butler [:lina]]

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

Comment 40

[Lina Butler [:lina]]

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

Comment 41

[Lina Butler [:lina]]

Comment on attachment 8691170 [details]
MozReview Request: Bug 1227300, Part 1 - Add an alert notification component. r=MattN,wchen

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

Comment 42

[Lina Butler [:lina]]

Comment on attachment 8696045 [details]
MozReview Request: Bug 1227300, Part 2 - Implement `showAlert`. r=MattN,wchen

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

Comment 43

[Lina Butler [:lina]]

Comment on attachment 8696046 [details]
MozReview Request: Bug 1227300, Part 3 - Implement `showAlert` for the OS X alerts backend. r=mstange

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

Comment 44

[Lina Butler [:lina]]

Comment on attachment 8696047 [details]
MozReview Request: Bug 1227300, Part 4 - Implement `showAlert` for the libnotify alerts backend. r=karlt

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

Comment 45

[Lina Butler [:lina]]

Comment on attachment 8696048 [details]
MozReview Request: Bug 1227300, Part 5 - Implement `showAlert` for the B2G alerts backend. r=mhenretty

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

Comment 46

[Lina Butler [:lina]]

Comment on attachment 8696049 [details]
MozReview Request: Bug 1227300, Part 6 - Use `showAlert` to display web notifications. r=wchen

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

Comment 47

[Lina Butler [:lina]]

Comment on attachment 8696050 [details]
MozReview Request: Bug 1227300, Part 7 - Update test interfaces. a=testonly

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

Comment 48

[William Chen [:wchen]]

https://reviewboard.mozilla.org/r/26089/#review24671

> I added `NS_ENSURE_ARG(aAlert)` for all backends in part 2. AFAICT, those are the only callees.

ContentParent::RecvShowAlert in part 2 still doesn't handle receiving a nullptr (AlertNotification is typedef'd to nsIAlertNotification*)

Comment 49

[William Chen [:wchen]]

Comment on attachment 8691170 [details]
MozReview Request: Bug 1227300, Part 1 - Add an alert notification component. r=MattN,wchen

https://reviewboard.mozilla.org/r/26089/#review24979

Comment 50

[Lina Butler [:lina]]

Comment on attachment 8696045 [details]
MozReview Request: Bug 1227300, Part 2 - Implement `showAlert`. r=MattN,wchen

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

Comment 51

[Lina Butler [:lina]]

Comment on attachment 8696046 [details]
MozReview Request: Bug 1227300, Part 3 - Implement `showAlert` for the OS X alerts backend. r=mstange

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

Comment 52

[Lina Butler [:lina]]

Comment on attachment 8696047 [details]
MozReview Request: Bug 1227300, Part 4 - Implement `showAlert` for the libnotify alerts backend. r=karlt

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

Comment 53

[Lina Butler [:lina]]

Comment on attachment 8696048 [details]
MozReview Request: Bug 1227300, Part 5 - Implement `showAlert` for the B2G alerts backend. r=mhenretty

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

Comment 54

[Lina Butler [:lina]]

Comment on attachment 8696049 [details]
MozReview Request: Bug 1227300, Part 6 - Use `showAlert` to display web notifications. r=wchen

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

Comment 55

[Lina Butler [:lina]]

Comment on attachment 8696050 [details]
MozReview Request: Bug 1227300, Part 7 - Update test interfaces. a=testonly

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

Comment 56

[William Chen [:wchen]]

Comment on attachment 8696045 [details]
MozReview Request: Bug 1227300, Part 2 - Implement `showAlert`. r=MattN,wchen

https://reviewboard.mozilla.org/r/27259/#review25001

::: dom/ipc/ContentParent.cpp:4422
(Diff revisions 2 - 3)
> +        return true;

Let's put a NS_WARNING or WARN_IF here because it doesn't make very much sense to show a null notification and it's better that it be handled at the caller before sending the null AlertNotification if this case ever comes up.

Comment 57

[William Chen [:wchen]]

https://reviewboard.mozilla.org/r/27259/#review25113

::: toolkit/components/alerts/nsAlertsService.cpp:107
(Diff revision 3)
> +  nsString imageUrl;

These temporary strings that are being passed to the getters should be nsAutoString.

Comment 58

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/27263/#review25461

::: toolkit/system/gnome/nsSystemAlertsService.cpp:44
(Diff revision 3)
> +    do_CreateInstance("@mozilla.org/alert-notification;1");

#include "nsComponentManagerUtils.h"

Comment 59

[Matthew N. [:MattN]]

Comment on attachment 8696045 [details]
MozReview Request: Bug 1227300, Part 2 - Implement `showAlert`. r=MattN,wchen

https://reviewboard.mozilla.org/r/27259/#review25493

::: toolkit/components/alerts/nsAlertsService.cpp:75
(Diff revision 3)
> +  nsCOMPtr<nsIAlertNotification> alert =
> +    do_CreateInstance("@mozilla.org/alert-notification;1");
> +  NS_ENSURE_TRUE(alert, NS_ERROR_FAILURE);
> +  nsresult rv = alert->Init(aAlertName, aImageUrl, aAlertTitle,
> +                            aAlertText, aAlertTextClickable,
> +                            aAlertCookie, aBidi, aLang, aData,
> +                            aPrincipal, aInPrivateBrowsing);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  return ShowAlert(alert, aAlertListener);
> +}

MDN makes it seem like [deprecated] issues a compiler warning but I don't think that will warn extensions who use XPCOM to show an alert. We should probably have a runtime warning at least once per startup in some builds.

::: toolkit/components/alerts/nsAlertsService.cpp:91
(Diff revision 3)
> +  nsresult rv;
> +
> +  nsString cookie;
> +  rv = aAlert->GetCookie(cookie);

Nit: `nsresult rv = aAlert->GetCookie(cookie);`

::: toolkit/components/alerts/nsAlertsService.cpp:163
(Diff revision 3)
>    // Use XUL notifications as a fallback if above methods have failed.
> -  rv = mXULAlerts.ShowAlertNotification(aImageUrl, aAlertTitle, aAlertText, aAlertTextClickable,
> -                                        aAlertCookie, aAlertListener, aAlertName,
> -                                        aBidi, aLang, aPrincipal, aInPrivateBrowsing);
> +  rv = mXULAlerts.ShowAlertNotification(imageUrl, title, text, textClickable,
> +                                        cookie, aAlertListener, name,
> +                                        bidi, lang, principal, inPrivateBrowsing);

The XUL implemenation should switch to taking an alert notification object eventually.

Comment 60

[Matthew N. [:MattN]]

Comment on attachment 8691170 [details]
MozReview Request: Bug 1227300, Part 1 - Add an alert notification component. r=MattN,wchen

https://reviewboard.mozilla.org/r/26089/#review25495

Sorry for the delay

Comment 61

[Karl Tomlinson (:karlt)]

Comment on attachment 8696047 [details]
MozReview Request: Bug 1227300, Part 4 - Implement `showAlert` for the libnotify alerts backend. r=karlt

https://reviewboard.mozilla.org/r/27263/#review25837

::: toolkit/system/gnome/nsAlertsIconListener.h:10
(Diff revision 3)
> +#include "nsIAlertsService.h"

Forward declare nsIAlertNotification instead of including a header for the declaration.

The header can be included from the .cpp file if appropriate.

::: toolkit/system/gnome/nsAlertsIconListener.h:33
(Diff revision 3)
> -  nsresult InitAlertAsync(const nsAString & aImageUrl,
> -                          const nsAString & aAlertTitle, 
> +  nsresult InitAlertAsync(nsIAlertNotification * aAlert,
> +                          nsIObserver * aAlertListener);

Please make this nsIAlertNotification*, and nsIObserver* while you are changing that line.

::: toolkit/system/gnome/nsAlertsIconListener.cpp:280
(Diff revision 3)
> -nsAlertsIconListener::InitAlertAsync(const nsAString & aImageUrl,
> -                                     const nsAString & aAlertTitle, 
> +nsAlertsIconListener::InitAlertAsync(nsIAlertNotification * aAlert,
> +                                     nsIObserver * aAlertListener)

nsIAlertNotification* and nsIObserver*

::: toolkit/system/gnome/nsAlertsIconListener.cpp:290
(Diff revision 3)
> -    nsCOMPtr<nsIStringBundleService> bundleService = 
> +    nsCOMPtr<nsIStringBundleService> bundleService =

Please remove the unrelated whitespace change.

::: toolkit/system/gnome/nsAlertsIconListener.cpp:340
(Diff revision 3)
>    nsCOMPtr<nsIObserverService> obsServ =
>        do_GetService("@mozilla.org/observer-service;1");
>    if (obsServ)
>      obsServ->AddObserver(this, "quit-application", true);

Please move this observer registration to after the new early return paths below, before the StartRequest() call.

It looks like this should actually be in ShowAlert(), after the (!mNotification) return path.  Moving it there would be good, if you like, but, given this is a separate issue, I'm happy for the call to stay in this method.

::: toolkit/system/gnome/nsAlertsIconListener.cpp:345
(Diff revision 3)
>    // Workaround for a libnotify bug - blank titles aren't dealt with
>    // properly so we use a space
> -  if (aAlertTitle.IsEmpty()) {
> +  nsAutoString title;
> +  rv = aAlert->GetTitle(title);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (title.IsEmpty()) {

Please leave the comment immediately before the IsEmpty() test.

::: toolkit/system/gnome/nsSystemAlertsService.cpp:43
(Diff revision 3)
> +  nsCOMPtr<nsIAlertNotification> alert =
> +    do_CreateInstance("@mozilla.org/alert-notification;1");

Please add a named constant to the .idl file for the CONTRACTID, available from C++, and use it here, so that the compiler does some spell checking.

::: toolkit/system/gnome/nsSystemAlertsService.cpp:54
(Diff revision 3)
> +NS_IMETHODIMP nsSystemAlertsService::ShowAlert(nsIAlertNotification * aAlert,
> +                                               nsIObserver * aAlertListener)

nsIAlertNotification*
nsIObserver*

Comment 62

[Lina Butler [:lina]]

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

Comment 63

[Lina Butler [:lina]]

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

Comment 64

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/27259/#review25493

> MDN makes it seem like [deprecated] issues a compiler warning but I don't think that will warn extensions who use XPCOM to show an alert. We should probably have a runtime warning at least once per startup in some builds.

Agreed. I'll leave this un-deprecated for now to unblock other bugs that depend on this patch. We can add the warning in bug 1236688.

> The XUL implemenation should switch to taking an alert notification object eventually.

Fixed in bug 1219855. :-)

Comment 65

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/27263/#review25837

> Please move this observer registration to after the new early return paths below, before the StartRequest() call.
> 
> It looks like this should actually be in ShowAlert(), after the (!mNotification) return path.  Moving it there would be good, if you like, but, given this is a separate issue, I'm happy for the call to stay in this method.

Thanks for catching that! I went ahead and moved it into ShowAlert().

Comment 66

[Lina Butler [:lina]]

Comment on attachment 8691170 [details]
MozReview Request: Bug 1227300, Part 1 - Add an alert notification component. r=MattN,wchen

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

Comment 67

[Lina Butler [:lina]]

Comment on attachment 8696045 [details]
MozReview Request: Bug 1227300, Part 2 - Implement `showAlert`. r=MattN,wchen

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

Comment 68

[Lina Butler [:lina]]

Comment on attachment 8696046 [details]
MozReview Request: Bug 1227300, Part 3 - Implement `showAlert` for the OS X alerts backend. r=mstange

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

Comment 69

[Lina Butler [:lina]]

Comment on attachment 8696047 [details]
MozReview Request: Bug 1227300, Part 4 - Implement `showAlert` for the libnotify alerts backend. r=karlt

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

Comment 70

[Lina Butler [:lina]]

Comment on attachment 8696048 [details]
MozReview Request: Bug 1227300, Part 5 - Implement `showAlert` for the B2G alerts backend. r=mhenretty

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

Comment 71

[Lina Butler [:lina]]

Comment on attachment 8696049 [details]
MozReview Request: Bug 1227300, Part 6 - Use `showAlert` to display web notifications. r=wchen

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

Comment 72

[Lina Butler [:lina]]

Comment on attachment 8696050 [details]
MozReview Request: Bug 1227300, Part 7 - Update test interfaces. a=testonly

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

Comment 73

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/26089/#review24671

> There are a couple of reasons. Eventually, I'd like to add some methods (I mentioned image loading in comment 35, and bug 1206560 will add favicons)...which is why I made it a component instead of a class. At that point, the cycle collector macros and getters felt like a lot of boilerplate for what's really a property bag with some convenience methods. And, for images and favicons, I think it's clearer to write JS functions than C++ callback classes.
> 
> That said, I'm way more comfortable with JS, so of course I'm going to find it clearer. If you are unpersuaded, I can change it back. :-)

I came around to keeping this in C++. :-) There's a fair bit of boilerplate for using the image loader from JS, and my initial implementation of favicons is awkward.

Comment 74

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/fx-team/rev/8bb1ec138e9c386902165007cdce0516bc6091a7
Bug 1227300, Part 1 - Add an alert notification component. r=MattN,wchen

https://hg.mozilla.org/integration/fx-team/rev/1b820caf5883a2a282471420687cbb2d7577f2f6
Bug 1227300, Part 2 - Implement `showAlert`. r=MattN,wchen

https://hg.mozilla.org/integration/fx-team/rev/9ea2c1e1e184715df971f1d2481f63667e8d7624
Bug 1227300, Part 3 - Implement `showAlert` for the OS X alerts backend. r=mstange

https://hg.mozilla.org/integration/fx-team/rev/e72101688a7eb111c6bb8c43a18e930a6ae661e4
Bug 1227300, Part 4 - Implement `showAlert` for the libnotify alerts backend. r=karlt

https://hg.mozilla.org/integration/fx-team/rev/c682473093dbf7aa6a503c4bbd58bc002552a838
Bug 1227300, Part 5 - Implement `showAlert` for the B2G alerts backend. r=mhenretty

https://hg.mozilla.org/integration/fx-team/rev/dcb0d94b98da487042afa8b812ca1eed471f984f
Bug 1227300, Part 6 - Use `showAlert` to display web notifications. r=wchen

https://hg.mozilla.org/integration/fx-team/rev/790fdcd1e1428d9e65314f8277d39fdc6eb582b5
Bug 1227300, Part 7 - Update test interfaces. a=testonly

Comment 75

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

https://hg.mozilla.org/mozilla-central/rev/8bb1ec138e9c
https://hg.mozilla.org/mozilla-central/rev/1b820caf5883
https://hg.mozilla.org/mozilla-central/rev/9ea2c1e1e184
https://hg.mozilla.org/mozilla-central/rev/e72101688a7e
https://hg.mozilla.org/mozilla-central/rev/c682473093db
https://hg.mozilla.org/mozilla-central/rev/dcb0d94b98da
https://hg.mozilla.org/mozilla-central/rev/790fdcd1e142