Last Comment Bug 782211 - Implement notification API spec
: Implement notification API spec
Status: RESOLVED FIXED
v2, interaction, UX-P1, [mozilla.dev....
: dev-doc-complete, uiwanted, verifyme
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal with 30 votes (vote)
: mozilla22
Assigned To: William Chen [:wchen] (Vacation until 8/25)
: Cornel Ionce [QA] (:cornel_ionce)
Mentors:
http://notifications.spec.whatwg.org/
: 594543 629280 779213 (view as bug list)
Depends on: 857962 866653 900634 901728 902147 763643 852461 853078 853104 853972 854025 856202 856858 858919 859466 860951 871460 874050 874090 875114 893501 900279 901736 935434
Blocks: 324570 html5test 813450 852917 862395 948136 1109395 594543 824549 828719 828923 846966 846968 852648 852658 855165 858884 899574 899585 900960 901214 919929
  Show dependency treegraph
 
Reported: 2012-08-13 02:07 PDT by William Chen [:wchen] (Vacation until 8/25)
Modified: 2016-08-04 13:53 PDT (History)
99 users (show)
wchen: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
22+


Attachments
Part 1: Factor out code to add event listeners so that it can be used by Notifications. v1 (7.48 KB, patch)
2012-08-13 02:09 PDT, William Chen [:wchen] (Vacation until 8/25)
no flags Details | Diff | Splinter Review
Part 2: Implemented additional functionality in Android to support Notification API. v1 (28.81 KB, patch)
2012-08-13 02:10 PDT, William Chen [:wchen] (Vacation until 8/25)
no flags Details | Diff | Splinter Review
Part 3: Updated the nsIAlertsService API to included required functionality. v1 (16.27 KB, patch)
2012-08-13 02:12 PDT, William Chen [:wchen] (Vacation until 8/25)
no flags Details | Diff | Splinter Review
Part 4: Update nsDesktopNotification to preserve existing behaviour. v1 (2.24 KB, patch)
2012-08-13 02:14 PDT, William Chen [:wchen] (Vacation until 8/25)
no flags Details | Diff | Splinter Review
Part 5: Implement Notification API. v1 (28.74 KB, patch)
2012-08-13 02:15 PDT, William Chen [:wchen] (Vacation until 8/25)
no flags Details | Diff | Splinter Review
Part 6: Updated the B2G notifications to implement the Notification API. v1 (4.79 KB, patch)
2012-08-13 02:16 PDT, William Chen [:wchen] (Vacation until 8/25)
no flags Details | Diff | Splinter Review
Part 7: Added permission prompt for desktop notifications on desktop platforms. v1 (11.87 KB, patch)
2012-08-13 02:16 PDT, William Chen [:wchen] (Vacation until 8/25)
no flags Details | Diff | Splinter Review
Part 8: Updated existing XUL alerts to implement the Notification API. v1 (34.07 KB, patch)
2012-08-13 02:17 PDT, William Chen [:wchen] (Vacation until 8/25)
no flags Details | Diff | Splinter Review
Part 9: Remove native notification systems on desktop platforms. v1 (223.01 KB, patch)
2012-08-13 02:18 PDT, William Chen [:wchen] (Vacation until 8/25)
no flags Details | Diff | Splinter Review
Part 1: Factor out code to add event listeners so that it can be used by Notifications. v1 (3.95 KB, patch)
2012-10-11 17:33 PDT, William Chen [:wchen] (Vacation until 8/25)
bugs: review+
Details | Diff | Splinter Review
Part 2: Implemented additional functionality in Fennec to support Notification API. v1 (12.37 KB, patch)
2012-10-11 17:33 PDT, William Chen [:wchen] (Vacation until 8/25)
wjohnston2000: review+
Details | Diff | Splinter Review
Part 3: Updated the nsIAlertsService API and its implementation to include support for bidi overrides and a method to close notifications. v1 (15.48 KB, patch)
2012-10-11 17:34 PDT, William Chen [:wchen] (Vacation until 8/25)
doug.turner: review+
Details | Diff | Splinter Review
Part 4: Make nsDesktopNotification generate unique name and cookies to prevent collision in the IPC observer and coalescing in the alerts service. v1 (2.22 KB, patch)
2012-10-11 17:35 PDT, William Chen [:wchen] (Vacation until 8/25)
doug.turner: review+
Details | Diff | Splinter Review
Part 5: Implement Notification API. v1 (34.38 KB, patch)
2012-10-11 17:35 PDT, William Chen [:wchen] (Vacation until 8/25)
doug.turner: review-
Details | Diff | Splinter Review
Part 6: Updated the B2G notifications to implement the Notification API. v1 (4.38 KB, patch)
2012-10-11 17:36 PDT, William Chen [:wchen] (Vacation until 8/25)
fabrice: review+
Details | Diff | Splinter Review
Part 7: Added permission prompt for desktop notifications on desktop platforms. v1 (23.97 KB, patch)
2012-10-11 17:36 PDT, William Chen [:wchen] (Vacation until 8/25)
no flags Details | Diff | Splinter Review
Part 8: Updated existing XUL alerts to implement the Notification API. v1 (23.81 KB, patch)
2012-10-11 17:37 PDT, William Chen [:wchen] (Vacation until 8/25)
jaws: review-
Details | Diff | Splinter Review
Part 9: Remove native notification systems on desktop platforms. v1 (46.42 KB, patch)
2012-10-11 17:37 PDT, William Chen [:wchen] (Vacation until 8/25)
doug.turner: review-
Details | Diff | Splinter Review
Part 10: Update webapp browser permission test to clean up alerts. v1 (2.26 KB, patch)
2012-10-11 17:38 PDT, William Chen [:wchen] (Vacation until 8/25)
fabrice: review+
Details | Diff | Splinter Review
Update Gaia with additional functionality to support Notification API. v1 (5.97 KB, patch)
2012-10-11 17:39 PDT, William Chen [:wchen] (Vacation until 8/25)
etienne: review+
Details | Diff | Splinter Review
Part 3: Updated the nsIAlertsService API and its implementation to include support for bidi overrides and a method to close notifications. v2 (15.53 KB, patch)
2012-10-16 12:59 PDT, William Chen [:wchen] (Vacation until 8/25)
doug.turner: review+
Details | Diff | Splinter Review
Part 4: Make nsDesktopNotification generate unique name and cookies to prevent collision in the IPC observer and coalescing in the alerts service. v2 (2.23 KB, patch)
2012-10-16 13:03 PDT, William Chen [:wchen] (Vacation until 8/25)
doug.turner: review+
Details | Diff | Splinter Review
Part 1: Factor out code to add event listeners so that it can be used by Notifications. v2 (6.03 KB, patch)
2012-10-16 14:06 PDT, William Chen [:wchen] (Vacation until 8/25)
bugs: review-
Details | Diff | Splinter Review
Part 5.1: Add function to remote test for permission in nsIPermissionManager. v1 (3.37 KB, patch)
2012-10-17 15:15 PDT, William Chen [:wchen] (Vacation until 8/25)
cjones.bugs: review+
jonas: review+
ladamski: review+
Details | Diff | Splinter Review
Part 5.2: Implement Notification API. v2 (29.92 KB, patch)
2012-10-17 15:16 PDT, William Chen [:wchen] (Vacation until 8/25)
doug.turner: review+
bugs: review-
Details | Diff | Splinter Review
Part 7: Added permission prompt for desktop notifications on desktop platforms. v2 (23.02 KB, patch)
2012-10-17 15:18 PDT, William Chen [:wchen] (Vacation until 8/25)
MattN+bmo: review-
Details | Diff | Splinter Review
Part 8: Updated existing XUL alerts to implement the Notification API. v2 (36.85 KB, patch)
2012-10-23 16:21 PDT, William Chen [:wchen] (Vacation until 8/25)
neil: review-
enndeakin: review-
Details | Diff | Splinter Review
Part 1: Factor out code to add event listeners so that it can be used by Notifications. v3 (5.43 KB, patch)
2012-10-23 16:23 PDT, William Chen [:wchen] (Vacation until 8/25)
bugs: review+
Details | Diff | Splinter Review
Part 7: Added permission prompt for desktop notifications on desktop platforms. v3 (31.93 KB, patch)
2012-10-23 16:27 PDT, William Chen [:wchen] (Vacation until 8/25)
no flags Details | Diff | Splinter Review
Part 7: Added permission prompt for desktop notifications on desktop platforms. v3 (51.43 KB, patch)
2012-10-25 14:26 PDT, William Chen [:wchen] (Vacation until 8/25)
MattN+bmo: review-
Details | Diff | Splinter Review
Part 5.2: Implement Notification API. v3 (31.65 KB, patch)
2012-10-26 15:44 PDT, William Chen [:wchen] (Vacation until 8/25)
bugs: review-
Details | Diff | Splinter Review
Added permission prompt for desktop notifications on desktop platforms. v4 (50.27 KB, patch)
2012-11-27 00:34 PST, William Chen [:wchen] (Vacation until 8/25)
no flags Details | Diff | Splinter Review
Part 5.2: Implement Notification API. v4 (29.81 KB, patch)
2012-11-27 00:36 PST, William Chen [:wchen] (Vacation until 8/25)
bugs: review-
Details | Diff | Splinter Review
Part 9: Remove native notification systems on desktop platforms. v2 (21.14 KB, patch)
2012-11-27 00:44 PST, William Chen [:wchen] (Vacation until 8/25)
doug.turner: review+
Details | Diff | Splinter Review
Patch with all changes (191.75 KB, patch)
2012-11-30 11:26 PST, William Chen [:wchen] (Vacation until 8/25)
no flags Details | Diff | Splinter Review
Part 5.2: Implement Notification API. v5 (29.75 KB, patch)
2013-01-16 14:46 PST, William Chen [:wchen] (Vacation until 8/25)
bugs: review+
Details | Diff | Splinter Review
Part 7: Added permission prompt for desktop notifications on desktop platforms. v5 (50.90 KB, patch)
2013-01-16 15:04 PST, William Chen [:wchen] (Vacation until 8/25)
MattN+bmo: review-
Details | Diff | Splinter Review
Updated existing XUL alerts to implement the Notification API. v3 (52.09 KB, patch)
2013-01-16 18:04 PST, William Chen [:wchen] (Vacation until 8/25)
no flags Details | Diff | Splinter Review
Part 8: Updated existing XUL alerts to implement the Notification API. (52.24 KB, patch)
2013-01-31 22:29 PST, William Chen [:wchen] (Vacation until 8/25)
enndeakin: review+
Details | Diff | Splinter Review
Part 7: Added permission prompt for desktop notifications on desktop platforms. v6 (50.60 KB, patch)
2013-01-31 22:35 PST, William Chen [:wchen] (Vacation until 8/25)
MattN+bmo: review+
Details | Diff | Splinter Review
make it compile on gcc 4.7 (6.42 KB, patch)
2013-03-18 08:49 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
wchen: review+
Details | Diff | Splinter Review
rebased gaia patch v2 (7.98 KB, patch)
2013-07-30 11:22 PDT, Julien Wajsberg [:julienw]
no flags Details | Diff | Splinter Review

Description William Chen [:wchen] (Vacation until 8/25) 2012-08-13 02:07:39 PDT
This is a bug to track the implementation of the w3c notification spec.

The UX is discussed in bug 629280.
Comment 1 William Chen [:wchen] (Vacation until 8/25) 2012-08-13 02:09:46 PDT
Created attachment 651293 [details] [diff] [review]
Part 1: Factor out code to add event listeners so that it can be used by Notifications. v1
Comment 2 William Chen [:wchen] (Vacation until 8/25) 2012-08-13 02:10:52 PDT
Created attachment 651294 [details] [diff] [review]
Part 2: Implemented additional functionality in Android to support Notification API. v1
Comment 3 William Chen [:wchen] (Vacation until 8/25) 2012-08-13 02:12:34 PDT
Created attachment 651295 [details] [diff] [review]
Part 3: Updated the nsIAlertsService API to included required functionality. v1
Comment 4 William Chen [:wchen] (Vacation until 8/25) 2012-08-13 02:14:07 PDT
Created attachment 651296 [details] [diff] [review]
Part 4: Update nsDesktopNotification to preserve existing behaviour. v1
Comment 5 William Chen [:wchen] (Vacation until 8/25) 2012-08-13 02:15:26 PDT
Created attachment 651297 [details] [diff] [review]
Part 5: Implement Notification API. v1
Comment 6 William Chen [:wchen] (Vacation until 8/25) 2012-08-13 02:16:06 PDT
Created attachment 651298 [details] [diff] [review]
Part 6: Updated the B2G notifications to implement the Notification API. v1
Comment 7 William Chen [:wchen] (Vacation until 8/25) 2012-08-13 02:16:49 PDT
Created attachment 651299 [details] [diff] [review]
Part 7: Added permission prompt for desktop notifications on desktop platforms. v1
Comment 8 William Chen [:wchen] (Vacation until 8/25) 2012-08-13 02:17:30 PDT
Created attachment 651300 [details] [diff] [review]
Part 8: Updated existing XUL alerts to implement the Notification API. v1
Comment 9 William Chen [:wchen] (Vacation until 8/25) 2012-08-13 02:18:23 PDT
Created attachment 651301 [details] [diff] [review]
Part 9: Remove native notification systems on desktop platforms. v1
Comment 10 William Chen [:wchen] (Vacation until 8/25) 2012-08-13 02:20:09 PDT
I have attached all the patches needed to implement the notification API. Right now it creates very basic and fairly ugly notification on desktop platforms. Work will be needed for UX.
Comment 11 Mounir Lamouri (:mounir) 2012-08-13 02:22:43 PDT
I don't understand part 9. We don't want to use native notifications? Why that?
Comment 12 :Ms2ger (⌚ UTC+1/+2) 2012-08-13 02:30:32 PDT
We do want to use native notifications; that's the point.
Comment 13 William Chen [:wchen] (Vacation until 8/25) 2012-08-20 16:56:15 PDT
(In reply to Mounir Lamouri (:mounir) from comment #11)
> I don't understand part 9. We don't want to use native notifications? Why
> that?

I removed Growl support because it does not support features needed to implement the spec. I also removed libnotify support because I can't find documentation about all the capabilities, so it's possible that libnotify could work but I just can't find it in the documentation.

The native notification systems need to be able to close notifications and have a callback on showing, clicking and closing a notification.
Comment 14 Marco Castelluccio [:marco] (PTO until August 24/25) 2012-08-20 17:40:22 PDT
I don't think we should completely remove the native notifications on systems that support them.  If they lack some needed features for the spec, you can avoid using them for your implementation. But I'd let Firefox itself use native notifications if they are present, because Firefox doesn't need so many features for its notifications.

AFAIK, with libnotify you have clicking and closing callbacks, but you can't assume they actually work (because it depends on the dbus server). I'm unsure about the showing callback, but I can't see what it could be used for (and you could probably "emulate" it by calling the callback just after calling notify_notification_show).
You can see some libnotify usage examples in its tests.
Comment 15 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-09-04 23:33:55 PDT
What do we need to get this fixed? Still UI/UX work?
(Btw, I think part 1 needs some heavy modifications because code in m-c has changed already.)
Comment 16 Doug Turner (:dougt) 2012-09-05 07:47:56 PDT
Yes, we are still waiting for something from UX.  Also see bug 629280.
Comment 17 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-09-19 05:40:28 PDT
I think we should land this with some UI and have it enabled only on Nightly.
Comment 18 Stephen Horlander [:shorlander] 2012-09-19 13:42:29 PDT
(In reply to Doug Turner (:dougt) from comment #16)
> Yes, we are still waiting for something from UX.  Also see bug 629280.

I created some mockups for permissions requests, permissions editing and for notifications without a system notification center. I uploaded them to a gallery:

http://shorlander.dropmark.com/80842


- Permission Request -
When a website wants to display a notification Firefox can use a standard arrow panel to request permission. This is the same behavior displayed for passwords, geolocation, apps, etc.

One difference here is that there are only have two options for web notifications: Allow/Deny. Other requests have three or more. We could consider simplifying and having two buttons instead of a split button.


-Permissions Management-
Once permission is granted we need a way to see which sites can show notifications and way to revoke that permission. Also a global option to disable web notifications.


-Notification Display-
When the system supplies a notification center we should use that. In cases where that isn't true we will need display our own custom notifications. This includes all versions of Windows before 8 and all versions of OSX before 10.8.

Currently on Windows we use a toast notification for things like updates and downloads. This has some limitations that make it a problem for notifications:

- Currently Windows only
- It is pretty janky
- Doesn't scale to more than one notification at a time

A notification UI would need:
- To be cross platform for all desktop OSes we support
- Multiple notifications will need to stack
- Need a close button if people get tired of looking at it before the timer expires

Platform specific considerations:
- Should appear starting in the top-right on OSX
- Should appear starting in the bottom-right on Windows
- Platform specific styling


***Note: Icons not final! :)
Comment 19 William Chen [:wchen] (Vacation until 8/25) 2012-10-11 17:33:13 PDT
Created attachment 670626 [details] [diff] [review]
Part 1: Factor out code to add event listeners so that it can be used by Notifications. v1
Comment 20 William Chen [:wchen] (Vacation until 8/25) 2012-10-11 17:33:49 PDT
Created attachment 670627 [details] [diff] [review]
Part 2: Implemented additional functionality in Fennec to support Notification API. v1
Comment 21 William Chen [:wchen] (Vacation until 8/25) 2012-10-11 17:34:29 PDT
Created attachment 670628 [details] [diff] [review]
Part 3: Updated the nsIAlertsService API and its implementation to include support for bidi overrides and a method to close notifications. v1
Comment 22 William Chen [:wchen] (Vacation until 8/25) 2012-10-11 17:35:00 PDT
Created attachment 670629 [details] [diff] [review]
Part 4: Make nsDesktopNotification generate unique name and cookies to prevent collision in the IPC observer and coalescing in the alerts service. v1
Comment 23 William Chen [:wchen] (Vacation until 8/25) 2012-10-11 17:35:33 PDT
Created attachment 670631 [details] [diff] [review]
Part 5: Implement Notification API. v1
Comment 24 William Chen [:wchen] (Vacation until 8/25) 2012-10-11 17:36:05 PDT
Created attachment 670632 [details] [diff] [review]
Part 6: Updated the B2G notifications to implement the Notification API. v1
Comment 25 William Chen [:wchen] (Vacation until 8/25) 2012-10-11 17:36:44 PDT
Created attachment 670634 [details] [diff] [review]
Part 7: Added permission prompt for desktop notifications on desktop platforms. v1
Comment 26 William Chen [:wchen] (Vacation until 8/25) 2012-10-11 17:37:17 PDT
Created attachment 670636 [details] [diff] [review]
Part 8: Updated existing XUL alerts to implement the Notification API. v1
Comment 27 William Chen [:wchen] (Vacation until 8/25) 2012-10-11 17:37:53 PDT
Created attachment 670637 [details] [diff] [review]
Part 9: Remove native notification systems on desktop platforms. v1
Comment 28 William Chen [:wchen] (Vacation until 8/25) 2012-10-11 17:38:34 PDT
Created attachment 670639 [details] [diff] [review]
Part 10: Update webapp browser permission test to clean up alerts. v1
Comment 29 William Chen [:wchen] (Vacation until 8/25) 2012-10-11 17:39:32 PDT
Created attachment 670640 [details] [diff] [review]
Update Gaia with additional functionality to support Notification API. v1
Comment 30 Doug Turner (:dougt) 2012-10-12 10:35:01 PDT
Comment on attachment 670628 [details] [diff] [review]
Part 3: Updated the nsIAlertsService API and its implementation to include support for bidi overrides and a method to close notifications. v1

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

r+ w/ consideration on the return code.

::: toolkit/components/alerts/nsAlertsService.cpp
@@ +206,5 @@
> +  // Try the system notification service.
> +  nsCOMPtr<nsIAlertsService> sysAlerts(do_GetService(NS_SYSTEMALERTSERVICE_CONTRACTID));
> +  if (sysAlerts) {
> +    nsresult rv = sysAlerts->CloseAlert(aAlertName);
> +    if (NS_SUCCEEDED(rv)) {

If this fails, should we really return NS_ERROR_NOT_IMPLEMENTED?  Maybe we should just return rv

::: toolkit/components/alerts/nsIAlertsService.idl
@@ +64,5 @@
> +    *
> +    * @param name           The name of the notification to close. If no name
> +    *                       is provided then only a notification created with
> +    *                       no name (if any) will be closed.
> +    */

Maybe you should define the return/throw code.
Comment 31 Doug Turner (:dougt) 2012-10-12 10:38:23 PDT
Comment on attachment 670629 [details] [diff] [review]
Part 4: Make nsDesktopNotification generate unique name and cookies to prevent collision in the IPC observer and coalescing in the alerts service. v1

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

r+ w/ style nit fixed.

::: dom/src/notification/nsDesktopNotification.cpp
@@ +39,5 @@
> +  // Generate a unique name (which will also be used as a cookie) because
> +  // the nsIAlertsService will coalesce notifications with the same name.
> +  // In the case of IPC, the parent process will use the cookie to map
> +  // to nsIObservers, thus cookies must be unique to differentiate observers.
> +  nsString uniqueName = NS_LITERAL_STRING("desktop-notification");

Maybe append a : or another - so that the unique name is :

desktop-notification:###

::: dom/src/notification/nsDesktopNotification.h
@@ +108,5 @@
>    nsCOMPtr<nsIPrincipal> mPrincipal;
>    bool mAllow;
>    bool mShowHasBeenCalled;
> +
> +  static uint32_t mCount;

this should be sCount.
Comment 32 Doug Turner (:dougt) 2012-10-12 11:10:08 PDT
Comment on attachment 670631 [details] [diff] [review]
Part 5: Implement Notification API. v1

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

I'd like someone else to also review this patch for the binding specific stuff including the CallCallback method.

::: dom/bindings/Bindings.conf
@@ +418,5 @@
> +        'getterOnly': [
> +            'onclick', 'onshow', 'onerror', 'onclose', 'permission'
> +        ]
> +    }
> +}],

I do not know if this is correct.

::: dom/ipc/PContent.ipdl
@@ +328,5 @@
> +     * NOTE: The principal is untrusted in the parent process. Only
> +     *       principals that can live in the content process should
> +     *       be provided.
> +     */
> +    sync TestPermissionFromPrincipal(Principal principal, nsCString type)

You should factor this change out into a different bug.  You will want cjones and/or jonas to review.

::: dom/src/notification/Notification.cpp
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=2 et sw=2 tw=80: */

So, Mode Lines are allowed/encouraged by our style guide, but they are dumb.  Your editor should be smart enough to not have to have this crap.  Up to you if you want to leave them here or not.

@@ +7,5 @@
> +#include "PCOMContentPermissionRequestChild.h"
> +#include "mozilla/dom/PBrowserChild.h"
> +
> +#include "mozilla/dom/Notification.h"
> +

remove extra line.

@@ +42,5 @@
> +    : mPrincipal(aPrincipal), mWindow(aWindow),
> +      mPermission(NotificationPermission::Default),
> +      mFunObj(aFunObj), mHasPermission(false)
> +  {
> +    mCx = GetScriptContextFromJSContext(aCx);

Assert that mCx is non-null?

@@ +137,5 @@
> +NotificationPermissionRequest::Run()
> +{
> +  if (mHasPermission) {
> +    return CallCallback();
> +  }

Having Run() be called twice (once for the prompting, and once when you have mHasPermission set) is a bit confusing on first read.  It might be clearer if you, instead, ran CallCallback with NS_NewRunnableMethod.  In this way, NotificationPermissionRequest::Run would only ever be used to bring up the permission request.  You could also remove the mHasPermission flag.

@@ +197,5 @@
> +NotificationPermissionRequest::Cancel()
> +{
> +  mPermission = NotificationPermission::Denied;
> +  mHasPermission = true;
> +  NS_DispatchToMainThread(this);

How about something like:

NS_NewRunnableMethod(this, &NotificationPermissionRequest::CallCallback);
NS_DispatchToMainThread(this);

@@ +243,5 @@
> +
> +  mPrincipal = nullptr;
> +  mWindow = nullptr;
> +
> +  return NS_OK;

Need a binding review to make sure that you're doing things right.

@@ +338,5 @@
> +    mTag(aTag), mIconUrl(aIconUrl), mIsClosed(false)
> +{
> +  SetIsDOMBinding();
> +
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);

Fatal assert if aWindow is null?

@@ +343,5 @@
> +  BindToOwner(window);
> +
> +  mWindow = do_QueryInterface(aWindow);
> +  nsCOMPtr<nsIDOMDocument> domdoc;
> +  mWindow->GetDocument(getter_AddRefs(domdoc));

Fatal assert if mWindow is null?

@@ +392,5 @@
> +  if (GetPermission(mWindow) == NotificationPermission::Granted) {
> +    nsCOMPtr<nsIAlertsService> alertService =
> +        do_GetService(NS_ALERTSERVICE_CONTRACTID);
> +
> +    if (alertService) {

return early to avoid nesting.

@@ +400,5 @@
> +        // Resolve image against document base URI.
> +        nsCOMPtr<nsIURI> baseUri = mDocument->GetBaseURI();
> +        nsCOMPtr<nsIURI> srcUri;
> +        nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(srcUri),
> +                                                  mIconUrl, mDocument, baseUri);

Why not just NS_NewURI?

@@ +415,5 @@
> +
> +      // In the case of IPC, the parent process uses the cookie to map to
> +      // nsIObserver. Thus the cookie must be unique to differentiate observers.
> +      nsString uniqueCookie = NS_LITERAL_STRING("notification");
> +      uniqueCookie.AppendInt(mCount++);

sCount

@@ +420,5 @@
> +      nsCOMPtr<nsIObserver> observer = new NotificationObserver(this);
> +      rv = alertService->ShowAlertNotification(absoluteUrl, mTitle, mBody, true,
> +                                               uniqueCookie, observer, alertName,
> +                                               DirectionToString(mDir), mLang);
> +      NS_ENSURE_SUCCESS(rv, rv);

return rv.  then no need for the 'else'

::: dom/webidl/Notification.webidl
@@ +41,5 @@
> +enum NotificationPermission {
> +  "default",
> +  "denied",
> +  "granted"
> +};

Isn't there an "unknown", which means "the permission has not been denied or granted yet"?  Or is that what default means?

@@ +49,5 @@
> +enum NotificationDirection {
> +  "auto",
> +  "ltr",
> +  "rtl"
> +};

You will want someone more familiar with the new bindings to review this.
Comment 33 Doug Turner (:dougt) 2012-10-12 11:19:49 PDT
Comment on attachment 670637 [details] [diff] [review]
Part 9: Remove native notification systems on desktop platforms. v1

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

I don't think we want do remove all native notifications.  josh just added notification center support on the mac.
Comment 34 :Ms2ger (⌚ UTC+1/+2) 2012-10-12 11:26:15 PDT
Comment on attachment 670631 [details] [diff] [review]
Part 5: Implement Notification API. v1

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

::: dom/bindings/Bindings.conf
@@ +412,5 @@
>  },
>  
> +'Notification': [
> +{
> +    'nativeType': 'mozilla::dom::Notification',

This isn't necessary

@@ +414,5 @@
> +'Notification': [
> +{
> +    'nativeType': 'mozilla::dom::Notification',
> +    'infallible': {
> +        'getterOnly': [

This should not be here

@@ +417,5 @@
> +    'infallible': {
> +        'getterOnly': [
> +            'onclick', 'onshow', 'onerror', 'onclose', 'permission'
> +        ]
> +    }

So I think you won't need to add anything here

::: dom/src/notification/Notification.cpp
@@ +341,5 @@
> +
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);
> +  BindToOwner(window);
> +
> +  mWindow = do_QueryInterface(aWindow);

Don't need mWindow, you already have mOwner.

@@ +344,5 @@
> +
> +  mWindow = do_QueryInterface(aWindow);
> +  nsCOMPtr<nsIDOMDocument> domdoc;
> +  mWindow->GetDocument(getter_AddRefs(domdoc));
> +  mDocument = do_QueryInterface(domdoc);

mDocument = mOwner->GetDoc();

@@ +439,5 @@
> +  // Get principal from global to make permission request for notifications.
> +  nsCOMPtr<nsIDOMWindow> window(do_QueryInterface(aGlobal));
> +  nsCOMPtr<nsIDOMDocument> domdoc;
> +  window->GetDocument(getter_AddRefs(domdoc));
> +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domdoc);

GetDoc()

@@ +455,5 @@
> +  // Get principal from global to check permission for notifications.
> +  nsCOMPtr<nsIDOMWindow> window(do_QueryInterface(aGlobal));
> +  nsCOMPtr<nsIDOMDocument> domdoc;
> +  window->GetDocument(getter_AddRefs(domdoc));
> +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domdoc);

GetDoc()

::: dom/src/notification/Notification.h
@@ +36,5 @@
> +               const nsAString& aBody, NotificationDirection aDir,
> +               const nsAString& aLang, const nsAString& aTag,
> +               const nsAString& aIconUrl);
> +
> +  static Notification* Constructor(nsISupports* aGlobal,

Should return already_AddRefed

@@ +57,5 @@
> +  NS_METHOD SetOnerror(JSContext* aCx, const JS::Value& val);
> +  NS_METHOD GetOnclose(JSContext* aCx, JS::Value* val);
> +  NS_METHOD SetOnclose(JSContext* aCx, const JS::Value& val);
> +
> +  nsISupports* GetParentObject()

nsPIDOMWindow*

::: dom/webidl/Notification.webidl
@@ +34,5 @@
> +  NotificationDirection dir = "auto";
> +  DOMString lang = "";
> +  DOMString body;
> +  DOMString tag;
> +  DOMString icon;

Why not '= ""' for those?
Comment 35 Matthew N. [:MattN] 2012-10-12 14:40:40 PDT
Comment on attachment 670634 [details] [diff] [review]
Part 7: Added permission prompt for desktop notifications on desktop platforms. v1

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

Does this patch fully cover bug 594543?

::: browser/base/content/urlbarBindings.xml
@@ +945,4 @@
>      </implementation>
>    </binding>
>  
> +  <binding id="desktop-notification-notification" extends="chrome://global/content/bindings/notification.xml#popup-notification">

The only difference between this and the binding being extended is one xul:spacer so it seems like this binding can probably be avoided so another fork doesn't have to be maintained.
Comment 36 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-10-14 14:59:38 PDT
Comment on attachment 670626 [details] [diff] [review]
Part 1: Factor out code to add event listeners so that it can be used by Notifications. v1


>-  IMPL_EVENT_HANDLER(loadstart)
>-  IMPL_EVENT_HANDLER(progress)
>-  IMPL_EVENT_HANDLER(abort)
>-  IMPL_EVENT_HANDLER(error)
>-  IMPL_EVENT_HANDLER(load)
>-  IMPL_EVENT_HANDLER(timeout)
>-  IMPL_EVENT_HANDLER(loadend)
>+  IMPL_JS_EVENT_HANDLER(loadstart)
>+  IMPL_JS_EVENT_HANDLER(progress)
>+  IMPL_JS_EVENT_HANDLER(abort)
>+  IMPL_JS_EVENT_HANDLER(error)
>+  IMPL_JS_EVENT_HANDLER(load)
>+  IMPL_JS_EVENT_HANDLER(timeout)
>+  IMPL_JS_EVENT_HANDLER(loadend)
Please don't change the name of the macro.

>+#define IMPL_JS_EVENT_HANDLER(_lowercase)                                     \
...so this should be without _JS
Comment 37 Etienne Segonzac (:etienne) 2012-10-16 09:50:03 PDT
Comment on attachment 670640 [details] [diff] [review]
Update Gaia with additional functionality to support Notification API. v1

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

Just a small nit.

But this is *really* exciting!

::: apps/system/js/notifications.js
@@ +332,5 @@
> +    event.initCustomEvent('mozContentEvent', true, true, {
> +      type: 'desktop-notification-close',
> +      id: notificationNode.dataset.notificationId
> +    });
> +    window.dispatchEvent(event);

Probably worth extracting the mozContentEvents in a function now (taking type, notificationId).
Comment 38 William Chen [:wchen] (Vacation until 8/25) 2012-10-16 12:59:19 PDT
(In reply to Doug Turner (:dougt) from comment #30)
> Comment on attachment 670628 [details] [diff] [review]
> Part 3: Updated the nsIAlertsService API and its implementation to include
> support for bidi overrides and a method to close notifications. v1
> 
> Review of attachment 670628 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ w/ consideration on the return code.
> 
> ::: toolkit/components/alerts/nsAlertsService.cpp
> @@ +206,5 @@
> > +  // Try the system notification service.
> > +  nsCOMPtr<nsIAlertsService> sysAlerts(do_GetService(NS_SYSTEMALERTSERVICE_CONTRACTID));
> > +  if (sysAlerts) {
> > +    nsresult rv = sysAlerts->CloseAlert(aAlertName);
> > +    if (NS_SUCCEEDED(rv)) {
> 
> If this fails, should we really return NS_ERROR_NOT_IMPLEMENTED?  Maybe we
> should just return rv
> 
That makes sense. I've also changed |showAlertNotification| to do the same thing if the system alert service fails.
> ::: toolkit/components/alerts/nsIAlertsService.idl
> @@ +64,5 @@
> > +    *
> > +    * @param name           The name of the notification to close. If no name
> > +    *                       is provided then only a notification created with
> > +    *                       no name (if any) will be closed.
> > +    */
> 
> Maybe you should define the return/throw code.
I don't think that there is a meaningful error code to document in the interface because in the later patches, all platforms will have an alert implementation.
Comment 39 William Chen [:wchen] (Vacation until 8/25) 2012-10-16 12:59:56 PDT
Created attachment 671979 [details] [diff] [review]
Part 3: Updated the nsIAlertsService API and its implementation to include support for bidi overrides and a method to close notifications. v2
Comment 40 William Chen [:wchen] (Vacation until 8/25) 2012-10-16 13:02:45 PDT
(In reply to Doug Turner (:dougt) from comment #31)
> Comment on attachment 670629 [details] [diff] [review]
> Part 4: Make nsDesktopNotification generate unique name and cookies to
> prevent collision in the IPC observer and coalescing in the alerts service.
> v1
> 
> Review of attachment 670629 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ w/ style nit fixed.
> 
> ::: dom/src/notification/nsDesktopNotification.cpp
> @@ +39,5 @@
> > +  // Generate a unique name (which will also be used as a cookie) because
> > +  // the nsIAlertsService will coalesce notifications with the same name.
> > +  // In the case of IPC, the parent process will use the cookie to map
> > +  // to nsIObservers, thus cookies must be unique to differentiate observers.
> > +  nsString uniqueName = NS_LITERAL_STRING("desktop-notification");
> 
> Maybe append a : or another - so that the unique name is :
> 
> desktop-notification:###
> 
> ::: dom/src/notification/nsDesktopNotification.h
> @@ +108,5 @@
> >    nsCOMPtr<nsIPrincipal> mPrincipal;
> >    bool mAllow;
> >    bool mShowHasBeenCalled;
> > +
> > +  static uint32_t mCount;
> 
> this should be sCount.

Done.
Comment 41 William Chen [:wchen] (Vacation until 8/25) 2012-10-16 13:03:20 PDT
Created attachment 671982 [details] [diff] [review]
Part 4: Make nsDesktopNotification generate unique name and cookies to prevent collision in the IPC observer and coalescing in the alerts service. v2
Comment 42 William Chen [:wchen] (Vacation until 8/25) 2012-10-16 13:17:55 PDT
(In reply to Doug Turner (:dougt) from comment #33)
> Comment on attachment 670637 [details] [diff] [review]
> Part 9: Remove native notification systems on desktop platforms. v1
> 
> Review of attachment 670637 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think we want do remove all native notifications.  josh just added
> notification center support on the mac.

There are problems with keeping the notification center stuff for mac. One is that we can't fully implement the new nsIAlertsService API with it. Another is that it doesn't properly implement the current nsIAlertsService API, we fake "alertfinished" and we don't have support for "alertclicked". If we really wanted to keep the mac notification center, we should bring it back under a very simple alert API with only the ability to show an alert and nothing else.
Comment 43 Wesley Johnston (:wesj) 2012-10-16 13:21:29 PDT
Comment on attachment 670627 [details] [diff] [review]
Part 2: Implemented additional functionality in Fennec to support Notification API. v1

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

I think this looks good.

Just to be careful, does the notification api specify what to do if the app is killed between the time it created the notification and the notification was tapped?
Comment 44 William Chen [:wchen] (Vacation until 8/25) 2012-10-16 14:05:29 PDT
(In reply to Olli Pettay [:smaug] from comment #36)
> Comment on attachment 670626 [details] [diff] [review]
> Part 1: Factor out code to add event listeners so that it can be used by
> Notifications. v1
> 
> 
> >-  IMPL_EVENT_HANDLER(loadstart)
> >-  IMPL_EVENT_HANDLER(progress)
> >-  IMPL_EVENT_HANDLER(abort)
> >-  IMPL_EVENT_HANDLER(error)
> >-  IMPL_EVENT_HANDLER(load)
> >-  IMPL_EVENT_HANDLER(timeout)
> >-  IMPL_EVENT_HANDLER(loadend)
> >+  IMPL_JS_EVENT_HANDLER(loadstart)
> >+  IMPL_JS_EVENT_HANDLER(progress)
> >+  IMPL_JS_EVENT_HANDLER(abort)
> >+  IMPL_JS_EVENT_HANDLER(error)
> >+  IMPL_JS_EVENT_HANDLER(load)
> >+  IMPL_JS_EVENT_HANDLER(timeout)
> >+  IMPL_JS_EVENT_HANDLER(loadend)
> Please don't change the name of the macro.
> 
> >+#define IMPL_JS_EVENT_HANDLER(_lowercase)                                     \
> ...so this should be without _JS


I changed the name of the macro due to a name conflict with a macro defined in WebSocket.h. I factored the code a bit more and renamed one of the macros to IMPL_JS_EVENT_HANDLER_OLD (I'm not sure what a good name would be). Can you review this again?
Comment 45 William Chen [:wchen] (Vacation until 8/25) 2012-10-16 14:06:24 PDT
Created attachment 672014 [details] [diff] [review]
Part 1: Factor out code to add event listeners so that it can be used by Notifications. v2
Comment 46 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-10-16 16:38:22 PDT
Comment on attachment 672014 [details] [diff] [review]
Part 1: Factor out code to add event listeners so that it can be used by Notifications. v2

so it is not _OLD, but _INLINE or something like that?

just make everything to use the same thing please (and that thing shouldn't have 
INLINE nor OLD)
This all is kind of about a separate bug, so feel free to deal it here or file a
new bug (which blocks this).
Comment 47 Josh Aas 2012-10-17 00:40:21 PDT
(In reply to William Chen [:wchen] from comment #42)
> (In reply to Doug Turner (:dougt) from comment #33)
> > Comment on attachment 670637 [details] [diff] [review]
> > Part 9: Remove native notification systems on desktop platforms. v1
> > 
> > Review of attachment 670637 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I don't think we want do remove all native notifications.  josh just added
> > notification center support on the mac.
> 
> There are problems with keeping the notification center stuff for mac. One
> is that we can't fully implement the new nsIAlertsService API with it.
> Another is that it doesn't properly implement the current nsIAlertsService
> API, we fake "alertfinished" and we don't have support for "alertclicked".
> If we really wanted to keep the mac notification center, we should bring it
> back under a very simple alert API with only the ability to show an alert
> and nothing else.

I was going to address the shortcomings of our Notification Center support this week. Given how it works, I'll probably have to tweak the existing alerts API, or make a second version of it. In any case, I'd really prefer that you not remove support for it.

Does your new implementation here work for general alerts, not just W3C spec alerts from websites?
Comment 48 William Chen [:wchen] (Vacation until 8/25) 2012-10-17 09:38:31 PDT
(In reply to Josh Aas (Mozilla Corporation) from comment #47)
> (In reply to William Chen [:wchen] from comment #42)
> > (In reply to Doug Turner (:dougt) from comment #33)
> > > Comment on attachment 670637 [details] [diff] [review]
> > > Part 9: Remove native notification systems on desktop platforms. v1
> > > 
> > > Review of attachment 670637 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I don't think we want do remove all native notifications.  josh just added
> > > notification center support on the mac.
> > 
> > There are problems with keeping the notification center stuff for mac. One
> > is that we can't fully implement the new nsIAlertsService API with it.
> > Another is that it doesn't properly implement the current nsIAlertsService
> > API, we fake "alertfinished" and we don't have support for "alertclicked".
> > If we really wanted to keep the mac notification center, we should bring it
> > back under a very simple alert API with only the ability to show an alert
> > and nothing else.
> 
> I was going to address the shortcomings of our Notification Center support
> this week. Given how it works, I'll probably have to tweak the existing
> alerts API, or make a second version of it. In any case, I'd really prefer
> that you not remove support for it.
> 
> Does your new implementation here work for general alerts, not just W3C spec
> alerts from websites?

What were you planning on tweaking? The new implementation does work for general alerts, I simply added some functionality to the current nsIAlertsService. If you can properly support the current API and the notification center provides support for closing alerts, then we can use native alerts on mac for the w3c notifications.
Comment 49 Josh Aas 2012-10-17 10:33:26 PDT
(In reply to William Chen [:wchen] from comment #48)

> What were you planning on tweaking? The new implementation does work for
> general alerts, I simply added some functionality to the current
> nsIAlertsService. If you can properly support the current API and the
> notification center provides support for closing alerts, then we can use
> native alerts on mac for the w3c notifications.

I'll give it a shot, but I'll wait for your work to land first so we're not stepping on each other trying to get things done.
Comment 50 William Chen [:wchen] (Vacation until 8/25) 2012-10-17 15:14:46 PDT
(In reply to Doug Turner (:dougt) from comment #32)
> Comment on attachment 670631 [details] [diff] [review]
> Part 5: Implement Notification API. v1
> 
> Review of attachment 670631 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like someone else to also review this patch for the binding specific
> stuff including the CallCallback method.
> 
> ::: dom/bindings/Bindings.conf
> @@ +418,5 @@
> > +        'getterOnly': [
> > +            'onclick', 'onshow', 'onerror', 'onclose', 'permission'
> > +        ]
> > +    }
> > +}],
> 
> I do not know if this is correct.
> 
> ::: dom/ipc/PContent.ipdl
> @@ +328,5 @@
> > +     * NOTE: The principal is untrusted in the parent process. Only
> > +     *       principals that can live in the content process should
> > +     *       be provided.
> > +     */
> > +    sync TestPermissionFromPrincipal(Principal principal, nsCString type)
> 
> You should factor this change out into a different bug.  You will want
> cjones and/or jonas to review.
> 
> ::: dom/src/notification/Notification.cpp
> @@ +1,2 @@
> > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > +/* vim: set ts=2 et sw=2 tw=80: */
> 
> So, Mode Lines are allowed/encouraged by our style guide, but they are dumb.
> Your editor should be smart enough to not have to have this crap.  Up to you
> if you want to leave them here or not.
> 
> @@ +7,5 @@
> > +#include "PCOMContentPermissionRequestChild.h"
> > +#include "mozilla/dom/PBrowserChild.h"
> > +
> > +#include "mozilla/dom/Notification.h"
> > +
> 
> remove extra line.
> 
> @@ +42,5 @@
> > +    : mPrincipal(aPrincipal), mWindow(aWindow),
> > +      mPermission(NotificationPermission::Default),
> > +      mFunObj(aFunObj), mHasPermission(false)
> > +  {
> > +    mCx = GetScriptContextFromJSContext(aCx);
> 
> Assert that mCx is non-null?
> 
> @@ +137,5 @@
> > +NotificationPermissionRequest::Run()
> > +{
> > +  if (mHasPermission) {
> > +    return CallCallback();
> > +  }
> 
> Having Run() be called twice (once for the prompting, and once when you have
> mHasPermission set) is a bit confusing on first read.  It might be clearer
> if you, instead, ran CallCallback with NS_NewRunnableMethod.  In this way,
> NotificationPermissionRequest::Run would only ever be used to bring up the
> permission request.  You could also remove the mHasPermission flag.
> 
> @@ +197,5 @@
> > +NotificationPermissionRequest::Cancel()
> > +{
> > +  mPermission = NotificationPermission::Denied;
> > +  mHasPermission = true;
> > +  NS_DispatchToMainThread(this);
> 
> How about something like:
> 
> NS_NewRunnableMethod(this, &NotificationPermissionRequest::CallCallback);
> NS_DispatchToMainThread(this);
> 
> @@ +243,5 @@
> > +
> > +  mPrincipal = nullptr;
> > +  mWindow = nullptr;
> > +
> > +  return NS_OK;
> 
> Need a binding review to make sure that you're doing things right.
> 
> @@ +338,5 @@
> > +    mTag(aTag), mIconUrl(aIconUrl), mIsClosed(false)
> > +{
> > +  SetIsDOMBinding();
> > +
> > +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);
> 
> Fatal assert if aWindow is null?
> 
> @@ +343,5 @@
> > +  BindToOwner(window);
> > +
> > +  mWindow = do_QueryInterface(aWindow);
> > +  nsCOMPtr<nsIDOMDocument> domdoc;
> > +  mWindow->GetDocument(getter_AddRefs(domdoc));
> 
> Fatal assert if mWindow is null?
> 
> @@ +392,5 @@
> > +  if (GetPermission(mWindow) == NotificationPermission::Granted) {
> > +    nsCOMPtr<nsIAlertsService> alertService =
> > +        do_GetService(NS_ALERTSERVICE_CONTRACTID);
> > +
> > +    if (alertService) {
> 
> return early to avoid nesting.
> 
> @@ +400,5 @@
> > +        // Resolve image against document base URI.
> > +        nsCOMPtr<nsIURI> baseUri = mDocument->GetBaseURI();
> > +        nsCOMPtr<nsIURI> srcUri;
> > +        nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(srcUri),
> > +                                                  mIconUrl, mDocument, baseUri);
> 
> Why not just NS_NewURI?
> 
Because NewURIWithDocumentCharset calls NS_NewURI and passes in the charset.
> @@ +415,5 @@
> > +
> > +      // In the case of IPC, the parent process uses the cookie to map to
> > +      // nsIObserver. Thus the cookie must be unique to differentiate observers.
> > +      nsString uniqueCookie = NS_LITERAL_STRING("notification");
> > +      uniqueCookie.AppendInt(mCount++);
> 
> sCount
> 
> @@ +420,5 @@
> > +      nsCOMPtr<nsIObserver> observer = new NotificationObserver(this);
> > +      rv = alertService->ShowAlertNotification(absoluteUrl, mTitle, mBody, true,
> > +                                               uniqueCookie, observer, alertName,
> > +                                               DirectionToString(mDir), mLang);
> > +      NS_ENSURE_SUCCESS(rv, rv);
> 
> return rv.  then no need for the 'else'
> 
> ::: dom/webidl/Notification.webidl
> @@ +41,5 @@
> > +enum NotificationPermission {
> > +  "default",
> > +  "denied",
> > +  "granted"
> > +};
> 
> Isn't there an "unknown", which means "the permission has not been denied or
> granted yet"?  Or is that what default means?
That's what default means.
> 
> @@ +49,5 @@
> > +enum NotificationDirection {
> > +  "auto",
> > +  "ltr",
> > +  "rtl"
> > +};
> 
> You will want someone more familiar with the new bindings to review this.

Done.(In reply to :Ms2ger from comment #34)
> Comment on attachment 670631 [details] [diff] [review]
> Part 5: Implement Notification API. v1
> 
> Review of attachment 670631 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bindings/Bindings.conf
> @@ +412,5 @@
> >  },
> >  
> > +'Notification': [
> > +{
> > +    'nativeType': 'mozilla::dom::Notification',
> 
> This isn't necessary
> 
> @@ +414,5 @@
> > +'Notification': [
> > +{
> > +    'nativeType': 'mozilla::dom::Notification',
> > +    'infallible': {
> > +        'getterOnly': [
> 
> This should not be here
> 
> @@ +417,5 @@
> > +    'infallible': {
> > +        'getterOnly': [
> > +            'onclick', 'onshow', 'onerror', 'onclose', 'permission'
> > +        ]
> > +    }
> 
> So I think you won't need to add anything here
> 
> ::: dom/src/notification/Notification.cpp
> @@ +341,5 @@
> > +
> > +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);
> > +  BindToOwner(window);
> > +
> > +  mWindow = do_QueryInterface(aWindow);
> 
> Don't need mWindow, you already have mOwner.
> 
> @@ +344,5 @@
> > +
> > +  mWindow = do_QueryInterface(aWindow);
> > +  nsCOMPtr<nsIDOMDocument> domdoc;
> > +  mWindow->GetDocument(getter_AddRefs(domdoc));
> > +  mDocument = do_QueryInterface(domdoc);
> 
> mDocument = mOwner->GetDoc();
> 
> @@ +439,5 @@
> > +  // Get principal from global to make permission request for notifications.
> > +  nsCOMPtr<nsIDOMWindow> window(do_QueryInterface(aGlobal));
> > +  nsCOMPtr<nsIDOMDocument> domdoc;
> > +  window->GetDocument(getter_AddRefs(domdoc));
> > +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domdoc);
> 
> GetDoc()
> 
> @@ +455,5 @@
> > +  // Get principal from global to check permission for notifications.
> > +  nsCOMPtr<nsIDOMWindow> window(do_QueryInterface(aGlobal));
> > +  nsCOMPtr<nsIDOMDocument> domdoc;
> > +  window->GetDocument(getter_AddRefs(domdoc));
> > +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domdoc);
> 
> GetDoc()
> 
> ::: dom/src/notification/Notification.h
> @@ +36,5 @@
> > +               const nsAString& aBody, NotificationDirection aDir,
> > +               const nsAString& aLang, const nsAString& aTag,
> > +               const nsAString& aIconUrl);
> > +
> > +  static Notification* Constructor(nsISupports* aGlobal,
> 
> Should return already_AddRefed
> 
> @@ +57,5 @@
> > +  NS_METHOD SetOnerror(JSContext* aCx, const JS::Value& val);
> > +  NS_METHOD GetOnclose(JSContext* aCx, JS::Value* val);
> > +  NS_METHOD SetOnclose(JSContext* aCx, const JS::Value& val);
> > +
> > +  nsISupports* GetParentObject()
> 
> nsPIDOMWindow*
> 
> ::: dom/webidl/Notification.webidl
> @@ +34,5 @@
> > +  NotificationDirection dir = "auto";
> > +  DOMString lang = "";
> > +  DOMString body;
> > +  DOMString tag;
> > +  DOMString icon;
> 
> Why not '= ""' for those?
Because that's how the IDL looks in the spec.

Done.
Comment 51 William Chen [:wchen] (Vacation until 8/25) 2012-10-17 15:15:48 PDT
Created attachment 672542 [details] [diff] [review]
Part 5.1: Add function to remote test for permission in nsIPermissionManager. v1
Comment 52 William Chen [:wchen] (Vacation until 8/25) 2012-10-17 15:16:49 PDT
Created attachment 672543 [details] [diff] [review]
Part 5.2: Implement Notification API. v2
Comment 53 William Chen [:wchen] (Vacation until 8/25) 2012-10-17 15:17:48 PDT
(In reply to Matthew N. [:MattN] from comment #35)
> Comment on attachment 670634 [details] [diff] [review]
> Part 7: Added permission prompt for desktop notifications on desktop
> platforms. v1
> 
> Review of attachment 670634 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does this patch fully cover bug 594543?
> 
> ::: browser/base/content/urlbarBindings.xml
> @@ +945,4 @@
> >      </implementation>
> >    </binding>
> >  
> > +  <binding id="desktop-notification-notification" extends="chrome://global/content/bindings/notification.xml#popup-notification">
> 
> The only difference between this and the binding being extended is one
> xul:spacer so it seems like this binding can probably be avoided so another
> fork doesn't have to be maintained.

Done.
Comment 54 William Chen [:wchen] (Vacation until 8/25) 2012-10-17 15:18:35 PDT
Created attachment 672546 [details] [diff] [review]
Part 7: Added permission prompt for desktop notifications on desktop platforms. v2
Comment 55 Matthew N. [:MattN] 2012-10-17 15:29:38 PDT
Comment on attachment 672546 [details] [diff] [review]
Part 7: Added permission prompt for desktop notifications on desktop platforms. v2

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

Please include 8 lines of context in future patches as 3 lines is not enough.

::: browser/base/content/browser.css
@@ +478,5 @@
>    max-width: 280px;
>  }
>  
> +#desktop-notification-notification {
> +  -moz-binding: url("chrome://global/content/bindings/notification.xml#popup-notification");

This is no longer needed now.
Comment 56 Jared Wein [:jaws] (please needinfo? me) 2012-10-17 16:11:38 PDT
Comment on attachment 672546 [details] [diff] [review]
Part 7: Added permission prompt for desktop notifications on desktop platforms. v2

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

Matt has said that he can review this part :)
Comment 57 Jared Wein [:jaws] (please needinfo? me) 2012-10-17 17:40:27 PDT
Comment on attachment 670636 [details] [diff] [review]
Part 8: Updated existing XUL alerts to implement the Notification API. v1

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

Can you include 8 lines of context in your diffs?

::: toolkit/components/alerts/nsXULAlerts.cpp
@@ +8,5 @@
> +#include "mozilla/LookAndFeel.h"
> +#include "nsIServiceManager.h"
> +#include "nsISupportsArray.h"
> +#include "nsISupportsPrimitives.h"
> +#include "nsIWindowWatcher.h"

mozilla/LookAndFeel.h was included last when these includes were in nsAlertsService.cpp. Why did they get reordered?

Also, I don't think nsIServiceManager.h needs to be included here. It wasn't included in nsAlertsService.cpp, and there don't appear to be any calls to getService or getServiceByContractID.

@@ +69,5 @@
> +  nsCOMPtr<nsIWindowWatcher> wwatch(do_GetService(NS_WINDOWWATCHER_CONTRACTID));
> +  nsCOMPtr<nsIDOMWindow> newWindow;
> +
> +  nsCOMPtr<nsISupportsArray> argsArray;
> +  rv = NS_NewISupportsArray(getter_AddRefs(argsArray));

Declare rv here,
nsresult rv = NS_NewISupportsArray(getter_AddRefs(argsArray));

@@ +172,5 @@
> +
> +  rv = wwatch->OpenWindow(0, ALERT_CHROME_URL, "_blank",
> +                   "chrome,dialog=yes,titlebar=no,popup=yes", argsArray,
> +                   getter_AddRefs(newWindow));
> +

nit: line these two lines up with the first argument of OpenWindow.

@@ +184,5 @@
> +{
> +  nsCOMPtr<nsIDOMWindow> domWindow = mNamedWindows.Get(aAlertName);
> +  if (domWindow) {
> +    domWindow->Close();
> +  }

if (domWindow) {
  return domWindow->Close();
}
return NS_OK;

::: toolkit/components/alerts/resources/content/alert.js
@@ +39,1 @@
>      case 6:

What happened to handling arguments[6] and arguments[7]?

@@ +52,5 @@
>        document.getElementById('alertTitleLabel').setAttribute('value', window.arguments[1]);
>      case 1:
> +      if (window.arguments[0].length > 0) {
> +        document.getElementById('alertImage').setAttribute('src', window.arguments[0]);
> +      }

if (window.arguments[0])
  document.getElementById('alertImage').setAttribute('src', window.arguments[0]);

@@ +97,5 @@
>    alertBox.setAttribute("animate", true);
> +
> +  if (gAlertListener) {
> +    gAlertListener.observe(null, "alertshow", gAlertCookie);
> +  }

nit: no need for curly brackets on single-line blocks. the preexisting code in this file doesn't have them, so it would be good to maintain consistency.

@@ +111,1 @@
>    window.close();

Might as well just remove this function and just swap closeAlert with window.close.

::: toolkit/components/alerts/resources/content/alert.xul
@@ +11,3 @@
>  <?xml-stylesheet href="chrome://global/content/alerts/alert.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://global/skin/alerts/alert.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://global/skin/notification.css" type="text/css"?>

How does notification.css help here?

@@ +25,5 @@
>  
>    <box id="alertBox" class="alertBox">
> +    <box onclick="onAlertClick()">
> +      <hbox id="alertImageBox" class="alertImageBox" align="center" pack="center">
> +        <image id="alertImage" src="chrome://browser/skin/notification-64.png"/>

Toolkit shouldn't reference browser, and this image shouldn't be hard-coded here like this. This image can be set using CSS.

@@ +35,5 @@
> +      </vbox>
> +    </box>
> +
> +    <vbox class="alertCloseBox">
> +      <toolbarbutton class="messageCloseButton"

Does this have to be a <xul:toolbarbutton>? It's not in a toolbar, so it is a little confusing. Can it just be a <xul:button>?

@@ +36,5 @@
> +    </box>
> +
> +    <vbox class="alertCloseBox">
> +      <toolbarbutton class="messageCloseButton"
> +                     label="&closeNotification.tooltip;"

This should probably have an X graphic for the image of this button. And it would be better to specify a new string to be used here instead of sharing the popup notification messages since other localizations may have text that doesn't apply here.

::: toolkit/themes/gnomestripe/global/alerts/alert.css
@@ +33,1 @@
>    background-image: linear-gradient(rgba(255,255,255,0.2), rgba(255,255,255,0.1));

The .alertCloseBox should have a :hover and a :hover:active style since it should look like a clickable button. The :hover style should not be specified for pinstripe.

Potential value for :hover can be:
background-image: linear-gradient(rgba(255,255,255,0.4), rgba(255,255,255,0.3));

Potential value for :hover:active can be:
background-image: linear-gradient(rgba(255,255,255,0.1), rgba(255,255,255,0.2));

::: toolkit/themes/pinstripe/global/alerts/alert.css
@@ +11,5 @@
> +@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> +
> +.alertBox {
> +  border: 1px solid threedshadow;
> +  border-radius: 3px;

Please remove this border-radius since we can't draw transparent windows on Mac.
Comment 58 Jared Wein [:jaws] (please needinfo? me) 2012-10-17 17:45:50 PDT
(In reply to Jared Wein [:jaws] from comment #57)
> > +
> > +.alertBox {
> > +  border: 1px solid threedshadow;
> > +  border-radius: 3px;
> 
> Please remove this border-radius since we can't draw transparent windows on
> Mac.

You can disregard this comment, I may be wrong here.
Comment 59 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-20 00:59:12 PDT
Comment on attachment 672542 [details] [diff] [review]
Part 5.1: Add function to remote test for permission in nsIPermissionManager. v1

The implementation here looks fine.

Just for my information, what resource are we trying to protect by using this test?

The reason I ask is that this exposes an information leak in which compromised subprocesses can TestPermission() URLs to indirectly query our permissions DB.  I'm not all that concerned about that though.

r=me if Lucas or Jonas signs off on it.
Comment 60 William Chen [:wchen] (Vacation until 8/25) 2012-10-22 09:50:40 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #59)
> Comment on attachment 672542 [details] [diff] [review]
> Part 5.1: Add function to remote test for permission in
> nsIPermissionManager. v1
> 
> The implementation here looks fine.
> 
> Just for my information, what resource are we trying to protect by using
> this test?
> 
> The reason I ask is that this exposes an information leak in which
> compromised subprocesses can TestPermission() URLs to indirectly query our
> permissions DB.  I'm not all that concerned about that though.
> 
> r=me if Lucas or Jonas signs off on it.

The reason I added the test isn't to protect a resource. It's to implement an attribute in the notification API (Notification.permission) that lets a webpage query the current permission for notifications.
Comment 61 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-22 16:31:04 PDT
Comment on attachment 672542 [details] [diff] [review]
Part 5.1: Add function to remote test for permission in nsIPermissionManager. v1

OK.  r=me still stands conditional on f+ from Lucas or Jonas.
Comment 62 Lucas Adamski [:ladamski] 2012-10-22 17:48:46 PDT
Comment on attachment 672542 [details] [diff] [review]
Part 5.1: Add function to remote test for permission in nsIPermissionManager. v1

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

I'm not worried about enumeration of permissions for a same-principal scenario, but we should not allow cross-principal testing for permissions.  It introduces all sorts of potential privacy leaks (i.e. remote content enumerating installed apps).  The latter requires that you can obtain/construct a principal for an arbitrary domain/app, which offhand I'm not actually sure is possible.
Comment 63 William Chen [:wchen] (Vacation until 8/25) 2012-10-23 16:21:04 PDT
(In reply to Jared Wein [:jaws] from comment #57)
> Comment on attachment 670636 [details] [diff] [review]
> Part 8: Updated existing XUL alerts to implement the Notification API. v1
> 
> Review of attachment 670636 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you include 8 lines of context in your diffs?
> 
> ::: toolkit/components/alerts/nsXULAlerts.cpp
> @@ +8,5 @@
> > +#include "mozilla/LookAndFeel.h"
> > +#include "nsIServiceManager.h"
> > +#include "nsISupportsArray.h"
> > +#include "nsISupportsPrimitives.h"
> > +#include "nsIWindowWatcher.h"
> 
> mozilla/LookAndFeel.h was included last when these includes were in
> nsAlertsService.cpp. Why did they get reordered?
> 
I was including headers as they were needed. I don't think the order matters here.
> Also, I don't think nsIServiceManager.h needs to be included here. It wasn't
> included in nsAlertsService.cpp, and there don't appear to be any calls to
> getService or getServiceByContractID.
> 
It was included in nsAlertsService.cpp. There are calls to do_getService and do_createInstance in |nsXULAlerts::ShowAlertNotification|.
> @@ +69,5 @@
> > +  nsCOMPtr<nsIWindowWatcher> wwatch(do_GetService(NS_WINDOWWATCHER_CONTRACTID));
> > +  nsCOMPtr<nsIDOMWindow> newWindow;
> > +
> > +  nsCOMPtr<nsISupportsArray> argsArray;
> > +  rv = NS_NewISupportsArray(getter_AddRefs(argsArray));
> 
> Declare rv here,
> nsresult rv = NS_NewISupportsArray(getter_AddRefs(argsArray));
> 
> @@ +172,5 @@
> > +
> > +  rv = wwatch->OpenWindow(0, ALERT_CHROME_URL, "_blank",
> > +                   "chrome,dialog=yes,titlebar=no,popup=yes", argsArray,
> > +                   getter_AddRefs(newWindow));
> > +
> 
> nit: line these two lines up with the first argument of OpenWindow.
> 
> @@ +184,5 @@
> > +{
> > +  nsCOMPtr<nsIDOMWindow> domWindow = mNamedWindows.Get(aAlertName);
> > +  if (domWindow) {
> > +    domWindow->Close();
> > +  }
> 
> if (domWindow) {
>   return domWindow->Close();
> }
> return NS_OK;
> 
> ::: toolkit/components/alerts/resources/content/alert.js
> @@ +39,1 @@
> >      case 6:
> 
> What happened to handling arguments[6] and arguments[7]?
> 
They are arguments for bidi and lang, currently it is only supported in the b2g implementation.
> @@ +52,5 @@
> >        document.getElementById('alertTitleLabel').setAttribute('value', window.arguments[1]);
> >      case 1:
> > +      if (window.arguments[0].length > 0) {
> > +        document.getElementById('alertImage').setAttribute('src', window.arguments[0]);
> > +      }
> 
> if (window.arguments[0])
>   document.getElementById('alertImage').setAttribute('src',
> window.arguments[0]);
> 
> @@ +97,5 @@
> >    alertBox.setAttribute("animate", true);
> > +
> > +  if (gAlertListener) {
> > +    gAlertListener.observe(null, "alertshow", gAlertCookie);
> > +  }
> 
> nit: no need for curly brackets on single-line blocks. the preexisting code
> in this file doesn't have them, so it would be good to maintain consistency.
> 
> @@ +111,1 @@
> >    window.close();
> 
> Might as well just remove this function and just swap closeAlert with
> window.close.
> 
> ::: toolkit/components/alerts/resources/content/alert.xul
> @@ +11,3 @@
> >  <?xml-stylesheet href="chrome://global/content/alerts/alert.css" type="text/css"?>
> >  <?xml-stylesheet href="chrome://global/skin/alerts/alert.css" type="text/css"?>
> > +<?xml-stylesheet href="chrome://global/skin/notification.css" type="text/css"?>
> 
> How does notification.css help here?
> 
I was using the styles in it for the close button. I have moved the relevant styles to alert.css.
> @@ +25,5 @@
> >  
> >    <box id="alertBox" class="alertBox">
> > +    <box onclick="onAlertClick()">
> > +      <hbox id="alertImageBox" class="alertImageBox" align="center" pack="center">
> > +        <image id="alertImage" src="chrome://browser/skin/notification-64.png"/>
> 
> Toolkit shouldn't reference browser, and this image shouldn't be hard-coded
> here like this. This image can be set using CSS.
> 
> @@ +35,5 @@
> > +      </vbox>
> > +    </box>
> > +
> > +    <vbox class="alertCloseBox">
> > +      <toolbarbutton class="messageCloseButton"
> 
> Does this have to be a <xul:toolbarbutton>? It's not in a toolbar, so it is
> a little confusing. Can it just be a <xul:button>?
> 
I tried switching to button but it doesn't behave like the other close buttons in the browser and required more styling tweaks to get rid of button UI.
> @@ +36,5 @@
> > +    </box>
> > +
> > +    <vbox class="alertCloseBox">
> > +      <toolbarbutton class="messageCloseButton"
> > +                     label="&closeNotification.tooltip;"
> 
> This should probably have an X graphic for the image of this button. And it
> would be better to specify a new string to be used here instead of sharing
> the popup notification messages since other localizations may have text that
> doesn't apply here.
> 
> ::: toolkit/themes/gnomestripe/global/alerts/alert.css
> @@ +33,1 @@
> >    background-image: linear-gradient(rgba(255,255,255,0.2), rgba(255,255,255,0.1));
> 
> The .alertCloseBox should have a :hover and a :hover:active style since it
> should look like a clickable button. The :hover style should not be
> specified for pinstripe.
> 
> Potential value for :hover can be:
> background-image: linear-gradient(rgba(255,255,255,0.4),
> rgba(255,255,255,0.3));
> 
> Potential value for :hover:active can be:
> background-image: linear-gradient(rgba(255,255,255,0.1),
> rgba(255,255,255,0.2));
> 
Done.
Comment 64 William Chen [:wchen] (Vacation until 8/25) 2012-10-23 16:21:52 PDT
Created attachment 674432 [details] [diff] [review]
Part 8: Updated existing XUL alerts to implement the Notification API. v2
Comment 65 William Chen [:wchen] (Vacation until 8/25) 2012-10-23 16:23:18 PDT
Created attachment 674435 [details] [diff] [review]
Part 1: Factor out code to add event listeners so that it can be used by Notifications. v3
Comment 66 William Chen [:wchen] (Vacation until 8/25) 2012-10-23 16:24:54 PDT
(In reply to Lucas Adamski from comment #62)
> Comment on attachment 672542 [details] [diff] [review]
> Part 5.1: Add function to remote test for permission in
> nsIPermissionManager. v1
> 
> Review of attachment 672542 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not worried about enumeration of permissions for a same-principal
> scenario, but we should not allow cross-principal testing for permissions. 
> It introduces all sorts of potential privacy leaks (i.e. remote content
> enumerating installed apps).  The latter requires that you can
> obtain/construct a principal for an arbitrary domain/app, which offhand I'm
> not actually sure is possible.

Do we have a mechanism to obtain the principal of the child without explicitly passing it in, or some way to validate principals?
Comment 67 William Chen [:wchen] (Vacation until 8/25) 2012-10-23 16:27:12 PDT
(In reply to Matthew N. [:MattN] from comment #55)
> Comment on attachment 672546 [details] [diff] [review]
> Part 7: Added permission prompt for desktop notifications on desktop
> platforms. v2
> 
> Review of attachment 672546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please include 8 lines of context in future patches as 3 lines is not enough.
> 
> ::: browser/base/content/browser.css
> @@ +478,5 @@
> >    max-width: 280px;
> >  }
> >  
> > +#desktop-notification-notification {
> > +  -moz-binding: url("chrome://global/content/bindings/notification.xml#popup-notification");
> 
> This is no longer needed now.

Done. Now with more context.
Comment 68 Matthew N. [:MattN] 2012-10-23 16:27:53 PDT
Comment on attachment 672546 [details] [diff] [review]
Part 7: Added permission prompt for desktop notifications on desktop platforms. v2

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

You need to enable git diffs so that binary contents are included, otherwise I only see the following:
diff -r b523cd456299 -r ec038535c166 browser/themes/gnomestripe/notification-16.png
Binary file browser/themes/gnomestripe/notification-16.png has changed
diff -r b523cd456299 -r ec038535c166 browser/themes/gnomestripe/notification-64.png
Binary file browser/themes/gnomestripe/notification-64.png has changed

See https://developer.mozilla.org/en-US/docs/Installing_Mercurial#Configuration for the ini changes.

Can we use "web-notifications" as the ID everywhere (note the 's')?  Having "web" in the name makes it more clear it's related to notifications for content whereas "desktop" leads me to believe the opposite. As a bonus, it's also shorter.

I would also replace "desktop" with "web" in the interfaces and other backend code as well but that's not my call to make.

The queue of patches currently doesn't compile for me on trunk with a clobber so it's hard to test the UI:
WebIDL.WebIDLError: error: invalid syntax, Notification.webidl line 15:9
  static readonly attribute NotificationPermission permission;

Are there tests for desktop in the other patches? I didn't see any at quick glance.  Could you at least add one for the notification permission doorhanger ensuring that notifications don't work until the permissions is granted?

I want to take another look when there is a working patch queue.

::: browser/base/content/browser.css
@@ +478,5 @@
>    max-width: 280px;
>  }
>  
> +#desktop-notification-notification {
> +  -moz-binding: url("chrome://global/content/bindings/notification.xml#popup-notification");

You need to add a line like https://hg.mozilla.org/mozilla-central/diff/a038c8c200f5/browser/base/content/browser.css for desktop-notification-icon and we should include an icon in the patch.

::: browser/components/nsBrowserGlue.js
@@ +1707,3 @@
>  
> +    // Set up and show the permission request prompt for geolocation.
> +    function promptGeo() {

Rather than forking this for promptNotification, I would prefer we refactor this to work for both since they only differ by strings, anchor and icon (plus the recently added telemetry).

The signature could be similar to the following:
_showPrompt(aPermission, aNotificationId, aAllowOnceString, aAllowAlwaysString, aNeverAllowString, aMessage, aFileMessage = null)

@@ +1755,5 @@
> +                                        mainAction, secondaryActions);
> +    }
> +
> +    // Set up and show the permission request prompt for notifications.
> +    function promptNotification() {

promptWebNotifications

@@ +1761,5 @@
> +        label: browserBundle.GetStringFromName("desktop-notification.showForSession"),
> +        accessKey: browserBundle.GetStringFromName("desktop-notification.showForSession.accesskey"),
> +        callback: function () {
> +          Services.perms.addFromPrincipal(requestingPrincipal, permissionKey, Ci.nsIPermissionManager.ALLOW_ACTION,
> +                                          Ci.nsIPermissionManager.EXPIRE_SESSION);

Please file a follow-up for nsISecurityUITelemetry reporting.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +277,5 @@
>  # or use "..." if \u2026 doesn't suit traditions in your locale.
>  geolocation.learnMore=Learn More…
>  
> +desktop-notification.showForSession=Show for this session
> +desktop-notification.showForSession.accesskey=a

It is preferred to choose an accesskey which is part of the label.  There is no "a" is "Show for this session" so can you find another available key? Perhaps consistency with other permission doorhangers is preferred.  You should probably check with #a11y.

@@ +282,5 @@
> +desktop-notification.alwaysShow=Always Show Notifications
> +desktop-notification.alwaysShow.accesskey=A
> +desktop-notification.neverShow=Always Block Notifications
> +desktop-notification.neverShow.accesskey=N
> +desktop-notification.showOnSite=Would you like to show notifications on %S?

s/on/from/ since the notifications will not be shown on the page itself.

::: dom/src/notification/Notification.cpp
@@ +6,1 @@
>  #include "mozilla/dom/PBrowserChild.h"

A DOM peer should review this file. Not sure whether is belongs in a different part/patch.
Comment 69 William Chen [:wchen] (Vacation until 8/25) 2012-10-23 16:27:57 PDT
Created attachment 674438 [details] [diff] [review]
Part 7: Added permission prompt for desktop notifications on desktop platforms. v3
Comment 70 Matthew N. [:MattN] 2012-10-23 18:12:09 PDT
Comment on attachment 674438 [details] [diff] [review]
Part 7: Added permission prompt for desktop notifications on desktop platforms. v3

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

::: browser/base/content/pageinfo/pageInfo.xul
@@ +330,5 @@
>            </hbox>
>          </vbox>
> +        <vbox class="permission" id="permNotificationRow">
> +          <label class="permissionLabel" id="permNotificationLabel"
> +                 value="&permNotification;" control="cookieRadioGroup"/>

cookieRadioGroup here seems like copypasta to update.
Comment 71 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-10-25 05:50:17 PDT
Comment on attachment 672543 [details] [diff] [review]
Part 5.2: Implement Notification API. v2


>+class NotificationPermissionRequest : public nsIContentPermissionRequest,
>+                                      public PCOMContentPermissionRequestChild,
>+                                      public nsIRunnable
>+{
>+public:
>+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>+  NS_DECL_NSICONTENTPERMISSIONREQUEST
>+  NS_DECL_NSIRUNNABLE
>+  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_AMBIGUOUS(NotificationPermissionRequest,
>+                                                         nsIContentPermissionRequest)
>+
>+  NotificationPermissionRequest(nsIPrincipal* aPrincipal, nsIDOMWindow* aWindow,
>+                                JSContext* aCx, JSObject* aFunObj)
>+    : mPrincipal(aPrincipal), mWindow(aWindow),
>+      mPermission(NotificationPermission::Default),
>+      mFunObj(aFunObj)
>+  {
>+    mCx = GetScriptContextFromJSContext(aCx);
>+    MOZ_ASSERT(mCx, "Context should not be null.");
>+    NS_HOLD_JS_OBJECTS(this, NotificationPermissionRequest);
>+  }
>+
>+  virtual ~NotificationPermissionRequest()
>+  {
>+    NS_DROP_JS_OBJECTS(this, NotificationPermissionRequest);
>+  }
>+
>+  bool Recv__delete__(const bool& aAllow);
>+  void IPDLRelease() { Release(); }
>+
>+protected:
>+  nsresult CallCallback();
>+  nsCOMPtr<nsIPrincipal> mPrincipal;
>+  nsCOMPtr<nsIDOMWindow> mWindow;
>+  NotificationPermission mPermission;
>+  nsCOMPtr<nsIScriptContext> mCx;
>+  JSObject* mFunObj;
>+};
This is horrible



>+
>+class NotificationObserver : public nsIObserver
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIOBSERVER
>+
>+  NotificationObserver(Notification* aNotification)
>+    : mNotification(aNotification) {}
>+
>+  virtual ~NotificationObserver() {}
>+
>+protected:
>+  nsRefPtr<Notification> mNotification;
>+};


So this is keeping Notification alive.
Is it guaranteed that the NotificationObserver object is short living object?
I recall that geolocation had some problems where it ended up keeping stuff alive
too long.


>+NS_IMPL_CYCLE_COLLECTION_CLASS(NotificationPermissionRequest)
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(NotificationPermissionRequest)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mWindow)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mCx)
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
You should NS_DROP_JS_OBJECTS here.

>+NotificationPermissionRequest::Cancel()
>+{
>+  mPermission = NotificationPermission::Denied;
>+  nsCOMPtr<nsIRunnable> callbackRunnable = NS_NewRunnableMethod(this,
>+      &NotificationPermissionRequest::CallCallback);
2 space indentation

>+  NS_DispatchToMainThread(callbackRunnable);
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+NotificationPermissionRequest::Allow()
>+{
>+  mPermission = NotificationPermission::Granted;
>+  nsCOMPtr<nsIRunnable> callbackRunnable = NS_NewRunnableMethod(this,
>+      &NotificationPermissionRequest::CallCallback);
ditto

>+NotificationPermissionRequest::CallCallback()
>+{
It is absolutely horrible to need to use JS API here.
Need to get Bug 779048 fixed asap.


>+  JSContext* cx = mCx->GetNativeContext();
cx can be null here, so you'd crash below.
Also, it would be better to take inner window as member variable and 
before using cx from the inner window, check that the inner window is still the current inner window.


>+  // Get the string value of the permission (eg. default, granted or denied).
>+  EnumEntry enumEntry =
>+      NotificationPermissionValues::strings[uint32_t(mPermission)];
2 space indentation.

>+Notification::Notification(nsISupports* aWindow, const nsAString& aTitle,
>+                           const nsAString& aBody, NotificationDirection aDir,
>+                           const nsAString& aLang, const nsAString& aTag,
>+                           const nsAString& aIconUrl)
>+  : mTitle(aTitle), mBody(aBody), mDir(aDir), mLang(aLang),
>+    mTag(aTag), mIconUrl(aIconUrl), mIsClosed(false)
>+{
>+  SetIsDOMBinding();
>+  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);
>+  MOZ_ASSERT(window, "Window should not be null.");
>+  BindToOwner(window);
>+  mDocument = GetOwner()->GetDoc();
Why you need mDocument? You can always get it from GetOwner()


>+Notification::ShowInternal()
>+{
>+  nsCOMPtr<nsIAlertsService> alertService =
>+      do_GetService(NS_ALERTSERVICE_CONTRACTID);
>+
>+  if (GetPermission(GetOwner()) != NotificationPermission::Granted ||
>+      !alertService) {
>+    // We do not have permission to show a notification or alert service
>+    // is not available.
>+    return DispatchEventInternal(NS_LITERAL_STRING("error"));
Notification inherits nsDOMEventTargetHelper which has
DispatchTrustedEvent. Use that and remove the implementation of DispatchEventInternal

>+Notification::Close()
>+{
>+  // Queue a task to close the notification.
>+  nsCOMPtr<nsIRunnable> showNotificationTask =
>+      new NotificationTask(this, NotificationTask::eClose);
2 space indentation

>+
>+  nsCOMPtr<nsIDOMWindow> mWindow;
You don't need this. GetOwner() returns the window.
Comment 72 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-10-25 05:52:22 PDT
(In reply to Olli Pettay [:smaug] from comment #71) 
> >+NotificationPermissionRequest::CallCallback()
> >+{
> It is absolutely horrible to need to use JS API here.
> Need to get Bug 779048 fixed asap.
> 

But I guess Bug 779048 shouldn't block this bug.
Comment 73 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-10-25 05:57:10 PDT
Comment on attachment 674435 [details] [diff] [review]
Part 1: Factor out code to add event listeners so that it can be used by Notifications. v3

A bit ugly, but ok.
Comment 74 William Chen [:wchen] (Vacation until 8/25) 2012-10-25 14:25:25 PDT
(In reply to Matthew N. [:MattN] from comment #68)
> Comment on attachment 672546 [details] [diff] [review]
> Part 7: Added permission prompt for desktop notifications on desktop
> platforms. v2
> 
> Review of attachment 672546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You need to enable git diffs so that binary contents are included, otherwise
> I only see the following:
> diff -r b523cd456299 -r ec038535c166
> browser/themes/gnomestripe/notification-16.png
> Binary file browser/themes/gnomestripe/notification-16.png has changed
> diff -r b523cd456299 -r ec038535c166
> browser/themes/gnomestripe/notification-64.png
> Binary file browser/themes/gnomestripe/notification-64.png has changed
> 
> See
> https://developer.mozilla.org/en-US/docs/Installing_Mercurial#Configuration
> for the ini changes.
> 
> Can we use "web-notifications" as the ID everywhere (note the 's')?  Having
> "web" in the name makes it more clear it's related to notifications for
> content whereas "desktop" leads me to believe the opposite. As a bonus, it's
> also shorter.
> 
I've changed the name of the prompt stuff to web-notifications.
I didn't change the name of the permission type because this is supposed to replace the old desktop notification and user preferences should remain the same. Unfortunately, the pageInfo parts strongly couple the name of the permission type to the name of the elements so those have not been changed.
> I would also replace "desktop" with "web" in the interfaces and other
> backend code as well but that's not my call to make.
> 
> The queue of patches currently doesn't compile for me on trunk with a
> clobber so it's hard to test the UI:
> WebIDL.WebIDLError: error: invalid syntax, Notification.webidl line 15:9
>   static readonly attribute NotificationPermission permission;
> 
This is because of bug 763643 (I hear that it's almost ready). When that bug is fixed, I'll post a link to my patch queue repo.
> Are there tests for desktop in the other patches? I didn't see any at quick
> glance.  Could you at least add one for the notification permission
> doorhanger ensuring that notifications don't work until the permissions is
> granted?
> 
There are tests in part 5.2.
> I want to take another look when there is a working patch queue.
> 
> ::: browser/base/content/browser.css
> @@ +478,5 @@
> >    max-width: 280px;
> >  }
> >  
> > +#desktop-notification-notification {
> > +  -moz-binding: url("chrome://global/content/bindings/notification.xml#popup-notification");
> 
> You need to add a line like
> https://hg.mozilla.org/mozilla-central/diff/a038c8c200f5/browser/base/
> content/browser.css for desktop-notification-icon and we should include an
> icon in the patch.
> 
That looks outdated. There is already an icon for the anchor.
> ::: browser/components/nsBrowserGlue.js
> @@ +1707,3 @@
> >  
> > +    // Set up and show the permission request prompt for geolocation.
> > +    function promptGeo() {
> 
> Rather than forking this for promptNotification, I would prefer we refactor
> this to work for both since they only differ by strings, anchor and icon
> (plus the recently added telemetry).
> 
> The signature could be similar to the following:
> _showPrompt(aPermission, aNotificationId, aAllowOnceString,
> aAllowAlwaysString, aNeverAllowString, aMessage, aFileMessage = null)
> 
> @@ +1755,5 @@
> > +                                        mainAction, secondaryActions);
> > +    }
> > +
> > +    // Set up and show the permission request prompt for notifications.
> > +    function promptNotification() {
> 
> promptWebNotifications
> 
> @@ +1761,5 @@
> > +        label: browserBundle.GetStringFromName("desktop-notification.showForSession"),
> > +        accessKey: browserBundle.GetStringFromName("desktop-notification.showForSession.accesskey"),
> > +        callback: function () {
> > +          Services.perms.addFromPrincipal(requestingPrincipal, permissionKey, Ci.nsIPermissionManager.ALLOW_ACTION,
> > +                                          Ci.nsIPermissionManager.EXPIRE_SESSION);
> 
> Please file a follow-up for nsISecurityUITelemetry reporting.
> 
> ::: browser/locales/en-US/chrome/browser/browser.properties
> @@ +277,5 @@
> >  # or use "..." if \u2026 doesn't suit traditions in your locale.
> >  geolocation.learnMore=Learn More…
> >  
> > +desktop-notification.showForSession=Show for this session
> > +desktop-notification.showForSession.accesskey=a
> 
> It is preferred to choose an accesskey which is part of the label.  There is
> no "a" is "Show for this session" so can you find another available key?
> Perhaps consistency with other permission doorhangers is preferred.  You
> should probably check with #a11y.
> 
> @@ +282,5 @@
> > +desktop-notification.alwaysShow=Always Show Notifications
> > +desktop-notification.alwaysShow.accesskey=A
> > +desktop-notification.neverShow=Always Block Notifications
> > +desktop-notification.neverShow.accesskey=N
> > +desktop-notification.showOnSite=Would you like to show notifications on %S?
> 
> s/on/from/ since the notifications will not be shown on the page itself.
> 
Done.

(In reply to Matthew N. [:MattN] from comment #70)
> Comment on attachment 674438 [details] [diff] [review]
> Part 7: Added permission prompt for desktop notifications on desktop
> platforms. v3
> 
> Review of attachment 674438 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/pageinfo/pageInfo.xul
> @@ +330,5 @@
> >            </hbox>
> >          </vbox>
> > +        <vbox class="permission" id="permNotificationRow">
> > +          <label class="permissionLabel" id="permNotificationLabel"
> > +                 value="&permNotification;" control="cookieRadioGroup"/>
> 
> cookieRadioGroup here seems like copypasta to update.

Fixed.
Comment 75 William Chen [:wchen] (Vacation until 8/25) 2012-10-25 14:26:33 PDT
Created attachment 675294 [details] [diff] [review]
Part 7: Added permission prompt for desktop notifications on desktop platforms. v3

dougt: can you review the changes in Notification.cpp?
Comment 76 Doug Turner (:dougt) 2012-10-25 22:51:00 PDT
Comment on attachment 675294 [details] [diff] [review]
Part 7: Added permission prompt for desktop notifications on desktop platforms. v3

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

::: dom/src/notification/Notification.cpp
@@ +140,5 @@
> +  bool isFile;
> +  uri->SchemeIs("file", &isFile);
> +  if (isFile) {
> +    mPermission = NotificationPermission::Granted;
> +  }

why don't file: urls get a prompt?
Comment 77 William Chen [:wchen] (Vacation until 8/25) 2012-10-26 11:37:46 PDT
(In reply to Doug Turner (:dougt) from comment #76)
> Comment on attachment 675294 [details] [diff] [review]
> Part 7: Added permission prompt for desktop notifications on desktop
> platforms. v3
> 
> Review of attachment 675294 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/src/notification/Notification.cpp
> @@ +140,5 @@
> > +  bool isFile;
> > +  uri->SchemeIs("file", &isFile);
> > +  if (isFile) {
> > +    mPermission = NotificationPermission::Granted;
> > +  }
> 
> why don't file: urls get a prompt?
The permission manager doesn't support permissions for file URIs and allowing files to show notifications doesn't seem like a big problem.
Comment 78 William Chen [:wchen] (Vacation until 8/25) 2012-10-26 15:43:43 PDT
(In reply to Olli Pettay [:smaug] from comment #71)
> Comment on attachment 672543 [details] [diff] [review]
> Part 5.2: Implement Notification API. v2
> 
> 
> >+class NotificationPermissionRequest : public nsIContentPermissionRequest,
> >+                                      public PCOMContentPermissionRequestChild,
> >+                                      public nsIRunnable
> >+{
> >+public:
> >+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> >+  NS_DECL_NSICONTENTPERMISSIONREQUEST
> >+  NS_DECL_NSIRUNNABLE
> >+  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_AMBIGUOUS(NotificationPermissionRequest,
> >+                                                         nsIContentPermissionRequest)
> >+
> >+  NotificationPermissionRequest(nsIPrincipal* aPrincipal, nsIDOMWindow* aWindow,
> >+                                JSContext* aCx, JSObject* aFunObj)
> >+    : mPrincipal(aPrincipal), mWindow(aWindow),
> >+      mPermission(NotificationPermission::Default),
> >+      mFunObj(aFunObj)
> >+  {
> >+    mCx = GetScriptContextFromJSContext(aCx);
> >+    MOZ_ASSERT(mCx, "Context should not be null.");
> >+    NS_HOLD_JS_OBJECTS(this, NotificationPermissionRequest);
> >+  }
> >+
> >+  virtual ~NotificationPermissionRequest()
> >+  {
> >+    NS_DROP_JS_OBJECTS(this, NotificationPermissionRequest);
> >+  }
> >+
> >+  bool Recv__delete__(const bool& aAllow);
> >+  void IPDLRelease() { Release(); }
> >+
> >+protected:
> >+  nsresult CallCallback();
> >+  nsCOMPtr<nsIPrincipal> mPrincipal;
> >+  nsCOMPtr<nsIDOMWindow> mWindow;
> >+  NotificationPermission mPermission;
> >+  nsCOMPtr<nsIScriptContext> mCx;
> >+  JSObject* mFunObj;
> >+};
> This is horrible
> 
:(
> 
> 
> >+
> >+class NotificationObserver : public nsIObserver
> >+{
> >+public:
> >+  NS_DECL_ISUPPORTS
> >+  NS_DECL_NSIOBSERVER
> >+
> >+  NotificationObserver(Notification* aNotification)
> >+    : mNotification(aNotification) {}
> >+
> >+  virtual ~NotificationObserver() {}
> >+
> >+protected:
> >+  nsRefPtr<Notification> mNotification;
> >+};
> 
> 
> So this is keeping Notification alive.
> Is it guaranteed that the NotificationObserver object is short living object?
> I recall that geolocation had some problems where it ended up keeping stuff
> alive
> too long.
> 
NotificationObserver should only be live as long as the notification is shown.
> 
> >+NS_IMPL_CYCLE_COLLECTION_CLASS(NotificationPermissionRequest)
> >+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(NotificationPermissionRequest)
> >+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mWindow)
> >+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mCx)
> >+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> You should NS_DROP_JS_OBJECTS here.
> 
> >+NotificationPermissionRequest::Cancel()
> >+{
> >+  mPermission = NotificationPermission::Denied;
> >+  nsCOMPtr<nsIRunnable> callbackRunnable = NS_NewRunnableMethod(this,
> >+      &NotificationPermissionRequest::CallCallback);
> 2 space indentation
> 
> >+  NS_DispatchToMainThread(callbackRunnable);
> >+  return NS_OK;
> >+}
> >+
> >+NS_IMETHODIMP
> >+NotificationPermissionRequest::Allow()
> >+{
> >+  mPermission = NotificationPermission::Granted;
> >+  nsCOMPtr<nsIRunnable> callbackRunnable = NS_NewRunnableMethod(this,
> >+      &NotificationPermissionRequest::CallCallback);
> ditto
> 
Done.
> >+NotificationPermissionRequest::CallCallback()
> >+{
> It is absolutely horrible to need to use JS API here.
> Need to get Bug 779048 fixed asap.
> 
It would be very nice if I didn't have to use the JS API.
> 
> >+  JSContext* cx = mCx->GetNativeContext();
> cx can be null here, so you'd crash below.
> Also, it would be better to take inner window as member variable and 
> before using cx from the inner window, check that the inner window is still
> the current inner window.
> 
Done.
> 
> >+  // Get the string value of the permission (eg. default, granted or denied).
> >+  EnumEntry enumEntry =
> >+      NotificationPermissionValues::strings[uint32_t(mPermission)];
> 2 space indentation.
> 
> >+Notification::Notification(nsISupports* aWindow, const nsAString& aTitle,
> >+                           const nsAString& aBody, NotificationDirection aDir,
> >+                           const nsAString& aLang, const nsAString& aTag,
> >+                           const nsAString& aIconUrl)
> >+  : mTitle(aTitle), mBody(aBody), mDir(aDir), mLang(aLang),
> >+    mTag(aTag), mIconUrl(aIconUrl), mIsClosed(false)
> >+{
> >+  SetIsDOMBinding();
> >+  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);
> >+  MOZ_ASSERT(window, "Window should not be null.");
> >+  BindToOwner(window);
> >+  mDocument = GetOwner()->GetDoc();
> Why you need mDocument? You can always get it from GetOwner()
> 
Fixed.
> 
> >+Notification::ShowInternal()
> >+{
> >+  nsCOMPtr<nsIAlertsService> alertService =
> >+      do_GetService(NS_ALERTSERVICE_CONTRACTID);
> >+
> >+  if (GetPermission(GetOwner()) != NotificationPermission::Granted ||
> >+      !alertService) {
> >+    // We do not have permission to show a notification or alert service
> >+    // is not available.
> >+    return DispatchEventInternal(NS_LITERAL_STRING("error"));
> Notification inherits nsDOMEventTargetHelper which has
> DispatchTrustedEvent. Use that and remove the implementation of
> DispatchEventInternal
> 
Done.
> >+Notification::Close()
> >+{
> >+  // Queue a task to close the notification.
> >+  nsCOMPtr<nsIRunnable> showNotificationTask =
> >+      new NotificationTask(this, NotificationTask::eClose);
> 2 space indentation
> 
> >+
> >+  nsCOMPtr<nsIDOMWindow> mWindow;
> You don't need this. GetOwner() returns the window.

Done.
Comment 79 William Chen [:wchen] (Vacation until 8/25) 2012-10-26 15:44:27 PDT
Created attachment 675715 [details] [diff] [review]
Part 5.2: Implement Notification API. v3
Comment 80 Mounir Lamouri (:mounir) 2012-10-28 03:46:00 PDT
(In reply to William Chen [:wchen] from comment #77)
> (In reply to Doug Turner (:dougt) from comment #76)
> > Comment on attachment 675294 [details] [diff] [review]
> > Part 7: Added permission prompt for desktop notifications on desktop
> > platforms. v3
> > 
> > Review of attachment 675294 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/src/notification/Notification.cpp
> > @@ +140,5 @@
> > > +  bool isFile;
> > > +  uri->SchemeIs("file", &isFile);
> > > +  if (isFile) {
> > > +    mPermission = NotificationPermission::Granted;
> > > +  }
> > 
> > why don't file: urls get a prompt?
> The permission manager doesn't support permissions for file URIs and
> allowing files to show notifications doesn't seem like a big problem.

I think it would be better to rely on the permission manager even for files. Files aren't important enough to have this workaround and we should probably fix the permission manager instead.
Comment 81 Matthew N. [:MattN] 2012-10-29 18:31:17 PDT
Comment on attachment 675294 [details] [diff] [review]
Part 7: Added permission prompt for desktop notifications on desktop platforms. v3

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

Looks pretty good but I have some questions.

(In reply to William Chen [:wchen] from comment #74)
> > Can we use "web-notifications" as the ID everywhere (note the 's')?  Having
> > "web" in the name makes it more clear it's related to notifications for
> > content whereas "desktop" leads me to believe the opposite. As a bonus, it's
> > also shorter.
> > 
> I've changed the name of the prompt stuff to web-notifications.
> I didn't change the name of the permission type because this is supposed to
> replace the old desktop notification and user preferences should remain the
> same. Unfortunately, the pageInfo parts strongly couple the name of the
> permission type to the name of the elements so those have not been changed.

I didn't realize we already shipped something similar and it's surprising that we called in desktop notifications since it was on mobile. I would prefer to remove all references to "desktop" and that could mean migrating the old permissions or ignoring them but I'm not sure others agree.

Doug, what do you think about cleaning up the legacy code references to "desktop" for the notification feature? It shouldn't be too hard to search and replace over the patches and I think now is a good time to do it since we're making breaking changes.

::: browser/components/nsBrowserGlue.js
@@ +1681,5 @@
> +   */
> +  _showPrompt: function CPP_showPrompt(aRequest, aMessage, aPermission, aIsAllowForSession,
> +                                       aAllowString, aAllowAccessKey,
> +                                       aAlwaysAllowString, aAlwaysAllowAccessKey,
> +                                       aNeverAllowString, aNeverAllowAccessKey,

I guess I should have made it more clear that I was leaving the accesskeys out of the argument list in my example signature since I meant to pass the string IDs rather than the strings themselves. The accesskey string could be retrieved by appending ".accesskey" to the button label.  Having the string lookups in this function would also reduce the string fetching duplication between _promptGeo and _promptWebNotifications.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +278,5 @@
>  geolocation.learnMore=Learn More…
>  
> +web-notifications.showForSession=Show for this session
> +web-notifications.showForSession.accesskey=s
> +web-notifications.alwaysShow=Always Show Notifications

Nit: We normally use camelCase over hyphens in string names so make the prefix "webNotifications."

::: browser/locales/en-US/chrome/browser/pageInfo.dtd
@@ +53,5 @@
>  <!ENTITY  permTab               "Permissions">
>  <!ENTITY  permTab.accesskey     "P">
>  <!ENTITY  permUseDefault        "Use Default">
>  <!ENTITY  permAskAlways         "Always ask">
> +<!ENTITY  permAsk               "Ask">

Why isn't permAskAlways suitable? How are notification permissions different than others that use "Always ask".  Is it only asking once per session when set to "Ask"?  I want to avoid adding more subtle differences in terminology to the permissions pane.

::: browser/themes/winstripe/jar.mn
@@ +39,5 @@
>          skin/classic/browser/menu-forward.png
>          skin/classic/browser/monitor.png
>          skin/classic/browser/monitor_16-10.png
> +        skin/classic/browser/notification-16.png
> +        skin/classic/browser/notification-64.png

Can you double-check that the images have been optimized? Running ImageOptim on  notification-64.png showed a 3% reduction.
Comment 82 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-10-30 08:20:40 PDT
Comment on attachment 675715 [details] [diff] [review]
Part 5.2: Implement Notification API. v3


>+class NotificationPermissionRequest : public nsIContentPermissionRequest,
>+                                      public PCOMContentPermissionRequestChild,
>+                                      public nsIRunnable
>+{
>+public:
>+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>+  NS_DECL_NSICONTENTPERMISSIONREQUEST
>+  NS_DECL_NSIRUNNABLE
>+  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_AMBIGUOUS(NotificationPermissionRequest,
>+                                                         nsIContentPermissionRequest)
>+
>+  NotificationPermissionRequest(nsIPrincipal* aPrincipal, nsPIDOMWindow* aWindow,
>+                                JSContext* aCx, JSObject* aFunObj)
>+    : mPrincipal(aPrincipal), mWindow(aWindow),
>+      mPermission(NotificationPermission::Default),
>+      mFunObj(aFunObj)
>+  {
>+    mCx = GetScriptContextFromJSContext(aCx);
>+    MOZ_ASSERT(mCx, "Context should not be null.");
>+    NS_HOLD_JS_OBJECTS(this, NotificationPermissionRequest);
>+    mInnerWindow = mWindow->GetCurrentInnerWindow();
>+  }
>+
>+  virtual ~NotificationPermissionRequest()
>+  {
>+    if (mFunObj) {
>+      NS_DROP_JS_OBJECTS(this, NotificationPermissionRequest);
>+    }
>+  }
>+
>+  bool Recv__delete__(const bool& aAllow);
>+  void IPDLRelease() { Release(); }
>+
>+protected:
>+  nsresult CallCallback();
>+  nsCOMPtr<nsIPrincipal> mPrincipal;
>+  nsCOMPtr<nsPIDOMWindow> mWindow;
>+  nsPIDOMWindow* mInnerWindow; // Weak
Why is this safe? Because it is used only in != check ?
Well, what if window is create using the exact same memory location



>+NotificationObserver::Observe(nsISupports* aSubject, const char* aTopic,
>+                              const PRUnichar* aData)
>+{
>+  nsresult rv;
>+  if (!strcmp("alertclickcallback", aTopic)) {
>+    rv = mNotification->DispatchTrustedEvent(NS_LITERAL_STRING("click"));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  } else if (!strcmp("alertfinished", aTopic)) {
>+    mNotification->mIsClosed = true;
>+    rv = mNotification->DispatchTrustedEvent(NS_LITERAL_STRING("close"));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  } else if (!strcmp("alertshow", aTopic)) {
>+    rv = mNotification->DispatchTrustedEvent(NS_LITERAL_STRING("show"));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
Since no one cares about the return value in this case, I'd just remove nsresult rv and its use.

Where is the tag handled? This part http://dvcs.w3.org/hg/notifications/raw-file/tip/Overview.html#show-steps
Comment 83 Evgueni Naverniouk 2012-10-30 08:52:52 PDT
Even though it's not in the W3 standard, can we consider adding support for "close notification after x seconds" so that the developers don't need to constantly use setTimeout to achieve the desired effect.

In real world cases web notifications are almost always going to need to auto-close after a period of time.
Comment 84 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-10-30 08:58:28 PDT
Please send feature requests to W3C (public-web-notification@w3.org).
Comment 85 Lucas Adamski [:ladamski] 2012-11-08 13:44:47 PST
Comment on attachment 672542 [details] [diff] [review]
Part 5.1: Add function to remote test for permission in nsIPermissionManager. v1

Double-checked with Jonas here and we should be fine, since this API is not exposed to JS.
Comment 86 Anne (:annevk) 2012-11-11 01:21:14 PST
FWIW, if http://notifications.spec.whatwg.org/ is incorrect or unclear please do let me know. Preferably by emailing the WHATWG list. Happy to clarify and/or fix problems.
Comment 87 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-11-13 03:20:38 PST
WebIDL callbacks are now supported, so you can simplify the patch.
Comment 88 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-11-16 16:08:44 PST
Comment on attachment 672542 [details] [diff] [review]
Part 5.1: Add function to remote test for permission in nsIPermissionManager. v1

Sorry about slow review :(
Comment 89 William Chen [:wchen] (Vacation until 8/25) 2012-11-27 00:33:20 PST
(In reply to Matthew N. [:MattN] from comment #81)
> Comment on attachment 675294 [details] [diff] [review]
> Part 7: Added permission prompt for desktop notifications on desktop
> platforms. v3
> 
> Review of attachment 675294 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks pretty good but I have some questions.
> 
> (In reply to William Chen [:wchen] from comment #74)
> > > Can we use "web-notifications" as the ID everywhere (note the 's')?  Having
> > > "web" in the name makes it more clear it's related to notifications for
> > > content whereas "desktop" leads me to believe the opposite. As a bonus, it's
> > > also shorter.
> > > 
> > I've changed the name of the prompt stuff to web-notifications.
> > I didn't change the name of the permission type because this is supposed to
> > replace the old desktop notification and user preferences should remain the
> > same. Unfortunately, the pageInfo parts strongly couple the name of the
> > permission type to the name of the elements so those have not been changed.
> 
> I didn't realize we already shipped something similar and it's surprising
> that we called in desktop notifications since it was on mobile. I would
> prefer to remove all references to "desktop" and that could mean migrating
> the old permissions or ignoring them but I'm not sure others agree.
> 
> Doug, what do you think about cleaning up the legacy code references to
> "desktop" for the notification feature? It shouldn't be too hard to search
> and replace over the patches and I think now is a good time to do it since
> we're making breaking changes.
> 
> ::: browser/components/nsBrowserGlue.js
> @@ +1681,5 @@
> > +   */
> > +  _showPrompt: function CPP_showPrompt(aRequest, aMessage, aPermission, aIsAllowForSession,
> > +                                       aAllowString, aAllowAccessKey,
> > +                                       aAlwaysAllowString, aAlwaysAllowAccessKey,
> > +                                       aNeverAllowString, aNeverAllowAccessKey,
> 
> I guess I should have made it more clear that I was leaving the accesskeys
> out of the argument list in my example signature since I meant to pass the
> string IDs rather than the strings themselves. The accesskey string could be
> retrieved by appending ".accesskey" to the button label.  Having the string
> lookups in this function would also reduce the string fetching duplication
> between _promptGeo and _promptWebNotifications.
> 
> ::: browser/locales/en-US/chrome/browser/browser.properties
> @@ +278,5 @@
> >  geolocation.learnMore=Learn More…
> >  
> > +web-notifications.showForSession=Show for this session
> > +web-notifications.showForSession.accesskey=s
> > +web-notifications.alwaysShow=Always Show Notifications
> 
> Nit: We normally use camelCase over hyphens in string names so make the
> prefix "webNotifications."
> 
Done.
> ::: browser/locales/en-US/chrome/browser/pageInfo.dtd
> @@ +53,5 @@
> >  <!ENTITY  permTab               "Permissions">
> >  <!ENTITY  permTab.accesskey     "P">
> >  <!ENTITY  permUseDefault        "Use Default">
> >  <!ENTITY  permAskAlways         "Always ask">
> > +<!ENTITY  permAsk               "Ask">
> 
> Why isn't permAskAlways suitable? How are notification permissions different
> than others that use "Always ask".  Is it only asking once per session when
> set to "Ask"?  I want to avoid adding more subtle differences in terminology
> to the permissions pane.
> 
Yes, that's why.
> ::: browser/themes/winstripe/jar.mn
> @@ +39,5 @@
> >          skin/classic/browser/menu-forward.png
> >          skin/classic/browser/monitor.png
> >          skin/classic/browser/monitor_16-10.png
> > +        skin/classic/browser/notification-16.png
> > +        skin/classic/browser/notification-64.png
> 
> Can you double-check that the images have been optimized? Running ImageOptim
> on  notification-64.png showed a 3% reduction.
These have been optimized.

Now that there are no more dependencies, here is a patch repository that should apply cleanly on top of the latest m-c as of today. http://hg.mozilla.org/users/wchen_mozilla.com/notifications/
Comment 90 William Chen [:wchen] (Vacation until 8/25) 2012-11-27 00:34:42 PST
Created attachment 685511 [details] [diff] [review]
Added permission prompt for desktop notifications on desktop platforms. v4
Comment 91 William Chen [:wchen] (Vacation until 8/25) 2012-11-27 00:36:33 PST
Created attachment 685513 [details] [diff] [review]
Part 5.2: Implement Notification API. v4

Now using webidl callbacks, no more JS API!
Comment 92 William Chen [:wchen] (Vacation until 8/25) 2012-11-27 00:44:45 PST
Created attachment 685515 [details] [diff] [review]
Part 9: Remove native notification systems on desktop platforms. v2

I updated the patch so that it does not remove native notifications for mac, but I've also disabled web notifications on mac. The plan is to have an implementation of the alerts service API for the notification center that can be used by web notifications soon after this bug lands.
Comment 93 Florian Bender 2012-11-27 00:53:21 PST
Sorry for Bugspam, but that's what I posted in another Bug just now: 
(In reply to Florian Bender from Bug 629280 comment #11)
> Update for Mac: We might want to use Notification Center here, or not,
> depending on how "good" we can interact with it (maybe just use this if Fx
> is not focused and only for the first notification?). Growl has been removed
> in current versions (I don't know which Train, but it is definitely gone
> from Central, I think even Fx 17 Release).

To add to this (sorry, I haven't read everything in this bug): There should be Fx (XUL) Notifications, and NC should alert those who have 10.8 if Fx is in the background. 


It's quite confusing what Bug handles what. If Bug 629280 is only for UX/UI discussion, it should be closed now as this bug implements a UX/UI (at least part of it?). There is also Bug 594543 which covers implementing this API for Desktop Firefox – which is also done in this bug according to the patch comments, so Bug 594543 should be closed, too, with reference to this bug.
Comment 94 :Ms2ger (⌚ UTC+1/+2) 2012-11-27 00:55:18 PST
Comment on attachment 685513 [details] [diff] [review]
Part 5.2: Implement Notification API. v4

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

::: dom/src/notification/Notification.cpp
@@ +91,5 @@
> +} // namespace dom
> +} // namespace mozilla
> +
> +using mozilla::ErrorResult;
> +using namespace mozilla::dom;

Just wrap the entire file in namespace {}

@@ +122,5 @@
> +    // Corresponding release occurs in DeallocPContentPermissionRequest.
> +    AddRef();
> +
> +    nsCString type = NS_LITERAL_CSTRING("desktop-notification");
> +    nsCString access = NS_LITERAL_CSTRING("unused");

NS_NAMED_LITERAL_CSTRING

@@ +190,5 @@
> +
> +NS_IMETHODIMP
> +NotificationPermissionRequest::GetAccess(nsACString& aAccess)
> +{
> +  aAccess = "unused";

AssignLiteral

@@ +197,5 @@
> +
> +NS_IMETHODIMP
> +NotificationPermissionRequest::GetType(nsACString& aType)
> +{
> +  aType = "desktop-notification";

Ditto

@@ +218,5 @@
> +NS_IMETHODIMP
> +NotificationTask::Run()
> +{
> +  nsresult rv = NS_OK;
> +  switch(mAction) {

switch (

@@ +221,5 @@
> +  nsresult rv = NS_OK;
> +  switch(mAction) {
> +  case eShow:
> +    rv = mNotification->ShowInternal();
> +    break;

Just 'return Foo();' and add a default: MOZ_NOT_REACHED() if you need to.

@@ +250,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_INHERITED_0(Notification, nsDOMEventTargetHelper)
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(Notification)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsDOMEventTargetHelper)

Do you need this?

@@ +265,5 @@
> +    mTag(aTag), mIconUrl(aIconUrl), mIsClosed(false)
> +{
> +  SetIsDOMBinding();
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);
> +  MOZ_ASSERT(window, "Window should not be null.");

I'm not convinced you can do that; in any case, the QI should be in Constructor()

@@ +292,5 @@
> +  }
> +
> +  if (aOptions.icon.WasPassed()) {
> +    iconUrl = aOptions.icon.Value();
> +  }

Should we be able to distinguish "not passed" and "empty string passed" for those? If not, just add '= ""' to the IDL

@@ +356,5 @@
> +Notification::RequestPermission(nsISupports* aGlobal,
> +                                NotificationPermissionCallback& aCallback)
> +{
> +  // Get principal from global to make permission request for notifications.
> +  nsCOMPtr<nsPIDOMWindow> window(do_QueryInterface(aGlobal));

window = do_...

@@ +370,5 @@
> +NotificationPermission
> +Notification::Permission(nsISupports* aGlobal)
> +{
> +  // Get principal from global to check permission for notifications.
> +  nsCOMPtr<nsPIDOMWindow> window(do_QueryInterface(aGlobal));

Ditto

@@ +432,5 @@
> +      do_GetService(NS_ALERTSERVICE_CONTRACTID);
> +
> +    if (alertService) {
> +      nsString alertName;
> +      rv = GetAlertName(alertName);

Declare rv here

::: dom/src/notification/Notification.h
@@ +57,5 @@
> +  JSObject* WrapObject(JSContext* aCx, JSObject* aScope,
> +                       bool* aTriedToWrap)
> +  {
> +    return mozilla::dom::NotificationBinding::Wrap(aCx, aScope, this,
> +                                                   aTriedToWrap);

Put this in the .cpp and add 'virtual' and 'MOZ_OVERRIDE' here.

::: dom/webidl/Notification.webidl
@@ +1,3 @@
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* 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,

"file," at the start of the next line

@@ +3,5 @@
> + * 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/.
> + *
> + * The origin of this IDL file is
> + * http://www.w3.org/TR/2011/WD-notifications-20110301/

http://notifications.spec.whatwg.org/

@@ +6,5 @@
> + * The origin of this IDL file is
> + * http://www.w3.org/TR/2011/WD-notifications-20110301/
> + *
> + * Copyright © 2012 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C
> + * liability, trademark and document use rules apply.

CC0 To the extent possible under law, the editors have waived all copyright and related or neighboring rights to this work.
Comment 95 William Chen [:wchen] (Vacation until 8/25) 2012-11-30 11:26:35 PST
Created attachment 687200 [details] [diff] [review]
Patch with all changes

By request, here is a single patch with all the changes.
Comment 96 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-11-30 13:34:26 PST
Comment on attachment 685513 [details] [diff] [review]
Part 5.2: Implement Notification API. v4

>+
>+NS_IMPL_CYCLE_COLLECTION_INHERITED_0(Notification, nsDOMEventTargetHelper)
>+
>+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(Notification)
>+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsDOMEventTargetHelper)
>+NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper)
>+
>+NS_IMPL_ADDREF_INHERITED(Notification, nsDOMEventTargetHelper)
>+NS_IMPL_RELEASE_INHERITED(Notification, nsDOMEventTargetHelper)
This stuff shouldn't be needed. Nor 
+  NS_DECL_ISUPPORTS_INHERITED
+  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(Notification,
+                                           nsDOMEventTargetHelper)
in the header. You aren't implementing new xpcom interfaces nor adding anything new to
cycle collection.


>+  if (Permission(GetOwner()) != NotificationPermission::Granted ||
>+    !alertService) {
missing two spaces before !alertService

>+    // We do not have permission to show a notification or alert service
>+    // is not available.
>+    return DispatchTrustedEvent(NS_LITERAL_STRING("error"));
>+  }
>+
>+  nsresult rv;
>+  nsAutoString absoluteUrl;
>+  if (mIconUrl.Length() > 0) {
>+    // Resolve image URL against document base URI.
>+    nsCOMPtr<nsIURI> baseUri = GetOwner()->GetDoc()->GetBaseURI();
Get* methods may return null. Add some checks.
And use GetExtantDoc(), not GetDoc()


>+    nsCOMPtr<nsIURI> srcUri;
>+    rv = nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(srcUri),
>+                                                   mIconUrl,
>+                                                   GetOwner()->GetDoc(),
GetExtantDoc


>+
>+  // In the case of IPC, the parent process uses the cookie to map to
>+  // nsIObserver. Thus the cookie must be unique to differentiate observers.
>+  nsString uniqueCookie = NS_LITERAL_STRING("notification:");
>+  uniqueCookie.AppendInt(sCount++);
Does this work also with multiple child processes?


>+void
>+Notification::RequestPermission(nsISupports* aGlobal,
>+                                NotificationPermissionCallback& aCallback)
>+{
>+  // Get principal from global to make permission request for notifications.
>+  nsCOMPtr<nsPIDOMWindow> window(do_QueryInterface(aGlobal));
>+  nsCOMPtr<nsIDocument> doc = window->GetDoc();
>+  nsCOMPtr<nsIPrincipal> principal = doc->NodePrincipal();

QI aGlobal to nsIScriptObjectPrincipal and call GetPrincipal()


>+Notification::Permission(nsISupports* aGlobal)
>+{
>+  // Get principal from global to check permission for notifications.
>+  nsCOMPtr<nsPIDOMWindow> window(do_QueryInterface(aGlobal));
>+  nsCOMPtr<nsIDocument> doc = window->GetDoc();
>+  nsCOMPtr<nsIPrincipal> principal = doc->NodePrincipal();
ditto



>+Notification::PrefEnabled()
>+{
>+#ifdef XP_MACOSX
>+  return false;
>+#else
>+  return true;
>+#endif
>+}
what is this? Not supported on osx yet?
Do we have followup bug filed to fix this issue.
Actually, I think we should not enable this in release builds before also OSX
is supported.


>+Notification::CloseInternal()
>+{
>+  nsresult rv;
>+
>+  if (!mIsClosed) {
>+    nsCOMPtr<nsIAlertsService> alertService =
>+      do_GetService(NS_ALERTSERVICE_CONTRACTID);
>+
>+    if (alertService) {
>+      nsString alertName;
>+      rv = GetAlertName(alertName);
define nsresult rv here, not at the beginning of the method.


>+Notification::GetAlertName(nsString& aAlertName)
>+{
>+  // Get the notification name that is unique per origin + tag.
>+
>+  // The name of the alert is of the form origin#tag
>+  nsresult rv = nsContentUtils::GetUTFOrigin(
>+    GetOwner()->GetDoc()->NodePrincipal(), aAlertName);
Null checks for Get* and GetExtandDoc()

>+class Notification : public nsDOMEventTargetHelper
>+{
>+  friend class NotificationTask;
>+  friend class NotificationPermissionRequest;
>+  friend class NotificationObserver;
>+public:
>+  NS_DECL_ISUPPORTS_INHERITED
>+  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(Notification,
>+                                           nsDOMEventTargetHelper)
As mentioned before, these two NS_DECL_ shouldn't be needed.

>+  static const nsString DirectionToString(NotificationDirection aDirection) {
{ goes to the next line


Could I get still a new patch. Looking good in general, but I need to look at the full patch too.
Comment 97 Doug Turner (:dougt) 2012-11-30 14:15:25 PST
Comment on attachment 685515 [details] [diff] [review]
Part 9: Remove native notification systems on desktop platforms. v2

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

This removes gnome native notifications (which may already be broken on at least ubuntu).  The code change lgmt, but since this does have some impact be sure to send mail to the platform newsgroup before landing.
Comment 98 Marco Castelluccio [:marco] (PTO until August 24/25) 2012-12-01 09:34:49 PST
> This removes gnome native notifications (which may already be broken on at
> least ubuntu).  The code change lgmt, but since this does have some impact
> be sure to send mail to the platform newsgroup before landing.

I don't get why we should remove Gnome native notifications. There were many problems with libnotify statically linked before, but we've fixed them by dinamically load it.
Comment 99 Mike Hommey [:glandium] 2012-12-01 10:56:51 PST
Yeah, notification just works with libnotify. The problem is however that it doesn't has enough flexibility for the notification API spec aiui. That being said, why remove it for *all* notifications?
Comment 100 Matthew N. [:MattN] 2012-12-05 14:56:13 PST
Comment on attachment 685511 [details] [diff] [review]
Added permission prompt for desktop notifications on desktop platforms. v4

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

These 2 comments weren't addressed yet:

(In reply to William Chen [:wchen] from comment #74)
> > Can we use "web-notifications" as the ID everywhere (note the 's')?  Having
> > "web" in the name makes it more clear it's related to notifications for
> > content whereas "desktop" leads me to believe the opposite. As a bonus, it's
> > also shorter.
> > 
> I've changed the name of the prompt stuff to web-notifications.
> I didn't change the name of the permission type because this is supposed to
> replace the old desktop notification and user preferences should remain the
> same. Unfortunately, the pageInfo parts strongly couple the name of the
> permission type to the name of the elements so those have not been changed.

I didn't realize we already shipped something similar and it's surprising that we called in desktop notifications since it was on mobile. I would prefer to remove all references to "desktop" and that could mean migrating the old permissions or ignoring them but I'm not sure others agree.

Doug, what do you think about cleaning up the legacy code references to "desktop" for the notification feature? It shouldn't be too hard to search and replace over the patches and I think now is a good time to do it since we're making breaking changes.

::: browser/locales/en-US/chrome/browser/pageInfo.dtd
@@ +53,5 @@
>  <!ENTITY  permTab               "Permissions">
>  <!ENTITY  permTab.accesskey     "P">
>  <!ENTITY  permUseDefault        "Use Default">
>  <!ENTITY  permAskAlways         "Always ask">
> +<!ENTITY  permAsk               "Ask">

Why isn't permAskAlways suitable? How are notification permissions different than others that use "Always ask".  Is it only asking once per session when set to "Ask"?  I want to avoid adding more subtle differences in terminology to the permissions pane.

::: browser/components/nsBrowserGlue.js
@@ +1799,5 @@
> +    var message = browserBundle.formatStringFromName("webNotifications.showFromSite",
> +                                                     [requestingURI.host], 1);
> +    var allowString = "webNotifications.showForSession";
> +    var alwaysAllowString = "webNotifications.alwaysShow";
> +    var neverAllowString = "webNotifications.neverShow";

No need for these 3 temporary variables anymore.
Comment 101 Matthew N. [:MattN] 2012-12-05 19:45:57 PST
What version of the spec are we implementing? Can I get a link? The current patch doesn't match the latest WhatWG version:

[Constructor(DOMString title, optional NotificationOptions options)]
interface Notification : EventTarget…
Comment 102 Dão Gottwald [:dao] 2012-12-07 04:00:08 PST
Comment on attachment 674432 [details] [diff] [review]
Part 8: Updated existing XUL alerts to implement the Notification API. v2

Redirecting review request to people who might be good reviewers for this, although I don't know who's available right now.
Comment 103 Neil Deakin 2012-12-07 10:52:24 PST
Comment on attachment 674432 [details] [diff] [review]
Part 8: Updated existing XUL alerts to implement the Notification API. v2

+      // We don't clear positions if the alert is being replaced so that
+      // the replacement keeps the same position.
+
+      // Clear all unused positions at the end.
+      uint32_t numToRemove = 0;
+      for (uint32_t i = windowPositions.Length(); i > 0; i--) {
+        nsCOMPtr<nsIDOMWindow> window = namedWindows.Get(windowPositions[i - 1]);
+        if (!window) {
+          // Window has been closed, position needs to be cleared.
+          numToRemove++;
+        } else {
+          break;
+        }
+      }
+
+      if (numToRemove > 0) {
+        windowPositions.TruncateLength(windowPositions.Length() - numToRemove);
+      }

I'm not sure I understand this. I think what you're trying to do is:
- if the last alert is closed, remove it from the positions list.
- if another alert is closed, do nothing.

This is so the position for an alert is kept at the same value no matter what opens and closes?

This needs much more detailed comments.

Also you can just write:

+      for (uint32_t i = windowPositions.Length() - 1; i >= 0; i--) {

instead of subtracting one every iteration.
 

+class nsXULAlerts : public nsISupports {

I don't see why this needs to implement nsISupports. You don't addref it anywhere either that I can see. I don't even see why it needs to be a different object when you can just put its methods directly in nsAlertsService, no?

+  // Adjust for the position index.
+  y += (gOrigin & NS_ALERT_TOP ? gPositionIndex : -gPositionIndex) * window.outerHeight;

Is every alert exactly the same size?

   // Offset the alert by 10 pixels from the edge of the screen
-  y += gOrigin & NS_ALERT_TOP ? 10 : -10;
+  y += (gOrigin & NS_ALERT_TOP ? 10 : -10) * (gPositionIndex + 1);
   x += gOrigin & NS_ALERT_LEFT ? 10 : -10;
 
Why does this need to be applied multiple times?
 
+    <vbox class="alertCloseBox">
+      <toolbarbutton class="alertCloseButton"
+                     label="&closeAlert.tooltip;"
+                     onclick="window.close()"/>

You use the label attribute but you seem to want to use tooltiptext.

Please use 'oncommand' not 'onclick'.
Comment 104 :Ms2ger (⌚ UTC+1/+2) 2012-12-07 11:23:31 PST
(In reply to Neil Deakin from comment #103)
> Comment on attachment 674432 [details] [diff] [review]
> Part 8: Updated existing XUL alerts to implement the Notification API. v2
> This needs much more detailed comments.
> 
> Also you can just write:
> 
> +      for (uint32_t i = windowPositions.Length() - 1; i >= 0; i--) {
> 
> instead of subtracting one every iteration.

Oh no you can't! i >= 0 is always true for an unsigned i.
Comment 105 neil@parkwaycc.co.uk 2012-12-09 11:16:04 PST
Comment on attachment 674432 [details] [diff] [review]
Part 8: Updated existing XUL alerts to implement the Notification API. v2

>+  nsXULAlerts.cpp \
I agree with Enn, I don't see the point of splitting this into a new file. (I agree with some of Enn's other comments to but I tried to avoid repeating them.)

>+    nsInterfaceHashtable<nsStringHashKey, nsIDOMWindow>& namedWindows =
>+        mXULAlerts->mNamedWindows;
>+    nsTArray<nsString>& windowPositions = mXULAlerts->mWindowPositions;
>+
>+    namedWindows.Remove(mAlertName);
>+
>+    if (!mXULAlerts->mIsReplacing) {
If you're replacing, then you probably don't need to remove the named window either.

>+      // Clear all unused positions at the end.
And you only need to do this if the last position is the one you just removed.

>+      uint32_t numToRemove = 0;
>+      for (uint32_t i = windowPositions.Length(); i > 0; i--) {
>+        nsCOMPtr<nsIDOMWindow> window = namedWindows.Get(windowPositions[i - 1]);
>+        if (!window) {
>+          // Window has been closed, position needs to be cleared.
>+          numToRemove++;
>+        } else {
>+          break;
>+        }
>+      }
>+
>+      if (numToRemove > 0) {
>+        windowPositions.TruncateLength(windowPositions.Length() - numToRemove);
>+      }
You shouldn't use the 1-argument form of Get to test for existence. Use the 2-argument form with a nullptr 2nd argument, or because you're not storing nullptr entries, you can alternatively use GetWeak instead.
You shouldn't mix else with break. You could write if (window) break; numToRemove++; or you could write something like this:

uint32_t l = windowPositions.Length();
while (l > 0 && !namedWindows.GetWeak(windowPositions[l - 1]))
  l--;
windowPositions.TruncateLength(l);

>+    CloseAlert(aAlertName);
Could just write previousAlert->Close();

>+  nsCOMPtr<nsIDOMWindow> previousAlert = mNamedWindows.Get(aAlertName);
aAlertName is optional, so you probably want to replace empty strings with some sort of unique index, otherwise you won't be able to track them internally.

>+  nsCOMPtr<nsISupportsPRInt32> positionIndex (do_CreateInstance(NS_SUPPORTS_PRINT32_CONTRACTID));
>+  NS_ENSURE_TRUE(positionIndex, NS_ERROR_FAILURE);
>+  positionIndex->SetData(position);
Decide whether position is signed or unsigned.

>+    nsCOMPtr<nsIObserver> alertObserver = new nsXULAlertObserver(this, aAlertName, aAlertListener);
>+    nsCOMPtr<nsISupports> iSupports(do_QueryInterface(alertObserver));
>+    ifptr->SetData(iSupports);
It's your observer, so you can just write:
nsCOMPtr<nsISupports> alertObserver = new nsXULAlertObserver(this, aAlertName, aAlertListener);
ifptr->SetData(alertObserver);

>+      if (window.arguments[0].length > 0)
if (window.arguments[0]) suffices.

>+  // Adjust for the position index.
>+  y += (gOrigin & NS_ALERT_TOP ? gPositionIndex : -gPositionIndex) * window.outerHeight;
I agree with Enn that this is a bad idea, but I don't have any good ideas for replacing alerts of variable sizes. (For showing alerts of various sizes, the position could be calculated from the position of the previous window.)

>-        onclick="onAlertClick();">
>+        onunload="onAlertUnload()">
Lost semicolon.

>+    <vbox class="alertCloseBox">
>+      <toolbarbutton class="alertCloseButton"
>+                     label="&closeAlert.tooltip;"
>+                     onclick="window.close()"/>
I actually might disagree with Enn here, depending on what you want a click that misses the button to do, the point being that if it's still within the alertCloseBox then nothing will happen...

> #alertImage {
>   max-width: 48px;
>   max-height: 48px;
>+  list-style-image: url(chrome://global/skin/alerts/notification-64.png);
Why not use a 48px image?
Comment 106 Dave Townsend [:mossop] 2012-12-11 12:53:14 PST
Comment on attachment 674432 [details] [diff] [review]
Part 8: Updated existing XUL alerts to implement the Notification API. v2

If I need to review (though I think reviews from the Neil's are enough) then I'll do so after the patch is updated.
Comment 107 Doug Turner (:dougt) 2012-12-12 08:52:48 PST
> Doug, what do you think about cleaning up the legacy code references to "desktop"

That can be done now, or in a follow up.  I think it is a good idea either way.  wchen, thoughts?
Comment 108 William Chen [:wchen] (Vacation until 8/25) 2013-01-16 14:45:05 PST
Sorry for the delay. I'm now back on notifications. Let's get this landed!

(In reply to :Ms2ger from comment #94)
> Comment on attachment 685513 [details] [diff] [review]
> Part 5.2: Implement Notification API. v4
> 
> Review of attachment 685513 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/webidl/Notification.webidl
> @@ +1,3 @@
> > +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > +/* 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,
> 
> "file," at the start of the next line
I'm leaving it that way to be consistent with all the other webidl headers.

All other comments addressed.

(In reply to Olli Pettay [:smaug] from comment #96)
> Comment on attachment 685513 [details] [diff] [review]
> 
> >+Notification::PrefEnabled()
> >+{
> >+#ifdef XP_MACOSX
> >+  return false;
> >+#else
> >+  return true;
> >+#endif
> >+}
> what is this? Not supported on osx yet?
> Do we have followup bug filed to fix this issue.
> Actually, I think we should not enable this in release builds before also OSX
> is supported.
In my earlier patches, OSX was supported but required removing the native mac notifications. I was told to leave the native notification in the tree and someone would work on implementing the notification center support. If we can't actually support the new API with notification center or if it takes too long, the plan is to remove the code and enable web notifications with XUL alerts.

All other comments addressed.

(In reply to Mike Hommey [:glandium] from comment #99)
> Yeah, notification just works with libnotify. The problem is however that it
> doesn't has enough flexibility for the notification API spec aiui. That
> being said, why remove it for *all* notifications?

For consistency and so that we don't have multiple slightly different alert implementations.
Comment 109 William Chen [:wchen] (Vacation until 8/25) 2013-01-16 14:46:07 PST
Created attachment 703047 [details] [diff] [review]
Part 5.2: Implement Notification API. v5
Comment 110 William Chen [:wchen] (Vacation until 8/25) 2013-01-16 15:03:10 PST
(In reply to Matthew N. [:MattN] from comment #100)
> Comment on attachment 685511 [details] [diff] [review]
> Added permission prompt for desktop notifications on desktop platforms. v4
> 
> Review of attachment 685511 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> These 2 comments weren't addressed yet:
> 
> (In reply to William Chen [:wchen] from comment #74)
> > > Can we use "web-notifications" as the ID everywhere (note the 's')?  Having
> > > "web" in the name makes it more clear it's related to notifications for
> > > content whereas "desktop" leads me to believe the opposite. As a bonus, it's
> > > also shorter.
> > > 
> > I've changed the name of the prompt stuff to web-notifications.
> > I didn't change the name of the permission type because this is supposed to
> > replace the old desktop notification and user preferences should remain the
> > same. Unfortunately, the pageInfo parts strongly couple the name of the
> > permission type to the name of the elements so those have not been changed.
> 
> I didn't realize we already shipped something similar and it's surprising
> that we called in desktop notifications since it was on mobile. I would
> prefer to remove all references to "desktop" and that could mean migrating
> the old permissions or ignoring them but I'm not sure others agree.
> 
> Doug, what do you think about cleaning up the legacy code references to
> "desktop" for the notification feature? It shouldn't be too hard to search
> and replace over the patches and I think now is a good time to do it since
> we're making breaking changes.
> 
We probably can't because this permission is already baked into b2g webapp manifests.
> ::: browser/locales/en-US/chrome/browser/pageInfo.dtd
> @@ +53,5 @@
> >  <!ENTITY  permTab               "Permissions">
> >  <!ENTITY  permTab.accesskey     "P">
> >  <!ENTITY  permUseDefault        "Use Default">
> >  <!ENTITY  permAskAlways         "Always ask">
> > +<!ENTITY  permAsk               "Ask">
> 
> Why isn't permAskAlways suitable? How are notification permissions different
> than others that use "Always ask".  Is it only asking once per session when
> set to "Ask"?  I want to avoid adding more subtle differences in terminology
> to the permissions pane.
> 
Yes, that is the reason. Always ask doesn't actually make sense for this API because the call to request permission is separate from the call to show a notification.
> ::: browser/components/nsBrowserGlue.js
> @@ +1799,5 @@
> > +    var message = browserBundle.formatStringFromName("webNotifications.showFromSite",
> > +                                                     [requestingURI.host], 1);
> > +    var allowString = "webNotifications.showForSession";
> > +    var alwaysAllowString = "webNotifications.alwaysShow";
> > +    var neverAllowString = "webNotifications.neverShow";
> 
> No need for these 3 temporary variables anymore.

Done.
Comment 111 William Chen [:wchen] (Vacation until 8/25) 2013-01-16 15:04:25 PST
Created attachment 703057 [details] [diff] [review]
Part 7: Added permission prompt for desktop notifications on desktop platforms. v5
Comment 112 William Chen [:wchen] (Vacation until 8/25) 2013-01-16 17:58:50 PST
(In reply to Neil Deakin from comment #103)
> Comment on attachment 674432 [details] [diff] [review]
> 
> +  // Adjust for the position index.
> +  y += (gOrigin & NS_ALERT_TOP ? gPositionIndex : -gPositionIndex) *
> window.outerHeight;
> 
> Is every alert exactly the same size?
> 
No, I've reworked the XUL alerts so that it should be able to handle different size alerts.
> 
> You use the label attribute but you seem to want to use tooltiptext.
> 
> Please use 'oncommand' not 'onclick'.
I'm going to keep it the way it is for the reason the other neil pointed out above.

(In reply to neil@parkwaycc.co.uk from comment #105)
> Comment on attachment 674432 [details] [diff] [review]
> Part 8: Updated existing XUL alerts to implement the Notification API. v2
> 
> >+  nsXULAlerts.cpp \
> I agree with Enn, I don't see the point of splitting this into a new file.
> (I agree with some of Enn's other comments to but I tried to avoid repeating
> them.)
> 
I split it off for organization. The implementation of the actual alerts on different platforms live in other files, it just made sense to do the same for XUL alerts. That way nsAlertsService just delegates the calls to the appropriate platform's alert service as well as remoting calls in the case of IPC.

> >+  nsCOMPtr<nsIDOMWindow> previousAlert = mNamedWindows.Get(aAlertName);
> aAlertName is optional, so you probably want to replace empty strings with
> some sort of unique index, otherwise you won't be able to track them
> internally.
> 
> > #alertImage {
> >   max-width: 48px;
> >   max-height: 48px;
> >+  list-style-image: url(chrome://global/skin/alerts/notification-64.png);
> Why not use a 48px image?
We could. I'll get a 48px image before landing.

All other comments addressed where applicable in the new code.
Comment 113 William Chen [:wchen] (Vacation until 8/25) 2013-01-16 18:04:42 PST
Created attachment 703136 [details] [diff] [review]
Updated existing XUL alerts to implement the Notification API. v3
Comment 114 Neil Deakin 2013-01-21 06:39:22 PST
>+    <vbox class="alertCloseBox">
>+      <toolbarbutton class="alertCloseButton"
>+                     label="&closeAlert.tooltip;"
>+                     onclick="window.close()"/>
> I actually might disagree with Enn here, depending on what you want a click
> that misses the button to do, the point being that if it's still within the
> alertCloseBox then nothing will happen...

Why can't you just leave the onclick on the window?

The code (the existing code prior to this patch) seems to imply that the alerts only appear for 4 seconds. Is someone expected to read and respond to the alert in that short time?
Comment 115 Matthew N. [:MattN] 2013-01-24 13:57:54 PST
Comment on attachment 703057 [details] [diff] [review]
Part 7: Added permission prompt for desktop notifications on desktop platforms. v5

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

BTW, we can move the UI code to bug 594543 if that's not too inconvenient so that we can unblock this bug and so it's easier to wade through comments.

(In reply to William Chen [:wchen] from comment #110)
> (In reply to Matthew N. [:MattN] from comment #100)
> > Comment on attachment 685511 [details] [diff] [review]
> > Added permission prompt for desktop notifications on desktop platforms. v4
> > 
> > Review of attachment 685511 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > These 2 comments weren't addressed yet:
> > 
> > (In reply to William Chen [:wchen] from comment #74)
> > > > Can we use "web-notifications" as the ID everywhere (note the 's')?  Having
> > > > "web" in the name makes it more clear it's related to notifications for
> > > > content whereas "desktop" leads me to believe the opposite. As a bonus, it's
> > > > also shorter.
> > > > 
> > > I've changed the name of the prompt stuff to web-notifications.
> > > I didn't change the name of the permission type because this is supposed to
> > > replace the old desktop notification and user preferences should remain the
> > > same. Unfortunately, the pageInfo parts strongly couple the name of the
> > > permission type to the name of the elements so those have not been changed.
> > 
> > I didn't realize we already shipped something similar and it's surprising
> > that we called in desktop notifications since it was on mobile. I would
> > prefer to remove all references to "desktop" and that could mean migrating
> > the old permissions or ignoring them but I'm not sure others agree.
> > 
> > Doug, what do you think about cleaning up the legacy code references to
> > "desktop" for the notification feature? It shouldn't be too hard to search
> > and replace over the patches and I think now is a good time to do it since
> > we're making breaking changes.
> > 
> We probably can't because this permission is already baked into b2g webapp
> manifests.

Not so much when I suggested the change in October :( As Doug said, this can be handled in a follow-up.

> > ::: browser/locales/en-US/chrome/browser/pageInfo.dtd
> > @@ +53,5 @@
> > >  <!ENTITY  permTab               "Permissions">
> > >  <!ENTITY  permTab.accesskey     "P">
> > >  <!ENTITY  permUseDefault        "Use Default">
> > >  <!ENTITY  permAskAlways         "Always ask">
> > > +<!ENTITY  permAsk               "Ask">
> > 
> > Why isn't permAskAlways suitable? How are notification permissions different
> > than others that use "Always ask".  Is it only asking once per session when
> > set to "Ask"?  I want to avoid adding more subtle differences in terminology
> > to the permissions pane.
> > 
> Yes, that is the reason. Always ask doesn't actually make sense for this API
> because the call to request permission is separate from the call to show a
> notification.

In that case, I don't think there's a need for permAsk because that is the default which the checkbox controls. There should be the checkbox plus 3 options like Cookies: Allow, Allow for Session, Block. Implementing Allow for Session is non-trivial since the dialog doesn't know about expireType yet because cookies use a special permission value of ACCESS_SESSION which you probably wouldn't want to use. You can handle this in a follow-up bug.

Note that you also need to add the permission information to about:permissions which can also be handled in a follow-up.

::: browser/base/content/pageinfo/permissions.js
@@ +30,5 @@
>      return ALLOW;
>    },
> +  "desktop-notification": function getNotificationDefaultPermission()
> +  {
> +    return UNKNOWN;

change to BLOCK when you remove permAsk.

::: browser/components/nsBrowserGlue.js
@@ +1521,5 @@
> +   * Show a permission prompt.
> +   *
> +   * @param aRequest               The permission request.
> +   * @param aMessage               The message to display on the prompt.
> +   * @param aPermission            The type of permission to prompt.

Ugh, I wish you could just use aRequest.type instead of aPermission but apparently geolocation uses "geo" for the permission but "geolocation" for the type.

@@ +1530,5 @@
> +   *                               for the session.
> +   * @param aAlwaysAllowString     A string to display for always allowing permission. If
> +   *                               null, an option to always allow permission will not be
> +   *                               provided.
> +   * @param aNeverAllowString      The string to display for never allowing permission. If

Since we have special cases for aIsAllowForSession and for geolocation telemetry, I think this function should take an array of actions, similar to PopupNotifications instead of the aIsAllowForSession, aAllowString, aAllowString & aNeverAllowString arguments.

aActions: [
  {
    "stringId": "geolocation.shareLocation",
    "action": null,
    "expireType": null,
    "callback": function geoAllowOnce() {
      secHistogram.add(Ci.nsISecurityUITelemetry.WARNING_GEOLOCATION_REQUEST_SHARE_LOCATION);
    }
  },
  {
    "stringId": "geolocation.alwaysShareLocation",
    "action": Ci.nsIPermissionManager.ALLOW_ACTION,
    "expireType": null,
    "callback": function geoAllow(){
      secHistogram.add(Ci.nsISecurityUITelemetry.WARNING_GEOLOCATION_REQUEST_ALWAYS_SHARE);
    }
  },
  {
    "stringId": "geolocation.neverShareLocation",
    "action": Ci.nsIPermissionManager.DENY_ACTION,
    "expireType": null,
    "callback": function geoBlock(){
      secHistogram.add(Ci.nsISecurityUITelemetry.WARNING_GEOLOCATION_REQUEST_NEVER_SHARE);
    }
  },
]

If action is null or ALLOW_ACTION then allow the request, otherwise cancel.

@@ +1539,5 @@
> +   */
> +  _showPrompt: function CPP_showPrompt(aRequest, aMessage, aPermission, aIsAllowForSession,
> +                                       aAllowString, aAlwaysAllowString, aNeverAllowString,
> +                                       aNotificationId, aAnchorId) {
> +    let secHistogram = Components.classes["@mozilla.org/base/telemetry;1"].

Nit in existing code:
let secHistogram = Services.telemetry.getHistogramById("SECURITY_UI");

@@ +1610,5 @@
> +        });
> +      }
> +    }
> +
> +    if (aPermission == "geolocation") {

I think you mean aNotificationId rather than aPermission.

::: browser/themes/pinstripe/browser.css
@@ +3105,5 @@
>    }
>  }
>  
> +#web-notifications-notification-icon {
> +  list-style-image: url(chrome://browser/skin/notification-16.png);

Include an @2x image for hi-dpi like the others.

@@ +3124,5 @@
>    }
>  }
>  
> +.popup-notification-icon[popupid="web-notifications"] {
> +  list-style-image: url(chrome://browser/skin/notification-64.png);

Same here

::: browser/themes/pinstripe/jar.mn
@@ +38,5 @@
>    skin/classic/browser/KUI-close.png
>    skin/classic/browser/menu-back.png
>    skin/classic/browser/menu-forward.png
> +  skin/classic/browser/notification-16.png
> +  skin/classic/browser/notification-64.png

Include @2x versions here too
Comment 116 Olli Pettay [:smaug] (vacation Aug 25-28) 2013-01-28 12:52:54 PST
Comment on attachment 703047 [details] [diff] [review]
Part 5.2: Implement Notification API. v5

>+NS_IMETHODIMP
>+NotificationPermissionRequest::Run()
>+{
>+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+    // because owner implements nsITabChild, we can assume that it is
>+    // the one and only TabChild.
>+    TabChild* child = GetTabChildFrom(mWindow->GetDocShell());
>+    if (!child) {
>+      return NS_ERROR_NOT_AVAILABLE;
>+    }
>+
>+    // Retain a reference so the object isn't deleted without IPDL's knowledge.
>+    // Corresponding release occurs in DeallocPContentPermissionRequest.
>+    AddRef();
      NS_ADDREF_THIS();

>+NotificationObserver::Observe(nsISupports* aSubject, const char* aTopic,
>+                              const PRUnichar* aData)
>+{
>+  if (!strcmp("alertclickcallback", aTopic)) {
>+    mNotification->DispatchTrustedEvent(NS_LITERAL_STRING("click"));
>+  } else if (!strcmp("alertfinished", aTopic)) {
>+    mNotification->mIsClosed = true;
>+    mNotification->DispatchTrustedEvent(NS_LITERAL_STRING("close"));
>+  } else if (!strcmp("alertshow", aTopic)) {
>+    mNotification->DispatchTrustedEvent(NS_LITERAL_STRING("show"));
>+  }
Could you use nsCRT::strcmp

>+  nsString uniqueCookie = NS_LITERAL_STRING("notification:");
>+  uniqueCookie.AppendInt(sCount++);
Does 
>+Notification::RequestPermission(nsISupports* aGlobal,
>+                                NotificationPermissionCallback& aCallback,
>+                                ErrorResult& aRv)
>+{
>+  // Get principal from global to make permission request for notifications.
>+  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal);
>+  nsCOMPtr<nsIScriptObjectPrincipal> sop = do_QueryInterface(aGlobal);
>+  if (!sop) {
>+    aRv.Throw(NS_ERROR_UNEXPECTED);
>+    return;
>+  }
>+  nsCOMPtr<nsIPrincipal> principal = sop->GetPrincipal();
Null-check principal


>+Notification::GetPermission(nsISupports* aGlobal, ErrorResult& aRv)
>+{
>+  // Get principal from global to check permission for notifications.
>+  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal);
>+  nsCOMPtr<nsIScriptObjectPrincipal> sop = do_QueryInterface(aGlobal);
>+  if (!sop) {
>+    aRv.Throw(NS_ERROR_UNEXPECTED);
>+    return NotificationPermission::Denied;
>+  }
>+  nsCOMPtr<nsIPrincipal> principal = sop->GetPrincipal();
ditto


>+Notification::PrefEnabled()
>+{
>+#ifdef XP_MACOSX
>+  return false;
>+#else
>+  return true;
>+#endif
>+}
So, what is the status of OSX support? I don't think we should enable this whole thing if we can't
support OSX. (I'd guess significant portion of web devs may use OSX).
We could disable this stuff on non-desktop, I guess.
Also, if we disable, better to make sure the API isn't visible to web pages.

r- mainly because of the OSX issue
Comment 117 Olli Pettay [:smaug] (vacation Aug 25-28) 2013-01-28 13:17:08 PST
Comment on attachment 703047 [details] [diff] [review]
Part 5.2: Implement Notification API. v5

Ok, I was told we want to land this before doing the OSX bits.

But this needs more tests, at least for tag handling.
Test different origins and same origin using same tag several times and
same origin using different tags.
Comment 118 William Chen [:wchen] (Vacation until 8/25) 2013-01-31 22:29:27 PST
Created attachment 708948 [details] [diff] [review]
Part 8: Updated existing XUL alerts to implement the Notification API.

(In reply to Neil Deakin from comment #114)
> 
> Why can't you just leave the onclick on the window?
> 
I didn't want to have a click event fired when clicking on the close button. I've updated to patch to put the onclick back on the window.
> The code (the existing code prior to this patch) seems to imply that the
> alerts only appear for 4 seconds. Is someone expected to read and respond to
> the alert in that short time?
That's the case for right now, I rather have non-essential UI/UX issues handled in follow-up bugs. The point of this bug is to get a minimal version of the Notification API working that can later be enhanced by UI/UX people, that way I don't get stuck maintaining all this notification code indefinitely while blocked on UI/UX.
Comment 119 William Chen [:wchen] (Vacation until 8/25) 2013-01-31 22:35:49 PST
Created attachment 708949 [details] [diff] [review]
Part 7: Added permission prompt for desktop notifications on desktop platforms. v6

(In reply to Matthew N. [:MattN] from comment #115)
> Comment on attachment 703057 [details] [diff] [review]
> Part 7: Added permission prompt for desktop notifications on desktop
> platforms. v5
> 
> Review of attachment 703057 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> BTW, we can move the UI code to bug 594543 if that's not too inconvenient so
> that we can unblock this bug and so it's easier to wade through comments.
I rather not do that because the UI code isn't independent of the other code in this bug.
> 
> Not so much when I suggested the change in October :( As Doug said, this can
> be handled in a follow-up.
> 
> In that case, I don't think there's a need for permAsk because that is the
> default which the checkbox controls. There should be the checkbox plus 3
> options like Cookies: Allow, Allow for Session, Block. Implementing Allow
> for Session is non-trivial since the dialog doesn't know about expireType
> yet because cookies use a special permission value of ACCESS_SESSION which
> you probably wouldn't want to use. You can handle this in a follow-up bug.
> 
> Note that you also need to add the permission information to
> about:permissions which can also be handled in a follow-up.
> 
I'll go through this bug and file the follow-ups prior to landing.
> 
> ::: browser/themes/pinstripe/browser.css
> @@ +3105,5 @@
> >    }
> >  }
> >  
> > +#web-notifications-notification-icon {
> > +  list-style-image: url(chrome://browser/skin/notification-16.png);
> 
> Include an @2x image for hi-dpi like the others.
> 
> @@ +3124,5 @@
> >    }
> >  }
> >  
> > +.popup-notification-icon[popupid="web-notifications"] {
> > +  list-style-image: url(chrome://browser/skin/notification-64.png);
> 
> Same here
> 
> ::: browser/themes/pinstripe/jar.mn
> @@ +38,5 @@
> >    skin/classic/browser/KUI-close.png
> >    skin/classic/browser/menu-back.png
> >    skin/classic/browser/menu-forward.png
> > +  skin/classic/browser/notification-16.png
> > +  skin/classic/browser/notification-64.png
> 
> Include @2x versions here too
I'm still waiting on the icons, but I'll include it before landing.

The other comments have been addressed.
Comment 120 Anne (:annevk) 2013-02-01 01:33:00 PST
FYI: the requestPermission() callback argument is now optional. See http://notifications.spec.whatwg.org/#api
Comment 121 Matthew N. [:MattN] 2013-02-06 23:20:17 PST
Comment on attachment 708949 [details] [diff] [review]
Part 7: Added permission prompt for desktop notifications on desktop platforms. v6

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

r=me with the changes and follow-ups discussed before. Thanks William.

::: browser/components/nsBrowserGlue.js
@@ +1562,5 @@
> +      var action = {
> +        label: browserBundle.GetStringFromName(promptAction.stringId),
> +        accessKey: browserBundle.GetStringFromName(promptAction.stringId + ".accesskey"),
> +        callback: function() {
> +          promptAction.callback();

Make promptAction.callback optional by checking the truthiness of it before calling it.

@@ +1581,5 @@
> +      };
> +
> +      // Don't offer action in PB mode if the action remembers permission for more than a session.
> +      if (!PrivateBrowsingUtils.isWindowPrivate(chromeWin) ||
> +          promptAction.expireType == Ci.nsIPermissionManager.EXPIRE_SESSION ||

Move this check above the declaration of |action| and have the loop continue if the opposite conditions are true so we avoid the string lookups and other work in this case.
Comment 122 Matthew N. [:MattN] 2013-03-05 03:41:46 PST
Hey William, what's the status here?
Comment 123 William Chen [:wchen] (Vacation until 8/25) 2013-03-10 23:08:36 PDT
(In reply to Matthew N. [:MattN] from comment #122)
> Hey William, what's the status here?

Landing these patches is now at the top of my priority list.
Comment 124 Lucas Adamski [:ladamski] 2013-03-15 09:11:48 PDT
Marking tracking as this blocks a bunch of notification-related certification bugs.  Not blocking as this is a new set of features that were not identified as core user stories for TEF or LEO.
Comment 125 Jason Smith [:jsmith] 2013-03-15 09:13:48 PDT
(In reply to Lucas Adamski from comment #124)
> Marking tracking as this blocks a bunch of notification-related
> certification bugs.  Not blocking as this is a new set of features that were
> not identified as core user stories for TEF or LEO.

That's not entirely true. We had to cut calendar sound customization support on notifications because this wasn't implemented. If this was implemented, this feature probably would have been part of v1.1.
Comment 127 Olli Pettay [:smaug] (vacation Aug 25-28) 2013-03-18 08:49:07 PDT
Created attachment 726188 [details] [diff] [review]
make it compile on gcc 4.7

https://tbpl.mozilla.org/?tree=Try&rev=bc4e95e02211
Comment 128 Olli Pettay [:smaug] (vacation Aug 25-28) 2013-03-18 10:28:03 PDT
Comment on attachment 726188 [details] [diff] [review]
make it compile on gcc 4.7

based on tryserver, seems to compile everywhere, and even pass the tests.
Comment 130 William Chen [:wchen] (Vacation until 8/25) 2013-03-18 15:33:41 PDT
gcc 4.7 fix

https://hg.mozilla.org/integration/mozilla-inbound/rev/061871df8f9e
Comment 131 Philip Chee 2013-03-18 21:28:45 PDT
https://tbpl.mozilla.org/php/getParsedLog.php?id=20802247&tree=Thunderbird-Trunk#error0

../../../../mailnews/base/src/nsMessengerUnixIntegration.cpp: In member function 'nsresult nsMessengerUnixIntegration::ShowAlertMessage(const nsAString_internal&, const nsAString_internal&, const nsACString_internal&)':
../../../../mailnews/base/src/nsMessengerUnixIntegration.cpp:361:62: error: no matching function for call to 'nsIAlertsService::ShowAlertNotification(const nsAFlatString&, const nsAString_internal&, const nsAString_internal&, bool, NS_ConvertASCIItoUTF16, nsMessengerUnixIntegration* const, const nsAFlatString&)'
../../../mozilla/dist/include/nsIAlertsService.h:35:60: note: candidate is: virtual nsresult nsIAlertsService::ShowAlertNotification(const nsAString_internal&, const nsAString_internal&, const nsAString_internal&, bool, const nsAString_internal&, nsIObserver*, const nsAString_internal&, const nsAString_internal&, const nsAString_internal&)
make[8]: *** [nsMessengerUnixIntegration.o] Error 1
Comment 132 Doug Turner (:dougt) 2013-03-19 07:00:45 PDT
chee, please file a follow up bug for Thunderbird.
Comment 133 Ryan VanderMeulen [:RyanVM] 2013-03-19 07:27:06 PDT
https://hg.mozilla.org/mozilla-central/rev/061871df8f9e
Comment 134 Doug Turner (:dougt) 2013-03-19 19:47:27 PDT
*** Bug 629280 has been marked as a duplicate of this bug. ***
Comment 135 Doug Turner (:dougt) 2013-03-19 19:47:43 PDT
*** Bug 594543 has been marked as a duplicate of this bug. ***
Comment 136 Madhava Enros [:madhava] 2013-03-20 09:23:25 PDT
Hi all -

This didn't get a UI-R, and there's polish that needs to happen before this ships for real. Is there a follow-up bug or should we re-open here?
Comment 137 Jason Smith [:jsmith] 2013-03-20 09:26:16 PDT
(In reply to Madhava Enros [:madhava] from comment #136)
> Hi all -
> 
> This didn't get a UI-R, and there's polish that needs to happen before this
> ships for real. Is there a follow-up bug or should we re-open here?

I'd suggest filing a followup for the changes needed.
Comment 138 Dave Townsend [:mossop] 2013-03-20 13:46:48 PDT
I'm seeing bug 852436 everytime my Firefox tries to show a XUL alert.
Comment 139 Cornel Ionce [QA] (:cornel_ionce) 2013-04-04 04:23:18 PDT
Is this page ok for testing? 
http://jsbin.com/notification/522/edit

Also, I was wondering if there are any other pages where I can test this?
Comment 140 Brian R. Bondy [:bbondy] 2013-04-04 15:46:36 PDT
I'm pretty sure this bug introduced a regression in alerts for both Metro and B2G.  I put a patch in this bug: bug 856202
Comment 141 Doug Turner (:dougt) 2013-04-04 20:10:18 PDT
cornel - what are you trying to test?

If I could ask for anything, I'd have you review the spec and ensure that we do everything correctly.  File bugs for any things that we do that don't work correctly.

@brian nice find.
Comment 142 Alex Keybl [:akeybl] 2013-04-05 11:58:55 PDT
This bug has been listed as part of the Aurora 22 release notes in either [1], [2], or both. If you find any of the information or wording incorrect in the notes, please email release-mgmt@mozilla.com asap. Thanks!

[1] https://www.mozilla.org/en-US/firefox/22.0a2/auroranotes/
[2] https://www.mozilla.org/en-US/mobile/22.0a2/auroranotes/
Comment 143 Julien Wajsberg [:julienw] 2013-04-24 04:58:27 PDT
*** Bug 779213 has been marked as a duplicate of this bug. ***
Comment 144 neil@parkwaycc.co.uk 2013-05-12 14:59:23 PDT
(In reply to Matthew N. from comment #115)
> (From update of attachment 703057 [details] [diff] [review])
> > +   * Show a permission prompt.
> > +   *
> > +   * @param aRequest               The permission request.
> > +   * @param aMessage               The message to display on the prompt.
> > +   * @param aPermission            The type of permission to prompt.
> Ugh, I wish you could just use aRequest.type instead of aPermission but
> apparently geolocation uses "geo" for the permission but "geolocation" for
> the type.
Did anyone file a bug to fix this?
Comment 145 Evgueni Naverniouk 2013-05-27 13:02:17 PDT
Are we sure this can be marked as Resolved without addressing https://bugzilla.mozilla.org/show_bug.cgi?id=862395 first?
Comment 146 Karl Tomlinson (:karlt) 2013-06-18 22:05:32 PDT
(In reply to Doug Turner (:dougt) from comment #97)
> This removes gnome native notifications (which may already be broken on at
> least ubuntu).  The code change lgmt, but since this does have some impact
> be sure to send mail to the platform newsgroup before landing.

William, can you link to this thread please?
(I haven't managed to find it.)
Comment 147 Julien Wajsberg [:julienw] 2013-07-27 07:55:31 PDT
The gaia part has not landed afaik.

William, can you do it yourself or do you want that me or someone else do it for you ?
Comment 148 Julien Wajsberg [:julienw] 2013-07-30 05:36:30 PDT
I'll land it.
Comment 149 Julien Wajsberg [:julienw] 2013-07-30 11:22:06 PDT
Created attachment 783252 [details] [diff] [review]
rebased gaia patch v2
Comment 150 Julien Wajsberg [:julienw] 2013-07-30 11:39:34 PDT
gaia master: 359b76d47ad2694c7237332797b818c3f5b0e53b

hopefully I haven't broken everything :)

I don't currently have a mozcentral build, and "verifyme" is already set. Jason, could you please do a quick sanity check around notifications (eg: receiving sms, missing calls, etc)
Comment 151 Jason Smith [:jsmith] 2013-07-30 11:46:34 PDT
Yup, sure. We're doing daily smoke testing for moz central + gaia master builds now, so this will be automatically regression tested on tomorrow's build generated.
Comment 152 Jason Smith [:jsmith] 2013-07-31 16:45:08 PDT
Backed out in https://github.com/mozilla-b2g/gaia/commit/715d314bbfa7a06315213ffbae923c15999689ea for breaking all the notifications (see bug 900225).

Opened bug 900279 to track getting the gaia master code here working correctly.
Comment 153 Julien Wajsberg [:julienw] 2013-08-01 01:40:32 PDT
Indeed I just noticed this too, thanks for backing out.
Comment 154 Julien Wajsberg [:julienw] 2013-08-02 07:36:30 PDT
Indeed the reviewed patches are not exactly like what was finally commited.
Comment 155 Steven Michaud [:smichaud] (Retired) 2013-08-02 08:27:35 PDT
(In reply to comment #13)

> I removed Growl support because it does not support features needed
> to implement the spec.

William, could you be more specific about which needed features are
missing from Growl?

It's not that I (necessarily) want Growl back.  I just want to
understand the issues better, and to have a fuller explanation "on
record".
Comment 157 Makoto Kato [:m_kato] 2013-09-17 01:57:54 PDT
Why does we keep toolkit/system/gnome/nsAlertsIconListener.cpp from tree?
Comment 158 bjo 2013-09-17 09:38:45 PDT
FF23 and TB24 show only XUL-notifications under XFCE on Arch Linux, do they have any support für libnotify?
Comment 159 Marco Castelluccio [:marco] (PTO until August 24/25) 2013-09-17 10:09:38 PDT
(In reply to bjo from comment #158)
> FF23 and TB24 show only XUL-notifications under XFCE on Arch Linux, do they
> have any support für libnotify?

See bug 858919 and bug 853104.

Karl did you by any chance talk with wchen? I'd like to see thread on the platform newsgroup too.
Comment 160 Karl Tomlinson (:karlt) 2013-09-17 14:54:26 PDT
I haven't heard from wchen.

This should be backed out IMHO.
Comment 161 [:fabrice] Fabrice Desré 2013-09-17 14:59:23 PDT
(In reply to Karl Tomlinson (:karlt) from comment #160)
> I haven't heard from wchen.
> 
> This should be backed out IMHO.

You mean backing out the full feature? At least let us branch b2g 1.2 before since we rely on it.
Comment 162 Karl Tomlinson (:karlt) 2013-09-17 17:31:20 PDT
(In reply to Fabrice Desré [:fabrice] from comment #161)
> You mean backing out the full feature?

Is it possible to back out only part 9?
Would that fix the regressions?
Comment 163 [:fabrice] Fabrice Desré 2013-09-17 17:40:16 PDT
(In reply to Karl Tomlinson (:karlt) from comment #162)
> (In reply to Fabrice Desré [:fabrice] from comment #161)
> > You mean backing out the full feature?
> 
> Is it possible to back out only part 9?
> Would that fix the regressions?

I'll let wchen answer that.
Comment 164 Marco Castelluccio [:marco] (PTO until August 24/25) 2013-10-08 14:50:21 PDT
Opened the discussion here: https://groups.google.com/forum/#!topic/mozilla.dev.platform/77i4nntE2Ug
Comment 165 Landry Breuil (:gaston) 2013-12-08 14:02:21 PST
(In reply to Makoto Kato (:m_kato) from comment #157)
> Why does we keep toolkit/system/gnome/nsAlertsIconListener.cpp from tree?

I stumbled upon the file looking for something else, and was wondering the same. If it is not linked to any kind of build option (since noone seem to care about fixing the regression & restoring libnotify support), it will only bitrot... and maybe should be removed aswell ?
Comment 166 Karl Tomlinson (:karlt) 2013-12-08 15:28:12 PST
(In reply to Landry Breuil (:gaston) from comment #165)
> (In reply to Makoto Kato (:m_kato) from comment #157)
> > Why does we keep toolkit/system/gnome/nsAlertsIconListener.cpp from tree?
> 
> I stumbled upon the file looking for something else, and was wondering the
> same. If it is not linked to any kind of build option (since noone seem to
> care about fixing the regression & restoring libnotify support), it will
> only bitrot... and maybe should be removed aswell ?

No, the file should be part of the build.  The fact that it is not is a bug.
Comment 167 Chris Forsythe 2014-01-31 12:20:15 PST
(In reply to Steven Michaud from comment #155)
> (In reply to comment #13)
> 
> > I removed Growl support because it does not support features needed
> > to implement the spec.
> 
> William, could you be more specific about which needed features are
> missing from Growl?
> 
> It's not that I (necessarily) want Growl back.  I just want to
> understand the issues better, and to have a fuller explanation "on
> record".


I would also like to find out why Growl was removed. We're getting user complaints because users don't understand why their firefox plugins are not notifying through Growl anymore. I'd like to explain it better than "because someone at Mozilla arbitrarily decided to remove it instead of talking to us about what features they needed". Which so far is exactly how it looks.

I just need a simple 1-3 sentence explanation that I can send to my end users as to why a feature they depended upon is no longer present. I would also like a much more technical explanation as to why Growl support was removed because obviously we'd like to address those kinds of issues going forward.

What I do not understand is that the feature for this was developed in our repository. The framework was paired down to work with the very stringent requirements that Mozilla has in order to get Growl support to work. The framework we provide now supports native notifications on 10.8 and 10.9. On 10.6 and 10.7 there are no native notifications so Growl is the only game in town, but you do not need Growl in your application, just the framework, in order to fire a notification to the end user.

We could have made the adjustments to our notification framework to give not only Mozilla what they wanted, but also provide those changes to the entire mac community that uses our framework. So if someone explains exactly what the problem is, we'd be happy to look at addressing those things.

Hopefully you can see why this is frustrating to me, and more importantly to our mutual end users who are the ones who are affected by this (currently) seemingly arbitrary change.

Feel free to email me.

Chris Forsythe
Project Lead - The Growl Project
chris@growl.info
Comment 168 Florian Bender 2014-01-31 13:51:35 PST
Not speaking for Mozilla here, but the general notion was (iirc): Removing Growl removed a giant blob of external, platform-specific code, updating to 2.x would have entailed quite some work, but most of all (and at the same time the biggest reason for the aforementioned arguments): The notification infrastructure was reworked and cross-platform (so-called) XUL notifications were implemented, i.e. Growl needed to be re-implemented as a notification provider for the new infrastructure (this is true until today – anybody is welcome to write a Growl notification provider and distribute as an addon) – it was easier to remove the code, and have the new cross-platform XUL notifications shipped before trying to implement a platform-specific backend.

Sidenote: A notification provider for Notification Center (since OS X 10.8) has been written and should be included in the next release (Fx 27).
Comment 169 Matthew N. [:MattN] 2014-01-31 14:43:24 PST
Chris, you should see bug 777409 comment 0 for more rationale. My understanding is that the performance issue (bug 759780) was the main problem.

This discussion would be better on mozilla.dev.platform as this bug was closed long ago.
Comment 170 Chris Forsythe 2014-01-31 15:07:04 PST
(In reply to Matthew N. [:MattN] from comment #169)
> Chris, you should see bug 777409 comment 0 for more rationale. My
> understanding is that the performance issue (bug 759780) was the main
> problem.
>


My apologies, but I disagree with the reasoning there. I could never get any information as to what security and performance issues he's referring to. 
 

I also disagree with removing functionality without having a replacement in place for the platform you support. Look further down in the ticket you reference and you'll see a lot of feedback as to why this is bad.


> This discussion would be better on mozilla.dev.platform as this bug was
> closed long ago.
 
It was closed without explaining what was asked in that comment, which is why I felt it was relevant to ask this here. I'd be happy to ask on whatever mozilla.dev.platform is but frankly I just wanted an email with what I asked for so that

- I would have something to provide to my end users.
- We could resolve whatever actual issues there are in Growl that caused this to happen.
Comment 171 Karl Tomlinson (:karlt) 2014-01-31 20:41:45 PST
(In reply to Matthew N. [:MattN] from comment #169)
> This discussion would be better on mozilla.dev.platform as this bug was
> closed long ago.

The discussion began in this bug before anything landed but the questions and advice were ignored.  Similarly there were no real answers to the m.d.p thread.
Don't be surprised that people still want answers to those questions.

Note You need to log in before you can comment on or make changes to this bug.