folder pane performance regression (possibly caused by #172545)

VERIFIED FIXED in mozilla1.3beta

Status

VERIFIED FIXED
16 years ago
14 years ago

People

(Reporter: sspitzer, Assigned: neil)

Tracking

Trunk
mozilla1.3beta
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

david detects a folder pane performance regression on the trunk.
(possibly caused by #172545)

the theory (bienvenu is going to verify) is that we are now crossing over to js 

(see msgMail3PaneWindow.js) for this:
 
+    getImageSource: function(row, colID)
+    {
+    },

I think only something in phoenix is using that.  

so my question for hyatt is:

should we add a boolean readonly attribute on the view, that you check once in 
nsTreeBodyFrame, to avoid calling getImageSource(), if the 
nsIXULTemplateBuilder knows there is not going to be any image source?

(in theory the same problem exists in bookmarks.xml, we'd have to fix that, too)

Comment 1

16 years ago
I think we need something better designed.
I filed bug 120071 long time ago.
Although in that time I thought we need just one listener instead of multiple
observers.

Comment 2

16 years ago
I can't tell for sure if this is slowing us down noticeably, but we know that
calls into js are expensive, and this causes a lot of them, especially if you're
showing 3 or 4 folder columns, so I think we should try to fix this.

Updated

16 years ago
Depends on: 120071

Updated

16 years ago
Keywords: nsbeta1

Updated

16 years ago
QA Contact: olgam → laurel

Updated

16 years ago
Keywords: nsbeta1 → nsbeta1+

Comment 3

16 years ago
Neil and I came to conclusion that we actually don't need getImageSource method
at all
Phoenix can just use |src="...#Icon"| in its template
Assignee: sspitzer → varga
(Assignee)

Comment 4

16 years ago
Jan: it already does!
(Assignee)

Comment 5

16 years ago
Created attachment 111704 [details] [diff] [review]
Proposed patch

No negative personality jokes, please :-)
(Assignee)

Updated

16 years ago
Attachment #111704 - Flags: superreview?(sspitzer)
Attachment #111704 - Flags: review?(varga)

Comment 6

16 years ago
Comment on attachment 111704 [details] [diff] [review]
Proposed patch

looks good r=varga
you did the right thing, that you didn't backout original patch completely,
since it contained other fix for image regions
Attachment #111704 - Flags: review?(varga) → review+

Comment 7

16 years ago
Comment on attachment 111704 [details] [diff] [review]
Proposed patch

sr=bienvenu, thx
Attachment #111704 - Flags: superreview?(sspitzer) → superreview+
sr=sspitzer, too.

re-assign to neil, set as 1.3 beta
Assignee: varga → neil
Target Milestone: --- → mozilla1.3beta
(Assignee)

Comment 9

16 years ago
Jan, can you check in the change to browser/ please?

Comment 10

16 years ago
I checked the Phoenix patch in.

Marking FIXED
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 11

16 years ago
The checked in patch includes a typo that breaks Bookmarks in Phoenix:

  XML Parsing Error: not well-formed
  Location: jar:resource:///chrome/browser.jar!/content/
                            browser/bookmarks/bookmarksTree.xml
  Line Number 1, Column 2:
  g<?xml version="1.0"?>  
  -^

  ###!!! ASSERTION: An XBL file is malformed.  Did you forget the XBL namespace    
  on the bindings tag?: 'Error', file nsXBLService.cpp, line 410
  Break: at file nsXBLService.cpp, line 410

Could someone please remove that initial 'g'?

Comment 12

16 years ago
done

Comment 13

16 years ago
*** Bug 184624 has been marked as a duplicate of this bug. ***

Comment 14

16 years ago
Using trunk build 20030228 on winxp, macosx and linux the folder pane loading
OK. If there is a specific setup I need to do other than to switch between
folders with lots of messages (662 - 3996) then comment with steps and I'll
retest this. Verified 
Status: RESOLVED → VERIFIED

Updated

16 years ago
QA Contact: laurel → esther
(Assignee)

Comment 15

16 years ago
Actually the performance affects folder pane painting, e.g. when scrolling.
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.