352851 - [FIX]Reflow count painting got removed in bug 317375

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Bug 317375 removed the DO_GLOBAL_REFLOW_COUNT_DSP and DO_GLOBAL_REFLOW_COUNT_DSP_J macros from nsPresContext.h.  Unfortunately, those were the way that the "Reflow Counts" layout debugger option worked...  These are really useful for evaluating performance issues where "too much reflow" is suspected or known (e.g. bug 320378).

What would it take to restore equivalent functionality in the new world?

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Instrument BuildDisplayList I guess. I don't really know how that stuff worked.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: This seems to do the trick

Looks like we only painted reflow counts for some frames...  So I added those back in.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Now I see what's going on, sorry.

Why not do this in one place by modifying nsFrame::BuildDisplayListForChild and parsing the frame name out of the result of GetFrameName? yeah, it's ugly but it's ugly in one place.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Why not do this in one place by modifying nsFrame::BuildDisplayListForChild

Well...  First of all, the existing code didn't paint reflow counts for all frames.  I suppose I could hardcode such a list in nsFrame::BuildDisplayListForChild...

> and parsing the frame name out of the result of GetFrameName?

I'd have to change the output of GetFrameName for some of these frames (list control frames, at the very least output "ListControl" for GetFrameName and want nsListControlFrame for this code).  But I could probably do that...  Want me to?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #4)
> > Why not do this in one place by modifying nsFrame::BuildDisplayListForChild
> 
> Well...  First of all, the existing code didn't paint reflow counts for all
> frames.  I suppose I could hardcode such a list in
> nsFrame::BuildDisplayListForChild...

Is there a reason to not do it for all frames?

> > and parsing the frame name out of the result of GetFrameName?
> 
> I'd have to change the output of GetFrameName for some of these frames (list
> control frames, at the very least output "ListControl" for GetFrameName and
> want nsListControlFrame for this code).  But I could probably do that...  Want
> me to?

Sure, if we're going to go this way. But there's no point in my suggestion if we have to hard-code a list of frame types somewhere.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Is there a reason to not do it for all frames?

I think it would be pretty unreadable, esp. since frames often have exactly coinciding origins...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Fixed.