68174 - Remove event handlers from within <template> (mail/ and mailnews/ part)

Status: RESOLVED WORKSFORME

(MailNews Core :: Backend, defect)

Description

[(not reading, please use [e-mail] instead)]

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.

Comment 1

[(not reading, please use [e-mail] instead)]

over to bhuvan to fix.

Comment 2

[racham]

taking the bug.

Comment 3

[(not reading, please use [e-mail] instead)]

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

Comment 4

[(not reading, please use [e-mail] instead)]

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

Comment 5

[(not reading, please use [e-mail] instead)]

adding gayatrib.

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

Comment 6

[selmer (gone)]

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

Comment 7

[Håkan Waara]

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

Comment 8

[(not reading, please use [e-mail] instead)]

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.

Comment 9

[scottputterman]

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.

Comment 10

[scottputterman]

moving to 0.9.3

Comment 11

[scottputterman]

moving to 0.9.4

Comment 12

[scottputterman]

moving to 0.9.5

Comment 13

[scottputterman]

moving to 0.9.6

Comment 14

[scottputterman]

really moving to 0.9.6

Comment 15

[scottputterman]

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

Comment 16

[[e-mail]]

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

Comment 17

[(not reading, please use [e-mail] instead)]

mass re-assign.

Comment 18

[basic]

status?

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (Av1-SM) <messengercompose.xul>

[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 20

[[e-mail]]

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

Comment 21

[Serge Gautherie (:sgautherie)]

(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.

Comment 22

[[e-mail]]

(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.

Comment 23

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-SM) <messengercompose.xul> ++

[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 24

[[e-mail]]

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.

Comment 25

[[e-mail]]

(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.

Comment 26

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1a-SM) <messengercompose.xul> ++ [Checkin: Comment 29]

[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).

Comment 27

[Serge Gautherie (:sgautherie)]

(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 ;-))

Comment 28

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1-TB) <messengercompose.xul> ++ [Checkin: Comment 31]

Bv1a-SM port/copy to TB.

Comment 29

[Reed Loden [:reed]]

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 30

[Phil Ringnalda (:philor)]

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

r=philringnalda, thanks.

Comment 31

[Reed Loden [:reed]]

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

Comment 32

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1-SM) <ABSearchDialog.xul> [Checkin: Comment 37]

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

Trivial.

Comment 33

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1-TB) <ABSearchDialog.xul> [Checkin: Comment 50]

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

Comment 34

[Serge Gautherie (:sgautherie)]

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 ?

Comment 35

[[e-mail]]

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

Comment 36

[Serge Gautherie (:sgautherie)]

(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 ?

Comment 37

[Reed Loden [:reed]]

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

Comment 38

[[e-mail]]

(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.

Comment 39

[Serge Gautherie (:sgautherie)]

(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 ;-)

Comment 40

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1-SM) <FilterListDialog.*> (+ NNTP server fix)

[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.

Comment 41

[Serge Gautherie (:sgautherie)]

(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)
]

Comment 42

[Serge Gautherie (:sgautherie)]

Attachment: (Gv1-TB) <FilterListDialog.*> (+ NNTP server fix)

Fv1-SM port/copy to TB.

Comment 43

[[e-mail]]

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.

Comment 44

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1a-SM) <FilterListDialog.*> (+ NNTP server fix) [Checkin: Comment 47]

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...

Comment 45

[Serge Gautherie (:sgautherie)]

Attachment: (Gv1a-TB) <FilterListDialog.*> (+ NNTP server fix) [Checkin: Comment 50]

Fv1a-SM port/copy to TB.

Comment 46

[Serge Gautherie (:sgautherie)]

(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.)

Comment 47

[Reed Loden [:reed]]

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

Comment 48

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1-SM) <mailWindowOverlay.xul> (1/2: Copy+File+Move)

[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...

Comment 49

[[e-mail]]

(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 :-\

Comment 50

[Reed Loden [:reed]]

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

Comment 51

[Serge Gautherie (:sgautherie)]

Attachment: (Iv1) <SearchDialog.js> "MsgMoveMessage" related nit

(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 52

[[e-mail]]

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.

Comment 53

[Serge Gautherie (:sgautherie)]

Attachment: (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)

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.

Comment 54

[Serge Gautherie (:sgautherie)]

(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).

Comment 56

[Wayne Mery (:wsmwk)]

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

Comment 57

[Serge Gautherie (:sgautherie)]

(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.

Comment 58

[Magnus Melin [:mkmelin]]

The template this bug was about has been removed since long.