William Chen [:wchen] Assignee
	Description • 12 years ago

This is a bug to track the implementation of the w3c notification spec.

The UX is discussed in bug 629280.

	William Chen [:wchen] Assignee
	Comment 10 • 12 years ago

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.

	Mounir Lamouri (:mounir)
	Comment 11 • 12 years ago

I don't understand part 9. We don't want to use native notifications? Why that?

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 12 • 12 years ago

We do want to use native notifications; that's the point.

	William Chen [:wchen] Assignee
	Comment 13 • 12 years ago

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

	Marco Castelluccio [:marco]
	Comment 14 • 12 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 15 • 12 years ago

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

	Doug Turner (:dougt)
	Comment 16 • 12 years ago

Yes, we are still waiting for something from UX.  Also see bug 629280.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 17 • 12 years ago

I think we should land this with some UI and have it enabled only on Nightly.

	Stephen Horlander [:shorlander]
	Comment 18 • 12 years ago

(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! :)

	Doug Turner (:dougt)
	Comment 30 • 12 years ago

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.

	Doug Turner (:dougt)
	Comment 31 • 12 years ago

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.

	Doug Turner (:dougt)
	Comment 32 • 12 years ago

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.

	Doug Turner (:dougt)
	Comment 33 • 12 years ago

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.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 34 • 12 years ago

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?

	Matthew N. [:MattN]
	Comment 35 • 12 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 36 • 12 years ago

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

	Etienne Segonzac (:etienne)
	Comment 37 • 12 years ago

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

	William Chen [:wchen] Assignee
	Comment 38 • 12 years ago

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

	William Chen [:wchen] Assignee
	Comment 40 • 12 years ago

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

	William Chen [:wchen] Assignee
	Comment 42 • 12 years ago

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

	Wesley Johnston (:wesj)
	Comment 43 • 12 years ago

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?

	William Chen [:wchen] Assignee
	Comment 44 • 12 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 46 • 12 years ago

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

	Josh Aas
	Comment 47 • 12 years ago

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

	William Chen [:wchen] Assignee
	Comment 48 • 12 years ago

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

	Josh Aas
	Comment 49 • 12 years ago

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

	William Chen [:wchen] Assignee
	Comment 50 • 12 years ago

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

	William Chen [:wchen] Assignee
	Comment 53 • 12 years ago

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

	Matthew N. [:MattN]
	Comment 55 • 12 years ago

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.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 56 • 12 years ago

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

	Jared Wein [:jaws] (please needinfo? me)
	Comment 57 • 12 years ago

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.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 58 • 12 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 59 • 12 years ago

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.

	William Chen [:wchen] Assignee
	Comment 60 • 12 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 61 • 12 years ago

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.

	Lucas Adamski [:ladamski]
	Comment 62 • 12 years ago

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.

	William Chen [:wchen] Assignee
	Comment 63 • 12 years ago

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

	William Chen [:wchen] Assignee
	Comment 66 • 12 years ago

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

	William Chen [:wchen] Assignee
	Comment 67 • 12 years ago

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

	Matthew N. [:MattN]
	Comment 68 • 12 years ago

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.

	Matthew N. [:MattN]
	Comment 70 • 12 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 71 • 12 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 72 • 12 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 73 • 12 years ago

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.

	William Chen [:wchen] Assignee
	Comment 74 • 12 years ago

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

	William Chen [:wchen] Assignee
	Comment 75 • 12 years ago

dougt: can you review the changes in Notification.cpp?

	Doug Turner (:dougt)
	Comment 76 • 12 years ago

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?

	William Chen [:wchen] Assignee
	Comment 77 • 12 years ago

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

	William Chen [:wchen] Assignee
	Comment 78 • 12 years ago

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

	Mounir Lamouri (:mounir)
	Comment 80 • 12 years ago

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

	Matthew N. [:MattN]
	Comment 81 • 12 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 82 • 12 years ago

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

	Evgueni Naverniouk
	Comment 83 • 12 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 84 • 12 years ago

Please send feature requests to W3C (public-web-notification@w3.org).

	Lucas Adamski [:ladamski]
	Comment 85 • 12 years ago

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.

	Anne (:annevk)
	Comment 86 • 12 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 87 • 12 years ago

WebIDL callbacks are now supported, so you can simplify the patch.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 88 • 12 years ago

Comment on attachment 672542 [details] [diff] [review]
Part 5.1: Add function to remote test for permission in nsIPermissionManager. v1

Sorry about slow review :(

	William Chen [:wchen] Assignee
	Comment 89 • 12 years ago

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

	William Chen [:wchen] Assignee
	Comment 91 • 12 years ago

Now using webidl callbacks, no more JS API!

	William Chen [:wchen] Assignee
	Comment 92 • 12 years ago

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.

	Florian Bender
	Comment 93 • 12 years ago

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.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 94 • 12 years ago

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.

	William Chen [:wchen] Assignee
	Comment 95 • 12 years ago

By request, here is a single patch with all the changes.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 96 • 12 years ago

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.

	Doug Turner (:dougt)
	Comment 97 • 12 years ago

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.

	Marco Castelluccio [:marco]
	Comment 98 • 12 years ago

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

	Mike Hommey [:glandium]
	Comment 99 • 12 years ago

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?

	Matthew N. [:MattN]
	Comment 100 • 12 years ago

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.

	Matthew N. [:MattN]
	Comment 101 • 12 years ago

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…

	Dão Gottwald [:dao]
	Comment 102 • 12 years ago

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.

	Neil Deakin
	Comment 103 • 12 years ago

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

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 104 • 12 years ago

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

	neil@parkwaycc.co.uk
	Comment 105 • 12 years ago

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?

	Dave Townsend [:mossop]
	Comment 106 • 12 years ago

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.

	Doug Turner (:dougt)
	Comment 107 • 12 years ago

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

	William Chen [:wchen] Assignee
	Comment 108 • 11 years ago

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.

	William Chen [:wchen] Assignee
	Comment 110 • 11 years ago

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

	William Chen [:wchen] Assignee
	Comment 112 • 11 years ago

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

	Neil Deakin
	Comment 114 • 11 years ago

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

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

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 116 • 11 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 117 • 11 years ago

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.

	William Chen [:wchen] Assignee
	Comment 118 • 11 years ago

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

	William Chen [:wchen] Assignee
	Comment 119 • 11 years ago

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

	Anne (:annevk)
	Comment 120 • 11 years ago

FYI: the requestPermission() callback argument is now optional. See http://notifications.spec.whatwg.org/#api

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

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.

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

Hey William, what's the status here?

	William Chen [:wchen] Assignee
	Comment 123 • 11 years ago

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

	Lucas Adamski [:ladamski]
	Comment 124 • 11 years ago

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.

	Jason Smith [:jsmith]
	Comment 125 • 11 years ago

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

	William Chen [:wchen] Assignee
	Comment 126 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e96b9c546f07
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8b3e4b6a8b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4508a65ccbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff9b93ce514c
https://hg.mozilla.org/integration/mozilla-inbound/rev/dde6b08c74f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/e84fb66cf7f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a471f858d6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d068e438438d
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9732200552d
https://hg.mozilla.org/integration/mozilla-inbound/rev/404720032d1c

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 127 • 11 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 128 • 11 years ago

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.

	Ed Morley [:emorley]
	Comment 129 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/e96b9c546f07
https://hg.mozilla.org/mozilla-central/rev/c8b3e4b6a8b5
https://hg.mozilla.org/mozilla-central/rev/a4508a65ccbe
https://hg.mozilla.org/mozilla-central/rev/ff9b93ce514c
https://hg.mozilla.org/mozilla-central/rev/dde6b08c74f7
https://hg.mozilla.org/mozilla-central/rev/e84fb66cf7f1
https://hg.mozilla.org/mozilla-central/rev/9a471f858d6b
https://hg.mozilla.org/mozilla-central/rev/d068e438438d
https://hg.mozilla.org/mozilla-central/rev/c9732200552d
https://hg.mozilla.org/mozilla-central/rev/404720032d1c

	William Chen [:wchen] Assignee
	Comment 130 • 11 years ago

gcc 4.7 fix

https://hg.mozilla.org/integration/mozilla-inbound/rev/061871df8f9e

	Philip Chee
	Comment 131 • 11 years ago

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

	Doug Turner (:dougt)
	Comment 132 • 11 years ago

chee, please file a follow up bug for Thunderbird.

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

https://hg.mozilla.org/mozilla-central/rev/061871df8f9e

	Madhava Enros [:madhava]
	Comment 136 • 11 years ago

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?

	Jason Smith [:jsmith]
	Comment 137 • 11 years ago

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

	Dave Townsend [:mossop]
	Comment 138 • 11 years ago

I'm seeing bug 852436 everytime my Firefox tries to show a XUL alert.

	Cornel Ionce [:noni] [Hubs QA]
	Comment 139 • 11 years ago

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?