573588 - Implement Desktop Notifications

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Doug Turner (:dougt)]

I reviewed the WebNotification spec:
   http://dev.w3.org/2006/webapi/WebNotifications/publish/

and the google spec:
   http://code.google.com/p/gears/wiki/NotificationAPI


and I think both are a bit more complex than we need.


My current thinking is title, description, uri to image, and a callback for if/when the user clicks on the notification:

navigator.notification.notify("title",
                              "description",
			      "uri to image",
			      function() {})

Comment 1

[Doug Turner (:dougt)]

Attachment: implementation v.1

Comment 2

[Doug Turner (:dougt)]

Attachment: ff implementation v.1 (wip)

Comment 3

[Doug Turner (:dougt)]

what is left to implemenent in FF:

1) gavin changed out permission infobars worked.  that should change in the wip patch.
2) tie into Page info -> Permissions so that the user can remove permissions.
3) Disable in PB mode
4) tie into Clear History

Comment 4

[Doug Turner (:dougt)]

implement.

Comment 5

[Doug Turner (:dougt)]

Attachment: ff implementation v.2 (wip)

updates to mozilla-central tip.  Fixes 1, 2, 3.  Not sure if I have to do anything special for 4.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

navigator.mozNotification, please. Also, you can just put it on nsIDOMNavigator, that interface isn't frozen.

Comment 7

[Simon Montagu :smontagu]

(In reply to comment #0)
> My current thinking is title, description, uri to image, and a callback for
> if/when the user clicks on the notification:

A "direction" property would also be good for localizability.

Comment 8

[imradyurrad]

Is there a mockup of this?

Comment 9

[Daniel Spiewak]

It would be nice to get desktop notifications in Firefox, but side-stepping the W3C spec simply because it looks "too complicated" just seems a little...anti-standard.  The ironic thing is aside from the permissions request side of things (which you're going to need eventually), the API you've created is very nearly identical to the mandatory part of the W3C spec.  Browsers don't *need* to implement the notifications.createWebNotification function.

The only other fundamental difference between your APIs seems to be your callback function, which I admit that I do like very much.  If you want this functionality though, you should push for it in the W3C spec, rather than inventing something new.

I'm voting for this bug because I think it's an important feature for the modern browser, but I strongly hope that Firefox ends up implementing the emerging standard, rather than running off in its own direction.

Comment 10

[Doug Turner (:dougt)]

Thanks for the feedback Daniel.  Please read:

http://lists.w3.org/Archives/Public/public-webapps/2010AprJun/1150.html

Comment 11

[Daniel Spiewak]

(In reply to comment #10)
> Thanks for the feedback Daniel.  Please read:
> 
> http://lists.w3.org/Archives/Public/public-webapps/2010AprJun/1150.html

Terrific!  This has certainly put to rest any doubts I had about implementation fragmentation on desktop notifications.  I look forward to the eventual results of this effort.

Comment 12

[imradyurrad]

Attachment: example

Comment 13

[Rangi]

I would LOVE this new feature, I really hope it makes it into Firefox soon. Chrome already has something similar I think (http://bit.ly/22J050).

Comment 14

[Doug Turner (:dougt)]

Attachment: implementation v.2

Comment 15

[Doug Turner (:dougt)]

Attachment: ff implementation v.2

Comment 16

[Doug Turner (:dougt)]

Attachment: tests

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 456225 [details] [diff] [review]
ff implementation v.2

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>+    // gavin, what should i use other than geo-notification-icon?

You need to add your own element to browser.xul (with relevant styling in browser.css) - see the "addons-notification-icon" and .popup-notification-icon parts of http://hg.mozilla.org/mozilla-central/rev/b1b50d483f1e for an example.

Comment 18

[Doug Turner (:dougt)]

Attachment: patch v.3

clean up.  requires bug 565187.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 456793 [details] [diff] [review]
patch v.3


>+
>+
>+//*****************************************************************************
>+//    nsNavigator::nsIDOMNavigatorDesktopNotification
>+//*****************************************************************************
>+
>+NS_IMETHODIMP nsNavigator::GetNotification(nsIDOMDesktopNotificationCenter **_retval)

aRetVal

>+{
>+  NS_ENSURE_ARG_POINTER(_retval);
>+  *_retval = nsnull;
>+
>+  nsRefPtr<nsDesktopNotificationCenter> notification = new nsDesktopNotificationCenter();
>+  if (!notification)
>+    return NS_ERROR_FAILURE;
>+
>+  if (!mDocShell)
>+    return NS_ERROR_FAILURE;
No need for this because do_GetInterface is null-safe.

>+
>+  nsCOMPtr<nsIDOMWindow> contentDOMWindow(do_GetInterface(mDocShell));
>+  if (!contentDOMWindow)
>+    return NS_ERROR_FAILURE;
>+

Create notification here and then you can
do 
if (!notification || NS_FAILED(notification->Init(contentDOMWindow))
  return NS_ERROR_FAILURE;

>+  if (NS_FAILED(notification->Init(contentDOMWindow)))
>+    return NS_ERROR_FAILURE;
>+  
>+  NS_ADDREF(*_retval = notification);    

Maybe something like
*aRetVal = notification.forget();


>+
>+[scriptable, function, uuid(4D3768A4-F224-4147-8134-7FF2200CE455)]
>+interface nsIDOMDesktopNotificationCallback : nsISupports {
>+  void handleEvent();
Why this? Why couldn't you just make this all use
real DOM events?

>+class nsDesktopNotificationCenter : public nsIDOMDesktopNotificationCenter, public nsDOMDeviceCenterBase
...in which case this could inherit nsDOMEventTargetHelper and you'd get all the event handling for free.

I don't want an interface which has onclick property, but isn't really an event target.

Comment 20

[Josh Gross (:jgross)]

Is there a reason this is being implemented as `navigator.notifications.notify,` rather than `window.notifications.createNotification`, as in the proposed standard? While some of the standard might be too verbose (requestPermission), putting the notifications object on `navigator` seems like a strange choice.

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to comment #20)
> Is there a reason this is being implemented as
> `navigator.notifications.notify,` rather than
> `window.notifications.createNotification`, as in the proposed standard? While
> some of the standard might be too verbose (requestPermission), putting the
> notifications object on `navigator` seems like a strange choice.

To avoid polluting the global namespace even more, I suppose.

Comment 22

[Tiago Sá]

I'm not involved, but I disagree. What if you're browsing in another window and a page issues a notification? You'll want to see the notification, right? I mean, if you see the notification when browsing other tabs in the same window, you should see the notification too when browsing other tabs in other windows. It's a matter of consistency, now that new windows on the browser don't mean jack.

Comment 23

[Doug Turner (:dougt)]

what Ms2Ger said.

Tiago, i do not understand what you are saying.

Comment 24

[Tiago Sá]

Sorry for the scruffy English.

What I'm saying is there is a difference between "window" and "navigator". Supposedly, "navigator" refers to the browser as a whole, and, if used, and I really have no technical background here, so bear with me please, if used, the notification would appear in the focused window, even when that window didn't have the tab that issued the notification. If "window" is used, on the other hand, it's fair to supposed that the notification would be tied to the window that houses the tab in question. Which is, in my opinion, less than optimal. Not only would it make notifications useless in tabless browsers or for users who use multiple windows instead of multiple tabs, but it would also be inconsistent with the way browsers themselves behave. At the end of the day, different browser windows don't mean different sessions or different profiles. They share the same settings, the same history, the same passwords and all that. Moreover, they will share the same App Tabs and on and on in Firefox 4, so I believe notifications should appear in whichever window is focused, and possibly even be shared between windows if the user changes window without dismissing the notification.

This will probably bring up some different design questions for the spec itself, but that's another thing, I believe.

I hope my English is better now :)

Comment 25

[Daniel Spiewak]

There's an even more compelling reason to use window rather than navigator: the W3C draft spec says it should be window.notifications.  As someone who is actually using desktop notifications in a fairly notifications-heavy production app, I cannot stress enough how much I *don't* want to code against three different APIs (the W3C draft spec API, Chrome's webkitNotifications API, and now Firefox's own special flavor).

Comment 26

[Tiago Sá]

Well, the standard is the standard, right? There's still no standard, so you can't expect one markup to work for all browsers. Maybe it will in some cases, but it's still not set in stone, so I don't see why we should simply discard possibly valid criticism or contributions...

IMHO.

Comment 27

[Daniel Spiewak]

(In reply to comment #26)
> Well, the standard is the standard, right? There's still no standard, so you
> can't expect one markup to work for all browsers.

That's true now, but I think we all agree that this situation is not what we want to end up with.  Standards only work if everyone agrees to implement them, and consciously moving in a different direction is not the way to go about it.

Comment 28

[Doug Turner (:dougt)]

taigo, I think totally understand what you are saying.  I think what you are
saying (if I may rephrase a bit) - these notifications are scoped by the app
tab, or by the window.  Therefore it makes sense to have the API be window.*
specific.

However, this feature is about the user agent (browser) sending notifications
to something that is system level (like growl on the mac, or libnotify).  So,
if you are using FF4 and have App Tabs and some page does a desktop
notification which you have opt'ed in for, you will see a notification at the
system level.


Daniel, when there is a reasonable spec, we will conform.  Right now there
isn't, but we are working on it.  I think that the W3C just pushed the spec out
of webapps and into a new WG.  When that starts, we can figure out what
namespace this lives in.

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

Should we dupe bug 527846 to this bug?

Comment 31

[Doug Turner (:dougt)]

Attachment: patch v.4

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

So where is the draft specification for the thing we're implementing here?

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 467540 [details] [diff] [review]
patch v.4

>+[scriptable, function, uuid(CCEA6185-0A3D-45AB-9058-1004DD4B8C50)]
>+interface nsIDOMDesktopNotificationCenter : nsISupports
>+{
>+  nsIDOMDesktopNotification createNotification(in DOMString title,
>+                                               in DOMString description,
>+                                               [optional] in DOMString iconURL);
>+};
For now this should be mozCreateNotification(...)

>+/**
>+ * Interface allows access to a notice and is passed to
>+ * the nsIDesktopNotificationPrompt so that the application can approve
>+ * or deny the request.
>+ */
>+[scriptable, uuid(A23B1236-9374-4591-97BF-5413FC4813A6)]
>+interface nsIDOMDesktopNotificationRequest : nsISupports {
>+
>+  readonly attribute nsIURI requestingURI;
>+  readonly attribute nsIDOMWindow requestingWindow;
>+
>+  void cancel();
>+  void allow();
Please document this and other interfaces.
I for example had to look at the source code to understand whether requestingURI means
the icon URI or the URI of the page.


>+class NotificationRequestAllowEvent : public nsRunnable {
'{' should be in the next line


>+public:
>+  NotificationRequestAllowEvent(nsDOMDesktopNotification* request)
>+    : mRequest(request)
>+  {
>+  }
>+  
>+  NS_IMETHOD Run() {
'{' should be in the next line

>+nsresult
>+nsDOMDesktopNotification::GetRequestingURI(nsIURI * *aRequestingURI)
Shouldn't this be NS_IMETHODIMP, not nsresult?

>+nsresult
>+nsDOMDesktopNotification::GetRequestingWindow(nsIDOMWindow * *aRequestingWindow)
Same here.

You must initialize nsDOMEventTargetHelper::mOwner and nsDOMEventTargetHelper::mScriptContext
somewhere.


>+nsDOMDesktopNotification::nsDOMDesktopNotification(const nsAString & title,
>+                                                   const nsAString & description,
>+                                                   const nsAString & iconURL,
>+                                                   nsDesktopNotificationCenter* notificationCenter)
>+{
>+  mTitle = title;
>+  mDescription = description;
>+  mIconURL = iconURL;
>+  mDesktopNotificationCenter = notificationCenter;
>+
>+  NS_ASSERTION(mDesktopNotificationCenter, "nsDOMDesktopNotification created without a parent");
>+}
So there isn't any kind of security check for mIconURL? IMHO there should be same-origin check.


>+void
>+nsDOMDesktopNotification::HandleAlertServiceNotification(const char *aTopic)
>+{
>+  if (!mDesktopNotificationCenter->WindowOwnerStillExists())
>+    return;
>+
>+  if (mOnClickCallback && !strcmp("alertclickcallback", aTopic))
>+    DispatchNotificationEvent(NS_LITERAL_STRING("onclick"));
>+
>+  else if (mOnCloseCallback && !strcmp("alertfinished", aTopic))
>+    DispatchNotificationEvent(NS_LITERAL_STRING("onclose"));
>+}
Er, what? Does this really work.
The event names don't start with "on" prefix.
They are sjust "click" and "close".
And why the mOnClickCallback/mOnCloseCallback checks?
Those can be null if the event listener is added using .addEventListener.
This all needs tests!



>+NS_IMETHODIMP nsDOMDesktopNotification::SetOnclick(nsIDOMEventListener * aOnclick)
>+{
>+  return RemoveAddEventListener(NS_LITERAL_STRING("onclick"),
>+                                mOnClickCallback, aOnclick);
"click"



>+NS_IMETHODIMP nsDOMDesktopNotification::SetOnclose(nsIDOMEventListener * aOnclose)
>+{
>+  return RemoveAddEventListener(NS_LITERAL_STRING("onclose"),
>+                                mOnCloseCallback, aOnclose);
"close"


>+nsDesktopNotificationCenter::CreateNotification(const nsAString & title,
>+                                               const nsAString & description,
>+                                               const nsAString & iconURL,
>+                                               nsIDOMDesktopNotification **_retval)
>+
>+  nsRefPtr<nsIDOMDesktopNotification> notification = new nsDOMDesktopNotification(title, description, iconURL, this);
>+  NS_IF_ADDREF(*_retval = notification);
Something like:
*_retval = notification.forget().get();
And please rename _retval. May aResult or aDesktopNotification


>+  NS_IMETHOD Run() {
'{' should be in the next line.

>+    nsCOMPtr<nsIDesktopNotificationPrompt> prompt =
>+      do_GetService(NS_DOM_DESKTOP_NOTIFICATION_PROMPT_CONTRACTID);
>+    NS_ASSERTION(prompt, "null notification prompt");
Useless assertion.


>+class AlertServiceObserver: public nsIObserver
...
>+  nsDOMDesktopNotification* mNotification;
What guarantees that this doesn't ever point to a dead object?

Comment 34

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to comment #33)
> Comment on attachment 467540 [details] [diff] [review]
> patch v.4
> >+nsDesktopNotificationCenter::CreateNotification(const nsAString & title,
> >+                                               const nsAString & description,
> >+                                               const nsAString & iconURL,
> >+                                               nsIDOMDesktopNotification **_retval)
> >+
> >+  nsRefPtr<nsIDOMDesktopNotification> notification = new nsDOMDesktopNotification(title, description, iconURL, this);
> >+  NS_IF_ADDREF(*_retval = notification);
> Something like:
> *_retval = notification.forget().get();
> And please rename _retval. May aResult or aDesktopNotification

(Or perhaps |notification.forget(aDesktopNotification);|.)

Comment 35

[Eli Grey (:sephr)]

(In reply to comment #33)
> So there isn't any kind of security check for mIconURL? IMHO there should be
same-origin check.

Why do you think that? It's mostly the norm that images are allowed to be displayed cross-domain as long as pixel data isn't exposed unless explicitly granted by Access Control headers.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #35)
> (In reply to comment #33)
> > So there isn't any kind of security check for mIconURL? IMHO there should be
> same-origin check.
> 
> Why do you think that? It's mostly the norm that images are allowed to be
> displayed cross-domain as long as pixel data isn't exposed unless explicitly
> granted by Access Control headers.
Yes, in web pages, but we're going to show the images in the OS notifications.
I think that is quite a different case.

Anyway, I just expressed my opinion. Doug, you may want to ask
dveditz or someone. This might a feature which requires a security review.

Comment 37

[Doug Turner (:dougt)]

scheduling now.  thanks for your comments, i will roll up another patch for your viewing pleasure (probably over the weekend)

Comment 38

[Doug Turner (:dougt)]

>>+nsresult
>>+nsDOMDesktopNotification::GetRequestingURI(nsIURI * *aRequestingURI)
>Shouldn't this be NS_IMETHODIMP, not nsresult?

>>+nsresult
>>+nsDOMDesktopNotification::GetRequestingWindow(nsIDOMWindow * *aRequestingWindow)
>Same here.

No.  the nsDesktoNotificationRequest is passed back to the front end code.  This allows the front end to figure out which window and what url the request is coming from.  The request holds onto a reference to the nsDOMDesktopNotification which was created from CreateNotification.  This nsDOMDesktopNotification is initialized with a url and a window.  So… the one in the nsDOMDesktopNotification just is plumbing code and doesn't need to be exposed via XPCOM and doesn't need to be NS_IMETHOD.


> So there isn't any kind of security check for mIconURL? IMHO there should be same-origin check.

Checking with mozilla security team.  will let you know.


> This all needs tests!

See attachment 456226 [details] [diff] [review]


> What guarantees that this doesn't ever point to a dead object?

the alert service observer is owned by the nsDOMDesktopNotification.  So, mNotification is basically a back pointer.  Comment added.

Comment 39

[Eli Grey (:sephr)]

(In reply to comment #36)
> Yes, in web pages, but we're going to show the images in the OS notifications.
> I think that is quite a different case.

Nothing would stop me from rehosting images from a site used for desktop notifications. This would end up making it harder for websites that store all of their images on a third-party CDN that they don't have header access to (in order to set the appropriate Access-Control headers that would be needed to use the images on the CDN).

Comment 40

[Doug Turner (:dougt)]

If I opt-in to something like desktop notifications, I do not think that I care if the images come from the same exact uri or a 3rd party or some totally unrelated site.  What precisely is the risk?

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #40)
> If I opt-in to something like desktop notifications, I do not think that I care
> if the images come from the same exact uri or a 3rd party or some totally
> unrelated site.  What precisely is the risk?
When a security bug is found in OS image libraries, there is no way to
opt-in only for some websites which are known to use only valid
images.
But I don't know if that is important case enough.

Comment 42

[Doug Turner (:dougt)]

Comment on attachment 456226 [details] [diff] [review]
tests

some sytem notifications can be "sticky".  These do not return an notification that something closed.  litmus tests are the way to go here.

Comment 43

[Doug Turner (:dougt)]

Attachment: patch v.5

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 469319 [details] [diff] [review]
patch v.5


>+NS_IMETHODIMP nsNavigator::GetMozNotification(nsIDOMDesktopNotificationCenter **aRetVal)
>+{
>+  NS_ENSURE_ARG_POINTER(aRetVal);
>+  *aRetVal = nsnull;
>+
>+  nsCOMPtr<nsPIDOMWindow> window(do_GetInterface(mDocShell));
>+  NS_ENSURE_TRUE(window, NS_ERROR_FAILURE);
>+    
>+  nsCOMPtr<nsIDocument> document = do_GetInterface(mDocShell);
>+  NS_ENSURE_TRUE(document, NS_ERROR_FAILURE);
>+
>+  nsIScriptGlobalObject *sgo = document->GetScopeObject();
>+  NS_ENSURE_TRUE(sgo, NS_ERROR_FAILURE);
>+
>+  nsIScriptContext *scx = sgo->GetContext();
>+  NS_ENSURE_TRUE(scx, NS_ERROR_FAILURE);
>+

This all is actually tricky.
What if a page has a iframe which has document in the same domain.
Then the main page takes reference to the iframe's navigator.
Then some new page is loaded to the iframe and
after that the main page uses navigator.mozNotification.
Can the main page then cause a notification which looks like a notification from the the page
loaded in iframe?
Please test. r- until this issue is resolved (either by showing that it isn't an issue at all
or by fixing it somehow.)


>+  nsRefPtr<nsDesktopNotificationCenter> notification =
>+    new nsDesktopNotificationCenter(window->GetCurrentInnerWindow(),
>+                                    scx);
>+
>+  if (!notification)
>+    return NS_ERROR_FAILURE;
>+
>+  NS_ADDREF(*aRetVal = notification);
You should do something like *aRetVal = notification.forget().get();


>+void
>+nsDOMDesktopNotification::DispatchNotificationEvent(const nsString& aName)
>+{
>+  if (NS_FAILED(CheckInnerWindowCorrectness()))
>+    return;
>+
>+  nsCOMPtr<nsIDOMEvent> event;
>+  nsresult rv = NS_NewDOMEvent(getter_AddRefs(event), nsnull, nsnull);
>+  if (NS_SUCCEEDED(rv))
>+  {
'{' should be in the previous line

>+    // it doesn't bubble, and it isn't cancelable
>+    rv = event->InitEvent(aName, PR_FALSE, PR_FALSE);
>+    if (NS_SUCCEEDED(rv))
>+    {
ditto


>+      nsCOMPtr<nsIPrivateDOMEvent> privateEvent = do_QueryInterface(event);
>+      privateEvent->SetTrusted(PR_TRUE);
>+      DispatchDOMEvent(nsnull, event, nsnull, nsnull);
>+    }
>+  }
>+}
>+
>+PRBool
>+nsDOMDesktopNotification::WindowOwnerStillExists()
You could just use nsDOMEventTargetHelper:: CheckInnerWindowCorrectness(), I think.


>+void
>+nsDOMDesktopNotification::HandleAlertServiceNotification(const char *aTopic)
>+{
>+  if (!WindowOwnerStillExists())
>+    return;
>+
>+  if (!strcmp("alertclickcallback", aTopic))
Add '{'

>+    DispatchNotificationEvent(NS_LITERAL_STRING("click"));
>+
>+  else if (!strcmp("alertfinished", aTopic)) {
'}' before else

>+nsDOMDesktopNotification::Show()
>+{
>+  nsCOMPtr<nsIRunnable> request;
>+  if (nsContentUtils::GetBoolPref("notification.prompt.testing", PR_FALSE) &&
>+      nsContentUtils::GetBoolPref("notification.prompt.testing.allow", PR_TRUE))
>+    request  = new NotificationRequestAllowEvent(this);
>+  else
>+    request = new nsDesktopNotificationRequest(this);
Same here. Add missing brackets

>+  NS_IMETHOD Run()
>+  {
>+    nsCOMPtr<nsIDesktopNotificationPrompt> prompt =
>+      do_GetService(NS_DOM_DESKTOP_NOTIFICATION_PROMPT_CONTRACTID);
>+    if (prompt)
brackets

Comment 45

[Doug Turner (:dougt)]

Attachment: patch v.6

> This all is actually tricky.

this is the same logic that the geolocation code uses too.  We better ask jst.

Comment 46

[Doug Turner (:dougt)]

(please ignore the printfs)

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #45)
> this is the same logic that the geolocation code uses too.  We better ask jst.
Not ask anyone, but test ;)

I was trying to break geolocation, but couldn't.

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

Ok. It seems to be ok because nsGlobalWindow::SetNewDocument calls
mNavigator->SetDocShell(nsnull).

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 472530 [details] [diff] [review]
patch v.6

>+NS_IMETHODIMP nsNavigator::GetMozNotification(nsIDOMDesktopNotificationCenter **aRetVal)
>+{
>+  NS_ENSURE_ARG_POINTER(aRetVal);
>+  *aRetVal = nsnull;
>+
>+  nsCOMPtr<nsPIDOMWindow> window(do_GetInterface(mDocShell));
>+  NS_ENSURE_TRUE(window, NS_ERROR_FAILURE);
>+    
>+  nsCOMPtr<nsIDocument> document = do_GetInterface(mDocShell);
>+  NS_ENSURE_TRUE(document, NS_ERROR_FAILURE);
>+
>+  nsIScriptGlobalObject *sgo = document->GetScopeObject();
Get sgo from window's current inner window.
nsCOMPtr<nsIScriptGlobalObject> sgo = window->GetCurrentInnerWindow();

>+
>+  if (!notification)
>+    return NS_ERROR_FAILURE;
Missing brackets


>+
>+XPIDLSRCS =                                    \
>+            nsIDOMNavigatorDesktopNotification.idl    \
>+            nsIDOMDesktopNotification.idl             \
>+            nsIDesktopNotificationPrompt.idl          \
>+            $(NULL)

Why the empty line before nsIDOMNavigatorDesktopNotification.idl?

>+[scriptable, function, uuid(00D49D61-3D08-4AC6-B662-5E421F60CC2F)]
>+interface nsIDesktopNotificationPrompt : nsISupports {
{ should be in the next line. Also in other interfaces.

>+nsDOMDesktopNotification::PostDesktopNotification()
>+{
>+  nsCOMPtr<nsIAlertsService> alerts = do_GetService("@mozilla.org/alerts-service;1");
>+  if (!alerts)
>+    return;
>+
>+  if (!mObserver)
>+    mObserver = new AlertServiceObserver(this);
Could you add the missing brackets

>+nsDOMDesktopNotification::~nsDOMDesktopNotification()
>+{
>+   if (mObserver)
>+      mObserver->Disconnect();
Brackets and fix indentation 


>+nsDOMDesktopNotification::DispatchNotificationEvent(const nsString& aName)
>+{
>+  if (NS_FAILED(CheckInnerWindowCorrectness()))
>+    return;
Brackets



>+nsDOMDesktopNotification::HandleAlertServiceNotification(const char *aTopic)
>+{
>+  if (NS_FAILED(CheckInnerWindowCorrectness()))
>+    return;
ditto

And remove printfs :)

Comment 50

[Axel Hecht [:Pike]]

Comment on attachment 456225 [details] [diff] [review]
ff implementation v.2

Can we have localization notes explaining the substitution in 
+desktopNotification.siteWantsToKnow=%S wants to use desktop notifications.
+desktopNotification.fileWantsToKnow=The file %S wants use desktop notifications.
? Just that it's host and path, resp.

Comment 51

[Doug Turner (:dougt)]

Attachment: fennec impelmentation v.1

we probably should also rename this file.

Comment 52

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 472530 [details] [diff] [review]
patch v.6

I would approve this if there were tests.

Comment 53

[Doug Turner (:dougt)]

alex - no problem... i will update that part of the patch, and wait for gavin's review on the rest.

Comment 54

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 472695 [details] [diff] [review]
fennec impelmentation v.1

>diff --git a/components/GeolocationPrompt.js b/components/GeolocationPrompt.js

GeolocationPrompt.js -> ContentPrompts.js

>+function DesktopPrompt() {}

DesktopPrompt -> NotificationPrompt

"Desktop" is too general and "Notification" is the real story here

>+  prompt: function(aRequest) {
>+
>+      
>+    let pm = Services.perms;

Remove the empty lines at top of | prompt |

>+    if (aRequest.requestingWindow) {
>+      let requestingWindow = aRequest.requestingWindow.top;
>+      let chromeWin = getChromeWindow(requestingWindow).wrappedJSObject;
>+      notificationBox = chromeWin.getNotificationBox(requestingWindow);
>+    } else {
>+      
>+        let chromeWin;// = aRequest.requestingElement.ownerDocument.defaultView;
>+      notificationBox = chromeWin.Browser.getNotificationBox();
>+    }

You must fix aRequest.requestingElement so it works. This is the only way this notification will work from remote pages

>+        label: browserBundle.GetStringFromName("desktop-notification.allow"),

>+        label: browserBundle.GetStringFromName("desktop-notification.dontAllow"),

>+      let message = browserBundle.formatStringFromName("desktop-notification.siteWantsTo",

We don't typically see "-" in string entities. Can you use "desktopNotification." ?

>+# Desktop notification UI
>+desktop-notification.allow=Allow
>+desktop-notification.dontAllow=Don't allow
>+desktop-notification.siteWantsTo=%S wants use notifications.

See above

r- for the aRequest.requestingElement fix

Comment 55

[Doug Turner (:dougt)]

Attachment: tests

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 472789 [details] [diff] [review]
tests

There are some tab characters, which is why indentation looks wrong.
Please fix that.

>diff --git a/dom/tests/html/desktop_notifications.html b/dom/tests/html/desktop_notifications.html
So this is for litmus? Could you change the filename to indicate that.

Can you think of any way to test click handling without litmus?
Could MockAlertsService just cause artificial click?

Comment 57

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 472789 [details] [diff] [review]
tests

>Bug 573588 - Implement Desktop Notifications - Tests r=
>
>diff --git a/dom/tests/html/desktop_notifications.html b/dom/tests/html/desktop_notifications.html

we should remove this test file from the patch.


>+++ b/dom/tests/mochitest/notification/test_basic_notification.html
>+
>+force_prompt(true);
>+SimpleTest.waitForExplicitFinish();
>+
>+ok(navigator.mozNotification, "test for notification.");
>+
>+var notification = navigator.mozNotification.createNotification("test", "test");
>+ok(notification, "test to ensure we can create a notification");
>+
>+notification.onclose =  function() {
>+  ok(1, "notification was display and is now closing");
>+  reset_prompt();
>+  SimpleTest.finish();
>+};
>+
>+notification.onclick =  function() {
>+  ok(false, "Strange.  Automated testing should have never been able to click this notification.");
>+}
>+
>+notification.show();

in a failure case I don't see how we reset_prompt().  If we get an onclick will we also get an onclose()?  I am just concerned because we are changing the alert service and future tests could depend on this and fail if this times out.

Also is this test expected to work on Fennec?  If not, we should skip it inside of the Makefile similar to this:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/Makefile.in#76

Lastly what about IPC?  I think this would work good as we could put notification_common.js into the chrome process and just send a force_prompt or reset_prompt message through messageManager to do the dirty work.

Comment 58

[Doug Turner (:dougt)]

Attachment: tests

adds new test for onclick.

addresses joels comments (and, yes, close is always called from the mock alert service.  IPC support will come as a follow up)

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 473189 [details] [diff] [review]
tests

You have ok(1, ...) in few places.
Use ok(true, ....)

Comment 60

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 473189 [details] [diff] [review]
tests

a=beltzner for the fennec implementation + tests

Comment 61

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 456225 [details] [diff] [review]
ff implementation v.2

I think we pretty explicitly do not want this for Firefox 4 / Gecko 2.0 at this time. We'll take it there after we branch. We haven't had the time to think through the design as much as I'd like us to have done.

Comment 62

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/da8eae9cbf24
http://hg.mozilla.org/mozilla-central/rev/5a9dc4ef0736


fennec part coming soon

Comment 63

[Doug Turner (:dougt)]

lets track fennec specific issues in bug 595094 and firefox specific issues in but 594543.

and now, lets close this bug.

Comment 64

[Eric Shepherd [:sheppy]]

Documentation complete:

https://developer.mozilla.org/en/DOM/Displaying_notifications
https://developer.mozilla.org/en/DOM/navigator.mozNotification
https://developer.mozilla.org/en/DOM/notification

And linked to at:

https://developer.mozilla.org/En/Mobile

Comment 65

[Dan Dascalescu]

The title of this ticket is "Implement Desktop Notifications".

Since notifications were only implemented for Firefox Mobile, I'd hardly call the issue fixed.

Please reopen until notifications work on Firefox Desktop as well. It's ridiculous that there have to be tons of extensions to emulate support for window.webkitNotifications.createNotification()

Comment 66

[Olli Pettay [:smaug][[e-mail]]]

Bug 782211 is about implementing Notifications API everywhere.

Comment 67

[Doug Turner (:dougt)]

Dan, also read comment 63.