Closed Bug 1024082 Opened 6 years ago Closed 2 years ago

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

Categories

(Core :: Layout, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: dholbert, Assigned: emilio)

References

Details

Attachments

(5 files)

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.
Depends on: 1024138
Bug 1024138 fixes the circular dependency described in comment 0, but there are then others.
Attached patch test patchSplinter Review
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.
Hah, I was removing nsFrameManagerBase today today and found this bug when filing.
Assignee: nobody → emilio
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...
Attachment #8956390 - Flags: review?(dholbert)
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.
Attachment #8956391 - Flags: review?(dholbert)
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.
Attachment #8956392 - Flags: review?(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?
Flags: needinfo?(emilio)
(Here's a patch to disable unified builds in layout/base and layout/generic, for testing/validation purposes: https://pastebin.mozilla.org/9079267 )
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.
Flags: needinfo?(emilio)
Attachment #8956610 - Flags: review?(dholbert)
Yeah, sorry - I did mean part 3 (the place where we start calling FrameConstructor().
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.
Attachment #8956390 - Flags: review?(dholbert) → review+
Comment on attachment 8956391 [details] [diff] [review]
Remove most of the nsIPresShell::FrameManager calls.

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

r=me
Attachment #8956391 - Flags: review?(dholbert) → review+
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 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.)
Attachment #8956392 - Flags: review?(dholbert) → review+
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)
Attachment #8956610 - Flags: review?(dholbert) → review+
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.)
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);
>                                    ^
(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!
(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.
Pushed by ecoal95@gmail.com:
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
You need to log in before you can comment on or make changes to this bug.