293100 - ComposeMessage logic is perf nightmare

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[timeless]

(esp for forward many messages as attachments)

1. the code spends 2n-1 times concatenating elements from an array. it could use
join(",") which would make spidermonkey *much* happier.
2. the code spends n+1 times trying to find an identity. This should at least be
optimizable so that it's optimistic and only looks for an identity while it
hasn't found one.
3. it recalculates whether it wants new windows each time through this loop.

Comment 1

[timeless]

Attachment: reorganize the code

Comment 2

[Stephen Donner [:stephend] Not actively reading bugmail]

Please capitalize names of buttons, as they are proper nouns and it makes for
easier perusal of the code.

Comment 3

[[e-mail]]

Comment on attachment 182754 [details] [diff] [review]
reorganize the code

>+  switch (type) {
>+  case msgComposeType.New: //new message
We indent case under switch, this should have the benefit of "fixing" the
whitespace changes (particularly as bugzilla can't convert a diff into a diff
-w).

>+    messenger.SetWindow(window, msgWindow);
Odd, I wonder what the point of that is; it gets set in the onload handler, so
it's unnecessary here. It's already been commented out of mail-offline.js too.

>+    var object = null;
Don't let me stop you from killing unused variables ;-)

>+    var useNewWin = type != msgComposeType.ForwardAsAttachment;
>+    if (!useNewWin) {
No point creating this variable for a single use. In fact I almost think this
should be a separate case "block".

>+/*
>+ type == msgComposeType.Reply || type == msgComposeType.ReplyAll || type == msgComposeType.ForwardInline ||
>+            type == msgComposeType.ReplyToGroup || type == msgComposeType.ReplyToSender || 
>+            type == msgComposeType.ReplyToSenderAndGroup ||
>+            type == msgComposeType.Template || type == msgComposeType.Draft;
>+ */
Don't see the point of this.

>+    var uri = "";
You don't use this any more, do you?

Comment 4

[Wayne Mery (:wsmwk)]

still worth pursuing?

Comment 5

[Wayne Mery (:wsmwk)]

would fixing this bug in any way improve Bug 130010?

Comment 6

[[e-mail]]

No, that bug is about setting the list of newsgroups after creating the window; this bug is just cleaning up some of the window creation logic.

Comment 7

[Wayne Mery (:wsmwk)]

assuming timeless still wants assignment

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 182754 [details] [diff] [review]
reorganize the code

Patch is obsolete - mailnews/base/resources/content/mailCommands.js file doesn't exist.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Is this bug still relevant - if so, timeless, any chance of a new patch?

Comment 10

[Ian Neal]

Attachment: Revised patch v0.2

Changes since last patch:
* Unbitrotted to take account of forking.
* Review comments addressed.
* Switched to using GetIdentityForHeader for SM.

Comment 11

[[e-mail]]

Comment on attachment 502009 [details] [diff] [review]
Revised patch v0.2

>+    case msgComposeType.ForwardAsAttachment:
>+      if (messageArray && messageArray.length)
...
>+    default:
Need to check messageArray != null here too?

>+      var messageUri;
>+      for (var i = 0; i < messageArray.length; ++i)
>+      {
>+        messageUri = messageArray[i];
Nit: could use var messageUri = messageArray[i];

Comment 12

[Ian Neal]

Attachment: Revised patch v2.1 [Checked in: Comment 13]

Changes since v2.0
* Added another null check on messageArray as suggested.
* Moved var declaration into for loop as suggested.

Carrying forward r= and requesting sr= to cover off Thunderbird as well as SeaMonkey

Comment 13

[Ian Neal]

Comment on attachment 506272 [details] [diff] [review]
Revised patch v2.1 [Checked in: Comment 13]

http://hg.mozilla.org/comm-central/rev/59dbf0a18c42