see #68074 for background. I've found 5 files (in the *.xul under mozilla/mailnews, we should check ns) for xul where we used templates and have event handlers on generated content. mailWindowOverlay.xul oncommand="MsgMoveMessage(event.target)" oncommand="MsgCopyMessage(event.target)" oncommand="MsgGetMessagesForAccount(event)" msgFolderPickerOverlay.xul oncommand="PickedMsgFolder(event.target,'msgNewFolderPicker')" etc. (there are a whole crap load like that) FilterListDialog.xul onclick="onToggleEnabled(event);" pref-composing_messages.xul oncreate="dump('CREATE MENU NOW\n');" messengercompose.xul oncommand="MessageFcc(event.target)" I'm most interested in messengecompose.xul and mailwindowOverlay.xul, since they are going to be hit on startup of message compose and the 3 pane. racham / ducarroz, I'll probably need your help fix this.

over to bhuvan to fix.

taking the bug.

the easy one, pref-composing_messages.xul, has been fixed on the mail performance branch.

strike that, I accidentally landed that on the trunk, but brendan let it slide.

adding gayatrib. I think she's got some of these changes in her local tree for search.

This looks like low hanging fruit for startup perf... nom nsbeta1.

Indeed, this could speed us up and would be pretty easy to fix I think.

when prioritizing this among your other bugs, you should see just how much of a win it would be. one way to estimate this is to take out event handlers and see if you get any performance increase.

moving to 0.9.2 for the moment. If someone can update this bug with whether or not it makes much of a difference, that would be appreciated.

moving to 0.9.3

moving to 0.9.4

moving to 0.9.5

moving to 0.9.6

really moving to 0.9.6

moving to 1.0. If someone believes this will make a noticeable difference then we can move it in.

Moving the event handlers should be a straightforward exercise. Should they go on the outermost menupopup or on the menupopup parent?

mass re-assign.

status?

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2008011403 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4) Both cases work. But I guess there would be a better test to use !? ***** Could someone post an up-to-date list of which files/cases need to be fixed ? (It seems searching for "oncommand" between "<template>.*</template>" is not possible with (LXR/)MXR.)

Comment on attachment 297072 [details] [diff] [review] (Av1-SM) <messengercompose.xul> >+ oncommand="MessageFcc((event.target.getAttribute('id') === '') ? event.target.parentNode.parentNode : event.target);"> Sorry, I don't like this. I can think of two alternatives: 1. Change MessageFcc to find the right id 2. Add another attribute, e.g. fcc="rdf:*", and use that instead

(In reply to comment #20) > (From update of attachment 297072 [details] [diff] [review]) > >+ oncommand="MessageFcc((event.target.getAttribute('id') === '') ? event.target.parentNode.parentNode : event.target);"> > Sorry, I don't like this. I can think of two alternatives: Me, neither: this was only how far I could go/show. > 1. Change MessageFcc to find the right id If you mean moving the test inside the function, this, I can do, but I'm not sure I like it. Yet, if you feel that it's a good solution, fine with me. > 2. Add another attribute, e.g. fcc="rdf:*", and use that instead That's more what I would like ... if it's no bad wrt perf/footprint. (I read doc. and tried a little, but didn't succeed.) But, for this, I would need a full example (with this file), which I could use for the other files as needed: helpwanted ! Thanks.

(In reply to comment #21) >I would need a full example (with this file) I'm sure you can work it out from these hints: <menupopup> - <menuitem label="&fileHereMenu.label;" - oncommand="MessageFcc(event.target.parentNode.parentNode)"/> + <menuitem label="&fileHereMenu.label;" resource="rdf:*"/> <menuseparator/> <menuitem uri="..." class="folderMenuItem menuitem-iconic" la bel="rdf:http://home.netscape.com/NC-rdf#Name" - oncommand="MessageFcc(event.target)" + resource="rdf:*" SpecialFolder="rdf:http://home.netscape.com/NC-rdf#SpecialFolder" Plus: add MessageFcc(event.target) on the root menuitem; change MessageFcc to use getAttribute("resource"); instead of id. Note: I arbitrarily chose "resource" as the attribute name, feel free to use a different name.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2008012001 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4) Av1-SM, with comment 22 update; and a rewrite of the function, by using early returns. (In reply to comment #22) > I'm sure you can work it out from these hints: Thanks, it helped me (to get all the pieces together the right way) !

Comment on attachment 298128 [details] [diff] [review] (Bv1-SM) <messengercompose.xul> ++ >+ msgCompFields.fcc2 = "nocopy://"; This "nocopy://" is weird. I wonder why we use it. As far as we can tell, the only reason we check for it is to set it back to blank again. I'll file a bug. >+ dump("FCC2: " + fccUri + "\n"); Let me find out whether we still want these dump()s.

(In reply to comment #24) >(From update of attachment 298128 [details] [diff] [review]) >>+ dump("FCC2: " + fccUri + "\n"); >Let me find out whether we still want these dump()s. No we don't, thanks.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2008012001 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4) Bv1-SM, with comment 25 update; and a little more rewrite(s).

(In reply to comment #24) > (From update of attachment 298128 [details] [diff] [review]) > >+ msgCompFields.fcc2 = "nocopy://"; > This "nocopy://" is weird. I wonder why we use it. As far as we can tell, the (That's how I felt too, but I didn't even try to look into that matter.) > only reason we check for it is to set it back to blank again. I'll file a bug. (You filed bug 413256 ;-))

Bv1a-SM port/copy to TB.

Checking in mailnews/compose/resources/content/MsgComposeCommands.js; /cvsroot/mozilla/mailnews/compose/resources/content/MsgComposeCommands.js,v <-- MsgComposeCommands.js new revision: 1.409; previous revision: 1.408 done Checking in mailnews/compose/resources/content/messengercompose.xul; /cvsroot/mozilla/mailnews/compose/resources/content/messengercompose.xul,v <-- messengercompose.xul new revision: 1.297; previous revision: 1.296 done

Comment on attachment 298984 [details] [diff] [review] (Cv1-TB) <messengercompose.xul> ++ [Checkin: Comment 31] r=philringnalda, thanks.

Checking in mail/components/compose/content/MsgComposeCommands.js; /cvsroot/mozilla/mail/components/compose/content/MsgComposeCommands.js,v <-- MsgComposeCommands.js new revision: 1.123; previous revision: 1.122 done Checking in mail/components/compose/content/messengercompose.xul; /cvsroot/mozilla/mail/components/compose/content/messengercompose.xul,v <-- messengercompose.xul new revision: 1.113; previous revision: 1.112 done

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2008012302 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4) Trivial.

Dv1-SM port/copy to TB, plus whitespace cleanup/sync.

To update comment 0, I checked out the SeaMonkeyMailNews cvs module (and did some "grep -l -z" + manual search): List of |oncommand=| left to be moved: mailnews/base/resources/content/mailWindowOverlay.xul lots of mailnews/base/resources/content/msgFolderPickerOverlay.xul lots of mailnews/base/search/resources/content/FilterListDialog.xul 3 in 1 template xpinstall/res/content/updateNotification.xul 1 at ligne 25 The <xpinstall/> file is now unused, isn't it ?

(In reply to comment #34) >The <xpinstall/> file is now unused, isn't it ? It's outside of the scope of this bug anyway.

(In reply to comment #34) > mailnews/base/search/resources/content/FilterListDialog.xul 3 in 1 template [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2008012802 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4) Neil, I started to test |<menulist id="serverMenu" ...|: Selecting: *Local/Pop3 account: triggers line 110 (uri); fine. *A newsgroup: triggers line 83 (uri); fine. *Nntp account, with no children: triggers nothing; is it fine ? *Nntp account, "this news server": triggers line 91 (uri) then line 83 (blank); odd ! See "1.67 scott%scott-macgregor.org 2004-04-18 22:15 fix for bug #19403". Are the behaviors of the 2 last ones correct ? If not, how to fix them ?

Checking in mailnews/base/search/resources/content/ABSearchDialog.xul; /cvsroot/mozilla/mailnews/base/search/resources/content/ABSearchDialog.xul,v <-- ABSearchDialog.xul new revision: 1.19; previous revision: 1.18 done

(In reply to comment #36) > I started to test |<menulist id="serverMenu" ...|: > Selecting: > *Local/Pop3 account: triggers line 110 (uri); fine. > *A newsgroup: triggers line 83 (uri); fine. > *Nntp account, with no children: triggers nothing; is it fine ? > *Nntp account, "this news server": triggers line 91 (uri) then line 83 (blank); > odd ! > > Are the behaviors of the 2 last ones correct ? > If not, how to fix them ? > First, I should say that I think this needs to be replaced by a tree-like widget such as the one used in the advanced search dialog (note that I don't expect you to do that). The last behaviour isn't exactly correct, it appears to be an attempt to reduce the number of event handlers - notice that there are no event handlers in the <rule nc:ServerType="nntp"> section. The third behaviour isn't exactly correct either, but it depends on what you expect for a news server without any subscribed groups.

(In reply to comment #34) > xpinstall/res/content/updateNotification.xul 1 at ligne 25 (In reply to comment #35) > (In reply to comment #34) > >The <xpinstall/> file is now unused, isn't it ? > It's outside of the scope of this bug anyway. Bug 406807 removed that file yesterday ;-)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2008013002 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4) Killing 2 birds with one stone: *Fixes current bug. *"Fixes" behavior on (both) NNTP _server_ items, by "backing out" most of what bug 19403 did to these files. Example of previously bad behavior: Having a NNTP server without groups, opening the filter dialog from it would load its filters, while selecting it in the dialog list would not.

(In reply to comment #38) > First, I should say that I think this needs to be replaced by a tree-like > widget such as the one used in the advanced search dialog (note that I don't > expect you to do that). It seems that change will be done in bug 413781 or a followup. Yet, I think it's better to fix the current bug on its own (first). > The last behaviour isn't exactly correct, it appears to be an attempt to reduce > the number of event handlers - notice that there are no event handlers in the > <rule nc:ServerType="nntp"> section. > > The third behaviour isn't exactly correct either, but it depends on what you > expect for a news server without any subscribed groups. I'm not familiar with bug 19403 new feature, but I believe the current code(s) is broken. See bug 19403 comment 9, as an hint: [ Scott MacGregor 2004-04-18 10:24:40 PST // XXX this "!itemURI" thing is a hack // we come into onFilterServerClick twice // and when you choose a news server, and the second time it is "" if (!itemURI || itemURI == gCurrentServerURI) ]

Fv1-SM port/copy to TB.

Comment on attachment 300788 [details] [diff] [review] (Fv1-SM) <FilterListDialog.*> (+ NNTP server fix) > label="rdf:http://home.netscape.com/NC-rdf#Name"> Can you add an itemUri to this menu too please, to work around a bug/feature in up/down key handling in menulists, thanks.

Fv1-SM, with comment 43 update. Restores selecting a NNTP server with children with keyboard, which previous patch would have removed. NB: Selecting a child/newsgroup by keyboard is still impossible. I guess this is (partly) why Neil spoke of "tree" in comment 38...

Fv1a-SM port/copy to TB.

(In reply to comment #44) > NB: Selecting a child/newsgroup by keyboard is still impossible. "... without opening the dropdown (Alt+Down) first", I mean :-> (In reply to comment #43) > [...] to work around a bug/feature in up/down key handling in menulists, thanks. Is that a filed "bug" ? (The closest bug I found is bug 302060, but it doesn't seem to be the same issue.)

Checking in mailnews/base/search/resources/content/FilterListDialog.js; /cvsroot/mozilla/mailnews/base/search/resources/content/FilterListDialog.js,v <-- FilterListDialog.js new revision: 1.69; previous revision: 1.68 done Checking in mailnews/base/search/resources/content/FilterListDialog.xul; /cvsroot/mozilla/mailnews/base/search/resources/content/FilterListDialog.xul,v <-- FilterListDialog.xul new revision: 1.71; previous revision: 1.70 done

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2008020202 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4) That's 7 times the "same" template, which has 6 rules: *1 & 2 items are disabled, then don't care. *3 & 6 are updated :-) *4 is empty, then don't care. *5 seems good as it is, but I'm not sure what its "test case" is. Can you confirm that I understood it well enough ? *** Then, 2 |oncommand="MsgGetMessagesForAccount(event)"| remain. They are mixed with other |command="..."| and |oncommand="..."|. Can you give me some "hints" on how to "merge" them, or I'll leave it to someone else...

(In reply to comment #48) >*5 seems good as it is, but I'm not sure what its "test case" is. >Can you confirm that I understood it well enough ? Rule 5 seems to be for accounts (I don't understand why rule 1 doesn't have the same content as rule 5). >Then, 2 |oncommand="MsgGetMessagesForAccount(event)"| remain. >They are mixed with other |command="..."| and |oncommand="..."|. Wow, what a headache... I'm not even sure that works properly :-\

Checking in mail/base/content/ABSearchDialog.xul; /cvsroot/mozilla/mail/base/content/ABSearchDialog.xul,v <-- ABSearchDialog.xul new revision: 1.18; previous revision: 1.17 done Checking in mail/base/content/FilterListDialog.js; /cvsroot/mozilla/mail/base/content/FilterListDialog.js,v <-- FilterListDialog.js new revision: 1.14; previous revision: 1.13 done Checking in mail/base/content/FilterListDialog.xul; /cvsroot/mozilla/mail/base/content/FilterListDialog.xul,v <-- FilterListDialog.xul new revision: 1.6; previous revision: 1.5 done

(Not worth creating a separate bug.) *"MsgMoveMessage" -> "MoveMessageInSearch", in |dump()| statement. This code is created/blamed to [ 1.33 gayatrib%netscape.com 2001-03-24 01:08 Implementing deleting/filing messages on single and multiple folders in search. Fixed regression introduced after merging performance branch that search does not display number of search hits. r=bhuvan,bienvenu. sr=bienvenu. ]

Comment on attachment 302295 [details] [diff] [review] (Iv1) <SearchDialog.js> "MsgMoveMessage" related nit I don't think it's worth it... also that 6-space indentation is wrong, and you should have used the -p option when making the patch.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008040902 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4) Hv1-SM, extended after bug 416669. Keeping sr+. The only difference in these (7) new occurrences is that the "outer" |oncommand=| implicitly applies to the (new) "inner" menus too.

(In reply to comment #53) > Created an attachment (id=314595) [details] > (Hv1a-SM) <mailWindowOverlay.xul> (1/2: Copy+File+Move) > > [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008040902 > SeaMonkey/2.0a1pre] (nightly) (W2Ksp4) PS: While testing this patch, I noticed that in the Thread and Message panes (at least ! Not in Menu and Toolbar ?), I could get the two recent (IMAP) folders I was testing with to: sometimes show up as 'folder - account' (as "always" in the Menu and Toolbar), sometimes show up as 'folder' only. For now, I assume this is an "issue / inconsistency" in the "datasource" and unrelated to this front-end patch(es).

does "1/2" imply is there a part 2 yet to be done for mailwindowOverlay for thunderbird?

(In reply to comment #56) > does "1/2" imply is there a part 2 yet to be done for mailwindowOverlay for > thunderbird? As you wrote in your email to me: yes, I'm waiting for bug 444913 to be fixed before trying to resume work here.