Fold nsIFrameDebug into ns(I)Frame

RESOLVED FIXED

Status

()

Core
Layout: Misc Code
P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: zwol, Assigned: zwol)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
Created attachment 394630 [details] [diff] [review]
patch v1

There's no good reason to have nsIFrameDebug be a separate class instead of an #ifdef DEBUG set of methods on nsIFrame/nsFrame.  It means every frame class has an extra vtable pointer in a debug build, it means extra code everywhere that wants to *use* those methods (they are already under #ifdef DEBUG, and now they have to call QueryFrame as well) and it's causing me grief (in the form of ambiguous base errors) over in bug 497495.

This patch folds the nsIFrameDebug methods into nsFrame/nsIFrame and eliminates the separate class.  Currently tested only in a debug build; need to do a non-debug test as well, just to be sure.
Attachment #394630 - Flags: superreview?(dbaron)
Attachment #394630 - Flags: review?(bzbarsky)
Attachment #394630 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 394630 [details] [diff] [review]
patch v1

Could you make the \s for the macros in nsFrame.h line up?


Any particular reason you put the static methods on nsFrame rather than 
nsIFrame?  It doesn't really matter much, but I probably would have gone
the other way.

sr=dbaron with the first issue fixed
(Assignee)

Comment 2

9 years ago
(In reply to comment #1)
> (From update of attachment 394630 [details] [diff] [review])
> Could you make the \s for the macros in nsFrame.h line up?

Sure.  Not till Monday, though.

> Any particular reason you put the static methods on nsFrame rather than 
> nsIFrame?  It doesn't really matter much, but I probably would have gone
> the other way.

Only that I initially tried to put everything on nsFrame (on the theory that that's where the code was anyway), but that didn't work, because some callers of ->List() and ->GetFrameName() only have an nsIFrame*.

Rather than change this I will offer to finish the patches that I started a while back that collapse together nsIFrame, nsBox, and nsFrame (calling the result nsFrame, but I could be talked out of that name).
(Assignee)

Comment 3

9 years ago
Created attachment 394896 [details] [diff] [review]
patch v2

This has the backslashes lined up; also, it has been confirmed to build in both --enable-debug and --disable-debug mode.
Attachment #394630 - Attachment is obsolete: true
Attachment #394896 - Flags: superreview+
Attachment #394896 - Flags: review?
Attachment #394630 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

9 years ago
Comment on attachment 394896 [details] [diff] [review]
patch v2

changing reviewers since roc is on vacation.  this had sr=dbaron before.
Attachment #394896 - Flags: superreview?(mrbkap)
Attachment #394896 - Flags: superreview+
Attachment #394896 - Flags: review?(dbaron)
Attachment #394896 - Flags: review?

Updated

9 years ago
Attachment #394896 - Flags: superreview?(mrbkap) → superreview+
Attachment #394896 - Flags: review?(dbaron) → review+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
I'll land this later today.
http://hg.mozilla.org/mozilla-central/rev/f9c0cfc46297
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
OS: Linux → All
Priority: -- → P2
Hardware: x86 → All
Resolution: --- → FIXED
This failed to compile for me in a non-debug build over here:

/Users/bzbarsky/mozilla/debug/mozilla/extensions/layout-debug/src/nsLayoutDebuggingTools.cpp: In function ‘void DumpFramesRecur(nsIDocShell*, FILE*)’:
/Users/bzbarsky/mozilla/debug/mozilla/extensions/layout-debug/src/nsLayoutDebuggingTools.cpp:436: error: ‘class nsIFrame’ has no member named ‘List’
make[2]: *** [nsLayoutDebuggingTools.o] Error 1

This happens reliably in 3 of my trees, but not in my 4th; I'm looking into the differences now.  They have to be in the mozconfig, since 2 of the 3 trees are clones of the one that doesn't have a problem.
Ah, it's simply a matter of --enable-extensions=layout-debug no longer compiling in a --disable-debug build.  I assume the test in comment 3 wasn't done with all extensions enabled?  :(
Perhaps DumpFramesRecur should just have its body inside #ifdef DEBUG?
(Assignee)

Comment 10

9 years ago
I, um, wasn't aware there was any such thing as --enable-extensions.  *grumble* We have too many configure options.

> Perhaps DumpFramesRecur should just have its body inside #ifdef DEBUG?

Yah, and also the do_QueryFrame to nsIFrameDebug needs taking out.
Ah, heh.  The stuff in extensions/ is only built if present in --enable-extensions with the default being some subset not equal to the whole set...

Your patch took out the do_QueryFrame already.

I pushed http://hg.mozilla.org/mozilla-central/rev/c4365eddea67 with that #ifdef
You need to log in before you can comment on or make changes to this bug.