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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 2

[Daniel Holbert [:dholbert]]

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

Comment 3

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/703d26e9d979

Comment 4

[Daniel Holbert [:dholbert]]

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/5466ec649b54
...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)

Comment 5

[Daniel Holbert [:dholbert]]

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 moz.build, 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.)

Comment 6

[Daniel Holbert [:dholbert]]

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

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix v2

Comment 8

[Daniel Holbert [:dholbert]]

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

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: fix v3

...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: https://tbpl.mozilla.org/?tree=Try&rev=8691970c0128

Comment 10

[Jonathan Watt [:jwatt]]

Comment on attachment 8438857 [details] [diff] [review]
fix v3

Nice. :)

Comment 11

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cc517e07d5e

Comment 12

[Daniel Holbert [:dholbert]]

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

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6cc517e07d5e