72923 - Use new command structure and refactor commands/broadcasters/keys

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Blake Ross]

Per bug 71470, change all observes= to command= to take advantage of hyatt's 
new, more efficient command system.

Comment 1

[David Hyatt]

Note, don't convert *all* of them.  Only convert those instances where the
observes="" is clearly being used to observe a command.  For any menuitem or key
element, this is probably the case.

Comment 2

[Blake Ross]

Hyatt, I'm a little confused.  The example you give in 71470 converts elements 
that are observing a broadcaster.  Has the fate of broadcasters been decided?

Comment 3

[Blake Ross]

Or are you saying just to make sure that the elements are observing 
broadcasters or commands with oncommand handlers?

Comment 4

[Mike Pinkerton (not reading bugmail)]

i want to be in on this just in case anything goes wrong on macos.

Comment 5

[Mike Pinkerton (not reading bugmail)]

(implying i don't trust hyatt's underlying code) ;)

Comment 6

[David Hyatt]

Blake, right.

Comment 7

[Blake Ross]

Work here involves:

  - Using new command structure (broadcaster/observes -> command/command)
  - Removing all the extraneous attributes on broadcasters that we have,
    putting them where they belong (usually on menuitems)
  - Stop being lazy and pulling in large collections of unused nodes, just
    for the convenience of being able to say, e.g.
    <broadcasterset id="broadcasterset"/> (for example, the bookmarks panel
    currently brings in task menuitems, page/print setup menuitems,
    new editor/navigator menuitems, etc.)

All of these should have a positive impact on performance.  Just began to do 
step 1 in navigator, and speedup is already noticeable (early quantify runs are 
also promising).

Comment 8

[Blake Ross]

Er, the bookmarks panel brings in the broadcasters for all of those actions 
currently, not the menuitems themselves.

Comment 9

[Blake Ross]

DOM trees show that many windows are bringing in far too many extraneous nodes 
for the sake of convenience, so at this point I'm eliminating all of the 
<keyset id="keyset"/> / <broadcasterset id="broadcasterset"/> craziness and 
making clients explicitly ask for specific nodes.  This, in combination with 
the switch to the new command structure, the moving of attributes to where they 
really belong, and the arbitrary cleaning up of attributes (e.g. removing id's 
from many elements that don't require them) has produced some extremely 
promising results.

I'm about 80% done with Navigator, and here are some nice results I'm seeing 
for the opening of a second Navigator window (comparison between two tip 
builds, one containing only these Navigator changes):

Attribute setting (nsXULElement::SetAttribute) is down from 3,627 to 2,015 
(and, largely as a result, calls to nsXULDocument::GetScriptGlobalObject -- 
which are cheap --  are down from 4,296 to 2,725).

Calls to XULBroadcastListener::ObservingEverything/Attribute are down from 481 
to 8.

Calls to nsXULElement::NodeInfo are split in half, from 32,721 to 16,194.

Calls to StyleSetImpl::FindPrimaryFrameFor are down from 2,421 to 525.

Not that any of these are significantly expensive, but there are many such 
notable cuts in other areas.  The nice thing is that Navigator doesn't use 
broadcasters/commands extensively; I look forward to results from editor and 
mail, which do.

Comment 10

[Blake Ross]

Attachment: [patch] xpfe patches

Comment 11

[Blake Ross]

Attachment: [patch] final xpfe patches (and necessary mail/editor changes)?

Comment 12

[Blake Ross]

Attachment: [patch] closer....

Comment 13

[Blake Ross]

Attachment: [patch] probably final

Comment 14

[Blake Ross]

So here's the plan: uh, I'm never checking anything in again without a 
carpool ;)  I'd like to land this in a mini-carpool and get some nightlies out 
first for some people to test, since this covers such a wide range of things.  
I've heard mailnews wants to do their own, so this patch only really touches 
the bare minimum that had to be done for it (as a result of touching the 
overlays).

Looking for r/sr from ben (emailed) and hyatt.

cc'ing the world so no one can complain that they weren't aware of this 
change ;)

Comment 15

[David Hyatt]

sr=hyatt.  Land as a carpool, and send email directly to notify some people who 
might not read their bugmail (Simon, Prass, Mscott, etc.)

Comment 16

[Blake Ross]

cc'ing leaf, who's going to help get some test builds on ftp once ben reviews 
(cough)

Comment 17

[Simon Fraser [no longer active]]

Why the heck was I not cc'ed on this? Bad boys!

Comment 18

[Blake Ross]

r=ben, with the note that this patch contains other cleanup work which will 
also help performance.

cc'ing the rest of the world to keep everyone apprised; people who complain 
after this landing will be silenced :P

Comment 19

[Stephen P. Morse]

I notice that in xpfe/components/prefwindow/resources you modified the following 
files:

   pref-cookies.xul
   pref-cookies.dtd
   pref-images.xul
   pref-images.dtd
   pref-wallet.xul
   pref-wallet.dtd
   pref-passwords.xul
   pref-passwords.dtd

Unfortunately these files are obsolete and not part of the build.  They have 
been replaced by files of the same number in either extensions/cookie/resources 
or extensions/wallet/resources.  But these files are not included in your patch.  
They should be.  That is, the change you were making to the xpfe versions should 
be made to these extensions versions instead.

Comment 20

[Stephen P. Morse]

Oops, above I said:

   replaced by files of the same number

whereas I meant to say

   replaced by files of the same name

Comment 21

[Brendan Eich [:brendan]]

Steve: why haven't the obsolete files been cvs removed?

/be

Comment 22

[Kathleen :Brade]

What is the plan for landing this?  Is there a carpool date/time scheduled yet?

Comment 23

[Daniel (Leaf) Nunes]

Once we get scc's string changes put into test builds and landed, i can spin 
test bits with these changes for testing, and we can then schedule a landing (my 
guess is tuesday or wednesday, depending on scc's landing.

Comment 24

[prass]

Can we create test bits for commercial as well? That we way the aim changes can 
also land at the same time this gets landed in mozilla.

Comment 25

[prass]

Jason has volunteered to create commercial optimized builds. Imqa will run some 
tests on it with Blake's patch and my patch. Ping Jason if you want to try the 
test builds.

Comment 26

[Blake Ross]

Attachment: [patch] updated

Comment 27

[Daniel (Leaf) Nunes]

We have tests builds available at 
http://ftp.mozilla.org/pub/mozilla/nightly/latest-72923 for linux and windows

Comment 28

[J.J. Enser]

Mac test build is up!
http://ftp.mozilla.org/pub/mozilla/nightly/latest-72923
or
http://ftp.mozilla.org/pub/mozilla/nightly/2001-04-04-14-72923

Note that the patch to mozilla/mailnews/compose/prefs/resources/content/pref-
messages.xul had to be removed from the most recent patch attached because this 
file (pref-messages.xul) is no longer pertinent.

Comment 29

[Zach Lipton [:zach]]

blake--Leme take your head! You landed something in bugzilla! http://
bonsai.mozilla.org/cvsquery.cgi?dir=mozilla/webtools/bugzilla/docs/
sgml&date=explicit&mindate=04/05/2001&maxdate=04/06/2001

Comment 30

[Blake Ross]

navigator stuff landed, uncc'ing people (that I cc'ed earlier) until we figure 
out what's happening next.

Comment 31

[Blake Ross]

Sick and tired of hearing complaints from everyone (especially a certain 
reviewer, who began complaining about the patch after-the-fact because he was 
too lazy to actually look at any of it).  Closing this for the work done on 
Navigator, and MailNews and Editor can feel free to take advantage of this ~7% 
performance boost on their own if they want.

Comment 32

[sairuh (rarely reading bugmail)]

okay.