Closed Bug 1024084 Opened 9 years ago Closed 9 years ago

Use more specific #includes / forward-declarations in nsFrameManager.h


(Core :: Layout, defect)

Not set





(Reporter: dholbert, Assigned: dholbert)




(1 file, 2 obsolete files)

No description provided.
Blocks: 1024082
Attached patch fix v1 (obsolete) — Splinter Review
Attachment #8438676 - Flags: review?(jwatt)
ChildListID now lives in nsFrameList.h, not nsIFrame.h -- hence the nsFrameList #include and the typedef-conversion.

(nsIFrame has its own convenience-typedef, but we don't need to depend on that here.)
Attachment #8438676 - Flags: review?(jwatt) → review+
Flags: in-testsuite-
Backed out:
...because this causes bustage in widget/android/nsAppShell.cpp, which #includes this header for no good reason (and depends on it to provide #includes of nsIContent.h and [indirectly] nsStyleContext.h)
Actually, the last part of comment 4 wasn't true - nsAppShell.cpp doesn't depend on this header to provide those things -- this header assumes those things, because (I'd forgotten) nsRefPtr<Foo> and nsCOMPtr<Foo> require that we've seen the class definition for Foo, since it expands to code that calls Foo's AddRef()/Release() methods.

I tested this more thoroughly by creating an empty .cpp file, adding it to SOURCES in, and giving it a single line "#include nsFrameManagerBase.h", and then (separately) "#include nsFrameManager.h", and rewrote this patch based on the results.

(Also, incidentally, we don't need the nsIPresShell include from the previous patch-version anymore, now that bug 1024138 has removed GetPresContext.)
Depends on: 1024138
(So in particular, the Android build failure was a hint that we need nsStyleContext.h and nsIContent.h, since we have a nsRefPtr of the former and a nsCOMPtr of the latter.)
Attached patch fix v2 (obsolete) — Splinter Review
Attachment #8438676 - Attachment is obsolete: true
Attachment #8438812 - Flags: review?(jwatt)
Comment on attachment 8438812 [details] [diff] [review]
fix v2

Some notes on the patch:

>diff --git a/layout/base/nsFrameManager.h b/layout/base/nsFrameManager.h
>+#include "nsAutoPtr.h"
>+#include "nsFrameList.h"
> #include "nsIContent.h"
>+#include "nsStyleContext.h"
> class nsContainerFrame;
>+class nsPlaceholderFrame;

(nsPlaceholderFrame fwd-decl is needed because we have a function that returns nsPlaceholderFrame*. Previously we got the nsPlaceholderFrame fwd-decl from the base header file, but the base header file doesn't actually need that fwd-decl, so I moved it here.)

>diff --git a/layout/base/nsFrameManagerBase.h b/layout/base/nsFrameManagerBase.h
>+#include "nsDebug.h"

(Needed because we have a NS_ASSERTION in this file)

> #include "pldhash.h"
>+class nsIFrame;
> class nsIPresShell;
> class nsStyleSet;
>-class nsIContent;
>-class nsPlaceholderFrame;
>-class nsIFrame;
>-class nsStyleContext;
>-class nsIAtom;
>-class nsStyleChangeList;
>-class nsILayoutHistoryState;

All the removed decls here aren't actually used in this file. (And I moved nsIFrame up top to preserve alphabetical order.)
Attached patch fix v3Splinter Review
...and that last patch didn't build, because a few other header files use nsStyleChangeList without forward-declaring it.

They were previously getting a forward-decl from nsFrameManagerBase.h, but they don't anymore, so they need their own forward-decl.  I've added them in this version.

Try run:
Attachment #8438857 - Flags: review?(jwatt)
Attachment #8438812 - Attachment is obsolete: true
Attachment #8438812 - Flags: review?(jwatt)
Comment on attachment 8438857 [details] [diff] [review]
fix v3

Nice. :)
Attachment #8438857 - Flags: review?(jwatt) → review+
(In reply to Daniel Holbert [:dholbert] from comment #4)
> [...] widget/android/nsAppShell.cpp, which
> #includes this header for no good reason

(FWIW, I filed bug 1024328 on removing this & other unnecessary {#include "nsFrameManager.h"} scattered across the tree.)
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.