101.16 KB, patch
|Details | Diff | Splinter Review|
5.36 KB, text/plain
3.98 KB, text/plain
improve 2nd compose window time by hiding compose window on send, and re-showing it after clearing, subject, etc putterman and I were talking about how to improve compose window time, and assuming that common usage is to have only one compose window open at a time, we think this might be a win. on send, instead of closing the compose window, we hide it (move it off screen?) then, on the next compose or reply, we clear out (or reset, for reply) the subject and addressing fields, the identity, the attachments, and the editor content. I'm going to go hack around and see if this buys us anything.
that hack patch just shows how we can hide the compose window on send, and show it again later the next time we go to open a compose window. todo list: 1) get sending to work, move code out of onclose and onloadhandlers if necessary 2) on reopen, clear or reset fields 3) handle multiple compose window case 4) make sure compose window doesn't show up in Tasks list when hidden.
Status: NEW → ASSIGNED
My main concern about this, besides sharing Scott's concern that things are completely cleared up after sending, is that closing down the last visible window really closes down the app (when it should, i.e., not on the mac, and not if quick launch is enabled). Also, probably, closing the last mail/news window should close the compose window too.
Also, on Linux, when we use to hide/show the compose window during send, the compose window was jumping around the screen! I thought brutal sharing should make a second instance of window way faster!
bienvenu: good point, I haven't thought about that yet. I'll add to my todo list to make sure closing the last window / quitting still behaves properly. ducarroz: > Also, on Linux, when we use to hide/show the compose window during send, the > compose window was jumping around the screen! That might be a window manager issue, as on linux we use the window manager to position the window. I'll make sure to investigate.
5) address last window / close issue 6) handle plain text / vs html (right now I'm thinking if you don't match the cached compose window format, you don't get the cached window) I've got a patch that's further along, I'll attach it. something that's making things difficult: if I an attribute gets set on a window that is hidden, it causes the window to become shown. I'll need to investigate.
Attachment #53760 - Attachment is obsolete: true
Attachment #53875 - Attachment is obsolete: true
1) reset title, clear attachments, options, fcc folder, etc. 2) issue, clearing fields and body before window goes away 3) handle multiple compose window case 4) make sure compose window doesn't show up in Tasks list when hidden. 5) exit, closing last window 6) html vs plain text issue 7) title 8) wm close 9) focus, dual cursors (addressee & editor) 10) delete all addresses 11) reply 12) identity switch 13) comment about the next new msg window after any send is fast.
Attachment #53904 - Attachment is obsolete: true
that last patch takes care of the addressing widget (removes all addresses, leaves just one, sets it blank and reset to "To:") and resets the title (hard coded for now), and clear out the Fcc2 field. (we allow the user to set an addition folder for fcc on send) some new things I need to reset: 1) char coding 2) send format 3) priority 4) anything in the editor toolbar (font style, text style, etc) I'm now going to go look into the hard part: reply / fwd / edit as new / draft / template on a cached window.
hyatt's interested in this hack for browser, see #105333 the last patch clears the compose window before hiding it, because if I do it after hiding, it will re-appear. hyatt suggests that is might be doing that because on focus change, we'll show the window. I'll investigate.
Attachment #53970 - Attachment is obsolete: true
I've got some of the basics working in the prototype, now would be a good time to stop and talk things over with ducarroz. to do this right, a lot of code is going to have to broken up and re-organized so that it can be run on with a compose window, or on a cached compose window. the code that needs breaking apart is the code need to do anything other than a "new" compose window. (reply, edit as new, forward, etc.) I think a good plan would be to have ducarroz and me get together, talk about this hack, and decide how to proceed. We'll definitely want a branch, and ideally, anyone doing a lot of hacking on mozilla/mailnews/compose would be on that branch to avoid the huge merge.
we'll want to disable undo, and batch the inserting of the reply text to get even better performance out of reply into a cached window. it will be interesting to work on performance of reply on a cached window after this fix lands, as the problem now becomes "how fast can we quote and insert the message into editor, create and set the subject, and populate a created addressing widget".
this could be used in "other" compose windows... cc jelwell
QA Contact: sheelar → stephend
Priority: -- → P1
Target Milestone: --- → mozilla0.9.6
Attachment #53990 - Attachment is obsolete: true
Attachment #54016 - Attachment is obsolete: true
Attachment #54724 - Attachment is obsolete: true
Attachment #54826 - Attachment is obsolete: true
Attachment #54928 - Attachment is obsolete: true
handing this off to ducarroz, the owner of the compose window. in addition to the performance work, this patch contains changes for: 1) removed ReleaseMessageServiceFromURI() 2) switched from kWindowMediatorCID to NS_WINDOWMEDIATOR_CONTRACTID 3) code cleanup todo list: 1) rename stuff nsIMsgComposeCloseListener interface to something better, nsIMsgCacheComposeWindowListener, something. rename gComposeCloseListener 2) code to clean up compose window on hiding is in nsMsgCompose::CloseWindow() and onClose() of gComposeCloseListener. I think it should be moved into the js. 3) on reusing cached window, need to reset the identity picker char coding send format priority anything in the editor toolbar (font style, text style, etc) 4) reply, somewhere I broke it so replying into a cached window no longer sets the subject. 5) the code that sets the title back to "Compose: (no subject)" is hard coded, probably in the wrong place 6) focus: on reusing the cached window, you'll get a cursor in the address widget and the body, should just have one in the address widget 7) focus: on reply, make sure the cursor is in the right place 8) make sure that on exit, if we've got a cached window, exiting works. 9) fwd, edit draft, edit as new, aren't hooked up yet. (only reply and new) 10) right now, we clear the compose window, then hide it. that needs to change to hide first, then clear. (on win32, a focus change will show a hidden window, so before you hide and clear, you'll need to set the focus to the subject area, or the window, so that the clearing doesn't cause a focus change) HEADS UP: due to "focus shows a hidden window on win32", if you assert while hiding the window, the cached window might come up "dead". to get out of this state, I usually do "new msg" again, and then close the window. I'll log a bug on the (existing) window mediator assert. we'll have to fix that before we land, otherwise developers will get a "dead" cached compose window when using this feature on debug builds. 11) look for and fix // switch to comptr to remove this 12) look for and fix // XXX what is this? 13) look for and fix // XXX comptr 14) look for and fix // why are we using xpcom here? 15) look for and address // XXX when do we break this ref to the listener? 16) remove dump statements, like dump("XXX changed? " + contentChanged + "," + msgCompose.bodyModified + "\n"); 17) look for and address // XXX clear the body (yuck, speed this up? is there a better way to do this?) 18) I'm using the window mediator to remove and ad backk the cached window. when the window gets added back, "" appears in the task menu. until the window title changes. It should have the proper title of the window (for reply and new window) 19) add a pref to enable / disable this feature, disabled by default. that way, you can start checking in and allow people to use it (and get some timing numbers) before it is 100% complete and polished. I'd want to use it as soon as the "reply subject" problem was fixed, it's really useful. let me know if you have questions. if I think of any more todo items, I'll let you know.
Assignee: sspitzer → ducarroz
Status: ASSIGNED → NEW
Not sure where you're at with this, but it seems like removing the old content as soon as possible would be good for footprint. This hidden window might sit around for the rest of the session not being used. In the close last window case, we should consider erring on the side of keeping the hidden window around longer. If the mail window gets closed and then opened again later and that is followed by a compose, it would be good to get the benefit. (Odds are that's going to be a lot of work for the benefit, but it should at least be considered.) This whole thing should get hooked into -turbo so we can create a hidden compose window whenever -turbo does its thing. This would allow the very first compose window to also benefit. (This might imply that the mail window gets cached by -turbo, another Good Thing (TM)) I'm really looking forward to seeing this get on the trunk!!!
work will continue on this now, but it probably won't land until 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Attachment #55008 - Attachment is obsolete: true
The new approche I took was instead of trying to cache the window and the nsIMsgCompose object associate with it, I am caching only the window (and the editor object). Sure we may have lost a little bit of the gain but the code is more simple, more generic and especially less risky for regression. Also, Instead of caching/recycling only one window, you can specify by pref how many you want to be able to cache. The hidden pref to activate this feature is "mail.compose.max_recycled_windows" which is an interger that indicate the maximum number of compose windows than can be cached simultaneously. If you set it to 0, the recycling feature is turn off. Example: user_pref("mail.compose.max_recycled_windows", 3); Will let you have up to 3 different windows cached at the same time.
Status: NEW → ASSIGNED
comments: 1) for interfaces, the convention is var nsIFoo = Components.interfaces.nsIFoo; it makes it clear it's an interface. please change from sFoo to nsIFoo. 2) ducarroz explained why he is doing sFoo and gBar. "I wanted a way to distinguish globals that are initialized once when js load and those we need to initialize every time we open a compose window" can you add that comment to the code before checking in? it make a lot of sense. 3) remove #define DEBUG_ducarroz 1 4) you should default that new pref in mailnews.js to 0
Attachment #56268 - Attachment is obsolete: true
sr=sspitzer. before you check in: 1) add a comment before ClearIdentityListPopup() explaing why it is needed 2) forcing a GC scares me (I don't know if it is safe), can you run it by someone in the js group to comment? "carbage collector for our window" should be "garbage collector for our window" I can't wait to try this out!
Comment on attachment 56341 [details] [diff] [review] updated patch which addresses Seth's comment r=varada
Attachment #56341 - Flags: review+
Fixed and checked in but not turn on by default. Thanks Seth for spending a lot of your time on this fix.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Summary: Action Pref Off Pref On % change --------------------------------------------------------------------------- Reply to All 5.200 4.250 18% ' 2.960 1.180 60% ' 2.956 1.166 61% --------------------------------------------------------------------------- New Window 3.279 3.308 <-1% ' 2.100 0.289 86% ' 2.095 0.278 87% Verified FIXED. Nice job, guys!
Status: RESOLVED → VERIFIED
MS-Windows: 98 Second Edition, 266 MHz Pentium II, 128M RAM, commercial builds The actual messages and folders I use to test are available via the links in the Methodology section on http://www.mozilla.org/mailnews/win_performance_results.html
>Reply to All 5.200 4.250 18% don't trust this number, it should be the same! looks like more a network interference (as the test are made with IMAP)
hey guys! is pref turned on by default yet? :-) also, do we know what is the impact on footprint?
Not on by default yet. And we use about 700KB more just doing "New Msg" 10x over the same procedures without the pref on.
Is the pref now on per default? Just asking because it's marked fixed and human beings have quite volatile memory, making them forget things like turning on a pref ;) (It's not really a Win2000 only thing, is it?)
No, the pref is not turned on by default. But we haven't forget! We need first to solve bug 109081. This bug has been marked has fixed to let the QA test it.
In bug 130581 several people are reporting problems with the compose window when used with KDE under Unix. I've gotten rid of these problems by turning off compose window recycling.
in bug 193158 problems are reported with this feature on Windows XP. Turning of recycling gets rid of these problems. Maybe recycling should be off by default.
The recycling compose window feature allows users to open a compose window quickly. This feature works well except for couple users using a different window manager than the one provided by the OS, mostly because the manager doesn't know how to deal correctly with hidden window!
FWIW, I enabled this for 1 window in prefs.js, using: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9a1) Gecko/20051021 SeaMonkey/1.1a Mnenhy/0.7.2.0 All was going well, until for about the third time, I clicked in the filter box of the view dialog to filter my Inbox for a particular sender. SeaMonkey exited, leaving the following in POPUPLOG.OS2: 10-26-2005 17:11:38 SYS3175 PID 0096 TID 0001 Slot 00d6 J:\MOZILLA.ORG\SEAMONKEY\SEAMONKEY.EXE c0000005 1535eb54 P1=00000001 P2=00000034 P3=XXXXXXXX P4=XXXXXXXX EAX=014879a0 EBX=00000034 ECX=00000000 EDX=01487918 ESI=00000034 EDI=00000002 DS=0053 DSACC=f0f3 DSLIM=ffffffff ES=0053 ESACC=f0f3 ESLIM=ffffffff FS=150b FSACC=00f3 FSLIM=00000030 GS=0000 GSACC=**** GSLIM=******** CS:EIP=005b:1535eb54 CSACC=f0df CSLIM=ffffffff SS:ESP=0053:0011fc60 SSACC=f0f3 SSLIM=ffffffff EBP=0011fc68 FLG=00012216 MAILNEWS.DLL 0001:0006eb54 Not believing that this crash could be related to mail.compose.max_recycled_windows, I restarted Moz, and went about my business. The same condition occurred an hour or so later, so I rolled mail.compose.max_recycled_windows back to 0. So far, the symptom has not recurred. Lewis PS - IMO, this bug should be changed to include all OSes and not just Windows.
Further to my comment #44: I should have read through this bug more thoroughly, and either opened a new report or followed up on an existing one concerning the issues raised with this pref turned on. Mea culpa. Apologies for the bug spam. Lewis
that pref is already on by default for windows, and has been for several years. So I doubt it has anything to do with your crash. It might have to do with extensions you have installed, or it might just be a separate bug.
(In reply to comment #46) > that pref is already on by default for windows, and has been for several years. > So I doubt it has anything to do with your crash. It might have to do with > extensions you have installed, or it might just be a separate bug. > That pref is not on by default in OS/2, David, and that's where I saw the crash. That said, I opened bug 314096. Again, apologies to everyone for the bug spam. Lewis
You need to log in before you can comment on or make changes to this bug.