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.
10 years ago
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
(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).
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.
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) → superreview+
10 years ago
Attachment #394896 - Flags: review?(dbaron) → review+
I'll land this later today.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
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: *** [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?
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
10 years ago
Depends on: 512518
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.