Last Comment Bug 573588 - Implement Desktop Notifications
: Implement Desktop Notifications
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal with 6 votes (vote)
: mozilla2.0b7
Assigned To: Doug Turner (:dougt)
:
Mentors:
http://dougt.org/mozilla/notification...
: 527846 (view as bug list)
Depends on: 594261 605040 605309
Blocks: 594543 595024 595094 595437
  Show dependency treegraph
 
Reported: 2010-06-21 15:20 PDT by Doug Turner (:dougt)
Modified: 2013-11-12 00:57 PST (History)
31 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
implementation v.1 (35.18 KB, patch)
2010-06-21 15:25 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
ff implementation v.1 (wip) (10.71 KB, patch)
2010-06-21 15:25 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
ff implementation v.2 (wip) (12.51 KB, patch)
2010-06-21 23:04 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
example (27.47 KB, image/png)
2010-06-28 14:52 PDT, imradyurrad
no flags Details
implementation v.2 (42.59 KB, patch)
2010-07-06 22:19 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
ff implementation v.2 (13.29 KB, patch)
2010-07-06 22:20 PDT, Doug Turner (:dougt)
dougt: review-
mbeltzner: approval2.0-
Details | Diff | Splinter Review
tests (10.47 KB, patch)
2010-07-06 22:21 PDT, Doug Turner (:dougt)
dougt: review-
Details | Diff | Splinter Review
patch v.3 (43.39 KB, patch)
2010-07-11 20:33 PDT, Doug Turner (:dougt)
bugs: review-
Details | Diff | Splinter Review
patch v.4 (40.87 KB, patch)
2010-08-19 14:10 PDT, Doug Turner (:dougt)
bugs: review-
Details | Diff | Splinter Review
patch v.5 (41.96 KB, patch)
2010-08-25 19:07 PDT, Doug Turner (:dougt)
bugs: review-
Details | Diff | Splinter Review
patch v.6 (41.56 KB, patch)
2010-09-06 21:36 PDT, Doug Turner (:dougt)
bugs: review+
mbeltzner: approval2.0+
Details | Diff | Splinter Review
fennec impelmentation v.1 (6.58 KB, patch)
2010-09-07 11:19 PDT, Doug Turner (:dougt)
mark.finkle: review-
Details | Diff | Splinter Review
tests (8.92 KB, patch)
2010-09-07 14:58 PDT, Doug Turner (:dougt)
bugs: review-
jmaher: feedback-
Details | Diff | Splinter Review
tests (9.43 KB, patch)
2010-09-08 13:06 PDT, Doug Turner (:dougt)
bugs: review+
mbeltzner: approval2.0+
Details | Diff | Splinter Review

Description Doug Turner (:dougt) 2010-06-21 15:20:38 PDT
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) 2010-06-21 15:25:24 PDT
Created attachment 452871 [details] [diff] [review]
implementation v.1
Comment 2 Doug Turner (:dougt) 2010-06-21 15:25:50 PDT
Created attachment 452872 [details] [diff] [review]
ff implementation v.1 (wip)
Comment 3 Doug Turner (:dougt) 2010-06-21 16:43:25 PDT
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) 2010-06-21 16:43:40 PDT
implement.
Comment 5 Doug Turner (:dougt) 2010-06-21 23:04:38 PDT
Created attachment 452980 [details] [diff] [review]
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 2010-06-21 23:26:04 PDT
navigator.mozNotification, please. Also, you can just put it on nsIDOMNavigator, that interface isn't frozen.
Comment 7 Simon Montagu :smontagu 2010-06-22 00:23:39 PDT
(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 2010-06-25 16:52:52 PDT
Is there a mockup of this?
Comment 9 Daniel Spiewak 2010-06-28 14:11:44 PDT
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) 2010-06-28 14:19:04 PDT
Thanks for the feedback Daniel.  Please read:

http://lists.w3.org/Archives/Public/public-webapps/2010AprJun/1150.html
Comment 11 Daniel Spiewak 2010-06-28 14:36:43 PDT
(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 2010-06-28 14:52:07 PDT
Created attachment 454633 [details]
example
Comment 13 Rangi 2010-07-01 13:33:05 PDT
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) 2010-07-06 22:19:28 PDT
Created attachment 456224 [details] [diff] [review]
implementation v.2
Comment 15 Doug Turner (:dougt) 2010-07-06 22:20:18 PDT
Created attachment 456225 [details] [diff] [review]
ff implementation v.2
Comment 16 Doug Turner (:dougt) 2010-07-06 22:21:01 PDT
Created attachment 456226 [details] [diff] [review]
tests
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-07-07 14:45:18 PDT
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) 2010-07-11 20:33:44 PDT
Created attachment 456793 [details] [diff] [review]
patch v.3

clean up.  requires bug 565187.
Comment 19 Olli Pettay [:smaug] 2010-07-19 04:34:04 PDT
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) 2010-07-30 00:15:41 PDT
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 2010-07-30 02:22:36 PDT
(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á 2010-07-30 02:28:10 PDT
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) 2010-07-30 09:26:33 PDT
what Ms2Ger said.

Tiago, i do not understand what you are saying.
Comment 24 Tiago Sá 2010-07-30 10:54:42 PDT
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 2010-07-30 10:57:38 PDT
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á 2010-07-30 10:59:16 PDT
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 2010-07-30 11:01:22 PDT
(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) 2010-07-30 11:07:50 PDT
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?) 2010-08-09 07:11:06 PDT
Should we dupe bug 527846 to this bug?
Comment 30 Doug Turner (:dougt) 2010-08-09 13:55:05 PDT
*** Bug 527846 has been marked as a duplicate of this bug. ***
Comment 31 Doug Turner (:dougt) 2010-08-19 14:10:30 PDT
Created attachment 467540 [details] [diff] [review]
patch v.4
Comment 32 Olli Pettay [:smaug] 2010-08-20 02:18:18 PDT
So where is the draft specification for the thing we're implementing here?
Comment 33 Olli Pettay [:smaug] 2010-08-20 03:04:12 PDT
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 2010-08-20 10:27:57 PDT
(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) 2010-08-20 10:34:53 PDT
(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] 2010-08-20 11:33:09 PDT
(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) 2010-08-20 11:36:31 PDT
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) 2010-08-21 21:33:28 PDT
>>+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) 2010-08-21 22:18:11 PDT
(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) 2010-08-21 22:28:35 PDT
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] 2010-08-22 10:25:46 PDT
(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) 2010-08-25 19:06:23 PDT
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) 2010-08-25 19:07:31 PDT
Created attachment 469319 [details] [diff] [review]
patch v.5
Comment 44 Olli Pettay [:smaug] 2010-09-06 06:36:09 PDT
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) 2010-09-06 21:36:52 PDT
Created attachment 472530 [details] [diff] [review]
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) 2010-09-06 21:39:07 PDT
(please ignore the printfs)
Comment 47 Olli Pettay [:smaug] 2010-09-06 23:48:48 PDT
(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] 2010-09-07 00:05:41 PDT
Ok. It seems to be ok because nsGlobalWindow::SetNewDocument calls
mNavigator->SetDocShell(nsnull).
Comment 49 Olli Pettay [:smaug] 2010-09-07 00:20:28 PDT
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] 2010-09-07 00:37:18 PDT
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) 2010-09-07 11:19:51 PDT
Created attachment 472695 [details] [diff] [review]
fennec impelmentation v.1

we probably should also rename this file.
Comment 52 Mike Beltzner [:beltzner, not reading bugmail] 2010-09-07 11:20:10 PDT
Comment on attachment 472530 [details] [diff] [review]
patch v.6

I would approve this if there were tests.
Comment 53 Doug Turner (:dougt) 2010-09-07 11:34:09 PDT
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?) 2010-09-07 11:45:18 PDT
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) 2010-09-07 14:58:38 PDT
Created attachment 472789 [details] [diff] [review]
tests
Comment 56 Olli Pettay [:smaug] 2010-09-08 06:33:43 PDT
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 ) 2010-09-08 11:44:54 PDT
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) 2010-09-08 13:06:47 PDT
Created attachment 473189 [details] [diff] [review]
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] 2010-09-08 13:14:37 PDT
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] 2010-09-08 13:52:37 PDT
Comment on attachment 473189 [details] [diff] [review]
tests

a=beltzner for the fennec implementation + tests
Comment 61 Mike Beltzner [:beltzner, not reading bugmail] 2010-09-08 14:08:00 PDT
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 63 Doug Turner (:dougt) 2010-09-10 00:05:40 PDT
lets track fennec specific issues in bug 595094 and firefox specific issues in but 594543.

and now, lets close this bug.
Comment 65 Dan Dascalescu 2012-11-11 05:41:43 PST
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] 2012-11-11 06:26:40 PST
Bug 782211 is about implementing Notifications API everywhere.
Comment 67 Doug Turner (:dougt) 2012-11-11 11:21:15 PST
Dan, also read comment 63.

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