William Chen [:wchen] Reporter
	Description • 11 years ago

Web notifications have landed using XUL alerts on mac. It would be nice to have Notification Center support.

	Jose Fandos
	Comment 1 • 11 years ago

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

	Florian Bender
	Comment 2 • 11 years ago

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.

	Josh Aas Assignee
	Comment 3 • 11 years ago

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.

	Florian Bender
	Comment 4 • 11 years ago

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.

	Josh Aas Assignee
	Comment 5 • 11 years ago

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.

	Josh Aas Assignee
	Comment 6 • 11 years ago

BTW, this should fall back to XUL notification on OS X < 10.8.

	Matthew N. [:MattN]
	Comment 7 • 11 years ago

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?

	Florian Bender
	Comment 8 • 11 years ago

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.

	Josh Aas Assignee
	Comment 9 • 11 years ago

10.9 doesn't add anything helpful here, 10.8 should be fine.

	Nomis101
	Comment 10 • 11 years ago

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).

	Reuben Morais [:reuben]
	Comment 11 • 11 years ago

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.

	Josh Aas Assignee
	Comment 12 • 11 years ago

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.

	Reuben Morais [:reuben]
	Comment 13 • 11 years ago

It does for me…

	Nomis101
	Comment 14 • 11 years ago

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.

	Florian Bender
	Comment 15 • 11 years ago

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.

	Josh Aas Assignee
	Comment 16 • 11 years ago

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.

	Josh Aas Assignee
	Comment 17 • 11 years ago

Ugh, got desperate, rebooted my laptop and notifications are back.

	Josh Aas Assignee
	Comment 18 • 11 years ago

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.

	Steven Michaud [:smichaud] (Retired)
	Comment 19 • 11 years ago

+- (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.

	Steven Michaud [:smichaud] (Retired)
	Comment 20 • 11 years ago

(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.

	Steven Michaud [:smichaud] (Retired)
	Comment 21 • 11 years ago

(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.

	Nomis101
	Comment 22 • 11 years ago

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.

	Josh Aas Assignee
	Comment 23 • 11 years ago

(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.

	Nomis101
	Comment 24 • 11 years ago

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.

	Josh Aas Assignee
	Comment 25 • 11 years ago

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.

	Florian Bender
	Comment 26 • 11 years ago

(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).

	Nomis101
	Comment 27 • 11 years ago

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.

	Josh Aas Assignee
	Comment 28 • 11 years ago

(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.

	Nomis101
	Comment 29 • 11 years ago

(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.

	William Chen [:wchen] Reporter
	Comment 30 • 11 years ago

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

	Josh Aas Assignee
	Comment 31 • 11 years ago

(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.

	Josh Aas Assignee
	Comment 32 • 11 years ago

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.

	Josh Aas Assignee
	Comment 33 • 11 years ago

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.

	Florian Bender
	Comment 34 • 11 years ago

(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?

	Josh Aas Assignee
	Comment 35 • 11 years ago

(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.

	Josh Aas Assignee
	Comment 36 • 11 years ago

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.

	Nomis101
	Comment 37 • 11 years ago

(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

	Josh Aas Assignee
	Comment 38 • 11 years ago

(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.

	Josh Aas Assignee
	Comment 39 • 11 years ago

Fixes tests and hopefully the Thunderbird problem.

	Nomis101
	Comment 40 • 11 years ago

(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?

	Josh Aas Assignee
	Comment 41 • 11 years ago

(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.

	Nomis101
	Comment 42 • 11 years ago

(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.

	Josh Aas Assignee
	Comment 43 • 11 years ago

Nomis - I missed something last time, this patch should fix the tbird problem. Let me know. Thanks for doing this testing.

	Nomis101
	Comment 44 • 11 years ago

(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.

	Josh Aas Assignee
	Comment 45 • 11 years ago

Updated patch to latest trunk.

	Josh Aas Assignee
	Comment 46 • 11 years ago

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?

	Matthew N. [:MattN]
	Comment 47 • 11 years ago

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

	Josh Aas Assignee
	Comment 48 • 11 years ago

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?

	Matthew N. [:MattN]
	Comment 49 • 11 years ago

(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.

	Josh Aas Assignee
	Comment 50 • 11 years ago

Compile fixes for OS X < 10.8.

	Josh Aas Assignee
	Comment 51 • 11 years ago

(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

	(no longer active)
	Comment 52 • 11 years ago

This patch should fix the build issue you're seeing.  Sorry about that!

	Chris Peterson [:cpeterson]
	Comment 53 • 11 years ago

Comment on attachment 8337117 [details] [diff] [review]
Fix the build issue

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

LGTM

	Matthew N. [:MattN]
	Comment 54 • 11 years ago

(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.

	(no longer active)
	Comment 55 • 11 years ago

Comment on attachment 8337117 [details] [diff] [review]
Fix the build issue

https://hg.mozilla.org/integration/mozilla-inbound/rev/3eebf7802406

	Phil Ringnalda (:philor)
	Comment 56 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/3eebf7802406

	William Chen [:wchen] Reporter
	Comment 57 • 11 years ago

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.

	Josh Aas Assignee
	Comment 58 • 11 years ago

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.

	William Chen [:wchen] Reporter
	Comment 59 • 11 years ago

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

	Josh Aas Assignee
	Comment 60 • 11 years ago

Fixes per wchen's comments, and I bumped up the icon timeout to six seconds.

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

	Josh Aas Assignee
	Comment 61 • 11 years ago

Requesting review from mstange, who can look over the Obj-C parts of the patch and provide cocoa widgets peer review.

	Josh Aas Assignee
	Comment 62 • 11 years ago

Should have noted that my updated patch v2.1 just adds obj-c exception guards.

	Markus Stange [:mstange]
	Comment 63 • 11 years ago

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?

	Josh Aas Assignee
	Comment 64 • 11 years ago

(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.

	Markus Stange [:mstange]
	Comment 66 • 11 years ago

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.

	Josh Aas Assignee
	Comment 67 • 11 years ago

Checked in, backed out:

http://hg.mozilla.org/integration/mozilla-inbound/rev/e018c1a15e00
http://hg.mozilla.org/integration/mozilla-inbound/rev/78fd1153718f

	Josh Aas Assignee
	Comment 68 • 11 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 69 • 11 years ago

Backed out again for build bustage. Please get a green run on Try before pushing this again.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6267453cd823

https://tbpl.mozilla.org/php/getParsedLog.php?id=31471877&tree=Mozilla-Inbound

	Josh Aas Assignee
	Comment 70 • 11 years ago

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.

	Josh Aas Assignee
	Comment 71 • 11 years ago

Bustage fix. Pushed to mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/e8f0814e85c5

	Josh Aas Assignee
	Comment 72 • 11 years ago

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

	Nomis101
	Comment 73 • 11 years ago

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.

	Carsten Book [:Tomcat]
	Comment 74 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/e8f0814e85c5
https://hg.mozilla.org/mozilla-central/rev/1bc1c2eacd59
https://hg.mozilla.org/mozilla-central/rev/0f99694a6079

	Florian Bender
	Comment 75 • 11 years ago

Awesome, thanks a lot, Josh! :)


(Is Bug 785897 still valid now?)

	Sören Hentzschel
	Comment 76 • 11 years ago

Hi,

in Thunderbird the notifications have two icons now (why?) and there are two sounds at once. Are there existing bugs for these issues?

	Florian Bender
	Comment 77 • 11 years ago

(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.

	Sören Hentzschel
	Comment 78 • 11 years ago

Okay, I files bug 947775 and bug 947776.

	Bogdan Maris, Desktop QA
	Comment 79 • 11 years ago

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.

	Mihaela Velimiroviciu (:mihaelav)
	Comment 80 • 10 years ago

Are there any more automated tests needed?

	Josh Aas Assignee
	Comment 81 • 10 years ago

I modified some existing notification tests but didn't add new ones.