104989 - improve 2nd compose window time by hiding compose window on send, and re-showing it after clearing, subject, etc

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P1)

Description

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

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.

Comment 1

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

Attachment: proof-of-concept patch

Comment 2

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

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.

Comment 3

[David :Bienvenu]

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.

Comment 4

[Jean-Francois Ducarroz]

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!

Comment 5

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

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.

Comment 6

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

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.

Comment 7

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

Attachment: patch

Comment 8

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

Attachment: patch

Comment 9

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

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.

Comment 10

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

Attachment: patch

Comment 11

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

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.

Comment 12

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

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.

Comment 13

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

Attachment: new patch, works with WM close.

Comment 14

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

Attachment: more hacking and slashing. attachments clearing, uses cached window on reply (body works)

Comment 15

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

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.

Comment 16

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

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

Comment 17

[Jason Kersey]

this could be used in "other" compose windows...  cc jelwell

Comment 18

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

Attachment: updated patch, reply works (subject, and addresses) but quoting style is broken.

Comment 19

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

Attachment: updated patch, use the window mediator to remove / add the cached window from the tasks list

Comment 20

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

Attachment: updated patch, remove ReleaseMessageServiceFromURI(), other clean up.

Comment 21

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

Attachment: fix, clean up and make it so if closing a cached window without changes doesn't prompt for saving.

Comment 22

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

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.

Comment 23

[selmer (gone)]

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

Comment 24

[scottputterman]

work will continue on this now, but it probably won't land until 0.9.7

Comment 25

[Jean-Francois Ducarroz]

Attachment: New patch redisigned. Everything seems to work correctly!

Comment 26

[Jean-Francois Ducarroz]

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.

Comment 27

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

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

Comment 28

[Jean-Francois Ducarroz]

Attachment: updated patch which addresses Seth's comment

Comment 29

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

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 30

[varada]

Comment on attachment 56341 [details] [diff] [review]
updated patch which addresses Seth's comment

r=varada

Comment 31

[Jean-Francois Ducarroz]

Fixed and checked in but not turn on by default. Thanks Seth for spending a lot
of your time on this fix.

Comment 32

[[e-mail] (gone - use [e-mail] instead)]

Attachment: Comparison (reply to all, new window time) between pref on/off

Comment 33

[[e-mail] (gone - use [e-mail] instead)]

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!

Comment 34

[[e-mail] (gone - use [e-mail] instead)]

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 

Comment 35

[Jean-Francois Ducarroz]

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

Comment 36

[[e-mail] (gone - use [e-mail] instead)]

Attachment: This proves that the Reply to All metric for the 1st window was off.

Comment 37

[Cathleen]

hey guys!  is pref turned on by default yet? :-)

also, do we know what is the impact on footprint?

Comment 38

[[e-mail] (gone - use [e-mail] instead)]

Not on by default yet.  And we use about 700KB more just doing "New Msg" 10x
over the same procedures without the pref on.

Comment 39

[Arthur]

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

Comment 40

[Jean-Francois Ducarroz]

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.

Comment 41

[Eric Vaandering (no email)]

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.

Comment 42

[Kostub]

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.

Comment 43

[Jean-Francois Ducarroz]

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!

Comment 44

[Lewis Rosenthal]

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.

Comment 45

[Lewis Rosenthal]

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

Comment 46

[David :Bienvenu]

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.

Comment 47

[Lewis Rosenthal]

(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