510651 - Fold nsIFrameDebug into ns(I)Frame

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Zack Weinberg (:zwol)]

Attachment: 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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 2

[Zack Weinberg (:zwol)]

(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).

Comment 3

[Zack Weinberg (:zwol)]

Attachment: patch v2

This has the backslashes lined up; also, it has been confirmed to build in both --enable-debug and --disable-debug mode.

Comment 4

[Zack Weinberg (:zwol)]

Comment on attachment 394896 [details] [diff] [review]
patch v2

changing reviewers since roc is on vacation.  this had sr=dbaron before.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'll land this later today.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/f9c0cfc46297

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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?  :(

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Zack Weinberg (:zwol)]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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