108761 - Need ability to control command updating for cmd_undo, cmd_redo and cmd_delete

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Scott MacGregor]

A while ago, we all made some nice performance improvements with regards to
command updating by re-organizing command updating and the events we try to
update commands for. This made a huge difference in various parts of the product
including mailnews compose and the mail 3-pane.

I'm noticing that there are 3 commands in the mail3-pane which aren't under our
control when it comes to updating:
cmd_undo
cmd_redo
cmd_delete

That's because these items come from utitlityOverlay.xul and that code doesn't
have the application defined smarts for when to perform updating or not. Focus
events trigger these things to get updated. 

Where is this an issue? Well I'm noticing during message display that we update
these 3 commands twice before we load the message and twice after we load the
message. Why? Because in loading a message we are creating a new content viewer
in the message pane docshell. Creation of a new content viewer is triggering
some  suppress focus code: SetSuppressFocus in docshell. We then simulate a
change in focus when the content is done loading. 

This is causing the command updating to happen.

I'm wondering if it's possible for me to somehow give my app instance control
over when these 3 commands are updated like the rest of the commands defined in
mailnews. Then we can decouple them from focus changes like the rest of the
mailnews commands. Obviously it's tricky because these commands are brought in
from utilityOverlay.xul and we want to continue bringing in utilityOverlay.xul. 

Is there a way to do this?

Comment 1

[Scott MacGregor]

re-wording the summary so it sounds like proper english.

Comment 2

[Scott MacGregor]

Here's the spot in utilityOverlay.xul where we set the events we want to update
these commands on. In some sense I want the ability to "override" the events for
these command sets. 


  <commandset id="globalEditMenuItems"
              commandupdater="true"
              events="focus"
              oncommandupdate="goUpdateGlobalEditMenuItems()"/>
  <commandset id="selectEditMenuItems"
              commandupdater="true"
              events="select"
              oncommandupdate="goUpdateSelectEditMenuItems()"/>

Comment 3

[Scott MacGregor]

Oh goody, my trial and error I discovered that I can redefine these command sets
in a separate overlay which gets brought in after utilityOverlay.xul. I can
specify one of our custom events in this overlay and instead of re-acting on
focus and on select, we only update these commands when we fire our custom events.

An alternative solution I was exploring was using removelement="true" and
manually removing the command sets from the mail 3-pane. This however requires a
fix for Bug #109200. And after Chris and I figured that out, I realized I didn't
really care about removing the command sets anyway. Just having the ability to
modify the event that triggers the command was plenty for me. 

I verified that the code in nsXULCommandDispatcher properly replaces the
existing event with my event when it merges the 2 together.

Patch coming.

Comment 4

[Scott MacGregor]

Attachment: over ride the default events for command updating for some command sets

Comment 5

[Scott MacGregor]

This patch now stops us from updating cmd_redo, cmd_undo and cmd_delete when
displaying a message (we were doing each one twice before). Those were the last
3 commands still getting updated every time we display a message. 

We'll also want to adopt this fix for the mail compose window as well. I'll file
a spin off bug. I'm going to move this bug back into the mail component since it
didn't require an XP-Apps change. 

Please note: utilityOverlay.xul still has a lot of stuff in it that most of the
consumers aren't really using. It will still be desireable to break it down into
smaller overlays. Joe Hewitt said that's on his plate of performance items to
look at. My little patch just fixes part of the problem for me =).

Comment 6

[Scott MacGregor]

Comment on attachment 57163 [details] [diff] [review]
over ride the default events for command updating for some command sets

I got an sr=sspitzer for this.

Comment 7

[Scott MacGregor]

fix checked in. And child bug spun off to make this same optimization in
messenger compose. 

Comment 8

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

This was one of those "only a developer could _truly_ verify this bug" deals, 
am I right? :-)

Comment 9

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

I tested the 27th build on my own time and found that we're 5.11 seconds to read
5 10KB messages in a local folder the 1st time, then 5.07 and 5.04 seconds each
consecutive time.  This brings us to 4.x parity! :-)

Verified FIXED.