Note: There are a few cases of duplicates in user autocompletion which are being worked on.

ComposeMessage logic is perf nightmare

RESOLVED FIXED in Thunderbird 3.3a3

Status

MailNews Core
Composition
--
minor
RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: timeless, Assigned: Ian Neal)

Tracking

({perf})

Trunk
Thunderbird 3.3a3

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

12 years ago
(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.
(Reporter)

Comment 1

12 years ago
Created attachment 182754 [details] [diff] [review]
reorganize the code
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 3

12 years ago
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?

Comment 6

11 years ago
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?
(Assignee)

Comment 10

7 years ago
Created attachment 502009 [details] [diff] [review]
Revised patch v0.2

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

Updated

7 years ago
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+
(Assignee)

Comment 12

7 years ago
Created attachment 506272 [details] [diff] [review]
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
Attachment #502009 - Attachment is obsolete: true
Attachment #506272 - Flags: superreview?(bienvenu)
Attachment #506272 - Flags: review+

Updated

7 years ago
Attachment #506272 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Comment 13

7 years ago
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]
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 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.