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

RESOLVED FIXED in Firefox 46

Status

()

Toolkit
Notifications and Alerts
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: edwong, Assigned: kitcambridge)

Tracking

(Blocks: 2 bugs)

44 Branch
mozilla46
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 wontfix, firefox45 affected, firefox46 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
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.
status-firefox44: --- → affected
status-firefox45: --- → affected
Created attachment 8686315 [details]
MozReview Request: Bug 1219855, Part 1 - Make `nsXULAlerts` implement `nsIAlertsService`. r=MattN,wchen

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

Updated

2 years ago
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
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...
Attachment #8686315 - Flags: feedback?(MattN+bmo)
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 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.
Attachment #8686315 - Flags: feedback?(MattN+bmo) → feedback+
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?
Attachment #8686315 - Flags: feedback?(wchen)
(Assignee)

Updated

2 years ago
Attachment #8686315 - Flags: feedback?(wchen)
Attachment #8686315 - Flags: feedback+
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 on attachment 8686315 [details]
MozReview Request: Bug 1219855, Part 1 - Make `nsXULAlerts` implement `nsIAlertsService`. r=MattN,wchen

Fixed `nsCOMPtr` usage...I think.
Attachment #8686315 - Flags: feedback?(wchen)
(Assignee)

Updated

2 years ago
Blocks: 1225388
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);

Updated

2 years ago
Attachment #8686315 - Flags: feedback?(wchen)
(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.
(Reporter)

Updated

2 years ago
status-firefox44: affected → wontfix
Duplicate of this bug: 1224914
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/
Attachment #8686315 - Attachment description: MozReview Request: Bug 1219855 - Always use XUL notifications if the system backend fails. → MozReview Request: Bug 1219855, Part 1 - Make `nsXULAlerts` implement `nsIAlertsService`. r?MattN,wchen
Attachment #8686315 - Flags: review?(wchen)
Attachment #8686315 - Flags: review?(MattN+bmo)
Created attachment 8698122 [details]
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
Attachment #8698122 - Flags: review?(wchen)

Updated

2 years ago
Attachment #8686315 - Flags: review?(wchen) → review+
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 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.
Attachment #8698122 - Flags: review?(wchen) → review+
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".
Attachment #8686315 - Flags: review?(MattN+bmo) → review+
(Assignee)

Updated

2 years ago
Blocks: 1237026
(Assignee)

Updated

2 years ago
Blocks: 1237031
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 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/
Attachment #8686315 - Attachment description: MozReview Request: Bug 1219855, Part 1 - Make `nsXULAlerts` implement `nsIAlertsService`. r?MattN,wchen → MozReview Request: Bug 1219855, Part 1 - Make `nsXULAlerts` implement `nsIAlertsService`. r=MattN,wchen
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/
Attachment #8698122 - Attachment description: MozReview Request: Bug 1219855, Part 2 - Always use XUL notifications if the system backend fails. r?wchen → MozReview Request: Bug 1219855, Part 2 - Always use XUL notifications if the system backend fails. r=wchen
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa5ca4516b64
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49454063677f
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 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/
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cb99a73a085
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 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

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

Comment 28

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6d0382921a68
https://hg.mozilla.org/mozilla-central/rev/743089edf783
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
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.
Flags: needinfo?(kcambridge)

Comment 30

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

Comment 31

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee46e4394151
Sorry about that; I didn't catch the warning. This should be fixed now.
Flags: needinfo?(kcambridge)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1217646
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1222480
You need to log in before you can comment on or make changes to this bug.