Closed Bug 293100 Opened 15 years ago Closed 9 years ago

ComposeMessage logic is perf nightmare

Categories

(MailNews Core :: Composition, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.3a3

People

(Reporter: timeless, Assigned: iann_bugzilla)

References

()

Details

(Keywords: perf)

Attachments

(1 file, 2 obsolete files)

(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.
Attached patch reorganize the code (obsolete) — Splinter Review
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #182754 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #182754 - Flags: review?(neil.parkwaycc.co.uk)
Please capitalize names of buttons, as they are proper nouns and it makes for
easier perusal of the code.
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?
Attachment #182754 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #182754 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #182754 - Flags: review-
still worth pursuing?
would fixing this bug in any way improve Bug 130010?
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.
assuming timeless still wants assignment
QA Contact: composition
Product: Core → MailNews Core
Whiteboard: [patchlove]
Attachment #182754 - Attachment is obsolete: true
Comment on attachment 182754 [details] [diff] [review]
reorganize the code

Patch is obsolete - mailnews/base/resources/content/mailCommands.js file doesn't exist.
Is this bug still relevant - if so, timeless, any chance of a new patch?
Attached patch Revised patch v0.2 (obsolete) — Splinter Review
Changes since last patch:
* Unbitrotted to take account of forking.
* Review comments addressed.
* Switched to using GetIdentityForHeader for SM.
Assignee: timeless → iann_bugzilla
Attachment #502009 - Flags: review?(neil)
Whiteboard: [patchlove]
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];
Attachment #502009 - Flags: review?(neil) → review+
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
Attachment #502009 - Attachment is obsolete: true
Attachment #506272 - Flags: superreview?(bienvenu)
Attachment #506272 - Flags: review+
Attachment #506272 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 506272 [details] [diff] [review]
Revised patch v2.1 [Checked in: Comment 13]

http://hg.mozilla.org/comm-central/rev/59dbf0a18c42
Attachment #506272 - Attachment description: Revised patch v2.1 → Revised patch v2.1 [Checked in: Comment 13]
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a3
You need to log in before you can comment on or make changes to this bug.