As far as I can tell it could be intended to be information-32.png? That image seems to be nearest to the old alert-message.gif image.

Are there cases where commonDialog.js hasn't been provided with an icon class?

To clarify: In mozilla-central, the message-icon class is only used as a fallback in commonDialog.js. I'd like to know whether that fallback makes sense in order to figure out if the class should be fixed or moved out of toolkit.

Bah, now not only do I find that we almost but not quite used it in the HTML mail question dialog, but we do (try to) use it in our sort bookmark dialog :-(

Dão: were gonna use it for bug 507682.

message-icon seems to exist specifically for commonDialog.xul, so other uses wouldn't need to be supported. Then again, I wonder why it's not defined in commonDialog.css. The problem with accepting the *-icon classes globally is that consumers may want different icon sizes depending on the context.

Comment on attachment 396427 [details] [diff] [review] Option 2: change the CSS >--- a/toolkit/themes/pinstripe/global/global.css Mon Aug 24 15:56:43 2009 -0700 >+++ b/toolkit/themes/pinstripe/global/global.css Tue Aug 25 14:33:21 2009 +0100 >-.message-icon { >- display: none; >-} Maybe this was done on purpose for commonDialog.xul?

Comment on attachment 396425 [details] [diff] [review] Option 1: rename the file to match Error, Warning etc. Option 2 seems more appealing, as the foobar-XX.png naming scheme is newer.

(In reply to comment #8) > >-.message-icon { > >- display: none; > >-} > Maybe this was done on purpose for commonDialog.xul? But as you already pointed out, no Firefox code can ever trigger this anyway.

I haven't pointed that out. I've noticed that commonDialog.js falls back to message-icon, but I don't know if that fallback is actually triggered.

Comment on attachment 396427 [details] [diff] [review] Option 2: change the CSS Somebody needs to figure this out: > commonDialog.js falls back to > message-icon, but I don't know if that fallback is actually triggered.

In all of comm-central, there are only two hits for message-icon in code. The first is in nsPromptService.cpp where it is used to initialise an unreferenced variable. The second is in commonDialog.js as you know, and the only caller in code is nsPromptService.cpp again. I also searched mxr-test's addons snapshot. There are are number of extension scripts referring to the message-icon class, although they are just rolling their own dialog and it's not clear if they actually end up using it. Some extensions also explicitly open commonDialog.xul and one of them does not seem to set the icon class and would therefore fall back to the message icon. One extension has two windows that only specify the message-icon class in the XUL.

So I looked through nsPromptService.cpp, and it doesn't look like it will ever open commonDialog.xul in such a way that commonDialog.js will fall back on message-icon. So can we remove that fallback as well as kWarningIconClass from nsPromptService.cpp?

(In reply to comment #14) > So I looked through nsPromptService.cpp, and it doesn't look like it will ever > open commonDialog.xul in such a way that commonDialog.js will fall back on > message-icon. So can we remove that fallback as well as kWarningIconClass from > nsPromptService.cpp? Do you want to assume that people will always pass in a non-empty class?

Is using message-icon instead of no icon reasonable, given that we don't have a case for message-icon right now (neither explicit nor via the empty argument) and pinstripe has code for hiding it? Shouldn't people pass message-icon if they want that?

(In reply to comment #16) > Is using message-icon instead of no icon reasonable, given that we don't have a > case for message-icon right now (neither explicit nor via the empty argument) > and pinstripe has code for hiding it? Shouldn't people pass message-icon if > they want that? Sure, but should the icon class-mangling code run if they pass no icon?

I would just not touch info.icon's class if that parameter is empty. People passing an empty class would get no icon.

Whoops, I accidentally duplicated bug 498893. Sorry. I've just noticed that message-icon etc. are documented on MDC!

Comment on attachment 403198 [details] [diff] [review] Remove bogus message-icon consumers >+ iconElement.className += " " + iconClass; iconElement.classList.add(iconClass);?

This landed 5 hours ago as: http://hg.mozilla.org/mozilla-central/rev/29e8f7716f6d However, every single one of the 8 Linux mochitest-plain runs since then have had test failures in "test_prompt.html" and have timed out the mochitest harness in "test_prompt_async.html": http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254169763.1254173049.17329.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254172373.1254175527.12761.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254174039.1254177588.4052.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254178446.1254181861.19312.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254169523.1254172494.11086.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254171028.1254173685.24517.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254172936.1254175535.12805.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254178822.1254181904.19656.gz It hasn't been as consistent on Mac & Windows, but there have been 3 windows tinderbox cycles with the same failures: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254176310.1254179363.24094.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254176781.1254179410.24491.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254179086.1254181393.13939.gz and one Mac tinderbox cycle with the same failure: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254175153.1254178088.9812.gz Given that this patch modifies prompt-related code, I think it's safe to assume this is related... I'm backing this out.

(I'm not sure why this bug has Version field set to "1.9.1" right now -- the patch landed on mozilla-central, so I'm setting this bug's Version field to "Trunk".)

backout complete: http://hg.mozilla.org/mozilla-central/rev/58da7dbc028e

Huh, weird. I don't see offhand why that should break the test. Maybe it's changing the timing of when the icon image loads, and that upsets the test's (buggy?) timing?

(In reply to comment #24) > (I'm not sure why this bug has Version field set to "1.9.1" right now Because the bug exists on 1.9.1 (which doesn't mean only 1.9.1). This is particularly relevant for Thunderbird. > the patch landed on mozilla-central That's the target milestone.

I happened to be running mochitests on a build including this patch (ie, before it was backed out). Running test_prompt.html plops a few of these into the error console when it fails: Error: uncaught exception: [Exception... "String contains an invalid character" code: "5" nsresult: "0x80530005 (NS_ERROR_DOM_INVALID_CHARACTER_ERR)" location: "chrome://global/content/commonDialog.js Line: 168"] Not sure if these have been there all along, or if it's related to the test failure...

Pushed changeset fbea0edb9622 to mozilla-central, but renaming the variable to iconClasses to remind us why comment #21 won't work.

So this leaves this css pointing to nonexistent images (and commonDialog consumers can trigger it). was that on purpose?

Comment on attachment 396427 [details] [diff] [review] Option 2: change the CSS Pushed changeset 5c8ab5dd1792 to mozilla-central. (In reply to comment #30) > So this leaves this css pointing to nonexistent images. was that on purpose? No, it was an oversight. > (and commonDialog consumers can trigger it) Although documented on devmo I was only able to find comm-central uses.

Comment on attachment 396427 [details] [diff] [review] Option 2: change the CSS Makes the message-icon class work as documented on all platforms. (Attachment 403198 [details] [diff] isn't a necessary part of the fix.)

Do we need to fix this on 1.9.2 as well, per the dup?

We could, I don't think we need to.

Not blocking, a192=beltzner though

Pushed changeset 1efddecb6ada to releases/mozilla-1.9.2