1024082 - Merge nsFrameManagerBase & nsFrameManager (after resolving any #include-order issues that cause nsFrameManagerBase to exist)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Right now, nsFrameManager derives from a class called nsFrameManagerBase, which has this arcane reason for existing:
> 17 /* part of nsFrameManager, to work around header inclusionordering */
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsFrameManagerBase.h?rev=58a202b28197#17

To the extent that this is still working around any header inclusion-order issues, it must be with respect to the headers included by nsIPresShell.h, since that's the only thing that #includes this (other than the base class nsFrameManager itself).

We can almost drop this #include from nsIPresShell.h entirely, except that nsIPresShell has this inline method:

> 434   virtual nsIFrame* GetRootFrameExternal() const;
> 435   nsIFrame* GetRootFrame() const {
> 436 #ifdef MOZILLA_INTERNAL_API
> 437     return mFrameManager->GetRootFrame();
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsIPresShell.h?rev=8968e7307508#435

...which requires that we know about the mFrameManager class-definition.

And similarly, nsFrameManager has this inline method:
> 160   nsPresContext* GetPresContext() const {
> 161     return mPresShell->GetPresContext();
> 162   }
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsFrameManager.h?rev=e09168ca3072#160

...which requires that we know about the nsIPresShell class-definition.

So, to get around this, we have this special "nsFrameManagerBase" class, and nsIPresShell::mFrameManager is just an instance of *that* class, so that nsIPresShell.h doesn't have to include nsFrameManager.h and end up with a circular dependency.

I'm betting there's a cleaner way we could resolve this, though... e.g. with a "nsFrameManagerInlines.h" file which would have the (inline) nsFrameManager::GetPresContext() implementation. Then, nsFrameManager could forward-declare nsIPresShell, and nsIPresShell.h could include nsFrameManager.h, and nsFrameManagerBase would no longer have any reason for being arbitrarily split out.

Comment 1

[Jonathan Watt [:jwatt]]

Bug 1024138 fixes the circular dependency described in comment 0, but there are then others.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: test patch

This applies on top of the two patches from this bug's two blockers. Not sure if I'll pursue this right now. Probably not for a day or two anyway, so feel free to take.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Hah, I was removing nsFrameManagerBase today today and found this bug when filing.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Remove nsFrameManagerBase

Instead move UndisplayedNode to its own file, which is what causes the include
hell due to requiring nsIContent / nsStyleContext.

Mozreview is not working for me again, and I don't know how to send Phabricator requests for multiple patches, so...

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Remove most of the nsIPresShell::FrameManager calls.

Most of them just want GetRootFrame(), and there's no need to explicitly go
through the frame manager for that, we have a handy alias in the shell.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Remove PresShell::FrameManager(), use PresShell::FrameConstructor instead.

No point in having two names for the same thing.

Unfortunately this means that we need to export a couple more headers, but that
should be ok.

Comment 7

[Daniel Holbert [:dholbert]]

This patch-stack isn't quite correct, RE having the needed #includes (though the problem is masked by unified builds).

If I remove the substring "UNIFIED_" from layout/base/moz.build and layout/generic/moz.build, I get the following error:
====
../../../mozilla/layout/base/RestyleTracker.cpp:315:17: error: member access into incomplete type 'nsCSSFrameConstructor'
                ->GetDisplayContentsStyleFor(element))) ||
                ^
../../../mozilla/layout/base/nsIPresShell.h:66:7: note: forward declaration of 'nsCSSFrameConstructor'
class nsCSSFrameConstructor;
      ^
1 error generated.
====

...and these two more errors:
====
../../../mozilla/layout/generic/nsPlaceholderFrame.cpp:193:23: error: cannot initialize a variable of type 'nsFrameManager *' with an rvalue of type 'nsCSSFrameConstructor *'
      nsFrameManager* fm = PresContext()->FrameConstructor();
                      ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../mozilla/layout/generic/nsPlaceholderFrame.cpp:221:40: error: member access into incomplete type 'nsCSSFrameConstructor'
      PresContext()->FrameConstructor()->GetDisplayContentsStyleFor(parentContent);
====



It looks like these are all cases where "part 2" (attachment 8956391 [details] [diff] [review]) here has switched us to use ->FrameConstructor() rather than ->FrameManager(), so we just need to #include nsCSSFrameConstructor.h in these .cpp files, in that patch?

Comment 8

[Daniel Holbert [:dholbert]]

(Here's a patch to disable unified builds in layout/base and layout/generic, for testing/validation purposes: https://pastebin.mozilla.org/9079267 )

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: fixup to part three.

I assume you meant part 3, which is the only thing that tweaks includes.

There's a fixup / interdiff that should fix those errors, which I'll squash in the same patch.

Comment 10

[Daniel Holbert [:dholbert]]

Yeah, sorry - I did mean part 3 (the place where we start calling FrameConstructor().

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8956390 [details] [diff] [review]
Remove nsFrameManagerBase

Review of attachment 8956390 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on this patch with nits addressed.

Not absolutely necessary, but if you like, it might be nice to refactor this patch into two pieces:
 (1) splitting out UndisplayedNode into its own header (and updating other files to #include that new header accordingly)
 (2) Remove nsFrameManagerBase

(These seem like two logically separate steps to me - grouped together here just because the former is a prereq for the latter)
(But if you do make this split, it'd probably be worth one more quick review pass on the newly-split parts.)

::: layout/base/UndisplayedNode.h
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsIContent.h"
> +#include "nsStyleContext.h"

Two missing things here:
 (1) Add a one line /*...*/ comment, just below the license block (separated by a blank line). Maybe something like:
 /* Linked list node for undisplayed element */

This is important/useful because DXR parses & displays these summaries alongside the file listing here:
 https://dxr.mozilla.org/mozilla-central/source/layout/base

 (2) After that, you need to add an #ifndef guard.  Based on coding style guide, I think it should be:
#ifndef mozilla_UndisplayedNode_h
#define mozilla_UndisplayedNode_h
...
#endif // mozilla_UndisplayedNode_h

::: layout/base/nsFrameManager.h
@@ +9,5 @@
>  #ifndef _nsFrameManager_h_
>  #define _nsFrameManager_h_
>  
> +#include "nsDebug.h"
> +#include "PLDHashTable.h"

Drop the "PLDHashTable.h" include here -- you don't actually need that. (You're copying it over from nsFrameManagerBase.h, but that file didn't need it either! :D  So let's just let it die with nsFrameManagerBase.h.)

(Presumably nsFrameManagerBase had it for the member-var "PLDHashTable mPlaceholderMap" that it had at one time:
https://searchfox.org/mozilla-central/rev/647b520991741a1f10edec4bc1d0fbbc3f1a64ab/layout/base/nsFrameManagerBase.h#59
...but that member-var was removed in bug 1368547. And there are no other mentions of "hash" in *this* file, except for in a code-comment, so I don't think we need PLDHashTable.h for anything else here.)

@@ +49,1 @@
>      mPresShell = aPresShell;

Get rid of this "mPresShell = aPresShell" assignment -- it's redundant, now that you're *also* using the init list to assign this variable.

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8956391 [details] [diff] [review]
Remove most of the nsIPresShell::FrameManager calls.

Review of attachment 8956391 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8956391 [details] [diff] [review]
Remove most of the nsIPresShell::FrameManager calls.

Review of attachment 8956391 [details] [diff] [review]:
-----------------------------------------------------------------

Oh, one nit - looks like the first-line commit message is wrapped to several lines right now:
> Subject: [PATCH 2/3] Bug 1024082: Remove most of the
>  nsIPresShell::FrameManager calls. r?dholbert

That might mean TreeHerder will interpret the commit message as being simply "[PATCH 2/3] Bug 1024082: Remove most of the".  Please unwrap that line, or make sure that whatever toolchain you're using will unwrap it as far as TreeHerder/HGWeb are concerned.

This applies to both Patch 2 and Patch 3.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8956392 [details] [diff] [review]
Remove PresShell::FrameManager(), use PresShell::FrameConstructor instead.

Review of attachment 8956392 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed:

> Unfortunately this means that we need to export a couple more headers, but that
> should be ok.

I didn't initially understand why this was the case -- the moz.build changes seemed unnecessary (though I figured it out after reverting them & rebuilding locally).

Maybe add a bit more explanation here, for posterity? Maybe something like: "(Specifically: we have to export some headers that are #included by nsCSSFrameConstructor.h, because nsCSSFrameConstructor.h itself will now be #included in more places outside of //layout, by .cpp files that don't necessarily have the ability to indirectly #include its other headers, unless we export them.)"

::: layout/generic/nsFrame.cpp
@@ +9920,4 @@
>            /* if next is true then it's really a request for the table frame's
>               parent context, see nsTable[Outer]Frame::GetParentStyleContext. */
>            pseudo == nsCSSAnonBoxes::tableWrapper) {
> +        nsCSSFrameConstructor* fm = PresContext()->FrameConstructor();

This file has an include for nsFrameManager.h which I think you can remove now.  (I don't see any other mentions of "FrameManager" in this file, now that you've removed this one, so I think the include is becoming stale.)

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8956610 [details] [diff] [review]
fixup to part three.

Review of attachment 8956610 [details] [diff] [review]:
-----------------------------------------------------------------

(r=me on the interdiff - let's indeed fold this into patch 3)

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8956390 [details] [diff] [review]
Remove nsFrameManagerBase

Review of attachment 8956390 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsFrameManager.h
@@ -14,3 @@
>  #include "nsFrameList.h"
> -#include "nsIContent.h"
> -#include "nsStyleContext.h"

Sorry, one more nit here -- you need to add a "class nsStyleContext;" forward-decl to this header (in the block of forward decls below this), since you're removing this nsStyleContext.h include & since this file *does* have usages of "nsStyleContext*" in its function signatures.

(Alternately, you could keep this include here -- but local testing seems to indicate that a forward-decl is sufficient after your changes.)

Comment 17

[Daniel Holbert [:dholbert]]

Just for completeness -- without comment 16 addressed, & with the additional #include-reordering patch in bug 1443672 layered on top, we hit this build error and several more like it:
> In file included from ../../../mozilla/layout/base/nsFrameManager.cpp:9:
> ../../../mozilla/layout/base/nsFrameManager.h:84:36: error: unknown type name 'nsStyleContext'
>                                    nsStyleContext* aStyleContext);
>                                    ^

Comment 18

[Emilio Cobos Álvarez (:emilio)]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> Comment on attachment 8956390 [details] [diff] [review]
> Remove nsFrameManagerBase
> 
> Review of attachment 8956390 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me on this patch with nits addressed.
> 
> Not absolutely necessary, but if you like, it might be nice to refactor this
> patch into two pieces:
>  (1) splitting out UndisplayedNode into its own header (and updating other
> files to #include that new header accordingly)
>  (2) Remove nsFrameManagerBase
> 
> (These seem like two logically separate steps to me - grouped together here
> just because the former is a prereq for the latter)
> (But if you do make this split, it'd probably be worth one more quick review
> pass on the newly-split parts.)

I agree, but I don't think it's worth to spend extra time on it at this point.

> ::: layout/base/UndisplayedNode.h
> @@ +4,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +#include "nsIContent.h"
> > +#include "nsStyleContext.h"
> 
> Two missing things here:
>  (1) Add a one line /*...*/ comment, just below the license block (separated
> by a blank line). Maybe something like:
>  /* Linked list node for undisplayed element */
> 
> This is important/useful because DXR parses & displays these summaries
> alongside the file listing here:
>  https://dxr.mozilla.org/mozilla-central/source/layout/base
> 
>  (2) After that, you need to add an #ifndef guard.  Based on coding style
> guide, I think it should be:
> #ifndef mozilla_UndisplayedNode_h
> #define mozilla_UndisplayedNode_h
> ...
> #endif // mozilla_UndisplayedNode_h

:O nice catch, can't believe I forgot the #include guard :(.

> ::: layout/base/nsFrameManager.h
> @@ +9,5 @@
> >  #ifndef _nsFrameManager_h_
> >  #define _nsFrameManager_h_
> >  
> > +#include "nsDebug.h"
> > +#include "PLDHashTable.h"
> 
> Drop the "PLDHashTable.h" include here -- you don't actually need that.
> (You're copying it over from nsFrameManagerBase.h, but that file didn't need
> it either! :D  So let's just let it die with nsFrameManagerBase.h.)
>
> (Presumably nsFrameManagerBase had it for the member-var "PLDHashTable
> mPlaceholderMap" that it had at one time:
> https://searchfox.org/mozilla-central/rev/
> 647b520991741a1f10edec4bc1d0fbbc3f1a64ab/layout/base/nsFrameManagerBase.h#59
> ...but that member-var was removed in bug 1368547. And there are no other
> mentions of "hash" in *this* file, except for in a code-comment, so I don't
> think we need PLDHashTable.h for anything else here.)

Indeed, nice :)
 
> @@ +49,1 @@
> >      mPresShell = aPresShell;
> 
> Get rid of this "mPresShell = aPresShell" assignment -- it's redundant, now
> that you're *also* using the init list to assign this variable.

Great catch again.

Thanks for the review!

Comment 19

[Emilio Cobos Álvarez (:emilio)]

(In reply to Daniel Holbert [:dholbert] from comment #13)
> Comment on attachment 8956391 [details] [diff] [review]
> Remove most of the nsIPresShell::FrameManager calls.
> 
> Review of attachment 8956391 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Oh, one nit - looks like the first-line commit message is wrapped to several
> lines right now:
> > Subject: [PATCH 2/3] Bug 1024082: Remove most of the
> >  nsIPresShell::FrameManager calls. r?dholbert
> 
> That might mean TreeHerder will interpret the commit message as being simply
> "[PATCH 2/3] Bug 1024082: Remove most of the".  Please unwrap that line, or
> make sure that whatever toolchain you're using will unwrap it as far as
> TreeHerder/HGWeb are concerned.
> 
> This applies to both Patch 2 and Patch 3.

Yeah, this is just an artifact of git-format-patch, doesn't affect the final patches as landed.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5936540dc379
Remove nsFrameManagerBase. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a387ecebd587
Remove most of the nsIPresShell::FrameManager calls. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a0bb325da831
Remove PresShell::FrameManager(), use PresShell::FrameConstructor instead. r=dholbert

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5936540dc379
https://hg.mozilla.org/mozilla-central/rev/a387ecebd587
https://hg.mozilla.org/mozilla-central/rev/a0bb325da831