Closed Bug 852648 Opened 7 years ago Closed 6 years ago

Implement notification center support for web notifications.

Categories

(Toolkit :: XUL Widgets, defect)

All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
relnote-firefox --- 28+

People

(Reporter: wchen, Assigned: jaas)

References

(Depends on 1 open bug, )

Details

(Keywords: feature, Whiteboard: [tb31features])

Attachments

(3 files, 16 obsolete files)

76.77 KB, image/jpeg
Details
1.18 KB, patch
cpeterson
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
25.71 KB, patch
Details | Diff | Splinter Review
Web notifications have landed using XUL alerts on mac. It would be nice to have Notification Center support.
The given URL http://jsbin.com/notification/1/edit can be used for testing.

Currently Web Notifications using XUL alerts look like this http://i49.tinypic.com/35b8e3c.png. Integrated with Notification Center would look similar to this http://i47.tinypic.com/ogw8xj.png. To test visit the given URL and with the settings shown in this image http://i46.tinypic.com/6jdk7p.png you should see an entry under the Notifications Center as shown for Chrome in this other image http://i46.tinypic.com/fwucrk.png
Status: NEW → ASSIGNED
Depends on: 785897
Bug 785897 Comment 5 (and following) indicate that NC does not provide what Mozilla wants to have. 

With Mavericks out now that includes changes to NC, and Safari using NC for Web/PushNotifications, I think this decision should be revisited.
I don't see any changes in 10.9 APIs that would impact this.

https://developer.apple.com/library/mac/releasenotes/General/APIDiffsMacOSX10_9/AppKit.html

We should look at what others are doing again, see how they're making use of the APIs.
There are several NS*Notification* classes, but only NSUserNotification* are responsible for the (user-facing) Notification Center (IIRC).

see e.g. https://developer.apple.com/library/mac/releasenotes/Foundation/RN-Foundation/index.html#//apple_ref/doc/uid/TP30000742
> NSUserNotification
> 
> Calling -removeDeliveredNotification: and -removeAllDeliveredNotifications: will now remove notifications that are displayed.
> 
> A new property on NSUserNotification, -identifier, can be used to uniquely identify an individual notification even across separate launches of an application. If a notification is delivered with the same identifier as an existing notification, it will replace the preexisting notification. This can be used to update existing notifications.
> 
> NSUserNotification has a new property to specify the image shown in the content of a notification.
> 
> NSUserNotification allows for a ‘quick reply.’ The property responsePlaceholder can be used to set a placeholder string and the response can be retrieved from the response property.

I remember being unable to set an image (different from the App icon, i.e. Firefox) was a show stopper for using NC in Firefox, this is now possible. Also, there was an issue with dismissal, which also got changed.

So, again, Safari is now using NC for their push support, so it should also be possible to show a notification even if the application is currently in the foreground (hopefully). I also remember this being cited as an issue before. 

Curiously, the changes cited above are not in the API diff view, if I see that correctly.
Attached patch Fix v0.7 (obsolete) — Splinter Review
I put this together in my spare time over the past couple of days. It's lightly tested and not ready for review (I worked quickly, code is pretty sloppy), but it works pretty well. I got past most of the issues that were problematic in the past.

I will keep hacking on this when I get time, feedback and bug reports would be appreciated.
BTW, this should fall back to XUL notification on OS X < 10.8.
Thanks Josh, I'm looking forward to this. I have done some quick testing with a test page I've made and I found some issues. I'm running 10.9 Mavericks.

Issues for parity with the current XUL notifications:
* The 'icon' property of the Notification is ignored. NSUserNotification.contentImage (10.9) can probably 
  be used.
* The 'close' event currently only fires as a result of calling .close() on a notification but not after a 
  click closes the notification or when the user/system closes it through other means.
* The 'tag' property don't seem to be supported for replacing an existing notification.   
  NSUserNotification.identifier (10.9) is probably useful for this.

Other issues to keep in mind for the future:
* 'dir' (RTL) support. This doesn't work with the XUL implementation but does on B2G. I filed bug 935434 
  for this.

Also, do you plan on fixing this up to implement the system alert service like the previous NC implementation was, rather than hacking this into nsAlertsService.cpp?
Awesome :)

(In reply to Josh Aas (Mozilla Corporation) from comment #6)
> BTW, this should fall back to XUL notification on OS X < 10.8.

If the API in 10.8 does not provide what you need, you can actually require 10.9. Given this is a free update, the number of users staying with 10.8 should be very low within a few months.
10.9 doesn't add anything helpful here, 10.8 should be fine.
I've tested patch v.07 with Thunderbird and this is a real improvement to the last (short) old implementation from Bug 728106. Because If I click on a notification (banner or in the NC), than this email opens, this was not the case with the old implementation. This is really cool. :) (I have not tested yet what will happen if I receive more than one new mail and than click on the notification).
I did some experimenting with the new 10.9 APIs. NSUserNotification.identifier is not terribly useful because it only replaces delivered notifications, if they're still being displayed they are not changed. contentImage should allow implementing the icon property of the Notifications API but I don't know what the API actually is there, since the docs haven't been updated yet. I don't think 10.9 changes anything regarding how to fire the alertfinished event, there's no way to detect a notification being closed or dismissed other than by clicking on it.
My patch doesn't work on 10.9. I wrote it on 10.8, things worked well, then I upgraded to 10.9 and now -[NSUserNotificationCenter deliverNotification:] appears to be called but does nothing. No debug messages printed to the console. Frustrating.
It does for me…
It does for me too. I've build my Thunderbird build with your patch with the 10.9 SDK and it works fine for me on 10.9.
Josh: I was going to say that there may be a certificate issue, though that does not explain that it works for Reuben or Nomis … anyway, might still help.
Strange. I still can't get any notifications to show up on OS X 10.9, loading a test HTML file locally or from a web server. Can't really move forward working on this patch until I figure that out.
Ugh, got desperate, rebooted my laptop and notifications are back.
Attached patch Fix v0.8 (obsolete) — Splinter Review
This cleans up the code a bit, fixes a memory leak, and most importantly it adds proper support for the closed notification. The key to this last bit was my discovery of this undocumented API on NSUserNotificationCenterDelegate:

+// This is an undocumented method that we need for parity with Safari.
+// Apple bug #15440664.
+- (void)userNotificationCenter:(NSUserNotificationCenter *)center
+  didRemoveDeliveredNotifications:(NSArray *)notifications

This is the most important thing I was missing the first time I implemented NC support a while ago. As you can see in the code comment, I have filed a bug with Apple.
Attachment #827669 - Attachment is obsolete: true
+- (void)userNotificationCenter:(NSUserNotificationCenter *)center
+  didRemoveDeliveredNotifications:(NSArray *)notifications

I didn't see this in a class-dump of the Mountain Lion Foundation framework.  So it's presumably only available on Mavericks and up.
(Following up comment #19)

Oops, I take that back.

This notification gets sent from [_NSConcreteUserNotificationCenter _deliveredMessagesRemoved:] on both Mavericks and Mountain Lion.  I haven't yet checked earlier versions of OS X.
(Following up comment #19 and comment #20)

The didRemoveDeliveredNotifications notification is unknown to the Lion Foundation framework, or presumably to earlier versions of OS X.
I'm not sure if this bug is Thunderbird-related, but I have set in Thunderbird to open messages in a new tab. But if I click on a NC message (with newest patch) it always opens in a new window. So the setting to open the message in a new tab is not respected. If this is a Core thing, it would be nice if you could fix it in your patch. If this is Thunderbird-related, than I will fill a Mailnews bug for this.
(In reply to Nomis101 from comment #22)

Do you see the same behavior without my patch? If this behavior you don't also happens without my patch (and it seems like it does) then it's outside the scope of this bug. Please file a Thunderbird bug.
If I'm honest, I've never tried that with a build without your patch. I did now, and it is unrelated with your patch, its TB-specific.
Attached patch Fix v1.0 (obsolete) — Splinter Review
This cleans up memory management, stability, and makes tests pass. Notification Center doesn't support custom icons, so if we want native notifications on OS X we'll have to use the Firefox icon every time.

Happy people have been testing with my patches, please continue to do so if you can, feedback appreciated.
Attachment #830503 - Attachment is obsolete: true
Attachment #832035 - Flags: review?(wchen)
(In reply to Josh Aas (Mozilla Corporation) from comment #25)
> Notification Center doesn't support custom icons, so if we want native
> notifications on OS X we'll have to use the Firefox icon every time.

Safari uses custom icons for their push notifications, and the API docs I mentioned above indicate support for specifying a custom icon (at least in 10.9).
With your current patch v1.0 nothing happens anymore if I click on a new mail notification in Thunderbird. With the patch v0.8 and 0.7 it was opening me the email. I don't hope it was the purpose to remove this.
(In reply to Nomis101 from comment #27)
> With your current patch v1.0 nothing happens anymore if I click on a new
> mail notification in Thunderbird. With the patch v0.8 and 0.7 it was opening
> me the email. I don't hope it was the purpose to remove this.

Are you sure? I am making some improvements to my patch and I tried to reproduce this, onclick seems to work just fine. I definitely didn't intend to change that.
(In reply to Josh Aas (Mozilla Corporation) from comment #28)
> (In reply to Nomis101 from comment #27)
> > With your current patch v1.0 nothing happens anymore if I click on a new
> > mail notification in Thunderbird. With the patch v0.8 and 0.7 it was opening
> > me the email. I don't hope it was the purpose to remove this.
> 
> Are you sure? I am making some improvements to my patch and I tried to
> reproduce this, onclick seems to work just fine. I definitely didn't intend
> to change that.
Yes, sorry, thats reproducible. I've tested this now several times and with different TB versions to be sure. Everytime I build with the old patch it works, with the new patch it doesn't. With the new patch TB only comes to the foreground after clicking, but it doesn't open the message. It does with the old patch.
Comment on attachment 832035 [details] [diff] [review]
Fix v1.0

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

::: toolkit/components/alerts/OSXNotificationCenter.mm
@@ +134,5 @@
> +{
> +  return !!NSClassFromString(@"NSUserNotification");
> +}
> +
> +nsresult OSXNotificationCenter::ShowAlertNotification(const nsAString & aAlertTitle, const nsAString & aAlertText,

The spec requires that a notification with the same origin and tag replaces any existing notification with the same origin and tag. In order to have this behavior, the alert service implementations need to replace alerts with the same alert name. I'm not sure if the notification center can actually do replacement (keeping the same position in the list), but the spec mentions that it can also be done by closing the old notification and displaying the new one.

http://notifications.spec.whatwg.org/#replace-steps
Attachment #832035 - Flags: review?(wchen)
(In reply to William Chen [:wchen] from comment #30)

Thanks for the heads up. I'm going to fix that, add support for icons on 10.9, and try to make an alerts service instead of hacking directly into nsAlertsService.cpp.
Attached patch Fix v1.1 (obsolete) — Splinter Review
Moves NC support to widget/cocoa, as a system service. Also resolves notification replacement issue.

Still need to figure out why Nomis is seeing bad behavior with Thunderbird and add support for icons on 10.9.
Attachment #832035 - Attachment is obsolete: true
Attached patch Fix v1.2 (obsolete) — Splinter Review
Adds support for icons on 10.9+. We need to have the icon before showing the notification (Cocoa API limitation), which requires a network load to complete before the notification shows. I have a timer that limits this load time to three seconds - if we don't have an icon by then we just show the notification without the icon.
Attachment #833372 - Attachment is obsolete: true
(In reply to Josh Aas (Mozilla Corporation) from comment #33
> Adds support for icons on 10.9+. We need to have the icon before showing the
> notification (Cocoa API limitation), which requires a network load to
> complete before the notification shows. I have a timer that limits this load
> time to three seconds - if we don't have an icon by then we just show the
> notification without the icon.

I assume the Firefox logo is shown in that case? You may want to show a generic globe icon (like the one in the identity bar) instead so the user knows this is not a Firefox but website message incoming. Also, is the icon stored and fetched from the cache?
(In reply to Florian Bender from comment #34)
> I assume the Firefox logo is shown in that case? You may want to show a
> generic globe icon (like the one in the identity bar) instead so the user
> knows this is not a Firefox but website message incoming. Also, is the icon
> stored and fetched from the cache?

The Firefox logo is always shown, whether or not we assign a content image. If there is a content image the Firefox logo gets smaller. As for the cache question, it should be cached via the HTTP cache.
Attached patch Fix v1.3 (obsolete) — Splinter Review
Among other things this cleans up memory management, passes the principal on to the image loading mechanism, and limits icon image sizes to height and width < 1000px.

One thing left to fix, a test failure, have a question for wchen about that, will ask offline.
Attachment #8334342 - Attachment is obsolete: true
(In reply to Josh Aas (Mozilla Corporation) from comment #32)
> Still need to figure out why Nomis is seeing bad behavior with Thunderbird
> and add support for icons on 10.9.

Any luck with finding out why clicking on the message in TB does not work anymore in your newer patches?
Could it be this is because in your newer patches you filtering for the existance of a name, but TB does not have aAlertName?
http://hg.mozilla.org/comm-central/file/ffe30ba8cd1b/mailnews/base/src/nsMessengerOSXIntegration.mm#l397
(In reply to Nomis101 from comment #37)
> Could it be this is because in your newer patches you filtering for the
> existance of a name, but TB does not have aAlertName?
> http://hg.mozilla.org/comm-central/file/ffe30ba8cd1b/mailnews/base/src/
> nsMessengerOSXIntegration.mm#l397

I'm pretty sure that is the reason, and it is confirmed as the reason the tests fail. My questions for wchen are about that.
Attached patch Fix v1.4 (obsolete) — Splinter Review
Fixes tests and hopefully the Thunderbird problem.
Attachment #8334600 - Attachment is obsolete: true
Attachment #8334837 - Flags: review?(wchen)
Attached image Massive icon in NC
(In reply to Josh Aas (Mozilla Corporation) from comment #39)
> Created attachment 8334837 [details] [diff] [review]
> Fix v1.4
> 
> Fixes tests and hopefully the Thunderbird problem.

Thanks, I will test this patch and then report if it is fixed.
But, question, is it wanted that the icon looks that massive?
(In reply to Nomis101 from comment #40)
> Thanks, I will test this patch and then report if it is fixed.
> But, question, is it wanted that the icon looks that massive?

So far as I know I don't have control over the size. Maybe I could re-size the image to something smaller, I didn't try, but I suspect you're seeing the same thing I did when I fed in a much bigger image. Seems fine to me, I don't really want to fine-tune that in this first round even if we can do a bit better in the end.
(In reply to Josh Aas (Mozilla Corporation) from comment #39)
> Created attachment 8334837 [details] [diff] [review]
> Fix v1.4
> 
> Fixes tests and hopefully the Thunderbird problem.
The TB thingy still does not work, sorry.
Attached patch Fix v1.5 (obsolete) — Splinter Review
Nomis - I missed something last time, this patch should fix the tbird problem. Let me know. Thanks for doing this testing.
Attachment #8334837 - Attachment is obsolete: true
Attachment #8334837 - Flags: review?(wchen)
(In reply to Josh Aas (Mozilla Corporation) from comment #43)
> Created attachment 8335476 [details] [diff] [review]
> Fix v1.5
> 
> Nomis - I missed something last time, this patch should fix the tbird
> problem. Let me know.
Yes, it does. Now it works again, like in your first patches. :-)


> Thanks for doing this testing.
You are welcome. This is a major thing I currently miss in FF/TB, so I'm more than happy that you fix this.
Attachment #8335476 - Flags: review?(wchen)
Attached patch Fix v1.6 (obsolete) — Splinter Review
Updated patch to latest trunk.
Attachment #8335476 - Attachment is obsolete: true
Attachment #8335476 - Flags: review?(wchen)
One more issue I forgot to figure out - my implementation is not thread safe, despite declaring thread-safe nsISupports, and nsAlertsService apparently being thread-safe. William - do you agree that my impl should be made thread-safe?
Comment on attachment 8335645 [details] [diff] [review]
Fix v1.6

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

I've been testing a few patches prior to this one and it's been working well with Gmail/GChat (using a greasemonkey script) including the notification images.

::: widget/cocoa/moz.build
@@ +55,5 @@
>      'nsStandaloneNativeMenu.mm',
>      'nsToolkit.mm',
>      'nsWidgetFactory.mm',
>      'nsWindowMap.mm',
> +    'OSXNotificationCenter.mm',

This doesn't link for me in UNIFIED_SOURCES. If I move it to SOURCES then it's fine:

Undefined symbols for architecture x86_64:
  "_sCocoaLog", referenced from:
      nsDragService::GetData(nsITransferable*, unsigned int) in Unified_mm_widget_cocoa1.o
ld: symbol(s) not found for architecture x86_64
Attached patch Fix v1.7 (obsolete) — Splinter Review
Talked to wchen, none of this stuff is or needs to be thread-safe. I removed the threadsafe nsISupports decl from my patch, as well as from the general alerts service.

MattN: I can't reproduce that build failure. Try a clobber?
Attachment #8335645 - Attachment is obsolete: true
Attachment #8335792 - Flags: review?(wchen)
(In reply to Josh Aas (Mozilla Corporation) from comment #48)
> MattN: I can't reproduce that build failure. Try a clobber?

I updated m-c and now it works fine.
Attached patch Fix v1.8 (obsolete) — Splinter Review
Compile fixes for OS X < 10.8.
Attachment #8335792 - Attachment is obsolete: true
Attachment #8335792 - Flags: review?(wchen)
Attachment #8336569 - Flags: review?(wchen)
(In reply to Matthew N. [:MattN] (catching up on reviews) from comment #47)
> Undefined symbols for architecture x86_64:
>   "_sCocoaLog", referenced from:
>       nsDragService::GetData(nsITransferable*, unsigned int) in
> Unified_mm_widget_cocoa1.o
> ld: symbol(s) not found for architecture x86_64

I'm seeing this now on try server with the patch I just posted:

https://tbpl.mozilla.org/?tree=Try&rev=52d5d49ee41e
This patch should fix the build issue you're seeing.  Sorry about that!
Attachment #8337117 - Flags: review?(cpeterson)
Comment on attachment 8337117 [details] [diff] [review]
Fix the build issue

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

LGTM
Attachment #8337117 - Flags: review?(cpeterson) → review+
(In reply to Josh Aas (Mozilla Corporation) from comment #51)
> (In reply to Matthew N. [:MattN] (catching up on reviews) from comment #47)
> > Undefined symbols for architecture x86_64:
> >   "_sCocoaLog", referenced from:
> >       nsDragService::GetData(nsITransferable*, unsigned int) in
> > Unified_mm_widget_cocoa1.o
> > ld: symbol(s) not found for architecture x86_64
> 
> I'm seeing this now on try server with the patch I just posted:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=52d5d49ee41e

Yeah, sorry, it was still a problem on my opt build. The build where it worked had a different .mozconfig and was a debug non-opt build. Glad you figured it out.
Whiteboard: [leave open]
Comment on attachment 8336569 [details] [diff] [review]
Fix v1.8

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

::: toolkit/components/alerts/nsIAlertsService.idl
@@ +27,5 @@
>      *                       doesn't care about callbacks.
> +    * @param name           The name of the notification. This is currently only
> +    *                       used on Android and OS X. On Android the name is
> +    *                       hashed and used as a notification ID. Notifications
> +    *                       will replace previous notifications with the same name.

The implementation in this patch has some quirks with regards to icon loading behaviour (timeout and size restriction) that should at least be documented here.

::: widget/cocoa/OSXNotificationCenter.h
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef OSXNOTIFICATIONCENTER_H

nit: OSXNotificationCenter_h

::: widget/cocoa/OSXNotificationCenter.mm
@@ +145,5 @@
> +{
> +  return (!!NSClassFromString(@"NSUserNotification")) ? NS_OK : NS_ERROR_FAILURE;
> +}
> +
> +NS_IMETHODIMP OSXNotificationCenter::ShowAlertNotification(const nsAString & aImageUrl, const nsAString & aAlertTitle, 

nit: For this method and all the methods below, the return value should go on its own line. Also remove the trailing whitespace here.

@@ +168,5 @@
> +  notification.userInfo = [NSDictionary dictionaryWithObjects:[NSArray arrayWithObjects:alertName, nil]
> +                                                      forKeys:[NSArray arrayWithObjects:@"name", nil]];
> +
> +  // Close any old alert with the same name. Eventually we can switch to real replacement.
> +  CloseAlertCocoaString(alertName);

This call to close the alert (as well as appending mAlerts below) should be moved so that this open happens right before we deliver the notification because in the case that we have an alert with an icon, there might be another alert created with the same name before the icon has loaded. If that happens we end up firing "alertfinished" on the first alert before it is shown. Also, when the icon loads, and the first notification is displayed, fires "alertshow" (after "alertclosed" is fired), and mAlerts will contain only the second alert but both notifications will be visible in NC.

@@ +196,5 @@
> +          // Set a timer for three seconds. If we don't have an icon by the time this
> +          // goes off then we go ahead without an icon.
> +          nsCOMPtr<nsITimer> timer = do_CreateInstance(NS_TIMER_CONTRACTID);
> +          osxni->mIconTimeoutTimer = timer;
> +          timer->InitWithCallback(this, 3000, nsITimer::TYPE_ONE_SHOT);

I don't think this is a good idea, but I do understand why we might want to do this. I just see this leading to a lot of confusion when developers or users don't see a icon or only sees it part of the time. If we do want this behaviour it needs to be publically documented somewhere (ideally worked into the spec), otherwise it is completely unexpected behaviour and may leads developers on a wild goose chase. The spec says that displaying a notification should wait for the icon to load.

@@ +305,5 @@
> +
> +  if (aType == imgINotificationObserver::SIZE_AVAILABLE) {
> +    // Kill off the load if image width or height is > 1000x1000.
> +    // Avoid long load times and other nastiness we might encounter
> +    // with huge images.

I don't think we should have this limit, the useragent could be on a very fast network, the icon might be on a local network or the filesystem, or the icon might be already in the cache from a different part of the webpage.

::: widget/cocoa/nsWidgetFactory.mm
@@ +153,5 @@
>  NS_DEFINE_NAMED_CID(NS_PRINTSESSION_CID);
>  NS_DEFINE_NAMED_CID(NS_PRINTSETTINGSSERVICE_CID);
>  NS_DEFINE_NAMED_CID(NS_PRINTDIALOGSERVICE_CID);
>  NS_DEFINE_NAMED_CID(NS_IDLE_SERVICE_CID);
> +NS_DEFINE_NAMED_CID(NS_ALERTSSERVICE_CID);

It looks like you actually want NS_SYSTEMALERTSSERVICE_CI here because you use the system-alerts-service contract id below.
Attachment #8336569 - Flags: review?(wchen)
Attached patch Fix v1.9 (obsolete) — Splinter Review
Thanks for the thoughtful review, wchen. This patch addresses your comments.

I am not comfortable with having no limit on the icon load, but this patch is much less restrictive. There is no limit on image size, and the loading time limit is now five seconds instead of three.

Nice catch on the issue of what happens when an alert is requested that shares a name with a pending alert. The only way to handle that nicely was to keep two lists - mActiveAlerts and mPendingAlerts.
Attachment #8336569 - Attachment is obsolete: true
Attachment #8338258 - Flags: review?(wchen)
Comment on attachment 8338258 [details] [diff] [review]
Fix v1.9

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

This looks good to me as far as implementing the alerts service API as long as either the icon loading time limit makes it into the spec, or it is removed. I glanced over the NC API details and the objective-c code style since I'm not familiar with it.

::: toolkit/components/alerts/nsIAlertsService.idl
@@ +28,5 @@
> +    * @param name           The name of the notification. This is currently only
> +    *                       used on Android and OS X. On Android the name is
> +    *                       hashed and used as a notification ID. Notifications
> +    *                       will replace previous notifications with the same name.
> +    *                       The OS X implemenation limits the amount of time it

The additional comment about icon loading should be added to the |iconUrl| section above instead of name.

::: widget/cocoa/OSXNotificationCenter.mm
@@ +339,5 @@
> +    OSXNotificationInfo *osxni = mPendingAlerts[i];
> +    if (timer == osxni->mIconTimeoutTimer.get()) {
> +      osxni->mIconTimeoutTimer = nullptr;
> +      if (osxni->mPendingNotifiction) {
> +        ShowPendingNotification(osxni);

you can break here
Attachment #8338258 - Flags: review?(wchen) → review+
Attached patch Fix v2.0 (obsolete) — Splinter Review
Fixes per wchen's comments, and I bumped up the icon timeout to six seconds.

https://tbpl.mozilla.org/?tree=Try&rev=a3ae8b97c877
Attachment #8338258 - Attachment is obsolete: true
Attached patch FIx v2.1 (obsolete) — Splinter Review
Requesting review from mstange, who can look over the Obj-C parts of the patch and provide cocoa widgets peer review.
Attachment #8341712 - Flags: review?(mstange)
Should have noted that my updated patch v2.1 just adds obj-c exception guards.
Comment on attachment 8341712 [details] [diff] [review]
FIx v2.1

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

::: toolkit/components/alerts/test/test_alerts.html
@@ +66,5 @@
>      ok(true, "showAlertNotification() succeeded.  Waiting for notification...");
> +    if (MAC) {
> +      // Notifications are native on OS X 10.8 and later, and when they are they
> +      // persist in the Notification Center. We need to close explicitly to avoid a hang.
> +      // This also works for XUL notifications when running this test on OS X < 10.9.

< 10.8

::: widget/cocoa/OSXNotificationCenter.h
@@ +16,5 @@
> +#include "imgRequestProxy.h"
> +#include "nsITimer.h"
> +
> +// The classes defined here frequently declare void pointers in order
> +// to avoid exposing Objective-C types. Proper type info is in comments.

What's bad about exposing Objective-C types? This file is only included in nsWidgetFactory.mm and OSXNotificationCenter.mm, both can deal with them. Regular .cpp files should never need to include this file.

@@ +18,5 @@
> +
> +// The classes defined here frequently declare void pointers in order
> +// to avoid exposing Objective-C types. Proper type info is in comments.
> +
> +class OSXNotificationInfo {

Please instead move this class down into the .mm file and forward declare it in the .h file. Then you can also reduce the #include list slighly. Also, please put it in the mozilla namespace since it doesn't have an ns prefix.

@@ +26,5 @@
> +  ~OSXNotificationInfo();
> +
> +  void *mName; // NSString*
> +  nsCOMPtr<nsIObserver> mObserver;
> +  PRUnichar *mCookie;

nsString mCookie;

@@ +32,5 @@
> +  void *mPendingNotifiction; // NSUserNotification, waiting for image load
> +  nsCOMPtr<nsITimer> mIconTimeoutTimer;
> +};
> +
> +class OSXNotificationCenter : public nsIAlertsService,

Please put this class into the mozilla namespace, too, or rename it to nsOSXNotificationCenter.

::: widget/cocoa/OSXNotificationCenter.mm
@@ +64,5 @@
> +
> +  NS_ASSERTION(name, "Cannot create OSXNotificationInfo without a name!");
> +  mName = [(NSString*)name retain];
> +  mObserver = observer;
> +  mCookie = ToNewUnicode(alertCookie);

With mCookie as nsString, this can just be mCookie...

@@ +75,5 @@
> +{
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
> +
> +  [(NSString*)mName release];
> +  free(mCookie);

... and this can go.

@@ +92,5 @@
> +@implementation mozNotificationCenterDelegate
> +
> +- (id)initWithOSXNC:(OSXNotificationCenter*)osxnc
> +{
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;

I'd really like us to get rid of exception guards in Obj-C methods. Exceptions originating from these methods are only dangerous if they propagate back into C++ callers, and since those C++ callers call into Obj-C they should already have their own exception guards.

@@ +131,5 @@
> +  didRemoveDeliveredNotifications:(NSArray *)notifications
> +{
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
> +
> +  for (NSUInteger i = 0; i < [notifications count]; i++) {

How about a fast enumeration loop here? for (id<FakeNSUserNotification> notification in notifications) {

@@ +142,5 @@
> +}
> +
> +@end
> +
> +id<FakeNSUserNotificationCenter> GetNotificationCenter() {

static

@@ +196,5 @@
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
> +
> +  Class unClass = NSClassFromString(@"NSUserNotification");
> +  id<FakeNSUserNotification> notification = [[unClass alloc] init];
> +  notification.title = [NSString stringWithCharacters:(const unichar *)aAlertTitle.BeginReading()

Does this do the right thing for non-ASCII text? Let's use [NSString stringWithUTF8String:NS_ConvertUTF16toUTF8(aAlertTitle).get()] instead. In the other places, too.

@@ +203,5 @@
> +                                                         length:aAlertText.Length()];
> +  notification.soundName = NSUserNotificationDefaultSoundName;
> +  notification.hasActionButton = NO;
> +  NSString *alertName = [NSString stringWithCharacters:(const unichar *)aAlertName.BeginReading() length:aAlertName.Length()];
> +  NS_ENSURE_TRUE(alertName, NS_ERROR_FAILURE);

According to http://markmail.org/message/shvdtr33dw47yhj5 the NS_ENSURE_* macros are outdated, please us a manual if.

@@ +217,5 @@
> +    mActiveAlerts.AppendElement(osxni);
> +    [GetNotificationCenter() deliverNotification:notification];
> +    [notification release];
> +    if (aAlertListener) {
> +      aAlertListener->Observe(nullptr, "alertshow", PromiseFlatString(aAlertCookie).get());

Other code uses NS_ConvertUTF16toUTF8 instead of PromiseFlatString here. It probably doesn't make a difference, though.

@@ +231,5 @@
> +        nsresult rv = il->LoadImage(imageUri, nullptr, nullptr, aPrincipal, nullptr,
> +                                    this, nullptr, nsIRequest::LOAD_NORMAL, nullptr,
> +                                    nullptr, getter_AddRefs(osxni->mIconRequest));
> +        if (NS_SUCCEEDED(rv)) {
> +          // Set a timer for three seconds. If we don't have an icon by the time this

Three?

@@ +235,5 @@
> +          // Set a timer for three seconds. If we don't have an icon by the time this
> +          // goes off then we go ahead without an icon.
> +          nsCOMPtr<nsITimer> timer = do_CreateInstance(NS_TIMER_CONTRACTID);
> +          osxni->mIconTimeoutTimer = timer;
> +          timer->InitWithCallback(this, 6000, nsITimer::TYPE_ONE_SHOT);

Or six?

@@ +271,5 @@
> +    return; // Can't do anything without a name
> +  }
> +
> +  NSArray *notifications = [GetNotificationCenter() deliveredNotifications];
> +  for (NSUInteger i = 0; i < [notifications count]; i++) {

fast enumeration

@@ +285,5 @@
> +  for (unsigned int i = 0; i < mActiveAlerts.Length(); i++) {
> +    OSXNotificationInfo *osxni = mActiveAlerts[i];
> +    if ([(NSString*)aAlertName isEqualToString:(NSString*)osxni->mName]) {
> +      if (osxni->mObserver) {
> +        osxni->mObserver->Observe(nullptr, "alertfinished", osxni->mCookie);

With mCookie as nsString, this will need to be NS_ConvertUTF16toUTF8(osxni->mCookie).get().

@@ +308,5 @@
> +  for (unsigned int i = 0; i < mActiveAlerts.Length(); i++) {
> +    OSXNotificationInfo *osxni = mActiveAlerts[i];
> +    if ([(NSString*)aAlertName isEqualToString:(NSString*)osxni->mName]) {
> +      if (osxni->mObserver) {
> +        osxni->mObserver->Observe(nullptr, "alertclickcallback", osxni->mCookie);

OK, this is the second place that becomes more complicated with mCookie as nsString. But it's worth it, IMO.

@@ +334,5 @@
> +  }
> +
> +  CloseAlertCocoaString((NSString*)osxni->mName);
> +
> +  // Move a smart ptr from one array to another is awkward.

It is! Let's use a mozilla::RefPtr, then. Make OSXNotificationInfo inherit from RefCounted<OSXNotificationInfo>, use RefPtr in all places that you use nsAutoPtr currently, remove the .forget(), and things should work.

@@ +364,5 @@
> +
> +  if (aType == imgINotificationObserver::LOAD_COMPLETE) {
> +    OSXNotificationInfo *osxni = nullptr;
> +    for (unsigned int i = 0; i < mPendingAlerts.Length(); i++) {
> +      if (aRequest == mPendingAlerts[i]->mIconRequest.get()) {

Is the .get() necessary?
(In reply to Markus Stange [:mstange] from comment #63)
> Comment on attachment 8341712 [details] [diff] [review]
> FIx v2.1

Thanks for the thorough review!

> ::: widget/cocoa/OSXNotificationCenter.h
> @@ +16,5 @@
> > +#include "imgRequestProxy.h"
> > +#include "nsITimer.h"
> > +
> > +// The classes defined here frequently declare void pointers in order
> > +// to avoid exposing Objective-C types. Proper type info is in comments.
> 
> What's bad about exposing Objective-C types? This file is only included in
> nsWidgetFactory.mm and OSXNotificationCenter.mm, both can deal with them.
> Regular .cpp files should never need to include this file.

That was left over from when this was hooked directly into the nsAlertsService implementation, not a service exposed from widget code. Will fix it.

> @@ +196,5 @@
> > +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
> > +
> > +  Class unClass = NSClassFromString(@"NSUserNotification");
> > +  id<FakeNSUserNotification> notification = [[unClass alloc] init];
> > +  notification.title = [NSString stringWithCharacters:(const unichar *)aAlertTitle.BeginReading()
> 
> Does this do the right thing for non-ASCII text? Let's use [NSString
> stringWithUTF8String:NS_ConvertUTF16toUTF8(aAlertTitle).get()] instead. In
> the other places, too.

It does the right thing. Input is unichar, that method handles unichar.

> @@ +217,5 @@
> > +    mActiveAlerts.AppendElement(osxni);
> > +    [GetNotificationCenter() deliverNotification:notification];
> > +    [notification release];
> > +    if (aAlertListener) {
> > +      aAlertListener->Observe(nullptr, "alertshow", PromiseFlatString(aAlertCookie).get());
> 
> Other code uses NS_ConvertUTF16toUTF8 instead of PromiseFlatString here. It
> probably doesn't make a difference, though.

Just going to leave it alone, seems like people do both and what I have works.

> @@ +285,5 @@
> > +  for (unsigned int i = 0; i < mActiveAlerts.Length(); i++) {
> > +    OSXNotificationInfo *osxni = mActiveAlerts[i];
> > +    if ([(NSString*)aAlertName isEqualToString:(NSString*)osxni->mName]) {
> > +      if (osxni->mObserver) {
> > +        osxni->mObserver->Observe(nullptr, "alertfinished", osxni->mCookie);
> 
> With mCookie as nsString, this will need to be
> NS_ConvertUTF16toUTF8(osxni->mCookie).get().

I think we can just use .get(), we're not converting to UTF-8 here.

> @@ +334,5 @@
> > +  }
> > +
> > +  CloseAlertCocoaString((NSString*)osxni->mName);
> > +
> > +  // Move a smart ptr from one array to another is awkward.
> 
> It is! Let's use a mozilla::RefPtr, then. Make OSXNotificationInfo inherit
> from RefCounted<OSXNotificationInfo>, use RefPtr in all places that you use
> nsAutoPtr currently, remove the .forget(), and things should work.

And swap the order of add/remove.
Attached patch Fix v2.2 (obsolete) — Splinter Review
Attachment #8341537 - Attachment is obsolete: true
Attachment #8341712 - Attachment is obsolete: true
Attachment #8341712 - Flags: review?(mstange)
Attachment #8341938 - Flags: review?(mstange)
Comment on attachment 8341938 [details] [diff] [review]
Fix v2.2

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

Looks great! There are two Obj-C methods that still have exception guards, you may want to remove them.
Attachment #8341938 - Flags: review?(mstange) → review+
Attached patch Fix v2.3 (obsolete) — Splinter Review
For some reason the 10.9 compiler doesn't need this, but whatever we have on tbox does.

https://tbpl.mozilla.org/?tree=Try&rev=09deb0332a47

Try run is all green.
Attachment #8341938 - Attachment is obsolete: true
I did a try run that was all green, per comment 68. Someone checked in the patch for bug 888689 right before I checked in, which broke my patch.
Attached patch Fix v2.4Splinter Review
Bustage fix. Pushed to mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/e8f0814e85c5
Attachment #8342354 - Attachment is obsolete: true
Try was green again before my latest push to m-i, frustrating. Probably has something to do with the fact that try clobbers and m-i doesn't.

Two bustage fixes (first one didn't work):

http://hg.mozilla.org/integration/mozilla-inbound/rev/1bc1c2eacd59
http://hg.mozilla.org/integration/mozilla-inbound/rev/0f99694a6079
Keywords: feature
Ah, and I thought it's just me, because I also had to do
>{ &kNS_SYSTEMALERTSSERVICE_CID, false, NULL, mozilla::OSXNotificationCenterConstructor },
and
>namespace mozilla {
>NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(OSXNotificationCenter, Init)
>}
to get your latest patch building for me.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Whiteboard: [tb31features]
Awesome, thanks a lot, Josh! :)


(Is Bug 785897 still valid now?)
Target Milestone: --- → mozilla28
Hi,

in Thunderbird the notifications have two icons now (why?) and there are two sounds at once. Are there existing bugs for these issues?
(In reply to Sören Hentzschel from comment #76)
> in Thunderbird the notifications have two icons now (why?) and there are two
> sounds at once. Are there existing bugs for these issues?

No, please file new bugs. Worst case they get duplicated, but that's ok.
Okay, I files bug 947775 and bug 947776.
Depends on: 948115
Since the merge to aurora is here, I tested this feature on Mac OS X 10.8.5 and Mac OS X 10.9 using latest Aurora and all looked fine from QA point of view. I used the demo from URL and tried various scenarios. 
If there is anything in particular you want to focus my testing, please let me know.
QA Contact: bogdan.maris
Depends on: 952771
Status: RESOLVED → VERIFIED
Are there any more automated tests needed?
Flags: needinfo?(joshmoz)
I modified some existing notification tests but didn't add new ones.
Flags: needinfo?(joshmoz)
You need to log in before you can comment on or make changes to this bug.