1227730 - Implement `closeAlert` for libnotify alerts

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Lina Butler [:lina]]

libnotify has a call for closing notifications: https://developer.gnome.org/libnotify/0.7/NotifyNotification.html#notify-notification-close

We should use it in https://dxr.mozilla.org/mozilla-central/rev/45273bbed8efaface6f5ec56d984cb9faf4fbb6a/toolkit/system/gnome/nsSystemAlertsService.cpp#51-55.

Comment 1

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1227730 - Support closing libnotify alerts. r?MattN r=karlt

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

Comment 2

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/34269/#review30985

::: toolkit/system/gnome/nsAlertsIconListener.cpp:397
(Diff revision 1)
> +  NS_ADDREF(this);

Actually, I don't think I need this anymore, now that `mActiveListeners` keeps the listener alive.

As currently written, it handles the "replaced alert" case noted in `RemoveIfActive`...but I can just check for an existing alert and destroy it before calling `Put`.

What do you think, :karlt?

Comment 3

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/34269/#review30985

> Actually, I don't think I need this anymore, now that `mActiveListeners` keeps the listener alive.
> 
> As currently written, it handles the "replaced alert" case noted in `RemoveIfActive`...but I can just check for an existing alert and destroy it before calling `Put`.
> 
> What do you think, :karlt?

Yes, the additional reference isn't required if appropriate changes can be
made elsewhere, but I think that would require dispatching alertfinished
differently and I'm not so comfortable with that.  It shouldn't be dispatched
from a destructor (during a Release()) if Observe() may call into JS, because
JS is evil, potentially running nested event loops, and I suspect Observe
could call into JS.

It also seems better to dispatch alertfinished when the alert is really
finished rather than assuming the native alert service will respond to a
close request immediately.

Still, with keeping the reference associated with the NotifyNotification,
closing mNotification on replace would be consistent with the documentation for Android and OS X - I guess that is what you mean by "destroy" and why there is both a cookie and a name.

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 8717691 [details]
MozReview Request: Bug 1227730 - Support closing libnotify alerts. r?MattN r=karlt

https://reviewboard.mozilla.org/r/34269/#review31351

::: toolkit/system/gnome/nsAlertsIconListener.h:62
(Diff revision 1)
> +  typedef bool (*notify_notification_close_t)(void*, char*);

Second parameter is GError**

::: toolkit/system/gnome/nsAlertsIconListener.h:74
(Diff revision 1)
> +  RefPtr<nsSystemAlertsService> mBackend;
> +  nsString mAlertName;

Place new members adjacent to similar existing members to improve packing.

::: toolkit/system/gnome/nsAlertsIconListener.cpp:200
(Diff revision 1)
> -  NS_ADDREF(this);
> +  SetActive();

This is too late to add the listener to the active set.  CloseAlert() may have already been called at this point.

Adding the listener to the active set in nsSystemAlertsService::ShowAlertNotification() would be the right time and consistent with removing in nsSystemAlertsService::CloseAlert().

::: toolkit/system/gnome/nsAlertsIconListener.cpp:291
(Diff revision 1)
> +  if (!libNotifyHandle || !notify_is_initted())

It should be possible to ensure that this listener doesn't get added to the active set if this is true.
I expect a null-check on mNotification will still be required, but I'm not sure that it should be an error if the notification has already been closed by the user.

::: toolkit/system/gnome/nsAlertsIconListener.cpp:370
(Diff revision 1)
> +  rv = aAlert->GetName(mAlertName);

Can you update the documentation please here?
https://dxr.mozilla.org/mozilla-central/rev/904f3554c08488c53d24deb20a486600ddddd56b/toolkit/components/alerts/nsIAlertsService.idl#32

::: toolkit/system/gnome/nsAlertsIconListener.cpp:407
(Diff revision 1)
> +  if (mBackend->mActiveListeners.Get(mAlertName, getter_AddRefs(alertListener)) &&

GetWeak() is a nicer API that avoids outparams and unnecessary ref-counting.

::: toolkit/system/gnome/nsSystemAlertsService.h:17
(Diff revision 1)
> +  friend class nsAlertsIconListener;

I find it more intuitive to track method callers than friend relationships like this.
See also https://google.github.io/styleguide/cppguide.html#Friends

I think this use case could be handled with a single NotifyClosed() or RemoveIfActive() public method on this class.

Note there was a previous similar attempt to implement this, which got as far
as https://bugzilla.mozilla.org/show_bug.cgi?id=858919#c54

Comment 5

[Armen [:armenzg]]

Any updates for this?
Is there a patch that I could use on try?

Thanks for looking into this!

Comment 6

[Lina Butler [:lina]]

You can try out the attached patch. I'll update it today with :karlt's suggestions.

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #3)
> Yes, the additional reference isn't required if appropriate changes can be
> made elsewhere, but I think that would require dispatching alertfinished
> differently and I'm not so comfortable with that.

The nsAlertsIconListener should not be permitted to be destroyed while
it has an mAlertListener observer and alertfinished has not yet been
dispatched.  It would be fine for the nsAlertsIconListener to be destroyed after
generating the NotifyNotification if there is no observer and the signal
handler on the NotifyNotification is either disconnected or never connected.

The only API I see in nsIAlertsService to deregister the observer is
closeAlert, which is not necessarily ideal.  That makes me suspect that
mAlertListener should be a weak reference, but nsIObserver doesn't guarantee
weak reference support.  This seems to be an existing bug in the ownership
model.

Comment 8

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/34269/#review30985

> Yes, the additional reference isn't required if appropriate changes can be
> made elsewhere, but I think that would require dispatching alertfinished
> differently and I'm not so comfortable with that.  It shouldn't be dispatched
> from a destructor (during a Release()) if Observe() may call into JS, because
> JS is evil, potentially running nested event loops, and I suspect Observe
> could call into JS.
> 
> It also seems better to dispatch alertfinished when the alert is really
> finished rather than assuming the native alert service will respond to a
> close request immediately.
> 
> Still, with keeping the reference associated with the NotifyNotification,
> closing mNotification on replace would be consistent with the documentation for Android and OS X - I guess that is what you mean by "destroy" and why there is both a cookie and a name.

Yes, when I wrote "destroy," I meant "close." I think we'll need to manually call `Close()` when we replace an alert that hasn't been shown yet, so that`"alertfinished"` can fire.

Comment 9

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/34269/#review31351

> Second parameter is GError**

Fixed for `notify_notification_close` and `notify_notification_show`, which also takes a GError**.

> Place new members adjacent to similar existing members to improve packing.

Ah, I was wondering how these were laid out. Now I know. Thanks!

> This is too late to add the listener to the active set.  CloseAlert() may have already been called at this point.
> 
> Adding the listener to the active set in nsSystemAlertsService::ShowAlertNotification() would be the right time and consistent with removing in nsSystemAlertsService::CloseAlert().

Done. I'm only adding the listener to the set if `InitAlertAsync` succeeds, but I can make it unconditional, if you prefer.

> It should be possible to ensure that this listener doesn't get added to the active set if this is true.
> I expect a null-check on mNotification will still be required, but I'm not sure that it should be an error if the notification has already been closed by the user.

I agree this shouldn't be an error if the user closed the notification. Also added code to cancel the image request, and fire `"alertfinished"` if `ShowAlert` hasn't been called yet. Does that seem sensible?

> Can you update the documentation please here?
> https://dxr.mozilla.org/mozilla-central/rev/904f3554c08488c53d24deb20a486600ddddd56b/toolkit/components/alerts/nsIAlertsService.idl#32

Done.

> GetWeak() is a nicer API that avoids outparams and unnecessary ref-counting.

Done.

> I find it more intuitive to track method callers than friend relationships like this.
> See also https://google.github.io/styleguide/cppguide.html#Friends
> 
> I think this use case could be handled with a single NotifyClosed() or RemoveIfActive() public method on this class.

Makes sense.

Ah, sorry; I didn't see that. Would it be better to use an `nsTArray`, as that patch does? Also, when you wrote "use a non-owning array," did you mean an array of raw pointers, or something different?

Comment 10

[Lina Butler [:lina]]

Comment on attachment 8717691 [details]
MozReview Request: Bug 1227730 - Support closing libnotify alerts. r?MattN r=karlt

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

Comment 11

[Lina Butler [:lina]]

(In reply to Kit Cambridge [:kitcambridge] from comment #9)
> Ah, sorry; I didn't see that. Would it be better to use an `nsTArray`, as
> that patch does? Also, when you wrote "use a non-owning array," did you mean
> an array of raw pointers, or something different?

Ugh, can't type today. I meant "an array of weak pointers."

Comment 12

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/34269/#review31857

::: toolkit/system/gnome/nsAlertsIconListener.cpp:109
(Diff revision 2)
> +  if (mBackend)

Oops, I don't think this will work as long as the hashtable keeps a strong-ref to the listener.

Comment 13

[Armen [:armenzg]]

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

Comment 14

[Armen [:armenzg]]

Hi kits, I tried your patch.
In the same push I've scheduled Buildbot jobs (current releng CI) and TaskCluster/docker jobs (new CI)

As far as I know, for TaskCluster only m10 e10s and bc4 e10s should have failed. Unfortunately, the number of oranges is rather high for both CIs.

I've retried a bunch of jobs and bunch still have to complete.

Here are the results for Buildbot:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d97da5c7096&group_state=expanded&selectedJob=16858075&filter-searchStr=Ubuntu

Here are the results for TaskCluster:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d97da5c7096&group_state=expanded&selectedJob=16858075&filter-searchStr=tc

Comment 15

[Lina Butler [:lina]]

Comment on attachment 8717691 [details]
MozReview Request: Bug 1227730 - Support closing libnotify alerts. r?MattN r=karlt

(In reply to Armen Zambrano [:armenzg] - Engineering productivity from comment #14)
> Hi kits, I tried your patch.
> In the same push I've scheduled Buildbot jobs (current releng CI) and
> TaskCluster/docker jobs (new CI)

Indeed, yesterday's revision is broken in many ways. :-/ Clearing r? for now.

Comment 16

[Lina Butler [:lina]]

Comment on attachment 8717691 [details]
MozReview Request: Bug 1227730 - Support closing libnotify alerts. r?MattN r=karlt

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

Comment 17

[Armen [:armenzg]]

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

Comment 18

[Armen [:armenzg]]

This patch seems to fix both m10 e10s and bc4 e10s \o/:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=80126944609b&group_state=expanded&filter-searchStr=tc%20desktop

Under Buildbot, mochitest-gl might be an issue (retrying and checking in the morning):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=80126944609b&group_state=expanded&filter-searchStr=Ubuntu

Comment 22

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/34269/#review32169

::: toolkit/system/gnome/nsAlertsIconListener.h:42
(Diff revision 3)
> +  inline void AlertName(nsAString& aAlertName) { aAlertName = mAlertName; }

No longer used.

::: toolkit/system/gnome/nsAlertsIconListener.cpp:412
(Diff revision 3)
> +void nsAlertsIconListener::NotifyFinished()

I think we need to call `mBackend->RemoveListener(mAlertName, this)` here, too.

::: toolkit/system/gnome/nsSystemAlertsService.cpp:99
(Diff revision 3)
> +  if (mActiveListeners.Get(aAlertName, getter_AddRefs(weakPrevObserver))) {

I wasn't sure if it's OK to use `GetWeak` here...other weak-ref map examples I saw in the tree used `Get`.

Comment 23

[Armen [:armenzg]]

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

Comment 24

[Armen [:armenzg]]

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

Comment 25

[Armen [:armenzg]]

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

Comment 26

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/34269/#review31351

> Done. I'm only adding the listener to the set if `InitAlertAsync` succeeds, but I can make it unconditional, if you prefer.

That sounded good, but I think this is also OK adding it regardless, as in the
last version.  Closing the old alert even if creating the new fails is kind of
consistent with async failures that may latter happen.

> I agree this shouldn't be an error if the user closed the notification. Also added code to cancel the image request, and fire `"alertfinished"` if `ShowAlert` hasn't been called yet. Does that seem sensible?

Yes, good, thanks.

I don't think it matters whether this is a array or hashtable.  Hashtable is
more efficient if the size could get large.

Comment 27

[Karl Tomlinson (:karlt)]

Comment on attachment 8717691 [details]
MozReview Request: Bug 1227730 - Support closing libnotify alerts. r?MattN r=karlt

https://reviewboard.mozilla.org/r/34269/#review32383

::: toolkit/components/alerts/nsIAlertsService.idl
(Diff revision 3)
> -   * @param alertListener  Used for callbacks. May be null if the caller
> -   *                       doesn't care about callbacks.

Keep this because it is not a parameter to
nsIAlertNotification.init()

::: toolkit/system/gnome/nsSystemAlertsService.h:28
(Diff revision 3)
> +  void AddListener(const nsAString& aAlertName,
> +                   nsAlertsIconListener* aListener);

This can be private or protected now.

::: toolkit/system/gnome/nsSystemAlertsService.h:38
(Diff revision 3)
> +  nsInterfaceHashtable<nsStringHashKey, nsIWeakReference> mActiveListeners;

These can just be raw pointers because they are cleared from
~nsAlertsIconListener.  I was using the term "weak" pointer in the sense of a
pointer without a strong reference that is cleared when the pointee object is
destroyed.

nsIWeakReference is one implementation of a weak pointer, but one that doesn't
callback into the pointing object to tell it that the pointee object is
destroyed.  This means that nsIWeakReference does not work well in lists or
sets because some other mechanism is required to reduce the size of the lists
or sets.  Implementing that mechanism often (including your implementation from
~nsAlertsIconListener) makes the use of nsIWeakReference unnecessary.

nsDataHashtable<nsStringHashKey, nsAlertsIconListener*> is simplest here.

A strong reference could work too as long as the reference is always released
eventually.  However, I find a raw pointer clearer that it is not what
keeps the nsAlertsIconListener alive.  It can also be easier to implement
clearing the pointer from the destructor when the last reference is removed
than to catch every case that needs to remove the strong reference.

::: toolkit/system/gnome/nsSystemAlertsService.cpp:115
(Diff revision 3)
> +  CloseInternal(aAlertName);
> +  mActiveListeners.Put(aAlertName, weakListener);

I'm a little uncomfortable with the synchronous alertfinished dispatch that
may happen in NotifyFinished() from Close().  Dispatching events synchronously
can surprise callers because event handlers may have changed anything during
the call.  However, this already happens for alertshow in this implementation
and alertfinished from nsXULAlerts, so this may be OK.

Please call CloseInternal() after mActiveListeners.Put() so as to
keep ordering of changes should an event handler cause mActiveListeners to be modified
again.

Comment 28

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/34269/#review32169

> I think we need to call `mBackend->RemoveListener(mAlertName, this)` here, too.

I think we can depend on the destructor for this and so this is not necessary.

> I wasn't sure if it's OK to use `GetWeak` here...other weak-ref map examples I saw in the tree used `Get`.

nsDataHashtable has UserDataType Get(KeyType aKey), which is the API is like.

CloseInternal() will still need to assign the result to a RefPtr to hold a
temporary strong reference to the nsAlertsIconListener while calling Close(),
so that clearing mIconRequest in Close() doesn't delete the
nsAlertsIconListener while it is in use.

Comment 29

[Armen [:armenzg]]

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

Comment 30

[Armen [:armenzg]]

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

Comment 31

[Lina Butler [:lina]]

Comment on attachment 8717691 [details]
MozReview Request: Bug 1227730 - Support closing libnotify alerts. r?MattN r=karlt

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

Comment 32

[Lina Butler [:lina]]

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

Comment 33

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/34269/#review32383

> These can just be raw pointers because they are cleared from
> ~nsAlertsIconListener.  I was using the term "weak" pointer in the sense of a
> pointer without a strong reference that is cleared when the pointee object is
> destroyed.
> 
> nsIWeakReference is one implementation of a weak pointer, but one that doesn't
> callback into the pointing object to tell it that the pointee object is
> destroyed.  This means that nsIWeakReference does not work well in lists or
> sets because some other mechanism is required to reduce the size of the lists
> or sets.  Implementing that mechanism often (including your implementation from
> ~nsAlertsIconListener) makes the use of nsIWeakReference unnecessary.
> 
> nsDataHashtable<nsStringHashKey, nsAlertsIconListener*> is simplest here.
> 
> A strong reference could work too as long as the reference is always released
> eventually.  However, I find a raw pointer clearer that it is not what
> keeps the nsAlertsIconListener alive.  It can also be easier to implement
> clearing the pointer from the destructor when the last reference is removed
> than to catch every case that needs to remove the strong reference.

I see now. Thank you for the thorough explanation! This cleans up the logic quite nicely, too; no more casts.

> I'm a little uncomfortable with the synchronous alertfinished dispatch that
> may happen in NotifyFinished() from Close().  Dispatching events synchronously
> can surprise callers because event handlers may have changed anything during
> the call.  However, this already happens for alertshow in this implementation
> and alertfinished from nsXULAlerts, so this may be OK.
> 
> Please call CloseInternal() after mActiveListeners.Put() so as to
> keep ordering of changes should an event handler cause mActiveListeners to be modified
> again.

Ironically, I ran into this when I tried calling `AddListener()` after `InitAlertAsync()`. Filed bug 1250246 as a follow-up.

Comment 34

[Armen [:armenzg]]

Attachment: disable test

kits: would you mind if we disable the test for a little bit until your patch lands; would this work for you?

I want to enable the job tomorrow morning and keep it visible to reduce the risk of a different test failure to be introduced.

Comment 35

[Lina Butler [:lina]]

Comment on attachment 8722186 [details] [diff] [review]
disable test

Review of attachment 8722186 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! This seems like a reasonable short-term fix for `test_alerts.html`. I think bug 1246019 is a better one to use for this patch, but it's your call.

I'm a bit worried about `browser/base/content/test/alerts/*.js`, because it looks like nearly all those tests call `notification.close()`. Out of curiosity, are you planning to disable those tests as well, or just hide that suite on Treeherder?

::: toolkit/components/alerts/test/mochitest.ini
@@ +3,5 @@
>  
>  # Synchronous tests like test_alerts.html must come before
>  # asynchronous tests like test_alerts_noobserve.html!
>  [test_alerts.html]
> +skip-if = toolkit == 'android' || toolkit == "gtk2" || toolkit == "gtk3 # Disable Linux until Bug 1227730 lands

Missing closing quote for "gtk3".

Comment 36

[Karl Tomlinson (:karlt)]

Comment on attachment 8717691 [details]
MozReview Request: Bug 1227730 - Support closing libnotify alerts. r?MattN r=karlt

https://reviewboard.mozilla.org/r/34269/#review32611

::: toolkit/system/gnome/nsSystemAlertsService.cpp:89
(Diff revisions 3 - 4)
> -  nsCOMPtr<nsIWeakReference> weakListener =
> +  RefPtr<nsAlertsIconListener> listener = mActiveListeners.Get(aAlertName);

Can you name this oldListener or similar please to clarify that it is different from aListener.

::: browser/base/content/test/alerts/browser_notification_replace.js:5
(Diff revision 4)
> +add_task(function* test_notificationReplace() {

I'm probably not the best reviewer for the test.  I don't know how function* differs from function, for example.  But this looks good to me.

::: toolkit/system/gnome/nsSystemAlertsService.cpp:76
(Diff revision 4)
> -  return NS_ERROR_NOT_IMPLEMENTED;
> +  RefPtr<nsAlertsIconListener> listener = mActiveListeners.Get(aAlertName);
> +  return listener ? listener->Close() : NS_OK;

Close() isn't really designed to be called twice as notify_notification_close() would be called twice, and I don't know whether that is expected.

I think it is best to

 if (!listener)
   return NS_OK;

 mActiveListeners.Remove(aAlertName)
 return listener->Close().

Comment 37

[Armen [:armenzg]]

(In reply to Kit Cambridge [:kitcambridge] from comment #35)
> I'm a bit worried about `browser/base/content/test/alerts/*.js`, because it
> looks like nearly all those tests call `notification.close()`. Out of
> curiosity, are you planning to disable those tests as well, or just hide
> that suite on Treeherder?
> 

I was hoping to hide the suite instead of disabling that many tests, however, we would then be exposing ourselves to same situation we're trying to prevent with this patch.

I will let you know. I will probably disable the tests.

I see that karl gave you an r+; should we just wait for your landing?

Comment 38

[Lina Butler [:lina]]

Comment on attachment 8717691 [details]
MozReview Request: Bug 1227730 - Support closing libnotify alerts. r?MattN r=karlt

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

Comment 39

[Lina Butler [:lina]]

Comment on attachment 8717691 [details]
MozReview Request: Bug 1227730 - Support closing libnotify alerts. r?MattN r=karlt

Matthew, could you have a quick look at the test, please?

Comment 40

[Lina Butler [:lina]]

(In reply to Armen Zambrano [:armenzg] - Engineering productivity from comment #37)
> I see that karl gave you an r+; should we just wait for your landing?

Sure, if that works for you. Could you push the latest patch to Try for me, please? I pushed with `try: -b d -p linux64_tc -u all -t none` in comment 32, but it didn't run your new job.

Thanks, Armen!

Comment 41

[Armen [:armenzg]]

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

Comment 42

[Armen [:armenzg]]

(In reply to Kit Cambridge [:kitcambridge] from comment #40)
> (In reply to Armen Zambrano [:armenzg] - Engineering productivity from
> comment #37)
> > I see that karl gave you an r+; should we just wait for your landing?
> 
> Sure, if that works for you. Could you push the latest patch to Try for me,
> please? I pushed with `try: -b d -p linux64_tc -u all -t none` in comment
> 32, but it didn't run your new job.
> 
> Thanks, Armen!

That syntax only works with an extra patch [1]. It is meant to *only* schedule the TaskCluster tasks and save resources for Buildbot.

I've done a push with your latest change (I did not use [1]):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=50581d785bcc&group_state=expanded&selectedJob=17097226

I think at this point, you can push and trigger specific platforms that your tests could affect (since I'm just guessing which platforms and jobs your change needs).

[1] https://hg.mozilla.org/try/rev/e51fabeb823b

Comment 43

[Matthew N. [:MattN]]

Comment on attachment 8717691 [details]
MozReview Request: Bug 1227730 - Support closing libnotify alerts. r?MattN r=karlt

https://reviewboard.mozilla.org/r/34269/#review32743

I have to go for lunch now but can look closer after

::: browser/base/content/test/alerts/browser_notification_replace.js:13
(Diff revision 5)
> +    let win = aBrowser.contentWindow.wrappedJSObject;
> +    let notification = win.showNotification1();

I'm not sure what the e10s team viewpoint on CPOWs is in tests. Could you use ContentTask here instead?

Comment 44

[Lina Butler [:lina]]

Comment on attachment 8717691 [details]
MozReview Request: Bug 1227730 - Support closing libnotify alerts. r?MattN r=karlt

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

Comment 45

[Matthew N. [:MattN]]

Comment on attachment 8717691 [details]
MozReview Request: Bug 1227730 - Support closing libnotify alerts. r?MattN r=karlt

https://reviewboard.mozilla.org/r/34269/#review32763

Comment 46

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/fx-team/rev/9b19f285db0f444895acd340bcc54499b49758c0
Bug 1227730 - Support closing libnotify alerts. r=karlt,MattN

Comment 47

[Armen [:armenzg]]

This was merged to m-c, however, there was no link pasted by sheriff.

Thanks Kit! We now have *all* TC L64 debug jobs running green and visible on Treeherder.

Comment 48

[Lina Butler [:lina]]

https://hg.mozilla.org/mozilla-central/rev/9b19f285db0f