97134 - nsMsgDBView::GetCommandStatus does far to much work

Status: VERIFIED WONTFIX

(MailNews Core :: Backend, defect)

Description

[Mike Gratton]

nsMsgDBView::GetCommandStatus gets called at least three or four times per focus
change around the mail window.

It needs to know how many current selected messages there are in the thread
outliner, and it does this be calling GetSelectedIndices. This builds a
nsUInt32Array and the length of this is used to determine how many messages are
selected. A much easier way of doing it would be to just call GetNumSelected
which directly queries the thread outliner.

This should be a lot faster - making mail/news seem more responsive and has the
nice bonus of fixing bug 44572.

Patch to follow.

Comment 1

[Mike Gratton]

Attachment: patch using GetNumSelected()

Comment 2

[Jarrod Gray]

cc: [e-mail] for review

Comment 3

[Håkan Waara]

great catch! r=hwaara

Send [e-mail] email for super-review; sspitzer is on vacation.

Also, make sure no focus situations regress with your patch.

Comment 4

[Håkan Waara]

Oh, BTW:

+  PRUint32 numselected= 0;

Please make that a space on each side of the equality operator. (' = 0').

:)

Comment 5

[Mike Gratton]

Attachment: patch using GetNumSelected(), sans assignment typo

Comment 6

[Håkan Waara]

Seth, can you review this patch?

Comment 7

[Mike Gratton]

I've played around with various focus transitions, and I don't think I've
regressed anything. Is there a less subjective, more empirical way of doing this?

Comment 8

[Mike Gratton]

This has been fixed (poorly, IMHO ;) in bug 84260.

*** This bug has been marked as a duplicate of 84260 ***

Comment 9

[Mike Gratton]

D'ohh.. wrong bug. sorry!

Comment 10

[Håkan Waara]

So this patch is still current and usable?

Comment 11

[Mike Gratton]

Attachment: updated patch for current tree

Comment 12

[Mike Gratton]

bienvenu's change, probably for bugs 91352, actually started using the array
returned by GetSelectedIndices(), so the new patch creates it only if needed.

Note that GetCommandStatus() gets called many times (2-4 per button and
command?) every focus change, so it's probably important that this is as fast as
possible.

Comment 13

[Håkan Waara]

Please cvs diff with the -u option for more readable patches.

Comment 14

[Mike Gratton]

Attachment: updated patch, using diff -u

Comment 15

[Håkan Waara]

Looks like a good optimization to me. Especially in the PR_FALSE case, where we
don't have to create all those unnecessary variables.

Please remove the space between the if and else clauses and you have: r=hwaara.

CC bienvenu for sr=

Comment 16

[Mike Gratton]

Attachment: updated patch, sans iffy space

Comment 17

[David :Bienvenu]

I don't think this change is worthwhile. If you think it's going to make a whit
of difference as far as performance is concerned, then you will be sorely sorely
disappointed. You could call this routine a few thousand times and it wouldn't
approach the time it takes to paint a single toolbar icon. Also,
GetSelectedIndices handles the standalone message window case, and
GetNumSelected does not.

Comment 18

[Mike Gratton]

Okay, well, with the perf work going on, I guess this is wontfix.

Comment 19

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

Trusting all in the bug to re-open if they think this _would_ be a performance
gain.  For now, verifying to get off QA radar.