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() {})

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

implement.

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

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

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

Is there a mockup of this?

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.

Thanks for the feedback Daniel. Please read: http://lists.w3.org/Archives/Public/public-webapps/2010AprJun/1150.html

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

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

clean up. requires bug 565187.

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.

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.

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

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.

what Ms2Ger said. Tiago, i do not understand what you are saying.

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

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

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.

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

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.

Should we dupe bug 527846 to this bug?

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

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?

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

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

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

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

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

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

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?

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

> This all is actually tricky. this is the same logic that the geolocation code uses too. We better ask jst.

(please ignore the printfs)

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

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

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

we probably should also rename this file.

Comment on attachment 472530 [details] [diff] [review] patch v.6 I would approve this if there were tests.

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

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

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 on attachment 473189 [details] [diff] [review] tests You have ok(1, ...) in few places. Use ok(true, ....)

Comment on attachment 473189 [details] [diff] [review] tests a=beltzner for the fennec implementation + tests

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.

http://hg.mozilla.org/mozilla-central/rev/da8eae9cbf24 http://hg.mozilla.org/mozilla-central/rev/5a9dc4ef0736 fennec part coming soon

lets track fennec specific issues in bug 595094 and firefox specific issues in but 594543. and now, lets close this bug.

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

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

Bug 782211 is about implementing Notifications API everywhere.

Dan, also read comment 63.