1219855 - notification.close() isn't closing notification on linux xul notification

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Edwin Wong [:edwong]]

1. on nightly (i'm on ubuntu) goto: https://people.mozilla.org/~mnoorenberghe/w3c_notifications.htm
2. click on 'show notification' button
3. accept permission if needed
4. click on 'close notification' button

acutal: notification doesn't close

expected: it should close, like it does on Fx25 (ubuntu fork) I'm guessing in Fx42 it works also... but haven't verified.

Comment 1

[Edwin Wong [:edwong]]

when i select gear > do not disturb i get this error in console:
NS_ERROR_NOT_IMPLEMENTED: Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIAlertsDoNotDisturb.manualDoNotDisturb]

I believe fx is confused with libnotify and falling back to xul.

Comment 2

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1219855, Part 1 - Make `nsXULAlerts` implement `nsIAlertsService`. r=MattN,wchen

Bug 1219855 - Always use XUL notifications if the system backend fails.

Comment 3

[Lina Butler [:lina]]

Comment on attachment 8686315 [details]
MozReview Request: Bug 1219855, Part 1 - Make `nsXULAlerts` implement `nsIAlertsService`. r=MattN,wchen

This patch refactors the fallback logic to always try the system-level backend, if one is available. If that fails, it'll retry with the XUL backend, and use XUL for all future notifications. I also made `nsXULAlerts` implement `nsIAlertsService`, so we can store it in the same pointer as the system backend.

Still need to fix up Do Not Disturb...

Comment 4

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/24987/#review22525

Thanks, this really cleans things up.

::: toolkit/components/alerts/nsAlertsService.h:51
(Diff revision 1)
> +      nsCOMPtr<nsIAlertsService> backend(nsXULAlerts::GetInstance());
> +      if (!backend) {

Can you call this xulBackend as I had a hard time distinguishing `mBackend` from `backend`?

::: toolkit/components/alerts/nsAlertsService.h:62
(Diff revision 1)
> +      nsCOMPtr<nsIAlertsService> backend(nsXULAlerts::GetInstance());
> +      if (!backend) {
> +        return NS_ERROR_FAILURE;

Ditto

::: toolkit/components/alerts/nsAlertsService.h:77
(Diff revision 1)
> +    nsCOMPtr<nsIAlertsService> backend(nsXULAlerts::GetInstance());
> +    return callback(backend);

Ditto

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8686315 [details]
MozReview Request: Bug 1219855, Part 1 - Make `nsXULAlerts` implement `nsIAlertsService`. r=MattN,wchen

I don't know much about the C++ style, correctness of the XPCOM goop or newer C++ syntax.

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8686315 [details]
MozReview Request: Bug 1219855, Part 1 - Make `nsXULAlerts` implement `nsIAlertsService`. r=MattN,wchen

William, could you have a look at this, please?

Comment 7

[Lina Butler [:lina]]

Comment on attachment 8686315 [details]
MozReview Request: Bug 1219855, Part 1 - Make `nsXULAlerts` implement `nsIAlertsService`. r=MattN,wchen

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

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8686315 [details]
MozReview Request: Bug 1219855, Part 1 - Make `nsXULAlerts` implement `nsIAlertsService`. r=MattN,wchen

Fixed `nsCOMPtr` usage...I think.

Comment 9

[William Chen [:wchen]]

https://reviewboard.mozilla.org/r/24987/#review23125

Ideally we would want to know/test the capabilities of the platform backend and use it if it supports everything we need and stick with it to keep things consistent across the session. For now to keep changes smaller, I think we should only force a fallback if it fails the first time we show a notification and then stick with the same backend. In the current patch, there is an opportunity for the fallback to happen every time we show or close a notification. If we switch the backend in the middle of a session, it's not only a weird experience, but we may also end up with subtle bugs from inconsistent internal state.

::: toolkit/components/alerts/nsAlertsService.h:48
(Diff revision 2)
> +  nsresult EnsureBackend(F aCallback)

I'm not sure what our policy is right now for using lambdas or lambdas as function arguments, but bug 1164522 is relevant here. Also, if we only fallback when we show notifications the first time, then using lambdas wouldn't be necessary since this code is only being used in one place.

::: toolkit/components/alerts/nsXULAlerts.cpp:22
(Diff revision 2)
> +nsXULAlerts* sXULAlerts = nullptr;

It doesn't look like anything guarantees that this stays alive so we should use a StaticRefPtr here.

::: toolkit/components/alerts/nsXULAlerts.cpp:55
(Diff revision 2)
> +    sXULAlerts = instance;

ClearOnShutdown(&sXULAlerts);

Comment 10

[Lina Butler [:lina]]

(In reply to William Chen [:wchen] from comment #9)
> https://reviewboard.mozilla.org/r/24987/#review23125
> 
> Ideally we would want to know/test the capabilities of the platform backend
> and use it if it supports everything we need and stick with it to keep
> things consistent across the session.

Yes. We'd need to generalize our logic to, "does this backend support every potential feature," rather than "can it show this specific notification." There are other cases where the backend can fail, but I'm coming around to treating those as bugs, rather than fallback cases.

> For now to keep changes smaller, I
> think we should only force a fallback if it fails the first time we show a
> notification and then stick with the same backend. In the current patch,
> there is an opportunity for the fallback to happen every time we show or
> close a notification.

Sorry, not sure I follow. You're suggesting we fall back to XUL once, then keep using the system backend? 

> If we switch the backend in the middle of a session,
> it's not only a weird experience, but we may also end up with subtle bugs
> from inconsistent internal state.

I filed bug 1225388 to make the fallback permanent. It's still weird to switch backends in the middle of the session, but at least subsequent sessions should use XUL.

> 
> ::: toolkit/components/alerts/nsAlertsService.h:48
> (Diff revision 2)
> > +  nsresult EnsureBackend(F aCallback)
> 
> I'm not sure what our policy is right now for using lambdas or lambdas as
> function arguments, but bug 1164522 is relevant here. Also, if we only
> fallback when we show notifications the first time, then using lambdas
> wouldn't be necessary since this code is only being used in one place.

I'm very likely missing something, but, reading through that bug...isn't `std::function` (or `mozilla::Function`) for the case where you can't use a template?

You're totally right, though; if we fall back just once, we don't need the lambda.

Comment 12

[Lina Butler [:lina]]

Comment on attachment 8686315 [details]
MozReview Request: Bug 1219855, Part 1 - Make `nsXULAlerts` implement `nsIAlertsService`. r=MattN,wchen

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

Comment 13

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1219855, Part 2 - Always use XUL notifications if the system backend fails. r=wchen

Bug 1219855, Part 2 - Always use XUL notifications if the system backend fails. r?wchen

Comment 14

[William Chen [:wchen]]

Comment on attachment 8686315 [details]
MozReview Request: Bug 1219855, Part 1 - Make `nsXULAlerts` implement `nsIAlertsService`. r=MattN,wchen

https://reviewboard.mozilla.org/r/24987/#review25219

::: toolkit/components/alerts/nsIAlertsService.idl:11
(Diff revision 3)
>  [scriptable, uuid(b26b4a67-81b0-4270-8311-1e00a097ef92)]

uuid

::: toolkit/components/alerts/nsIAlertsService.idl:32
(Diff revision 3)
> +  boolean isActionable();

Move the comment that defines actionable to here (from nsAlertUtils)

Comment 15

[William Chen [:wchen]]

Comment on attachment 8698122 [details]
MozReview Request: Bug 1219855, Part 2 - Always use XUL notifications if the system backend fails. r=wchen

https://reviewboard.mozilla.org/r/27813/#review25291

::: toolkit/components/alerts/nsAlertsService.cpp:245
(Diff revision 1)
> +  NS_ENSURE_TRUE(alertsDND, nullptr);

This line is almost redundant. If you are using this for the warning then NS_WARN_IF(!alertsDND) would be sufficient.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8686315 [details]
MozReview Request: Bug 1219855, Part 1 - Make `nsXULAlerts` implement `nsIAlertsService`. r=MattN,wchen

https://reviewboard.mozilla.org/r/24987/#review25499

Thanks

::: toolkit/components/alerts/AlertNotification.js:17
(Diff revision 3)
> +const ALERT_BUNDLE = Services.strings.createBundle(
> +  "chrome://alerts/locale/alert.properties");
> +const BRAND_BUNDLE = Services.strings.createBundle(
> +  "chrome://branding/locale/brand.properties");

Toolkit/Firefox normally wraps at the periods in newer code so this looks foreign. You don't need to wrap here since it's < 100 characters.

::: toolkit/components/alerts/AlertNotification.js:55
(Diff revision 3)
> +    if (type == Ci.nsIAlertNotification.LABEL_SOURCE) {
> +      return ALERT_BUNDLE.formatStringFromName(
> +        "source.label", [this.principal.URI.hostPort], 1);
> +    }
> +    if (type == Ci.nsIAlertNotification.LABEL_DO_NOT_DISTURB) {

Nit: why not a `switch (type) {`?

::: toolkit/components/alerts/AlertNotification.js:56
(Diff revision 3)
> +      return ALERT_BUNDLE.formatStringFromName(
> +        "source.label", [this.principal.URI.hostPort], 1);
> +    }
> +    if (type == Ci.nsIAlertNotification.LABEL_DO_NOT_DISTURB) {
> +      return ALERT_BUNDLE.formatStringFromName(
> +        "doNotDisturb.label", [BRAND_NAME], 1);
> +    }
> +    if (type == Ci.nsIAlertNotification.LABEL_DISABLE) {
> +      return ALERT_BUNDLE.formatStringFromName(
> +        "webActions.disableForOrigin.label", [this.principal.URI.hostPort], 1);

Similar here for wrapping: don't wrap after an opening `(`. You can wrap between arguments instead.

::: toolkit/components/alerts/nsAlertsService.cpp
(Diff revision 3)
> -#ifdef MOZ_WIDGET_ANDROID
>    mozilla::AndroidBridge::Bridge()->ShowAlertNotification(imageUrl, title, text, cookie,

Eventually we need to move Android to a system alert backend like the others.

::: toolkit/components/alerts/resources/content/alert.js:28
(Diff revision 3)
>    // unwrap all the args....
> -  // arguments[0] --> the image src url
> -  // arguments[1] --> the alert title
> -  // arguments[2] --> the alert text
> -  // arguments[3] --> is the text clickable?
> -  // arguments[4] --> the alert cookie to be passed back to the listener
> +  // arguments[0] --> the alert
> +  // arguments[1] --> the alert origin reported by the look and feel
> +  // arguments[2] --> replaced alert window (nsIDOMWindow)
> +  // arguments[3] --> an optional callback listener (nsIObserver)
> +

This is going to break add-on compat as there are extensions that open alert.xul themselves IIRC. add-ons MXR seems broken ATM. so I can't check how many are affected. Can we move the window argument change to a new bug to unblock this landing? Then we can investigate the breakage this will cause and mark that bug as "addon-compat".

Comment 17

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/24987/#review25499

> Eventually we need to move Android to a system alert backend like the others.

Filed bug 1237026. For now, I think it's OK to keep as-is, since we don't fall back to XUL on Android.

> This is going to break add-on compat as there are extensions that open alert.xul themselves IIRC. add-ons MXR seems broken ATM. so I can't check how many are affected. Can we move the window argument change to a new bug to unblock this landing? Then we can investigate the breakage this will cause and mark that bug as "addon-compat".

Bug 1237031.

Comment 18

[Lina Butler [:lina]]

Comment on attachment 8686315 [details]
MozReview Request: Bug 1219855, Part 1 - Make `nsXULAlerts` implement `nsIAlertsService`. r=MattN,wchen

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

Comment 19

[Lina Butler [:lina]]

Comment on attachment 8698122 [details]
MozReview Request: Bug 1219855, Part 2 - Always use XUL notifications if the system backend fails. r=wchen

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

Comment 20

[Lina Butler [:lina]]

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

Comment 21

[Lina Butler [:lina]]

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

Comment 22

[Lina Butler [:lina]]

Comment on attachment 8686315 [details]
MozReview Request: Bug 1219855, Part 1 - Make `nsXULAlerts` implement `nsIAlertsService`. r=MattN,wchen

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

Comment 23

[Lina Butler [:lina]]

Comment on attachment 8698122 [details]
MozReview Request: Bug 1219855, Part 2 - Always use XUL notifications if the system backend fails. r=wchen

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

Comment 24

[Lina Butler [:lina]]

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

Comment 25

[Lina Butler [:lina]]

Comment on attachment 8686315 [details]
MozReview Request: Bug 1219855, Part 1 - Make `nsXULAlerts` implement `nsIAlertsService`. r=MattN,wchen

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

Comment 26

[Lina Butler [:lina]]

Comment on attachment 8698122 [details]
MozReview Request: Bug 1219855, Part 2 - Always use XUL notifications if the system backend fails. r=wchen

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

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6d0382921a68
https://hg.mozilla.org/integration/fx-team/rev/743089edf783

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6d0382921a68
https://hg.mozilla.org/mozilla-central/rev/743089edf783

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Hey Kit, wanna fix up those two warnings? Seems like you could reuse the sbs defined above, right?

 1:47.37 /Users/ckerschb/Documents/moz/mc/widget/cocoa/OSXNotificationCenter.mm:299:31: warning: declaration shadows a local variable [-Wshadow]
 1:47.37     nsCOMPtr<nsIStringBundle> bundle;
 1:47.37                               ^
 1:47.37 /Users/ckerschb/Documents/moz/mc/widget/cocoa/OSXNotificationCenter.mm:274:29: note: previous declaration is here
 1:47.37   nsCOMPtr<nsIStringBundle> bundle;
 1:47.37                             ^
 1:47.37 Warning: -Wshadow in /Users/ckerschb/Documents/moz/mc/widget/cocoa/OSXNotificationCenter.mm: declaration shadows a local variable
 1:47.37 /Users/ckerschb/Documents/moz/mc/widget/cocoa/OSXNotificationCenter.mm:300:38: warning: declaration shadows a local variable [-Wshadow]
 1:47.37     nsCOMPtr<nsIStringBundleService> sbs = do_GetService(NS_STRINGBUNDLE_CONTRACTID);
 1:47.37                                      ^
 1:47.37 /Users/ckerschb/Documents/moz/mc/widget/cocoa/OSXNotificationCenter.mm:275:36: note: previous declaration is here
 1:47.37   nsCOMPtr<nsIStringBundleService> sbs = do_GetService(NS_STRINGBUNDLE_CONTRACTID);
 1:47.37                                    ^
 1:47.37 2 warnings generated.

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ee46e4394151

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ee46e4394151

Comment 32

[Lina Butler [:lina]]

Sorry about that; I didn't catch the warning. This should be fixed now.