Closed Bug 777732 Opened 12 years ago Closed 9 years ago

Remove vestiges of compose window recycling

Categories

(MailNews Core :: Composition, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 48.0

People

(Reporter: jcranmer, Assigned: jorgk-bmo)

References

Details

(Keywords: dev-doc-needed)

Attachments

(7 files, 4 obsolete files)

(deleted), patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
(deleted), patch
rkent
: review+
philip.chee
: review+
Details | Diff | Splinter Review
(deleted), patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
(deleted), patch
philip.chee
: review+
Details | Diff | Splinter Review
(deleted), patch
mkmelin
: review+
philip.chee
: review+
Details | Diff | Splinter Review
(deleted), application/x-xpinstall
Details
(deleted), patch
aceman
: review+
Details | Diff | Splinter Review
The title says it all.
Compose window recycling is partly removed in bug 777063
Depends on: 777063
I am all in favour of removing the recycling stuff. It's a BIG headache in composition. It's also obsolete IMHO, since the software doesn't run on a Pentium II 200 MHz with 128 MB RAM any more. Also see bug 779074.
I agree ... as long as we have good testing - since the default is 1 = enabled ;) see also bug 814651, bug 547270, bug 833909
OK, I can remove it, although it's no vestige, it is real and executed on every message written with the default settings. I've removed heaps of code in my life, so I'm good at that ;-) The problem is: Do we have agreement? Joshua seems to be in favour. But how about our more conservative developers. Magnus, Kent: Can I have the "go ahead"?
Flags: needinfo?(rkent)
Flags: needinfo?(mkmelin+mozilla)
Yes composer window recycling is a pain, and it would be good to remove it. But that is about all I can say. I don't know the original reasons why composer window recyling was added, nor what problem it was supposed to solve. I've sort of assumed it was a performance thing, and 10 years later perhaps it is no longer needed. Does anyone know what problem composer recycling was supposed to solve?
Flags: needinfo?(rkent)
Performance, see bug 779074.
(In reply to Kent James (:rkent) from comment #5) > Yes composer window recycling is a pain, and it would be good to remove it. > But that is about all I can say. I don't know the original reasons why > composer window recyling was added, nor what problem it was supposed to > solve. I've sort of assumed it was a performance thing, and 10 years later > perhaps it is no longer needed. > > Does anyone know what problem composer recycling was supposed to solve? The original reason was performance. I at one point timed that compose window caching actually took more time than starting it up from scratch. A secondary reason, one that caused bienvenu to oppose this path, was that caching the compose window helped minimize the issues of compose leaks, of which we've certainly had some trouble.
Yes, certainly moving forward there's little room for this. IIRC recycling was broken all through the tb24(?) cycle and nobody really noticed.
Flags: needinfo?(mkmelin+mozilla)
It's still broken. See Bug 1046207 ("Compose Message window ghost exposes contents of previous composition for a moment when starting a new message") but can't quite be preferenced out due to Bug 814651 (breaking Sendfilters).
Assignee: nobody → mozilla
On a debug trunk build, there is still a perf difference between using a recycled window and a new one (tested by opening compose in HTML mode and then in plain text mode, I think that needs to build a new compose window without recycling). But on TB38 release (optimized), I see no difference optically. Both opens are instant (on my beefy 3.6Ghz machine). When I underclock it to 800Mhz. The different opens (with different compose modes) are not instant, they take a bit longer, and they are not the same speed, but still very close. And even the slower open (without recycling) is acceptable to me. So I would also support removing the code complexity when it brings no gain anymore.
Thanks for doing some timings here. This is next on my list right after Magnus approves my Eudora kill. (Aceman, this has been sitting there for over a month, perhaps you want to steal the review in bug 1243498.)
It was noted in bug #1046207 that a proposed fix for this bug might adversely impact the "Send Filter" capability of Thunderbird. The reply to that comment cited the Send Filter extension. The Send Filter extension for Thunderbird was rendered obsolete by the implementation of its functionality into "vanilla" Thunderbird. I was using that extension but removed it about a year ago. See bug #11039. Thus, it is important that correcting this bug not adversely impact the inherent "send filter" capability.
Attached patch Remove recycling (v1). (obsolete) (deleted) — Splinter Review
Let's see what breaks. I gave it a quick test and everything still seemed to work. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c7200a7764a5
Comment on attachment 8735298 [details] [diff] [review] Remove recycling (v1). OK, Magnus said he's available, so r? Of course the try isn't finished yet. This is all straight forward deletion. The diff looks confusing where if/else parts got removed and what was not removed had the indentation level changed. I've done the SM part as well, frankly, no need to ask them for review. If mailnews goes away, they have no chance ;-)
Attachment #8735298 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8735298 [details] [diff] [review] Remove recycling (v1). Review of attachment 8735298 [details] [diff] [review]: ----------------------------------------------------------------- Looks ok to me. r=mkmelin if the try run is fine
Attachment #8735298 - Flags: review?(mkmelin+mozilla) → review+
Almost worked, one failure: test-charset-upgrade.js::test_encoding_upgrade_plaintext_compose
Are all the relevant cleanups from gComposeRecyclingListener.onClose done elsewhere on window close?
This is about the weirdest thing I've seen in a while. Hard to believe, but this fails on: compWin.keypress(null, "VK_RETURN", {shiftKey: true, accelKey: true}); with SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\mail\test\mozmill\composition\test-charset-upgrade.js | test-ch arset-upgrade.js::test_encoding_upgrade_plaintext_compose EXCEPTION: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendKeyEvent] at: nonesuch line 342 This failure in fact has nothing to do with my patch. It can be made to fail without the patch by adding Services.prefs.setIntPref("mail.compose.max_recycled_windows", 0); to the test. So the test_encoding_upgrade_plaintext_compose test only ever worked in recycling mode. It's ever weirder, I stripped it down to this failing: function test_encoding_upgrade_plaintext_compose() { Services.prefs.setBoolPref("mail.identity.default.compose_html", false); let compWin = open_compose_new_mail(); Services.prefs.setBoolPref("mail.identity.default.compose_html", true); compWin.type(null, "someone-else@example.com"); compWin.type(compWin.eid("msgSubject"), "encoding upgrade test - plaintext"); // Ctrl+Shift+Return = Send Later. compWin.keypress(null, "VK_RETURN", {shiftKey: true, accelKey: true}); } This already fails!!! There are actually very few spots where we simulate <ctrl><shift><enter> to send later. The only other occurrence uses: + // Send it later. + plan_for_window_close(compWin); // Ctrl+Shift+Return = Send Later compWin.keypress(null, "VK_RETURN", {shiftKey: true, accelKey: true}); + wait_for_window_close(compWin); but that doesn't help here.
Attached patch Showing the test failure. (obsolete) (deleted) — Splinter Review
Apply this patch WITHOUT applying the other one. Then run: mozmake SOLO_TEST=composition/test-charset-upgrade.js mozmill-one
The patch strips down test-charset-upgrade.js to the bit that fails. If you additionally comment out Services.prefs.setBoolPref("mail.identity.default.compose_html", false); you'll get a different error "There was an error saving the message to Outbox. Retry?" Totally weird.
Capturing some of the conversation with Aceman on IRC: He was asking whether "send later" relies on window recycling, that is, the hidden composition window might be accessed to create the message to store in the outbox. Of course if the composition window gets destroyed too early, the "send later" will fail. I'll investigate along those lines. In nsMsgCompose::SendMsg we see: i'm assuming the compose window is still up at this point
Attached patch Some more tweaks. (obsolete) (deleted) — Splinter Review
OK, I tried sending a 10 MB to the outbox. There is no problem. The composition windows stays open until the message has made it's way to the outbox. In the end, this turned out to be a timing problem. Placing sending the <ctrl><shift><enter> into a setTimeout with zero timeout solved the problem.
Attachment #8735418 - Attachment is obsolete: true
Let's see whether this works, it's fine on local Windows: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a5b2331e69a4
Here is version v1 together with the few tweaks from the "more tweaks" patch (comments, fixed test failure).
Attachment #8735298 - Attachment is obsolete: true
Attachment #8735453 - Attachment is obsolete: true
Attachment #8735506 - Flags: review+
Part 1 landed: https://hg.mozilla.org/comm-central/rev/158bced7b129 Let the regressions roll in ;-) SM people, can you please build and see whether you can still send e-mail. Part 2 will be about removing all the listeners to compose-window-close and compose-window-reopen since these events aren't sent any more. Patch coming up.
Status: NEW → ASSIGNED
Keywords: leave-open
Why would compose-window-close go away? That event still does happen. I can understand compose-window-reopen.
(In reply to :aceman from comment #30) > Why would compose-window-close go away? That event still does happen. I can > understand compose-window-reopen. Nope, this event does not happen any more: https://hg.mozilla.org/comm-central/rev/158bced7b129#l1.79 This was the *only* place to dispatch the event: https://dxr.mozilla.org/comm-central/search?q=compose-window-close&redirect=true&case=false
I mean why it is OK to remove it? We still do close the window (whether there is any recycling or not). Surely some extensions listen to the event?
When running with mail.compose.max_recycled_windows = 0, the event never fired. When running with mail.compose.max_recycled_windows = 1, it never fired on the second concurrent window, not even at application shutdown time. Now we *destroy* windows and that will destroy everything that was attached to them. Destroying will run ComposeUnload() in MsgComposeCommands.js.
Yes, and before destroying the window extensions want to run their stuff. So is there any event dispatched in ComposeUnload they could listen to? Are you sure when the window was being destroyed that gComposeRecyclingListener.onClose() wasn't run?
There has been some discussion on IRC whether we should now call compose-window-close in ComposeUnload() since about 36 add-ons (I've checked on https://mxr.mozilla.org/addons) were listening to that call. I think this makes no sense. The default was always mail.compose.max_recycled_windows = 1, that is, a second or third concurrent composition just got destroyed with no such event being fired. All the add-ons relying on this event were in fact wrong and didn't handle concurrent composition. They also didn't handle mail.compose.max_recycled_windows = 0. If someone is interested the compose window being closed/destroyed, we could invent a new event compose-window-destroy and fire it.
Attachment #8735550 - Flags: review?(mkmelin+mozilla)
Attachment #8735506 - Attachment description: Part 1: Remove recycling (v1a = v1 + more tweaks) → Part 1: Remove recycling (v1a = v1 + more tweaks) [landed, comment #29]
(In reply to :aceman from comment #34) > Are you sure when the window was being destroyed that > gComposeRecyclingListener.onClose() wasn't run? Yes. I am sure. And BTW, you know it, too. Remember bug 1246517 where we had a real bad time finding a solution to always remove the spellcheck-dictionary-remove listener once only. There were various cases: 1) Window got recycled, that removed the listener. 2) Window got destroyed without being recycled, that also removed the listener. 3) Window got recycled first, and then destroyed. That double-removed the listener until we fixed that. In the past, windows *did* get destroyed without the gComposeRecyclingListener.onClose() ever being run on them.
So can we move that event to ComposeUnload? Can you look into the addons and try to guess what they actually intended? If they wanted to listen to the window being closed or removed, making the event actually work would be better than making all the addons adapt to a new event name, that does the same.
Please read comment #35: It makes no sense. I'd like to hear Magnus's opinion here. Also, I'm not quite sure how these listeners work. Sure, you can run a listener on a window that is being hidden. There add-ons can reach into the window's document and do their thing. If the windows is being destroyed, it would be absolutely fatal to call the same event with changed semantics. Whatever they did before might not work on a destroyed window. And no, I won't look at 36 add-ons and guess. This is being landed on TB 48, so they have four cycles to fix their add-ons.
Or let's ask Neil: Neil, as part of the removal of compose window recycling, gComposeRecyclingListener.onClose() and gComposeRecyclingListener.onReopen() were removed together with a whole lot of other processing: https://hg.mozilla.org/comm-central/rev/158bced7b129#l1.13 Consequently the events compose-window-close/reopen are no longer fired. All windows are destroyed via ComposeUnload(). Question: Should we implement some event in ComposeUnload() or even call compose-window-close from there? And should some of the processing that was removed from gComposeRecyclingListener.onClose() be restored back to ComposeUnload(). So far I have assumed that the answer is "no", since with mail.compose.max_recycled_windows = 0 windows were simply destroyed without any of the gComposeRecyclingListener.onClose() being executed. Equally with mail.compose.max_recycled_windows = n (where n > 0), the concurrent composition number (n+k) (k > 0) was just destroyed.
Flags: needinfo?(neil)
Comment on attachment 8735550 [details] [diff] [review] Part 2: Remove listeners to compose-window-close/reopen Review of attachment 8735550 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/compose/content/bigFileObserver.js @@ -47,5 @@ > - bucket.removeEventListener("attachments-removed", this, false); > - bucket.removeEventListener("attachments-uploading", this, false); > - bucket.removeEventListener("attachment-uploaded", this, false); > - bucket.removeEventListener("attachment-upload-failed", this, false); > - bucket.removeEventListener("attachments-converted", this, false); It it OK to NOT remove listeners that we added (above) even if the window is being destroyed? Won't that leak something?
(In reply to Jorg K (GMT+1) from comment #38) > Also, I'm not quite sure how these listeners work. Sure, you can run a > listener on a window that is being hidden. There add-ons can reach into the > window's document and do their thing. If the windows is being destroyed, it > would be absolutely fatal to call the same event with changed semantics. > Whatever they did before might not work on a destroyed window. No, they ran the code while the window was still existing (the event is dispatched before destroying it). So I do not see a difference between running the code before the window is just hidden (due to recycling) and when it is removed completely. A difference may be that some of that code was just doing reseting the addon widgets to initial state so that the the window can be reused on next unhiding. Similar code that you just remove here. Then that code is no longer needed. Anyway, we need to document this change in the proper 'TB changelog for developers' page so that addon authors can decide whether to remove that code, or start watching the new event you propose (and I support there being at least some event). > And no, I won't look at 36 add-ons and guess. I meant a random sample.
Keywords: dev-doc-needed
(In reply to :aceman from comment #40) > It it OK to NOT remove listeners that we added (above) even if the window is > being destroyed? Won't that leak something? The window/document destruction should take care of it. Note that other listeners are now no longer removed, for example: https://hg.mozilla.org/comm-central/rev/158bced7b129#l1.35 https://hg.mozilla.org/comm-central/rev/158bced7b129#l1.39 The ones that *must* be removed since they are *not* attached to the window/document are still removed here, for example: http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2422
Oops, this is inconsequential, but should be fixed.
Attachment #8735901 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8735901 [details] [diff] [review] Part 1a: Remove recycling, part 1a, missed bits [landed comment #45] Review of attachment 8735901 [details] [diff] [review]: ----------------------------------------------------------------- These changes are simple missed parts of the original, so rs+=me
Attachment #8735901 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 8735901 [details] [diff] [review] Part 1a: Remove recycling, part 1a, missed bits [landed comment #45] Part 1a landed, sorry about the missed bits: https://hg.mozilla.org/comm-central/rev/e013e00901f0
Attachment #8735901 - Attachment description: Part 1: Remove recycling, part 1a, missed bits. → Part 1a: Remove recycling, part 1a, missed bits [landed comment #45]
Comment on attachment 8735901 [details] [diff] [review] Part 1a: Remove recycling, part 1a, missed bits [landed comment #45] rs=me thanks!
Attachment #8735901 - Flags: review+
Comment on attachment 8735550 [details] [diff] [review] Part 2: Remove listeners to compose-window-close/reopen Review of attachment 8735550 [details] [diff] [review]: ----------------------------------------------------------------- I think this is ok. People can probably listen to a window "close" event for the same purpose too (didn't try!).
Attachment #8735550 - Flags: review?(mkmelin+mozilla) → review+
Thanks for the review. Since Aceman seems to be opposed to tossing it all, I've written to Patrick Brunschwig, the maintainer of Enigmail. Enigmail listens to the discontinued event. They also listen to the "unload" event (which is what you meant, I assume). Let's land this and be done here. Note the suite part here in the patch.
In TB 38.5 without this patch applied, I just had an experience of: 1. Receive a forwarded message which had a space for a banner image showing just the "file link broken" icon and image box, which also had an attached png image with the banner that presumably was meant to go in that slot. 2. Click to forward the message. A Compose window opens, showing that the png file will be attached. 3. Edit the message text and send it. It sends successfully and the window closes. 4. Receive another message (on a different account, if that matters) with an image attached, and the text refers to the image as being "attached" with no apparent spot for it in the body of the message. 5. Click to forward this message as well (to a different recipient). 6. Edit the text in that message and prepare to send. 7. Notice that in the Attachments section, *BOTH IMAGES* are listed as being attached: the one that should be attached, AND the one from the previous message which should NOT be attached or sent to the recipient of the second forward. The expected behavior is that only the image attached to the message of Step 4 is attached to the forward. This seems to be not trivially reproducible, so I'm wondering if it would be worth investing the time into writing this up as a separate bug or if that'd be closed right away with a reference to the patch in this one.
(In reply to Jorg K (GMT+1) from comment #48) > Thanks for the review. > > Since Aceman seems to be opposed to tossing it all, I've written to Patrick > Brunschwig, the maintainer of Enigmail. Enigmail listens to the discontinued > event. They also listen to the "unload" event (which is what you meant, I > assume). > > Let's land this and be done here. Note the suite part here in the patch. I'm glad to see that this gets landed. Enigmail does not do anything (anymore) with the unload event. If windows are no longer re-opened, then there is no need for these events for Enigmail anymore.
Can we now remove code that was only called from the removed .onClose() and was only clearing out the compose widgets for next composition? E.g. awResetAllRows(), ClearIdentityListPopup(), maybe EditorResetFontAndColorAttributes() (references recycling). ReleaseGlobalVariables() is still used in TB, but not in SM. Why?
Flags: needinfo?(mkmelin+mozilla)
(In reply to Jorg K (GMT+1) from comment #48) > They also listen to the "unload" event (which is what you meant, I > assume). Can you check if listening to the window "unload" runs extension code before the onunload=ComposeUnload we have in base TB ? If yes, that could be enough.
Yes there are probably a whole lot of other simplifications and removals that can be done.
Flags: needinfo?(mkmelin+mozilla)
(In reply to WBT from comment #49) > This seems to be not trivially reproducible, so I'm wondering if it would be > worth investing the time into writing this up as a separate bug or if that'd > be closed right away with a reference to the patch in this one. We won't investigate anything that will be fix by this bug here. You can use a Daily (I'm using the one compiled 2016-03-29) and see whether it still shows problems. (In reply to :aceman from comment #51) > Can we now remove code that was only called from the removed .onClose() and > was only clearing out the compose widgets for next composition? > E.g. awResetAllRows(), ClearIdentityListPopup(), maybe > EditorResetFontAndColorAttributes() (references recycling). Will do. Thanks for checking. editor/ was the only directory I didn't check for "recycl*", sorry. > ReleaseGlobalVariables() is still used in TB, but not in SM. Why? You'd have to ask the SM people. (In reply to :aceman from comment #52) > Can you check if listening to the window "unload" runs extension code before > the onunload=ComposeUnload we have in base TB ? Not easy to check, I'd have to register that in an extension and see what happens. If you insist, I'll do so. However, why would that make a difference? The windows/document goes away and everyone shuts down their stuff. Is the order important?
Splitting part 2.
Attachment #8735550 - Attachment is obsolete: true
Attachment #8736192 - Flags: review+
OK, here is the SM part split off so the SM people can give their OK. The problem is that onAbClearSearch() is no longer used, so I removed it. TB still uses it in DirPaneSelectionChange(), so the SM folks might want to preserve it for future use.
Attachment #8736196 - Flags: review?(philip.chee)
Comment on attachment 8736192 [details] [diff] [review] Part 2: Remove listeners to compose-window-close/reopen - TB only [landed, comment #57] Part 2 (TB only) landed: https://hg.mozilla.org/comm-central/rev/452569be7f75 Part 3 coming up for some further clean-up, see comment #51.
Attachment #8736192 - Attachment description: Part 2: Remove listeners to compose-window-close/reopen - TB only → Part 2: Remove listeners to compose-window-close/reopen - TB only [landed, comment #57]
(In reply to Jorg K (GMT+1) from comment #54) > (In reply to :aceman from comment #52) > > Can you check if listening to the window "unload" runs extension code before > > the onunload=ComposeUnload we have in base TB ? > Not easy to check, I'd have to register that in an extension and see what > happens. > If you insist, I'll do so. However, why would that make a difference? The > windows/document goes away and everyone shuts down their stuff. Is the order > important? Yes, it makes a difference. IF ComposeUnload runs first, and runs ReleaseGlobalVariables(), running any code in addons after that may be nonsense. The addons may need the variables initialized and whole window still working. So if our ComposeUnload runs first, I would still think we need to provide a new event (with new name) as the first thing in ComposeUnload. If addons run first, we do not need the event.
OK, as mentioned in comment #51, awResetAllRows(), ClearIdentityListPopup() and EditorResetFontAndColorAttributes() can also go.
Attachment #8736362 - Flags: review?(philip.chee)
Attachment #8736362 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8736196 [details] [diff] [review] Part 2: Remove listeners to compose-window-close/reopen - SM [landed modified, comment #71] (In reply to Jorg K (GMT+1) from comment #56) > OK, here is the SM part split off so the SM people can give their OK. > > The problem is that onAbClearSearch() is no longer used, so I removed it. TB > still uses it in DirPaneSelectionChange(), so the SM folks might want to > preserve it for future use. I think we should retain onAbClearSearch() to make it easier to port Thunderbird patches to SeaMonkey MailNews. CC IanN: what do you think?
Attachment #8736196 - Flags: review?(philip.chee)
Attachment #8736196 - Flags: review?(iann_bugzilla)
Attachment #8736196 - Flags: review+
(In reply to :aceman from comment #58) > So if our ComposeUnload runs first, I would still think we need to provide a > new event (with new name) as the first thing in ComposeUnload. If addons run > first, we do not need the event. The result/debug with this add-on is shown below: Add-on> Preparing compose window and later ... TB core: ComposeUnload started TB core: ComposeUnload exiting Add-on> Unload arrived So the add-on receives the unload event after we're done in ComposeUnload(). I think that is expected. ComposeUnload() is triggered from here: https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#34 The unload event for the window gets triggered when the window is destroyed by Gecko. May I enquire one more time: In the past, with mail.compose.max_recycled_windows set to 1, many windows got destroyed without any notification, that is, all the second and third, etc. compositions. They *never* received any notification and to my knowledge, that has never caused a problem. So now all composition windows get destroyed without further notification, so why would that suddenly be a problem when it was part of the normal accepted behaviour of the system? The notifications compose-window-close/reopen were dispatched so listeners could "clean" the window for further reuse. I have an add-on myself (dictionary for recipient) which listens to these events since it stores stuff in the window and needs to do a clean-up when the window gets recycled. Of course my add-on works on windows which get destroyed immediately. Look, it's a five minute job to code: document.getElementById("msgcomposeWindow").dispatchEvent( new Event("compose-window-about-to-be-destroyed", { bubbles: false , cancelable: false })); I just don't see the necessity. Even the very sophisticated add-on Enigmail has no need for such an event. And I repeat one more time: If some add-on does something on a second composition and doesn't clean up, then it has had the problem until today an will keep having the problem, only more frequently.
Attachment #8736362 - Flags: review?(philip.chee) → review+
(In reply to Jorg K (GMT+1) from comment #61) > So the add-on receives the unload event after we're done in ComposeUnload(). > > The notifications compose-window-close/reopen were dispatched so listeners > could "clean" the window for further reuse. I have an add-on myself > (dictionary for recipient) which listens to these events since it stores > stuff in the window and needs to do a clean-up when the window gets > recycled. Of course my add-on works on windows which get destroyed > immediately. Then why are addons (and even our code) listening for "compose-window-init" event? Why is window.onload not enough? I think exactly because the want to run only after the window and variables is also set up. Therefore "compose-window-init" is run late in ComposeLoad() and ComposeStartup(). I just want to provide a symmetric event when the window is going away. I don't say it is required now, just that somebody could use it. You said you didn't look at all those addons. Maybe some of them was observing "compose-window-close" just because there was nothing better available (even if unreliable). And window.onunload does not get the job done as the variables are teared down. Is that understandable? > Look, it's a five minute job to code: > document.getElementById("msgcomposeWindow").dispatchEvent( > new Event("compose-window-about-to-be-destroyed", { bubbles: false , > cancelable: false })); Yes, that is all that is needed (at the top of ComposeUnload). If you do not want to do it here, I can do it in a new bug.
(In reply to :aceman from comment #62) > Yes, that is all that is needed (at the top of ComposeUnload). If you do not > want to do it here, I can do it in a new bug. That would be better so we can refer people to this new bug with few comments instead of this bug which is quickly becoming very messy. Shall we call it "compose-window-unload"?
Comment on attachment 8736487 [details] [diff] [review] Part 2: Correction, bringing back the erroneously removed onComposerReOpen as onComposerLoad. [landed comment #67] Review of attachment 8736487 [details] [diff] [review]: ----------------------------------------------------------------- Yes, without this the View -> "Message security info" does not work as the SMIME variables are not initialized.
Attachment #8736487 - Flags: review?(acelists) → review+
Comment on attachment 8736487 [details] [diff] [review] Part 2: Correction, bringing back the erroneously removed onComposerReOpen as onComposerLoad. [landed comment #67] This is just putting back code that was mistakenly removed so please your rs here ;)
Attachment #8736487 - Flags: review?(philip.chee)
Comment on attachment 8736487 [details] [diff] [review] Part 2: Correction, bringing back the erroneously removed onComposerReOpen as onComposerLoad. [landed comment #67] Correction of Part 2 (TB only) landed: https://hg.mozilla.org/comm-central/rev/e2b4f6083662
Attachment #8736487 - Attachment description: Part 2: Correction, bringing back the erroneously removed onComposerReOpen as onComposerLoad. → Part 2: Correction, bringing back the erroneously removed onComposerReOpen as onComposerLoad. [landed comment #67]
Attachment #8736487 - Flags: review?(philip.chee)
> Correction of Part 2 (TB only) landed: > https://hg.mozilla.org/comm-central/rev/e2b4f6083662 What about SeaMonkey???
Flags: needinfo?(mozilla)
Sorry if this is confusing. Part 1 and Part 1a, TB+SM: https://hg.mozilla.org/comm-central/rev/158bced7b129 https://hg.mozilla.org/comm-central/rev/e013e00901f0 Part 2, TB only, and it's correction: https://hg.mozilla.org/comm-central/rev/452569be7f75 https://hg.mozilla.org/comm-central/rev/e2b4f6083662 Part 2, SM is awaiting Ian's review. Part 3, TB+SM is awaiting Magnus' review. Still to review/land: Part 2, SM and Part 3 TB+SM.
Flags: needinfo?(neil)
Flags: needinfo?(mozilla)
Attachment #8736362 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 8736196 [details] [diff] [review] Part 2: Remove listeners to compose-window-close/reopen - SM [landed modified, comment #71] I've landed this but I've maintained onAbClearSearch() as requested by Ratty for possible future ports.
Attachment #8736196 - Flags: review?(iann_bugzilla)
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 48.0
Attachment #8736196 - Attachment description: Part 2: Remove listeners to compose-window-close/reopen - SM → Part 2: Remove listeners to compose-window-close/reopen - SM [landed modified, comment #71]
Attachment #8736362 - Attachment description: Part 3: Remove awResetAllRows(), ClearIdentityListPopup(), EditorResetFontAndColorAttributes() - TB+SM → Part 3: Remove awResetAllRows(), ClearIdentityListPopup(), EditorResetFontAndColorAttributes() - TB+SM [landed, comment #71]
Thunderbird 38.7.1 was recently released. This bug report is scheduled for Thunderbird 48. When might that version be released?
Please refer to the release calendar: https://wiki.mozilla.org/RapidRelease/Calendar Thunderbird 48 will be available as beta version shortly after 2016-06-07. The next official ESR release is TB 52 due in early 2017. It's not on the calendar yet.
Blocks: 1262262
Depends on: 1267273
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: