When writing test_idcheck.xul, I had to leave out chrome://messenger/content/messenger.xul chrome://messenger/content/messengercompose/messengercompose.xul chrome://messenger/content/addressbook/addressbook.xul because they require a mail account to work, hence the account wizard pops up, killing the test by timout. This bug is about finding a way to check these main windows as well, maybe by setting up the account preferences in the test and deleting them again afterwards?

This is something I haven't figured out yet. We've got various scenarios where doing chrome-level tests are going to want mail accounts etc setting up/to be available, and we won't really want the tests dependent on the previous leaving the data in an appropriate state. For now, you could probably reuse some of the mail bloat test profile setup code, if I can get dmose to actually review it, and we'll figure out the rest later. Not sure how this would hook into the chrome test setup though.

This patch modifies the test profile preferences to contain at least one basic mail account (Local Folders for this special purpose), so that the account creation wizard does not chime in when testing ids. Currently, this detects over 20 id collisions in mailnews/addressbook ui, which I will fix in subpatches here.

Comment on attachment 337514 [details] [diff] [review] activate double id check for mailnews >+ // fake a mail account (Local folders in this case here) >+ // to avoid the account creation wizard nsIMessengerMigrator::createLocalMailAccount(false); Or is that too simple?

(In reply to comment #3) > (From update of attachment 337514 [details] [diff] [review]) > >+ // fake a mail account (Local folders in this case here) > >+ // to avoid the account creation wizard > nsIMessengerMigrator::createLocalMailAccount(false); > > Or is that too simple? Or just do it like we do for unit tests: http://mxr.mozilla.org/comm-central/source/mailnews/test/resources/mailTestUtils.js#43

Alas, unit test js files are not available to chrome tests, so I had to drag it into the chrome dir by some makefile voodoo...

(Forgot one file in /editor.)

Comment on attachment 338401 [details] [diff] [review] fix double ids, v1 >@@ -263,17 +261,16 @@ <commandset id="mailToolbarItems" > <command id="button_delete"/> > <command id="button_mark"/> > <command id="button_getNewMessages"/> > <command id="button_print"/> > <command id="button_next"/> > <command id="button_goBack"/> > <command id="button_goForward"/> > <command id="button_file"/> >- <command id="cmd_delete"/> > <command id="cmd_shiftDelete" oncommand="goDoCommand('cmd_shiftDelete');"/> > <command id="button_junk"/> > </commandset> I'm worried that this will break the delete key in certain edge cases, because it is updated when the focus changes (see FocusRingUpdate_Mail). For example, put the focus in the empty quick search box, or on a special folder, and notice that the Delete menuitem is disabled. Then tab to the thread pane and press the delete key.

Comment on attachment 338401 [details] [diff] [review] fix double ids, v1 >- <command id="cmd_delete"/> Confirmed that this breaks updating the delete key when changing focus. sr=me on the rest of the patch.

Like v2, but: - checks messageWindow.xul, too - moves charset list exception handling into its own function, because we'll need it again when the languages pref panel gets in (it contains yet another RDF driven charset list) Taking over sr, rerequesting r?.

Same as v1, except special weirdoism for updating cmd_delete from somewhere else. (Actually, I think that the way how goUpdateMailMenuItems works is not sane.)

Comment on attachment 338475 [details] [diff] [review] fix double ids, v2 >+ <command id="button_junk"/> > <command id="cmd_shiftDelete" oncommand="goDoCommand('cmd_shiftDelete');"/> >- <command id="button_junk"/> Any particular reason to move this? >+ <!-- avoid a double id cmd_delete, but make sure that the command update >+ is available along with the other commands in this group --> >+ <command commandupdater="true" >+ events="mail-toolbar" >+ oncommandupdate="goUpdateCommand('cmd_delete')"/> Could you make this a commandset for consistency? Or just add the goUpdateCommand to the parent's oncommandupdate handler? Or maybe we should just fix FocusRingUpdate_Mail to update manually instead of via this commandset.

Same as v1, but moved the goUpdateCommand into the commandset handler. This does look much cleaner, indeed. If we need more instances of such special casing, we probably should just alter goUpdateMailMenuItems to check an attribute "updateid" before taking the element id...

No real changes from v3, just fixing the bustage which would have been caused by bug 444411. ;-)

Comment on attachment 338737 [details] [diff] [review] activate double id check for mailnews, v4 [checked in] r=me if you include the Makefile.in change from v2.

Comment on attachment 338514 [details] [diff] [review] fix double ids, v3 [checked in] Landed on trunk as <http://hg.mozilla.org/comm-central/rev/6798c5a94726>.

Comment on attachment 338737 [details] [diff] [review] activate double id check for mailnews, v4 [checked in] Landed this along with the Makefile.in change of v2 on trunk as <http://hg.mozilla.org/comm-central/rev/cb70018a34dc>.