References: Bug 629280 - Show W3C Desktop Notifications Bug 782211 (Implement notification API spec): > This is a bug to track the implementation of the w3c notification spec. See: Bug 782211 Part 7: Added permission prompt for desktop notifications on desktop. Test page: http://jsbin.com/notification/1/edit

ui-r? to Stefanh for the Mac parts. > +++ b/suite/browser/pageinfo/pageInfo.xul > > <richlistitem class="permission" orient="vertical"> > + <label id="permNotificationLabel" class="permissionLabel" > + value="&permNotifications;" control="desktop-notificationRadioGroup"/> > + <hbox role="group" aria-labelledby="permNotificationLabel"> > + <checkbox class="indent" id="desktop-notificationDef" > + command="cmd_desktop-notificationDef" label="&permUseDefault;"/> > + <spacer flex="1"/> > + <radiogroup id="desktop-notificationRadioGroup" orient="horizontal"> > + <radio id="desktop-notification-1" command="cmd_desktop-notificationToggle" label="&permAllow;"/> > + <radio id="desktop-notification-2" command="cmd_desktop-notificationToggle" label="&permBlock;"/> There should be a third state (Allow for Session) but I don't know how to implement this. Firefox pageInfo implementation is two-state like here. > +++ b/suite/browser/pageinfo/permissions.js > + "desktop-notification": function getNotificationDefaultPermission() Hyphens cause JS to complain. > function onRadioClick(aPartId) > { > var radioGroup = document.getElementById(aPartId + "RadioGroup"); > var id = radioGroup.selectedItem.id; > - var permission = id.split('-')[1]; > + var permission = id.slice(id.lastIndexOf("-") + 1); > Services.perms.add(gPermURI, aPartId, permission); Firefox avoids the hyphen problem completely by using "#" as the separator. > +++ b/suite/common/src/nsSuiteGlue.js > > - if (aRequest.type != "geolocation") > + var requestMap = new Map([["geolocation", "geo"], > + ["desktop-notification", "desktop-notification"] > + ]); Eventually the pointer lock prompt should also go here. > - function allowCallback(remember) { > + function allowCallback(remember, expireType) { > if (remember) > - Services.perms.addFromPrincipal(requestingPrincipal, "geo", Services.perms.ALLOW_ACTION); > + Services.perms.addFromPrincipal(requestingPrincipal, perm, > + Services.perms.ALLOW_ACTION, > + expireType); > aRequest.allow(); > } Desktop Notifications have a "Remember for session" option hence the expireType. > +++ b/suite/locales/en-US/chrome/common/notification.properties > +# Desktop Notifications > +webNotifications.showForSession=Show for this session > +webNotifications.showForSession.accesskey=w > +webNotifications.dontShowForSession=Don't Show for this session > +webNotifications.dontShowForSession.accesskey=o > +webNotifications.alwaysShow=Always Show Notifications > +webNotifications.alwaysShow.accesskey=A > +webNotifications.neverShow=Never Show Notifications > +webNotifications.neverShow.accesskey=N > +webNotifications.showFromSite=Would you like to show notifications from %S? > +webNotifications.remember=Remember for this website Need suggestions for better accesskeys? > diff --git a/suite/themes/classic/communicator/icons/notification-16.png b/suite/themes/classic/communicator/icons/notification-16.png > diff --git a/suite/themes/classic/communicator/icons/notification-16@2x.png b/suite/themes/classic/communicator/icons/notification-16@2x.png > diff --git a/suite/themes/classic/communicator/icons/notification-64.png b/suite/themes/classic/communicator/icons/notification-64.png > diff --git a/suite/themes/classic/communicator/icons/notification-64@2x.png b/suite/themes/classic/communicator/icons/notification-64@2x.png These images (from Firefox and Toolkit) are terribly bland. Where can we get better ones? > +@media (min-resolution: 2dppx) { > + .popup-notification-icon[popupid="web-notifications"] { > + list-style-image: url("chrome://communicator/skin/icons/notification-64@2x.png"); > + } > +} Any reviewer with a HiDPI machine? > +++ b/suite/themes/modern/global/alerts/alert.css > #alertImage { > max-width: 48px; > max-height: 48px; > + list-style-image: url("chrome://communicator/skin/icons/notification-48.png"); Meh. > +.alertCloseButton { > + list-style-image: url("chrome://global/skin/icons/close.gif"); > + -moz-appearance: none; > + padding: 4px 2px; > + border: none; > +} > + > +.alertCloseButton:hover { > + list-style-image: url("chrome://global/skin/icons/close-hov.gif"); > +} > + > +.alertCloseButton:hover:active { > + list-style-image: url("chrome://global/skin/icons/close-act.gif"); > +} I could use closebox.gif but then I wouldn't have the hover/active states.

(In reply to Philip Chee from comment #1) > (From update of attachment 747490 [details] [diff] [review]) > > + "desktop-notification": function getNotificationDefaultPermission() > Hyphens cause JS to complain. Bah, why did they have to choose such an unfortunate name... > > + var requestMap = new Map([["geolocation", "geo"], > > + ["desktop-notification", "desktop-notification"] > > + ]); > Eventually the pointer lock prompt should also go here. A Map looks like overkill for this, but exactly what I would use depends on what pointer lock would need. > Any reviewer with a HiDPI machine? Is doing HiDPI piecemeal actually viable? > > +.alertCloseButton { Wasn't there another patch where you were changing the alert styles?

Comment on attachment 747490 [details] [diff] [review] Patch v1.0 Proposed implementation. >+ if (site && !this.usePrivateBrowsing) { Eek. Should backport this I guess...

(In reply to comment #2) > (In reply to Philip Chee from comment #1) > > (From update of attachment 747490 [details] [diff] [review]) > > > +.alertCloseButton { > Wasn't there another patch where you were changing the alert styles? Ah yes, newmailalert.css - we should synchronise from that.

(In reply to comment #2) > (In reply to Philip Chee from comment #1) > > (From update of attachment 747490 [details] [diff] [review]) > > > + var requestMap = new Map([["geolocation", "geo"], > > > + ["desktop-notification", "desktop-notification"] > > > + ]); > > Eventually the pointer lock prompt should also go here. > A Map looks like overkill for this, but exactly what I would use depends on > what pointer lock would need. Some options spring to mind: 1. Persuade geolocation to change its request type to geo to match the permission 2. JS Object, as per IRC link to nsBrowserGlue.js 3. var permissionKey = request.type; switch (permissionKey) { case "geolocation": permissionKey = "geo"; case "desktop-notification": case "pointerLock": break; default: return; } 4. var permissionKey = "geo"; if (request.type == "pointerLock" || request.type == "desktop-notification") permissionKey = request.type; else if (request.type != "geolocation") return;

>> Any reviewer with a HiDPI machine? > Is doing HiDPI piecemeal actually viable? I guess not. Removed all 2x stuff. >> +.alertCloseButton { > Wasn't there another patch where you were changing the alert styles? > Ah yes, newmailalert.css - we should synchronise from that. OK Synchronized from that. > #alertImage { > - max-width: 48px; > - max-height: 48px; > + width: 48px; > + height: 48px; > + list-style-image: url("chrome://branding/content/icon48.png"); I'm using the SeaMonkey branding icon rather than the generic toolkit notification icon. > +} > + > +#alertNotification { > + border: ridge #5486DA 4px; > } I've copied this from the Mail Alert CSS but I'm not sure if you want this. >> A Map looks like overkill for this, but exactly what I would use depends on >> what pointer lock would need. > Some options spring to mind: > 1. Persuade geolocation to change its request type to geo to match the permission > 2. JS Object, as per IRC link to nsBrowserGlue.js KISS I've chosen Option 2.

Comment on attachment 748798 [details] [diff] [review] Patch v1.1 address feedback. >- aRequest.window >- .QueryInterface(Components.interfaces.nsIInterfaceRequestor) >- .getInterface(Components.interfaces.nsIWebNavigation) >- .QueryInterface(Components.interfaces.nsIDocShell) >- .chromeEventHandler.parentNode.wrappedJSObject >- .showGeolocationPrompt(path, host, >- allowCallback, >- cancelCallback); >+ var nb = aRequest.window >+ .QueryInterface(Components.interfaces.nsIInterfaceRequestor) >+ .getInterface(Components.interfaces.nsIWebNavigation) >+ .QueryInterface(Components.interfaces.nsIDocShell) >+ .chromeEventHandler.parentNode.wrappedJSObject; [window is an XPCNativeWrapper which at one point meant that everything else became an XPCNativeWrapper. At some point (I can't remember when) all chrome objects got unwrapped automatically so technically the .wrappedJSObject is unnecessary.]

Comment on attachment 748798 [details] [diff] [review] Patch v1.1 address feedback. + <image id="web-notifications-notification-icon" class="notification-anchor-icon" role="button"/> What do I have to do to see this? The notification doesn't show up below the urlbar, it shows up like a desktop notification (in the upper right corner of the desktop).

> What do I have to do to see this? The notification doesn't show up below the urlbar, > it shows up like a desktop notification (in the upper right corner of the desktop) Go about:config and toggle browser.doorhanger.enabled

(In reply to Philip Chee from comment #9) > > What do I have to do to see this? The notification doesn't show up below the urlbar, > > it shows up like a desktop notification (in the upper right corner of the desktop) > > Go about:config and toggle browser.doorhanger.enabled (sorry for the late reply) Right, but I never see the web-notifications-notification-icon. 1) If I have browser.doorhanger.enabled = "true" I get the doorhanger when I click "Authorize" on the test page. That doorhanger use the other icon. And the web notification itself (when you click "Show") appears as a desktop notification (with the other icon). 2) If i have browser.doorhanger.enabled = "false" I get the panel instead of the doorhanger when i click "Authorize". That panel uses the '.messageImage[type="info"]' icon (http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/themes/osx/global/notification.css#42). The web notification is the same as the one in 1).

(In reply to Stefan [:stefanh] from comment #10) > 2) If i have browser.doorhanger.enabled = "false" I get the panel instead of > the doorhanger when i click "Authorize". That panel uses the Oops, when I said "panel" I ment "notification bar".

Comment on attachment 748798 [details] [diff] [review] Patch v1.1 address feedback. >+# Desktop Notifications >+webNotifications.showForSession=Show for this session >+webNotifications.showForSession.accesskey=w >+webNotifications.dontShowForSession=Don't Show for this session >+webNotifications.dontShowForSession.accesskey=o >+webNotifications.alwaysShow=Always Show Notifications >+webNotifications.alwaysShow.accesskey=A >+webNotifications.neverShow=Never Show Notifications >+webNotifications.neverShow.accesskey=N >+webNotifications.showFromSite=Would you like to show notifications from %S? >+webNotifications.remember=Remember for this website geolocation goes for Share/Don't Share/Always Share/Never Share, I think we should follow that model (s/Share/Show of course).

Comment on attachment 748798 [details] [diff] [review] Patch v1.1 address feedback. >- var permission = id.split('-')[1]; >+ var permission = id.slice(id.lastIndexOf("-") + 1); id.replace(/.*-/, ""); is another alternative.

With the patch applied I see the megaphone if I manually invoke showAlertNotification with a null imageUri. Is this intentional?

This patch should be applied on top of the patch in Bug 870728 (Geolocation PB). > With the patch applied I see the megaphone if I manually invoke > showAlertNotification with a null imageUri. Is this intentional? Yes. > [window is an XPCNativeWrapper which at one point meant that everything else > became an XPCNativeWrapper. At some point (I can't remember when) all chrome > objects got unwrapped automatically so technically the .wrappedJSObject is > unnecessary.] Removed .wrappedJSObject. > geolocation goes for Share/Don't Share/Always Share/Never Share, I think we > should follow that model (s/Share/Show of course). Fixed. I used "Show Notifications" because Geolocation uses "Share Location:. >>- var permission = id.split('-')[1]; >>+ var permission = id.slice(id.lastIndexOf("-") + 1); > id.replace(/.*-/, ""); is another alternative. Fixed. > With the patch applied I see the megaphone if I manually invoke > showAlertNotification with a null imageUri. Is this intentional? Yes for Classic. See: http://hg.mozilla.org/mozilla-central/annotate/ca7f8131a8d4/toolkit/themes/windows/global/alerts/alert.css#l42 In my patch, for modern I'm using "chrome://branding/content/icon48.png" which is the generic SeaMonkey icon. Should I switch it back to the notification-48.png ?

Comment on attachment 752268 [details] [diff] [review] Patch v1.2 fix review issues. Looks/works fine now with Mac Classic, thanks.

Comment on attachment 752268 [details] [diff] [review] Patch v1.2 fix review issues. >+ // Create a dummy checkbox so file requests don't try to remember. Nit: no file requests, so change this to private requests >+ if (site && !this.usePrivateBrowsing) { Nit: site is always set here >diff --git a/suite/themes/classic/communicator/icons/notification-16.png b/suite/themes/classic/communicator/icons/notification-16.png [optipng said that notification-16.png wasn't optimised, although notification-64.png was.] > .alertImageBox { >- padding: 8px; >+ padding: 4px; > width: 64px; > } > > .alertTextBox { >- padding: 8px; >+ padding: 4px; Why these changes? > #alertImage { >- max-width: 48px; >- max-height: 48px; >+ width: 48px; >+ height: 48px; Why this change? >+ list-style-image: url("chrome://branding/content/icon48.png"); To answer your question, yes, this will need to be chrome://global/skin/alerts/notification-48.png (suitably copied). >+.alertCloseButton { >+ list-style-image: url("chrome://global/skin/icons/closebox.gif"); >+ -moz-appearance: none; >+ padding: 2px 0px 0px; >+ border: none; >+} Do we need to uplift the CSS changes so that regular alerts operate correctly?

>>+ // Create a dummy checkbox so file requests don't try to remember. > Nit: no file requests, so change this to private requests Fixed. >>+ if (site && !this.usePrivateBrowsing) { > Nit: site is always set here Fixed. >>diff --git a/suite/themes/classic/communicator/icons/notification-16.png b/suite/themes/classic/communicator/icons/notification-16.png > [optipng said that notification-16.png wasn't optimised, although notification-64.png was.] Ran notification-16.png through pngcrush and optipng. >> .alertImageBox { >>- padding: 8px; >>+ padding: 4px; >> width: 64px; >> } >> >> .alertTextBox { >>- padding: 8px; >>+ padding: 4px; > Why these changes? I think you said to sync from newmailalert.css. ----------------------------------------------- >> Wasn't there another patch where you were changing the alert styles? > Ah yes, newmailalert.css - we should synchronise from that. ----------------------------------------------- >> #alertImage { >>- max-width: 48px; >>- max-height: 48px; >>+ width: 48px; >>+ height: 48px; > Why this change? Can't remember. Reverted this. >>+ list-style-image: url("chrome://branding/content/icon48.png"); > To answer your question, yes, this will need to be chrome://global/skin/alerts/notification-48.png (suitably copied). Fixed. >>+.alertCloseButton { >>+ list-style-image: url("chrome://global/skin/icons/closebox.gif"); >>+ -moz-appearance: none; >>+ padding: 2px 0px 0px; >>+ border: none; >>+} > Do we need to uplift the CSS changes so that regular alerts operate correctly? The target milestone for Bug 782211 (Implement notification API spec) is mozilla22 which corresponds to SeaMonkey 2.19. So the answer to this is yes. But the trees are closed at the moment.

(In reply to Philip Chee from comment #18) >>> .alertImageBox { >>>- padding: 8px; >>>+ padding: 4px; >>> width: 64px; >>> } >>> >>> .alertTextBox { >>>- padding: 8px; >>>+ padding: 4px; >> Why these changes? > I think you said to sync from newmailalert.css. Ah yes, but unfortunately the alert image in newmailalert.css is bigger, so we should probably compensate for the smaller image in alert.css by keeping the larger padding. (As it turns out it doesn't affect the height because there's a minimum height enforced anyway.) r=me with that fixed.

>>>> .alertImageBox { >>>>- padding: 8px; >>>>+ padding: 4px; >>>> width: 64px; >>>> } >>>> >>>> .alertTextBox { >>>>- padding: 8px; >>>>+ padding: 4px; >>> Why these changes? >> I think you said to sync from newmailalert.css. > Ah yes, but unfortunately the alert image in newmailalert.css is bigger, so we > should probably compensate for the smaller image in alert.css by keeping the > larger padding. (As it turns out it doesn't affect the height because there's > a minimum height enforced anyway.) r=me with that fixed. Done.

Pushed http://hg.mozilla.org/comm-central/rev/800dca20e0a2