Closed Bug 562093 Opened 12 years ago Closed 12 years ago

Add reflow tracing for InitConstraints, InitOffsets, InitFrameType

Categories

(Core :: Layout: Block and Inline, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: zwol, Assigned: zwol)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
This patch adds reflow tracing (the kind enabled by  GECKO_DISPLAY_REFLOW_RULES_FILE) to InitConstraints, InitOffsets, and InitFrameType.  I don't know if we actually *want* this, but it has enormously clarified what is going wrong with a patch that I'm working on, so I thought I'd see if anyone else would find it useful.
Attachment #441853 - Flags: review?(bzbarsky)
Attachment #441853 - Flags: review?(bzbarsky) → review?(dbaron)
Comment on attachment 441853 [details] [diff] [review]
patch

David, can you review this?  If you're swamped I can try to do it, but it'll be at least a week...
I don't mind if this has to wait a while for review, regardless of who does it.
Comment on attachment 441853 [details] [diff] [review]
patch

It looks like there are a bunch of good improvements here.

I didn't go over it line-by-line, and I don't think I need to since it's #ifdef DEBUG.

The one comment I'd make is that I think it's pretty odd to have nsHTMLReflowState/nsCSSOffsetState methods defined in nsFrame.cpp.  I think they should either be moved into the appropriate file or moved to class nsFrame.  I'd prefer moving the methods to class nsFrame to keep all the display-reflow code together.

r=dbaron with one of those changes (preferably the latter)
Attachment #441853 - Flags: review?(dbaron) → review+
(In reply to comment #3)
> 
> The one comment I'd make is that I think it's pretty odd to have
> nsHTMLReflowState/nsCSSOffsetState methods defined in nsFrame.cpp.  I think
> they should either be moved into the appropriate file or moved to class
> nsFrame.  I'd prefer moving the methods to class nsFrame to keep all the
> display-reflow code together.

I agree that it is odd, but unfortunately, neither of your suggestions is feasible.  The new nsHTMLReflowState and nsCSSOffsetState methods have to be (static) methods of those classes so they can access protected instance variables, and they have to be in nsFrame.cpp because DR_state and related classes are not defined in any header file.

Counterproposal: I take all of the DR_state code and break it out to its own file (layout/generic/DisplayReflow.cpp, say?)
I guess it's ok as-is if you add comments in the .h files saying what .cpp file the methods are defined in.
Ok, this does that.  Will land shortly.
Attachment #441853 - Attachment is obsolete: true
Attachment #442463 - Flags: review+
... FSVO "shortly" which may be "tomorrow", given that the tree was just closed for the add-on manager rewrite landing.
http://hg.mozilla.org/mozilla-central/rev/b830eb32915d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.