Closed
Bug 560695
Opened 15 years ago
Closed 14 years ago
clicking (more) widget in headers with lots of recipients too slow
Categories
(Thunderbird :: Message Reader UI, defect)
Tracking
(blocking-thunderbird3.1 rc1+, thunderbird3.1 rc1-fixed)
RESOLVED
FIXED
People
(Reporter: dmosedale, Assigned: dmosedale)
References
Details
(Keywords: perf, regression)
Attachments
(4 files, 1 obsolete file)
This is a known regression from bug 550487. Current working theory (thanks to DTrace) is that we're reflowing after each address we add.
Flags: wanted-thunderbird+
Comment 1•15 years ago
|
||
IIRC I saw evidence of this is related to the .collapsed state (see the patch there).
And the collapsed in turn is we think due to unused headers lines (CC/BCC/Reply-To) still being collapsed.
First step would be to remove that "uncollapse all parents" hack and fix it by preventing the node from being collapsed, e.g. by uncollapsing the needed header line earlier.
Assignee | ||
Comment 2•15 years ago
|
||
This would be consistent with my current working theory, mentioned above: when things are collapsed, layout probably doesn't feel the need to reflow as much.
Comment 3•15 years ago
|
||
IIRC, the evidence I saw pointed the other direction: it's slow in collapsed state and faster when uncollapsed. But I may have been confused.
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → dmose
Assignee | ||
Updated•15 years ago
|
Whiteboard: [ETA for review-ready patch: Friday, May 7]
Assignee | ||
Updated•15 years ago
|
Whiteboard: [ETA for review-ready patch: Friday, May 7] → [ETA for review-ready patch: Tuesday, May 11]
Assignee | ||
Comment 4•15 years ago
|
||
Some massaging of the code coaxed dtrace into revealing that reading .clientWidth was the thing that was taking literally 95% of the time, presumably because each read was causing Gecko to flush its state and reflow.
As it turns out, there's no need to do that in the case where (X more) has been clicked and we're displaying all the headers, because we no longer need to detect overflow in order to place the (X more) node.
So now we don't. :-)
Given that there's nothing functionally different here, there doesn't seem to be much benefit in writing a test. That said, I'll add an in-testsuite? flag, because once we actually have a performance testing framework, this would be a good candidate for that.
Attachment #444222 -
Flags: review?(bwinton)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [ETA for review-ready patch: Tuesday, May 11] → [has patch; needs review bwinton, landing]
Assignee | ||
Updated•15 years ago
|
Flags: in-testsuite?
Comment 5•15 years ago
|
||
Great!
r=BenB
Updated•15 years ago
|
Attachment #444222 -
Flags: review+
Comment 6•15 years ago
|
||
Comment on attachment 444222 [details] [diff] [review]
fix, v1
>+++ b/mail/base/content/mailWidgets.xml
>@@ -405,23 +405,27 @@
>- // Calculate width and lines
We seem to have lost this comment. Was that on purpose?
Other than that, I like it. It's understandable, and it makes things faster for me.
Later,
Blake.
Attachment #444222 -
Flags: review?(bwinton) → review+
Assignee | ||
Updated•15 years ago
|
Whiteboard: [has patch; needs review bwinton, landing] → [has reviewed patch, needs comment tweak & landing dmose]
Assignee | ||
Comment 7•15 years ago
|
||
Added back the comment as requested by Blake and rebased against the Jim's patch from bug 563612, as I'll be landing them together.
Attachment #444222 -
Attachment is obsolete: true
Attachment #445483 -
Flags: review+
Assignee | ||
Comment 8•15 years ago
|
||
Landed:
http://hg.mozilla.org/comm-central/rev/84dd77b453af
http://hg.mozilla.org/releases/comm-1.9.2/rev/000ce50ec4f3
status-thunderbird3.1:
--- → rc1-fixed
Whiteboard: [has reviewed patch, needs comment tweak & landing dmose]
Assignee | ||
Updated•15 years ago
|
Attachment #445483 -
Attachment description: fix, v2 → fix, v2 (checked in on 1.9.3 trunk and 1.9.2)
Comment 9•15 years ago
|
||
Feedback:
Excellent Perf gain <1sec on "more".
Minor nit, if View>>Headers>>All only 1 cc per line is display.
(but maybe that's been there all along, not sure)
Comment 10•15 years ago
|
||
Comment 11•15 years ago
|
||
> (comment #9) Excellent Perf gain <1sec on "more".
Agreed, this is much of an improvement!
> Minor nit, if View>>Headers>>All only 1 cc per line is display.
> (but maybe that's been there all along, not sure)
Not really, your window is just narrow enough so that only a single address fits into one line, then it is wrapped. If you widen it the second addresses should become visible. There is a boundary between the box showing the addresses and the ones with the buttons and menus, it will wrap as soon as
you cross it (red line - the message is old, pictured with today's build).
Comment 12•15 years ago
|
||
This is not a showstopper, by any means. See attachment.
Comment 13•15 years ago
|
||
My guess is that the alignment of the boxes shifts between Normal and All headers. Your example "Content-Transfer-Encoding:" would certainly move the boundaries of the left-most header-label box to the right, thus reducing the space left for the headers themselves, causing this borderline appearance.
Comment 14•14 years ago
|
||
Dan: Is this fixed now? Should the comments after the landing be in follow-up bugs?
Comment 15•14 years ago
|
||
The comments on the alignment of the boxes are unrelated to this bug, this is
a different issue and should be handled separately if it's a problem.
Assignee | ||
Comment 16•14 years ago
|
||
Mark: good catch; I should have closed this earlier. Agreed that the box alignment issue wants to be spun off to a separate bug.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•