I came up with the following scheme to ease the conversion of notifications to doorhangers. The first step is to move all existing notification creation code into notification.xml. We then create a derived binding that is only used by the tabbed browser (doorhangers only work with tabbed browsers). In the derived binding we then add code that implements the notifications using doorhangers.

Looks like notifications will be the next thing I stop working on.

(In reply to comment #2) > Looks like notifications will be the next thing I stop working on. ITYM "Looks like doorhangers will be the next Firefox code I stop blindly copying"

IMHO, one of the largest failures of the SeaMonkey project is that we waste a lot of precious developer time by re-inventing wheels that Firefox has already invented by trying to skew them our way without changing any bit of functionality. If we used that time to actually make visible-to-the-user improvements, we'd not be running behind Firefox, but make a better and perhaps even innovative suite. But that doesn't seem to be the goal of some people around here.

(In reply to comment #3) > (In reply to comment #2) > > Looks like notifications will be the next thing I stop working on. > ITYM > "Looks like doorhangers will be the next Firefox code I stop blindly copying" Heres the rub imo, we need to make sure the code we "copy" meshes with our general code-style (but can divert a bit imo) and meshes with API differences and alternatives we have, nicely. Doing wheel-reinvention wastes time all around. And review is still useful, as it helps us catch potentially missed bugs in the Firefox implementation, and then BOTH projects can benefit.

In any case, I seriously hope you will backport this new approach to FF, in which case I'm probably happy with it to some extent (i.e. I'm happy as long as it's done in time for the 2.1 feature/beta1 freeze which is tentatively at the end of September). If you're serious about this, you should do that backport in any case, because we're one Mozilla project after all, Firefox and us are friends, and there's no reasonable argument to differ in code for a thing like that. If there's a really better way to do doorhanger, I'm sure gavin is happy to have it on their side as well. What I want to prevent in any way possible is to run into a NIH syndrome of "Firefox always does bad code style and we are so much better" on things where we don't differ in any significant user-facing way. There are good reason for different to Firefox, but only where we can argument an actual user-facing difference. In all other cases, we should either swallow what they are doing or backport our improvements if they are such.

Most of our current notifications are already handled by notification.xml in a completely different way to the way Firefox does it yet nobody complained then.

You're right, I should have complained and requested a backport back then already, but in the mean time my mindset changed a lot towards liking and respecting Firefox, and seeing the pros of backporting improvements much more. On one hand, way more people get to use better code, on the other, it's much easier to port further work and improvements back and forth. Next to that, if we improve Firefox, they respect us more and see us as a legitimate part of the community - and we can really need that.

(In reply to comment #6) > What I want to prevent in any way possible is to run into a NIH syndrome of > "Firefox always does bad code style and we are so much better" on things where > we don't differ in any significant user-facing way. Sorry, but that's totally nonsense. Porting stuff doesn't mean copying code around one may not even completely understand just because it runs. (That may work, but usually doesn't.) If you're touching code anyway, it's a very cheap occasion to fix recognized weaknesses and shortcomings - a few minutes spent now will mean a lot of saved minutes later! Furthermore, visibility to the user is not a valid argument here - even if there were _no_ visible clou that things are different in the backend, it's much better to have maintainable and readable source than burden future patchers with figuring out strange odds and ends... > There are good reason for different to Firefox, but only where we can argument > an actual user-facing difference. That's just wrong. Especially you should know that we don't have the resources to alter code a gazillion times until it's straight - any minute spent on better code saves us wasting time later! > In all other cases, we should either swallow what they are doing or > backport our improvements if they are such. Surely we should try to backport stuff, no doubt, but fixing us _and_ them may drain our resources more than necessary, occasionally...

(In reply to comment #9) > If you're touching code anyway, it's a very cheap occasion to fix recognized > weaknesses and shortcomings - a few minutes spent now will mean a lot of saved > minutes later! True, if I backport them to the "upstream" I have taken the code from. If I don't I'm guilty of abusing open source, not using it. > Especially you should know that we don't have the resources to alter code a > gazillion times until it's straight - any minute spent on better code saves us > wasting time later! Well, we are spending a ton of resources to alter code a gazillion times until our review thinks it's straight. If we seem to not even want to give back the improvements, I don't want to be part of that way of working, so I'll unassigned myself from any bugs where I did some work but we IMHO run afoul of that. > Surely we should try to backport stuff, no doubt, but fixing us _and_ them may > drain our resources more than necessary, occasionally... Not more than we drain our resources already by thinking we need to rewrite every piece of code they have already written. But thanks for at least admitting we should try to backport our changes.

(If I may, this more global discussion seems legitimate to have, but maybe not in this/a bug?)

Comment on attachment 474666 [details] [diff] [review] Part 1: move the rights notification into notification.xml [Checked in: Comment 13] http://hg.mozilla.org/comm-central/rev/bcc0a391a42b

Comment on attachment 476376 [details] [diff] [review] Part 2: move the geolocation notification into notification.xml [Checked in: See comment 16] Why the change from PRIORITY_INFO_HIGH to PRIORITY_INFO_LOW? r=me with that addressed

(In reply to comment #14) > (From update of attachment 476376 [details] [diff] [review]) > Why the change from PRIORITY_INFO_HIGH to PRIORITY_INFO_LOW? Copy & paste error on my part, sorry.

Comment on attachment 476376 [details] [diff] [review] Part 2: move the geolocation notification into notification.xml [Checked in: See comment 16] http://hg.mozilla.org/comm-central/rev/9f57d359479c

This differs slightly from the approaches in other bugs: This patch uses the host for a site, but the local file path for a file; bug 570004 uses the host for a site, but the URI path for a file; bug 580260 uses the spec throughout, with different wording for a file.

With string change accidentally omitted from previous patch.

Comment on attachment 478409 [details] [diff] [review] Part 3: Support geolocation notifications from files [Checked in: See comment 20] If we're using path and host, why are the variables not called that? Either way can you be consistent, you use file and site in notification.xml and file and host in nsSuiteGlue.js r=me with that addressed.

Comment on attachment 478409 [details] [diff] [review] Part 3: Support geolocation notifications from files [Checked in: See comment 20] http://hg.mozilla.org/comm-central/rev/b30b6002d7f5

(In reply to comment #20) > Comment on attachment 478409 [details] [diff] [review] > Part 3: Support geolocation notifications from files > [Checked in: See comment 20] > > http://hg.mozilla.org/comm-central/rev/b30b6002d7f5 Serge, please, if there is a conditional review, could you let the patch author make the corrections.

What I expected was either of: file and site to be used in both notification.xml and nsSuiteGlue.js path and host to be used in both notification.xml and nsSuiteGlue.js Instead we got: file and site used in notification.xml and path and host used in nsSuiteGlue.js

(In reply to comment #21) > (In reply to comment #20) > > Comment on attachment 478409 [details] [diff] [review] [details] > > Part 3: Support geolocation notifications from files > > [Checked in: See comment 20] > > > > http://hg.mozilla.org/comm-central/rev/b30b6002d7f5 > > Serge, please, if there is a conditional review, could you let the patch author > make the corrections. Sorry, just looked at the pushlog and saw that Neil did the push and you did the update to the bug. Looks like I was not explicit enough in my review comment.

(In reply to comment #22) > What I expected was either of: > file and site to be used in both notification.xml and nsSuiteGlue.js > path and host to be used in both notification.xml and nsSuiteGlue.js > > Instead we got: > file and site used in notification.xml and path and host used in nsSuiteGlue.js Sorry about that. The string names are "siteWantsToKnow" and "fileWantsToKnow" so I used site and file as the argument names. But the properties used are the URL's host and the file's path so I used host and path as the variable names.

Actually most of this patch is identical to KaiRo's patch in bug 570004.

The patch misses a test. Could you please port the test from bug 570004 as well and care that it works? Thanks a lot!

Comment on attachment 478648 [details] [diff] [review] Part 4: implement geolocation doorhanger [Checked in: See comment 38] What is the reasoning behind having the lazy getter in navigator.js rather than nsBrowserStatusHandler.js? As one of the patches on this bug please port browser_popupNotification.js from Firefox (as Robert suggests). Is it expected that when you have a doorhanger showing that it uses the first click on an other window to dismiss the doorhanger and you have to click a second time to bring that other window up? r=me with those points addressed

(In reply to comment #25) > Created attachment 478648 [details] [diff] [review] > Part 4: implement geolocation doorhanger > > Actually most of this patch is identical to KaiRo's patch in bug 570004. Oh and the patch is bitrotted slightly in browser-prefs.js

(In reply to comment #27) > What is the reasoning behind having the lazy getter in navigator.js rather than > nsBrowserStatusHandler.js? IIRC, to be able to have others use it as well and have all the getters for those things in one place. > Is it expected that when you have a doorhanger showing that it uses the first > click on an other window to dismiss the doorhanger and you have to click a > second time to bring that other window up? At least that matches the Firefox behavior.

(In reply to comment #29) > (In reply to comment #27) > > Is it expected that when you have a doorhanger showing that it uses the first > > click on an other window to dismiss the doorhanger and you have to click a > > second time to bring that other window up? > > At least that matches the Firefox behavior. To me that sounds like a bug, but I am ok with matching behavior/bug here for now

Comment on attachment 478648 [details] [diff] [review] Part 4: implement geolocation doorhanger [Checked in: See comment 38] The patch misses adding geolocation-16.png and geolocation-64.png for the Mac theme. This lead to "RuntimeError: File "mac/communicator/icons/geolocation-16.png" not found in /builds/slave/comm-central-trunk-macosx/build/suite/themes/classic" on our MacOSX builders.

These are the portions of Part 4 that didn't land because of the l10n freeze. The fix for bug 602150 affects this part of the patch too.

(In reply to comment #32) > Created attachment 481165 [details] [diff] [review] > Part 4, l10n only (with fix for bug 602150) > > These are the portions of Part 4 that didn't land because of the l10n freeze. So what's the state of this bug in Beta 1, in layman's terms? I'm currently preparing an SMTT blog post and also want to be prepared to answer people's questions in forums/NGs.

I think the status is that login manager will try to use doorhangers unless you disable them via the pref.

The previous patch also makes the login manager try to use doorhangers. Unfortunately some of our CSS needs to be tweaked to display this correctly. I copied the mozapps/passwordmgr images to Modern from winstripe. KaiRo, while writing this patch, I notice that Firefox doesn't appear to set the width and height on the notification icons. Any idea why? (I just copied your patch from bug 570004, so I thought you might have some insight...)

(In reply to comment #35) > Created attachment 482157 [details] [diff] [review] > Part 5: Support login manager prompter > > The previous patch also makes the login manager try to use doorhangers. > > Unfortunately some of our CSS needs to be tweaked to display this correctly. > > I copied the mozapps/passwordmgr images to Modern from winstripe. > > KaiRo, while writing this patch, I notice that Firefox doesn't appear to set > the width and height on the notification icons. Any idea why? (I just copied > your patch from bug 570004, so I thought you might have some insight...) What about for mac too?

With Mac changes. [My question to KaiRo still stands, but I notice he's CCed anyway.]

Comment on attachment 478648 [details] [diff] [review] Part 4: implement geolocation doorhanger [Checked in: See comment 38] http://hg.mozilla.org/comm-central/rev/c55ed62b37cd Part 4: implement geolocation doorhanger + http://hg.mozilla.org/comm-central/rev/95784e294722 Part 4: missing Mac icons + http://hg.mozilla.org/comm-central/rev/2342338033d4 Part 4: fix test failures caused by using reserved attribute name + http://hg.mozilla.org/comm-central/rev/aa87a683cf1d Partial backout of changeset c55ed62b37cd due to l10n freeze

Comment on attachment 481165 [details] [diff] [review] Part 4, l10n only (with fix for bug 602150) [Checked in: Comment 42] http://hg.mozilla.org/comm-central/rev/317093217f10 Part 4: reland geolocation doorhanger implementation

(In reply to comment #39) > http://hg.mozilla.org/comm-central/rev/317093217f10 > Part 4: reland geolocation doorhanger implementation Causing test fails in http://mxr.mozilla.org/comm-central/source/mozilla/content/html/document/test/test_bug369370.html?force=1 I don't see anything at a glance in the patchset to cause this, but helpful: Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebProgress.addProgressListener]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://communicator/content/bindings/notification.xml :: addProgressListener :: line 67" data: no] Appears twice when the test runs (once for each launched SeaMonkey window) JS frame :: chrome://communicator/content/bindings/notification.xml :: addProgressListener :: line 71 Source File: chrome://communicator/content/bindings/notification.xml Line: 73 JS frame :: chrome://communicator/content/bindings/notification.xml :: :: line 491 Source File: chrome://communicator/content/bindings/notification.xml Line: 73 From manual frame inspection I'm backing this out due to the test failures, [sorry]. Neil, can we (please) split out remaining work to new bugs so we can properly track this landing. Serge filed a bug for this error, not knowing it was causing test failures at the time though, lets track this actual fix [and relanding of this patch] in Bug 603228 because of that, and file new bugs for additional work here...

(In reply to comment #40) > I'm backing this out due to the test failures, [sorry]. Done: http://hg.mozilla.org/comm-central/rev/8b5776bb0c0e

Comment on attachment 481165 [details] [diff] [review] Part 4, l10n only (with fix for bug 602150) [Checked in: Comment 42] http://hg.mozilla.org/comm-central/rev/d7fd18b29794

Comment on attachment 482164 [details] [diff] [review] Part 5: Support login manager prompter [Checked in: Comment 44] r=me Really need to document in help how to dismiss and bring back the door hanger as well as any changes that are needed to help due to the change to using door hangers. If it's not going to be in this bug could you open a new one for it. Thanks.

Comment on attachment 482164 [details] [diff] [review] Part 5: Support login manager prompter [Checked in: Comment 44] http://hg.mozilla.org/comm-central/rev/8033e2000e5d

toEM was just too complicated for me to bother duplicating it, so I just look to see if it's been declared in the window scope and use that instead. Part 6c will also want to open the addons manager so this makes it easier to be consistent.

This will allow doorhangers to override it in part 6d.

Comment on attachment 485616 [details] [diff] [review] Part 6a: Drop local copy of toEM [Checked in: Comment 53] >- function managePlugins(aEvent){ ... >+ event.target.addEventListener("click", function() { Oops, the aEvent parameter got lost :-(

Comment on attachment 485616 [details] [diff] [review] Part 6a: Drop local copy of toEM [Checked in: Comment 53] r=me with aEvent issue fixed

Comment on attachment 485617 [details] [diff] [review] Part 6b: Break out addonInstallBlocked into its own method [Checked in: Comment 54] >+++ b/suite/common/bindings/notification.xml >@@ -456,16 +407,72 @@ >+ <method name="addonInstallBlocked"> >+ <parameter name="installInfo"/> >+ <body> >+ <![CDATA[ >+ var notificationName, messageString, buttons, host; As you have moved where host is used, could move the declaration of that variable to there too (just before the try). >+ >+ if (!this._prefs.getBoolPref("xpinstall.enabled")) { >+ notificationName = "xpinstall-disabled"; >+ if (this._prefs.prefIsLocked("xpinstall.enabled")) { >+ messageString = this._stringBundle.GetStringFromName("xpinstallDisabledMessageLocked"); >+ buttons = []; >+ } else { >+ var prefBranch = this._prefs; Also, could the declaration of prefBranch be moved earlier so it can be used instead of this._prefs in the two if statements? r=me with those points addressed/commented on

Comment on attachment 485618 [details] [diff] [review] Part 6c: Add support for completed and failed addon installation notifications [Checked in: Comment 55] >+++ b/suite/common/bindings/notification.xml >+ <method name="addonInstallFailed"> >+ <parameter name="installInfo"/> >+ <body> >+ <![CDATA[ >+ var notificationName = "addon-error"; >+ if (!this.getNotificationWithValue(notificationName)) { >+ var messageString, host; You declare messageString later on so no need to here. r=me with that addressed.

Comment on attachment 485616 [details] [diff] [review] Part 6a: Drop local copy of toEM [Checked in: Comment 53] http://hg.mozilla.org/comm-central/rev/85a9c52f1c35

Comment on attachment 485617 [details] [diff] [review] Part 6b: Break out addonInstallBlocked into its own method [Checked in: Comment 54] http://hg.mozilla.org/comm-central/rev/4418b324bd58

Comment on attachment 485618 [details] [diff] [review] Part 6c: Add support for completed and failed addon installation notifications [Checked in: Comment 55] http://hg.mozilla.org/comm-central/rev/fad2356f3cf8

Neil, are you planning more work here or is this bug fixed now?

Well, I need to write part 6d at the very least.

Note that currently neither the notificationbar nor the doorhanger are triggered if xpinstall.enabled has been set to false because of bug 613385.

> Note that currently neither the notificationbar nor the doorhanger are > triggered if xpinstall.enabled has been set to false because of bug 613385. I guess your patch needs to be updated after that bug now.

(In reply to comment #59) > > Note that currently neither the notificationbar nor the doorhanger are > > triggered if xpinstall.enabled has been set to false because of bug 613385. > I guess your patch needs to be updated after that bug now. No, the patch has all the right code, but the AddonsManager wasn't calling it.

Sorry, I hadn't realised he ended up changing the API.

Comment on attachment 493220 [details] [diff] [review] Part 6d: Add support for disabled addon install notifications [Checked in: Comment 72] Just to be explicit, is it intended that this patch contains notification.xml only, and not the other css+xul?

(In reply to comment #64) > (From update of attachment 493220 [details] [diff] [review]) > Just to be explicit, is it intended that this patch contains notification.xml > only, and not the other css+xul? No doorhangers are involved here. I'll update the doorhanger patch separately.

(In reply to comment #65) > No doorhangers are involved here. I'll update the doorhanger patch separately. 'Part 6d' in new patch name confused me then ;->

Had I known in advance, it would have been part of 6b, so maybe Part 6b++ ;-)

Comment on attachment 493221 [details] [diff] [review] -w diff of the interesting bits >- notificationName = "xpinstall-disabled"; >+ var notificationName = "xpinstall-disabled"; Hmm. Should I rename this to "addon-install-disabled" too?

(In reply to comment #69) > Comment on attachment 493221 [details] [diff] [review] > -w diff of the interesting bits > > >- notificationName = "xpinstall-disabled"; > >+ var notificationName = "xpinstall-disabled"; > Hmm. Should I rename this to "addon-install-disabled" too? Probably best so that it matches the rest.

KaiRo actually said that some of these notifications don't need to be converted to doorhangers. But I think they look better in notification.xml anyway.

Comment on attachment 493220 [details] [diff] [review] Part 6d: Add support for disabled addon install notifications [Checked in: Comment 72] http://hg.mozilla.org/comm-central/rev/b78db221b0b1

Comment on attachment 493272 [details] [diff] [review] Part 6e: implement addon doorhanger [Checked in: Comment 73] http://hg.mozilla.org/comm-central/rev/e53cca758048

Toolkit Bug 610130 - Doorhanger notifications confused when switching between tabs with different types of doorhangers

Comment on attachment 499296 [details] [diff] [review] Part 7: move the update checking notification into notification.xml [Checked in: See comment 80] >+++ b/suite/common/bindings/notification.xml Wed Dec 22 15:58:44 2010 +0000 >+ box.persistence = -1; // Until user closes it Nit: Comment should end with a full stop if it starts with a capital letter. >+++ b/suite/common/src/nsSuiteGlue.js Wed Dec 22 15:58:44 2010 +0000 >@@ -247,7 +247,7 @@ SuiteGlue.prototype = { > } > // Detect if updates are off and warn for outdated builds. > if (this._shouldShowUpdateWarning()) >- this._showUpdateWarning(aWindow); >+ aWindow.getBrowser().getNotificationBox().showUpdateWarning(); If at the beginning of the _onBrowserStartup function you do something like: var notifyBox = aWindow.getBrowser().getNotificationBox(); You can then use: this._showRightsNotification(notifyBox); and simplify the _showRightsNotification function. It can be also used for when _showPlacesLockedNotificationBox is moved and for this move too. r=me with that fixed/addressed

Comment on attachment 499576 [details] [diff] [review] Part 8a: add support for indexedDB notifications [Checked in: See comment 81] >+++ b/suite/browser/navigator.js >@@ -519,16 +519,18 @@ function Startup() > XPCOMUtils.defineLazyGetter(window, "PopupNotifications", function() { > var tmp = {}; > Components.utils.import("resource://gre/modules/PopupNotifications.jsm", tmp); > return XULBrowserWindow.popupNotifications = new tmp.PopupNotifications( > getBrowser(), > document.getElementById("notification-popup"), > document.getElementById("notification-popup-box")); > }); >+ // This calls the constructor again, so we have to destroy it manually. >+ gBrowser.getNotificationBox().destroy(); > gBrowser.setAttribute("popupnotification", "true"); > // This resets popup window scrollbar visibility, so override it. It would be useful for both the comments to be more explicit as to what "This" is. >+++ b/suite/locales/en-US/chrome/common/notification.properties >+# IndexedDB >+offlineApps.permissions=This website (%S) is asking to store data on your computer for offline use. >+offlineApps.quota=This website (%1$S) is attempting to store more than %2$SMB of data on your computer for offline use. Is it always going to be a website? We don't seem to be very consistent (compare with geolocation) r=me with that fixed/addressed.

(In reply to comment #77) > >+offlineApps.permissions=This website (%S) is asking to store data on your computer for offline use. > >+offlineApps.quota=This website (%1$S) is attempting to store more than %2$SMB of data on your computer for offline use. > > Is it always going to be a website? We don't seem to be very consistent > (compare with geolocation) Yes, indexedDB only works on a website, while geolocation supports files (although it prompts every time, you can't save geolocation permission.)

(In reply to comment #78) > (In reply to comment #77) > > >+offlineApps.permissions=This website (%S) is asking to store data on your computer for offline use. > > >+offlineApps.quota=This website (%1$S) is attempting to store more than %2$SMB of data on your computer for offline use. > > > > Is it always going to be a website? We don't seem to be very consistent > > (compare with geolocation) > > Yes, indexedDB only works on a website, while geolocation supports files > (although it prompts every time, you can't save geolocation permission.) What I meant was that geolocation has: geolocation.siteWantsToKnow=%S wants to know your location. geolocation.fileWantsToKnow=The file %S wants to know your location. So should they be something like: geolocation.siteWantsToKnow=This website (%S) wants to know your location. geolocation.fileWantsToKnow=This file (%S) wants to know your location.

Comment on attachment 499296 [details] [diff] [review] Part 7: move the update checking notification into notification.xml [Checked in: See comment 80] http://hg.mozilla.org/comm-central/rev/59fc64bec3a9

Comment on attachment 499576 [details] [diff] [review] Part 8a: add support for indexedDB notifications [Checked in: See comment 81] http://hg.mozilla.org/comm-central/rev/c62bebd4807b

Neil, would you port bug 570012 ("addon-install-started") + bug 623192 here too? (or in a separate bug?)

(In reply to comment #82) > Neil, would you port bug 570012 ("addon-install-started") + bug 623192 here > too? (or in a separate bug?) I probably need to do it here, as it's a two-step process. (In reply to comment #79) > >+offlineApps.permissions=This website (%S) is asking to store data on your computer for offline use. > >+offlineApps.quota=This website (%1$S) is attempting to store more than %2$SMB of data on your computer for offline use. > > Is it always going to be a website? We don't seem to be very consistent > (compare with geolocation) > > What I meant was that geolocation has: > geolocation.siteWantsToKnow=%S wants to know your location. > geolocation.fileWantsToKnow=The file %S wants to know your location. > > So should they be something like: > geolocation.siteWantsToKnow=This website (%S) wants to know your location. > geolocation.fileWantsToKnow=This file (%S) wants to know your location. KaiRo, can we just change the strings in en-US and notify the l10n community, seeing as we're just changing the appearance rather than the meaning?

I notice that we don't actually seem to have help for this button. (Moving it to notification.xml makes it possible to trigger manually, of course; I can't seem to trigger it for real!) KaiRo, is there already a bug on file for that?

(In reply to comment #83) > (In reply to comment #82) > > Neil, would you port bug 570012 ("addon-install-started") + bug 623192 here > > too? (or in a separate bug?) > > I probably need to do it here, as it's a two-step process. Gah I'd think even everything that is in here should have been split across different bugs, but wth... > (In reply to comment #79) > KaiRo, can we just change the strings in en-US and notify the l10n community, > seeing as we're just changing the appearance rather than the meaning? It's not too god, but it could work. Note that I think "This website (%S)" is bad phrasing altogether, just using "%S" instead should be enough, IMHO.

(In reply to comment #85) > (In reply to comment #83) > > (In reply to comment #79) > > KaiRo, can we just change the strings in en-US and notify the l10n community, > > seeing as we're just changing the appearance rather than the meaning? > It's not too god, but it could work. Note that I think "This website (%S)" is > bad phrasing altogether, just using "%S" instead should be enough, IMHO. We also already use "this website (%S)" in the xpinstall blocked notification.

Some more stuff landed in Firefox since I wrote the previous patch: * Option to defer the question without setting a particular permission * Request automatically times out after 5 minutes (think background tab) * Allow the back end to cancel the quota request

Bug 615315 should be ported in here or as a followup as well, just for your notice. It's a simple change, but apparently one that makes things easier for users to understand.

(In reply to comment #88) > Bug 615315 should be ported in here or as a followup as well, just for your > notice. It's a simple change, but apparently one that makes things easier for > users to understand. By my reading of the patch it's a base binding change so we should automatically pick it up.

(In reply to comment #89) > (In reply to comment #88) > > Bug 615315 should be ported in here or as a followup as well, just for your > > notice. It's a simple change, but apparently one that makes things easier for > > users to understand. > > By my reading of the patch it's a base binding change so we should > automatically pick it up. I just saw in the changeset it had browser/ changes as well, something in a binding and some test addition that probably would be good to port as well (I hope we added tests for the other notifications as well, but this is in the basic one I'm pretty sure you added).

(In reply to comment #90) > (In reply to comment #89) > > (In reply to comment #88) > > > Bug 615315 should be ported in here or as a followup as well, just for your > > > notice. It's a simple change, but apparently one that makes things easier for > > > users to understand. > > By my reading of the patch it's a base binding change so we should > > automatically pick it up. > I just saw in the changeset it had browser/ changes as well That's probably part of bug 570012 in that case. > (I hope we added tests for the other notifications as well No, but I'll file a helpwanted bug if you like.

(In reply to comment #91) > (In reply to comment #90) > > (In reply to comment #89) > > > (In reply to comment #88) > > > > Bug 615315 should be ported in here or as a followup as well, just for your > > > > notice. It's a simple change, but apparently one that makes things easier for > > > > users to understand. > > > By my reading of the patch it's a base binding change so we should > > > automatically pick it up. > > I just saw in the changeset it had browser/ changes as well > That's probably part of bug 570012 in that case. Well, just look into http://hg.mozilla.org/mozilla-central/rev/bdadb18a5adf yourself, that's what landed for bug 615315. > > (I hope we added tests for the other notifications as well > No, but I'll file a helpwanted bug if you like. At least that would be good. (In reply to comment #84) > Created attachment 503131 [details] [diff] [review] > Part 9: move the places locked notification into notification.xml > > I notice that we don't actually seem to have help for this button. (Moving it > to notification.xml makes it possible to trigger manually, of course; I can't > seem to trigger it for real!) KaiRo, is there already a bug on file for that? I think I filed one, yes.

Comment on attachment 503131 [details] [diff] [review] Part 9: move the places locked notification into notification.xml [Checked in: Comment 95] Didn't apply cleanly to nsSuiteGlue.js due to the changes to the preceding comment, but once corrected was fine. r=me

Comment on attachment 503674 [details] [diff] [review] Part 8b: add more support for indexedDB notifications [Checked in: Comment 96] r=me

Comment on attachment 503131 [details] [diff] [review] Part 9: move the places locked notification into notification.xml [Checked in: Comment 95] http://hg.mozilla.org/comm-central/rev/9420f64e395b

Comment on attachment 503674 [details] [diff] [review] Part 8b: add more support for indexedDB notifications [Checked in: Comment 96] http://hg.mozilla.org/comm-central/rev/034b1563e3d9

This landed in Firefox as part of bug 617553, although I'm not sure whether they actually use it, so I test it by opening the Error Console and evaluating > top.opener.PopupNotifications.show(top.opener.getBrowser().selectedBrowser, "test-default", "Test of the default notification icon"); I also tweaked Modern to improve the text display, c.f. bug 615481.

This patch ports all the changes to the tests across from mozilla/browser/base/content/test's version of browser_popupNotification.js It does fail on test 19 though with: TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | notification anchored to tab - Got , expected tabbrowser-tab I'm not sure if that is due to any difference's between implementations or something else though

Also: * Fixed a bug where quota requests didn't work. ("usage" is correct usage!) * Corrected the notification name for indexedDB notifications. * Added a missing icon for password change doorhangers. * Tweaked the Modern doorhanger background to work with the question icon. Note that you cannot cancel the indexedDB doorhanger; you can dismiss it and then wait for it to auto-cancel after 30 seconds.

(In reply to comment #98) > It does fail on test 19 though with: > TEST-UNEXPECTED-FAIL | > chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js > | notification anchored to tab - Got , expected tabbrowser-tab > I'm not sure if that is due to any difference's between implementations or > something else though Odd; an ad hoc test I did locally returned tabbrowser-tab as expected.

(In reply to comment #100) > (In reply to comment #98) > > It does fail on test 19 though with: > Odd; an ad hoc test I did locally returned tabbrowser-tab as expected. Ian, had you applied part 10 patch? (Because this test part is from bug 617553.)

Comment on attachment 505266 [details] [diff] [review] Part 10: support default notification icon [Checked in: Comment 102] http://hg.mozilla.org/comm-central/rev/864643d7a758

Comment on attachment 507645 [details] [diff] [review] Revised tests v0.1 Hopefully, this will fix (most of) http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1296201988.1296203807.31400.gz OS X 10.6 comm-central-trunk debug test mochitest-other on 2011/01/28 00:06:28 { mochitest-browser-chrome: 10865/127/8 }

(In reply to comment #98) > This patch ports all the changes to the tests across from > mozilla/browser/base/content/test's version of browser_popupNotification.js > It does fail on test 19 though with: > TEST-UNEXPECTED-FAIL | > chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js > | notification anchored to tab - Got , expected tabbrowser-tab > I'm not sure if that is due to any difference's between implementations or > something else though The issue here is that Firefox goes pseudo-fullscreen when about:addons is loaded, which we don't do. We could adapt that test, but we'd have to use an alternate method of temporarily hiding the navigation bar.

Comment on attachment 507645 [details] [diff] [review] Revised tests v0.1 > this.notifyObj = new basicNotification(), Any chance you could fix these while you're here^ >+ // Test notification when chrome is hidden r- on this test only. All the other tests are fine.

(In reply to comment #104) > The issue here is that Firefox goes pseudo-fullscreen when about:addons is > loaded, which we don't do. We could adapt that test, but we'd have to use an > alternate method of temporarily hiding the navigation bar. We could trigger the toolbargrippy, i.e. collapse the toolbar. ;-)

Actually there are at least four ways of doing it...

Changes since first version of revised tests: * Fixed the (), issues. * Removed test 19 as we don't currently hide the chrome, if/when we do, the test can be added.

(In reply to comment #108) > * Removed test 19 as we don't currently hide the chrome, if/when we do, the > test can be added. Maybe just comment it out, with a comment.

Comment on attachment 508179 [details] [diff] [review] Fixed tests v0.2 [Checked in: Comment 110] http://hg.mozilla.org/comm-central/rev/3f017c3e9f46

(In reply to comment #103) > Hopefully, this will fix (most of) > mochitest-browser-chrome: 10865/127/8 http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1296435270.1296437024.12735.gz OS X 10.6 comm-central-trunk debug test mochitest-other on 2011/01/30 16:54:30 { mochitest-browser-chrome: 11122/2/8 } :-)

Comment on attachment 507708 [details] [diff] [review] Part 8c: implement indexedDB doorhanger [Checked in: Comment 112] http://hg.mozilla.org/comm-central/rev/559827cc8d53

(In reply to comment #113) > Created attachment 512062 [details] [diff] [review] > Part 6f: Add support for addon download progress notifications I get the following in the error console with this patch applied (and strict javascript errors enabled) and I download an add-on: Warning: reference to undefined property installInfo.installs Source File: chrome://communicator/content/bindings/notification.xml Line: 921 Warning: reference to undefined property installInfo.installs Source File: chrome://communicator/content/bindings/notification.xml Line: 930 Warning: reference to undefined property installInfo.installs Source File: chrome://communicator/content/bindings/notification.xml Line: 942 Warning: reference to undefined property this.installInfo.installs Source File: chrome://communicator/content/bindings/notification.xml Line: 1619 As I am prompted through a doorhanger with if I want to "Install Software" or "Not Now" before I start the download, should I still be getting the "Software Installation" dialog after the download has completed with "Cancel" and "Install Now" buttons?

(In reply to comment #114) > I get the following in the error console with this patch applied (and strict > javascript errors enabled) and I download an add-on: > Warning: reference to undefined property installInfo.installs > Source File: chrome://communicator/content/bindings/notification.xml > Line: 921 > Warning: reference to undefined property installInfo.installs > Source File: chrome://communicator/content/bindings/notification.xml > Line: 930 > Warning: reference to undefined property installInfo.installs > Source File: chrome://communicator/content/bindings/notification.xml > Line: 942 > Warning: reference to undefined property this.installInfo.installs > Source File: chrome://communicator/content/bindings/notification.xml > Line: 1619 So do I. The strange part is that installInfo.installs is an array, so it's very much defined... might be a bug in tracemonkey or something. > As I am prompted through a doorhanger with if I want to "Install Software" or > "Not Now" before I start the download, should I still be getting the "Software > Installation" dialog after the download has completed with "Cancel" and > "Install Now" buttons? The first prompt is the permission prompt, you don't get that on whitelisted sites. You always get the Software Installation dialog though.

Please, check whether bug 633218 (wrt 'addon-install-complete') needs to be ported.

Comment on attachment 512062 [details] [diff] [review] Part 6f: Add support for addon download progress notifications [Checked in: Comment 117] http://hg.mozilla.org/comm-central/rev/edc24697f35e

(In reply to comment #116) > Please, check whether bug 633218 (wrt 'addon-install-complete') needs to be > ported. I'm not sure. If it did, it would need a separate bug.

Comment on attachment 512062 [details] [diff] [review] Part 6f: Add support for addon download progress notifications [Checked in: Comment 117] >diff --git a/suite/locales/en-US/chrome/common/notification.properties b/suite/locales/en-US/chrome/common/notification.properties >--- a/suite/locales/en-US/chrome/common/notification.properties >+++ b/suite/locales/en-US/chrome/common/notification.properties >@@ -17,16 +17,23 @@ xpinstallHostNotAvailable=unknown host > xpinstallPromptWarning=%S prevented this website (%S) from asking you to install software on your computer. > xpinstallPromptInstallButton=Install Software⦠> xpinstallPromptInstallButton.accesskey=I > xpinstallDisabledMessageLocked=Software installation has been disabled by your system administrator. > xpinstallDisabledMessage=Software installation is currently disabled. Click Enable and try again. > xpinstallDisabledButton=Enable > xpinstallDisabledButton.accesskey=n > >+addonDownloading=Add-on downloading:;Add-ons downloading: >+addonDownloadCancelled=Add-on download cancelled.; Add-on downloads cancelled. >+addonDownloadCancelButton=Cancel >+addonDownloadCancelButton.accesskey=C >+addonDownloadRestartButton=Restart >+addonDownloadRestartButton.accesskey=R >+ A L10n note here please.

(In reply to comment #119) > A L10n note here please. Sorry about that. I don't know why I didn't just copy and paste the block. I'll roll it into the next patch, something like this: # LOCALIZATION NOTE (addonDownloading, addonDownloadCancelled): # Semi-colon list of plural forms. See: # http://developer.mozilla.org/en/Localization_and_Plurals # The number of add-ons is not itself substituted in the string. # See https://bugzilla.mozilla.org/show_bug.cgi?id=570012 for mockups

Also sporting a couple of minor fixes to the existing code.

Comment on attachment 513890 [details] [diff] [review] Part 6g: Add rest of addon doorhanger >+++ b/suite/common/bindings/notification.xml >+ <method name="addonInstallStarted"> >+ <parameter name="installInfo"/> >+ <body> >+ <![CDATA[ >+ var tmp = {}; >+ Components.utils.import("resource://gre/modules/AddonManager.jsm", tmp); >+ if (installInfo.installs.every(function(aInstall) { >+ return aInstall.state == tmp.AddonManager.STATE_DOWNLOADED; >+ })) >+ return; >+ >+ var sourceURI = installInfo.originatingWindow.document.documentURIObject; >+ Components.utils.import("resource://gre/modules/PluralForm.jsm", tmp); >+ var notificationName = "addon-progress"; This variable doesn't seem to be used anywhere, cut and paste slip? r=me with that addressed.

Patch as pushed; the review comment also alerted me to check for other nits which I've just rolled in.

Comment on attachment 514612 [details] [diff] [review] Part 6g: Add rest of addon doorhanger [Checked in: Comment 124] http://hg.mozilla.org/comm-central/rev/59ef5d14407a

D'oh!

Comment on attachment 519053 [details] [diff] [review] Part 6h: Missing hunk from part 6f [Checked in: Comment 126] http://hg.mozilla.org/comm-central/rev/333c20646727

[I don't know whether this is the right bug, but probably close enough.] I've noticed that the add-on download doorhanger (or is it the add-on install doorhanger?) is changing its width while progressing. Since I couldn't see the same with Minefield I guess we're missing some CSS. Do we have a bug for such things already?

Comment on attachment 524172 [details] [diff] [review] Part 2b: restore the Learn More link to the geolocation doorhanger [Checked in: Comment 130] r=me - issue about why it ignores link behaviour preferences should be in a followup bug.

Comment on attachment 524172 [details] [diff] [review] Part 2b: restore the Learn More link to the geolocation doorhanger [Checked in: Comment 130] Pushed changeset 18a491487cd9 to comm-central.

This is a branch-safe patch that doesn't move the strings from navigator.properties into notification.properties; I don't know whether to check this in on trunk first or write a completely separate patch.

Comment on attachment 527873 [details] [diff] [review] Part 11pre: add support for lightweight themes [Checked in: Comment 134] I rather have separate branch and trunk patches, so if you want this patch to go into the branch, then that is fine, but I'd like the trunk patch to be "not-branch-safe".

Comment on attachment 527873 [details] [diff] [review] Part 11pre: add support for lightweight themes [Checked in: Comment 134] http://hg.mozilla.org/releases/comm-2.0/rev/0cd3be53dd8b

We can haz FIXdz?

Since it's ready this is all one big patch, although I could split it into two smaller patches (70% for the first part and 30% for the rest) if you prefer.

Comment on attachment 528831 [details] [diff] [review] Part 11: add support for lightweight themes When installing/wearing a new persona I get in the error console: Error: uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIStringBundle.GetStringFromName]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: chrome://communicator/content/bindings/notification.xml :: lwthemeInstallNotification :: line 1821" data: no]

My previous patch was in Windows-1252 instead of UTF-8. Oops.

This is now fixed with my final checkin to comm-central: 9e5f4a666601