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

RESOLVED FIXED in mozilla33

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla33
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

4 years ago
Blocks: 1024082
(Assignee)

Comment 1

4 years ago
Created attachment 8438676 [details] [diff] [review]
fix v1
Attachment #8438676 - Flags: review?(jwatt)
(Assignee)

Comment 2

4 years ago
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.)

Updated

4 years ago
Attachment #8438676 - Flags: review?(jwatt) → review+
(Assignee)

Updated

4 years ago
Flags: in-testsuite-
(Assignee)

Comment 4

4 years ago
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)
(Assignee)

Comment 5

4 years ago
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.)
Depends on: 1024138
(Assignee)

Comment 6

4 years ago
(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.)
(Assignee)

Comment 7

4 years ago
Created attachment 8438812 [details] [diff] [review]
fix v2
Attachment #8438676 - Attachment is obsolete: true
Attachment #8438812 - Flags: review?(jwatt)
(Assignee)

Comment 8

4 years ago
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.)
(Assignee)

Comment 9

4 years ago
Created attachment 8438857 [details] [diff] [review]
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
Attachment #8438857 - Flags: review?(jwatt)
(Assignee)

Updated

4 years ago
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+
(Assignee)

Comment 12

4 years ago
(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.)
https://hg.mozilla.org/mozilla-central/rev/6cc517e07d5e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.