As bug 446027 showed, it's rather likely to inadvertently add double ids in the preferences. We should have a chrome(?) test for that.

creating tests is nice but does not block a release. Sure, those tests are wanted and could help a lot, but we _can_ release without them.

Test preferences and some other windows for duplicate ids. MailNews windows don't work yet, because no accounts are defined. Addressbook should work, but doesn't because of bug 37919. Chatzilla/Inspector/Venkman aren't available during chrome tests (yet?).

Comment on attachment 331178 [details] [diff] [review] duplicate id tests + $(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/chrome/$(relativesrcdir) You now need to change $(DEPTH)/_tests to $(DEPTH)/mozilla/_tests because of the hg migration + // Have all loaded windows been closed again? + function AllWindowsClosed() + { + for each (var e in window.gLoadedWindows) + return false; + return true; + } Either this is wrong, or you could do return window.gLoadedWindows; preferences, navigator and editor all fail, but I guess you could check this in with just the console test enabled.

(In reply to comment #3) > (From update of attachment 331178 [details] [diff] [review]) > + $(INSTALL) $(foreach f,$^,"$f") > $(DEPTH)/_tests/testing/mochitest/chrome/$(relativesrcdir) > > You now need to change $(DEPTH)/_tests to $(DEPTH)/mozilla/_tests because of > the hg migration Actually, pleas use $(MOZDEPTH)/_tests as that variable always points to the mozilla dir even if we might consider renaming it one time.

Before I can check this in, I'll post a couple of patches to avoid turning the tree orange right away... ;-)

Fix up the easy cases (printButton and bundlePreferences), plus collateral cleanup.

Comment on attachment 333469 [details] [diff] [review] double id cleanup, part 1: preferences #1 [checked in] Landed on trunk.

The key ids are never actually used. I checked the sidebar element removal with the sidebar in browser, composer and mail editor.

Comment on attachment 333648 [details] [diff] [review] double id cleanup, part 2: browser window [checked in without sidebarOverlay.xul] Second pair of knowing sidebar eyes needed. ;-)

Comment on attachment 333648 [details] [diff] [review] double id cleanup, part 2: browser window [checked in without sidebarOverlay.xul] >- <popupset id="contentAreaContextSet"/> >+ <!--popupset id="contentAreaContextSet"/--> Assume that the + line does not exist, only the - one, so that the element gets removed completely.

Comment on attachment 333648 [details] [diff] [review] double id cleanup, part 2: browser window [checked in without sidebarOverlay.xul] me=r+ (first time, hope I've done this correctly) >>- <popupset id="contentAreaContextSet"/> >>+ <!--popupset id="contentAreaContextSet"/--> > >Assume that the + line does not exist, only the - one, so that the element gets >removed completely. while grepping through comm-central I noticed that debugQATextEditorShell.xul pulls in contentAreaContextOverlay.xul for no apparent reason, but that's grist for another bug.

This fixes the doubled IDs in EditorContextMenuOverlay.xul and editor, accidentally also fixing the wrong application of acceltexts to the "Select Row" context menu instead of to the menu menu (see <http://mxr.mozilla.org/seamonkey/source/editor/ui/composer/content/editor.js#638>).

Comment on attachment 333648 [details] [diff] [review] double id cleanup, part 2: browser window [checked in without sidebarOverlay.xul] >- <popupset id="contentAreaContextSet"/> >+ <!--popupset id="contentAreaContextSet"/--> The problem here as I recall is that not all windows with a sidebar have a content area; specifically Editor and Address Book don't normally require the contentAreaContextSet so for the sidebar to be able to use the content area context menu it needed to provide an overlay point for it.

Comment on attachment 333813 [details] [diff] [review] double id cleanup, part 3: editor window [checked in] I wonder why these are still observes= instead of command= (I didn't try to see whether command= works or not).

- switched to $(MOZDEPTH) - fixed a reentrancy problem in the paneload mechanism, causing certain overlays to be applied twice (they were too fast for the core to bark!) - had specialcase the character_encoding_pane, because (only?) RDF templates create fancy ids - typo Hence rerequesting review.

One last double id in preferences.

(In reply to comment #13) > The problem here as I recall is that not all windows with a sidebar have a > content area; specifically Editor Neither mail editor nor Composer show any regressions as far as I can tell. What am I missing? > and Address Book But Address Book doesn't even have a sidebar at all?!

Comment on attachment 334051 [details] [diff] [review] double id cleanup, part 4: preferences #2 [checked in] Landed on trunk.

Comment on attachment 333648 [details] [diff] [review] double id cleanup, part 2: browser window [checked in without sidebarOverlay.xul] >- <popupset id="contentAreaContextSet"/> >+ <!--popupset id="contentAreaContextSet"/--> This breaks the context menu in sidebar's webpanels, e.g. the Tinderbox panel. Since there's no easy way to say "only add the element with this id if the id isn't used elsewhere in this window already"¹, I'll specialcase this for the browser window in the test itself. :-| I landed this on trunk, but *WITHOUT* the changes to suite/common/sidebar/sidebarOverlay.xul! ¹ Using the removeelement attribute doesn't help; killing the second id instance via JS either doesn't see the outer element yet or may run after the test...

Allow ignoring certain ids, and do so for contentAreaContextSet when checking navigator.xul.

Comment on attachment 333813 [details] [diff] [review] double id cleanup, part 3: editor window [checked in] diff --git a/editor/ui/composer/content/editor.xul b/editor/ui/composer/content/editor.xul --- a/editor/ui/composer/content/editor.xul +++ b/editor/ui/composer/content/editor.xul @@ -154,7 +154,7 @@ <menuitem id="viewAllTagsMode"/> <menuitem id="viewSourceMode"/> <menuitem id="viewPreviewMode"/> - <menuseparator id="viewSep1"/> + <menuseparator id="viewSep2"/> <menu id = "composerCharsetMenu" /> </menupopup> </menu> I haven't tested this, but it looks like you'll need to update http://mxr.mozilla.org/comm-central/source/mozilla/editor/ui/composer/content/editor.js#453

(In reply to comment #21) > I haven't tested this, but it looks like you'll need to update > http://mxr.mozilla.org/comm-central/source/mozilla/editor/ui/composer/content/editor.js#453 No, I don't need to do that. But I ghad to kill my mozilla-central clone and accidently killed my diff defaults, hence the -U3. The patch touches this: 152 <menuseparator id="viewSep1"/> 153 <menuitem id="viewNormalMode" checked="true"/> 154 <menuitem id="viewAllTagsMode"/> 155 <menuitem id="viewSourceMode"/> 156 <menuitem id="viewPreviewMode"/> 157 <menuseparator id="viewSep1"/> 158 <menu id = "composerCharsetMenu" />

Comment on attachment 333813 [details] [diff] [review] double id cleanup, part 3: editor window [checked in] OK, looks good then.

(In reply to comment #21) > http://mxr.mozilla.org/comm-central/source/mozilla/editor/ui/composer/content/editor.js#453 See also my bug 428852 Cv1 patch...

Comment on attachment 333813 [details] [diff] [review] double id cleanup, part 3: editor window [checked in] Landed on trunk.

Comment on attachment 334185 [details] [diff] [review] duplicate id tests, v3 [checked in] Landed on trunk.

I filed bug 451952 for the mailnews main window tests I had to comment out.

(In reply to comment #11) > I noticed that debugQATextEditorShell.xul pulls in contentAreaContextOverlay.xul for no apparent reason I filed bug 451959. (In reply to comment #14) > I wonder why these are still observes= instead of command= I filed bug 451960.