1437623 - Preemptively fix unified bustage in several layout subdirectories

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Daniel Holbert [:dholbert]]

This bug is one in a series of "fix latent missing-#include/declaration/namespace compile errors which are currently being hidden by unified builds" bugs.  (This is worth doing periodically, because otherwise these issues are just landmines that get discovered when we add new .cpp files or make another change that reshuffles the unification.)

I've got patches for a few layout subdirectories which I'll post shortly here.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1437623 part 1: (layout/generic) Add missing includes/namespaces to preemptively fix unified bustage.

Review commit: https://reviewboard.mozilla.org/r/219628/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219628/

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1437623 part 2: (layout/base) Add missing includes/namespaces to preemptively fix unified bustage.

Review commit: https://reviewboard.mozilla.org/r/219630/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219630/

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Bug 1437623 part 3: (layout/painting) Add missing includes/namespaces to preemptively fix unified bustage.

Review commit: https://reviewboard.mozilla.org/r/219632/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219632/

Comment 4

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8950423 [details]
Bug 1437623 part 3: (layout/painting) Add missing includes/namespaces to preemptively fix unified bustage.

https://reviewboard.mozilla.org/r/219632/#review225416

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 8950421 [details]
Bug 1437623 part 1: (layout/generic) Add missing includes/namespaces to preemptively fix unified bustage.

https://reviewboard.mozilla.org/r/219628/#review225638

::: layout/generic/nsBulletFrame.cpp:52
(Diff revision 1)
>  using namespace mozilla::gfx;
> +using namespace mozilla::gfx;
>  using namespace mozilla::image;

no need to do it twice

::: layout/generic/nsFrame.cpp:26
(Diff revision 1)
> +#include "nsIBaseWindow.h"
>  #include "nsIContent.h"
>  #include "nsIContentInlines.h"
>  #include "nsContentUtils.h"
> +#include "nsCSSFrameConstructor.h"
>  #include "nsCSSPseudoElements.h"
> +#include "nsCSSRendering.h"
>  #include "nsAtom.h"

Erh, these dependencies kinda sucks.  Why are they needed?  Can we work around the compilation issues in some other way that would avoid these #includes?

::: layout/generic/nsFrame.cpp:95
(Diff revision 1)
> +#include "gfxPrefs.h"
>  #include "nsAbsoluteContainingBlock.h"

Fine I guess, but I'd like to avoid it if possible.
Why is it needed exactly?

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 8950422 [details]
Bug 1437623 part 2: (layout/base) Add missing includes/namespaces to preemptively fix unified bustage.

https://reviewboard.mozilla.org/r/219630/#review225640

::: layout/base/ServoRestyleManager.cpp:20
(Diff revision 1)
> +#include "nsAccessibilityService.h"
>  #include "nsBlockFrame.h"

wrap it in #ifdef ACCESSIBILITY please

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 8950421 [details]
Bug 1437623 part 1: (layout/generic) Add missing includes/namespaces to preemptively fix unified bustage.

https://reviewboard.mozilla.org/r/219628/#review225644

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8950421 [details]
Bug 1437623 part 1: (layout/generic) Add missing includes/namespaces to preemptively fix unified bustage.

https://reviewboard.mozilla.org/r/219628/#review225676

::: layout/generic/nsBulletFrame.cpp:52
(Diff revision 1)
>  using namespace mozilla::gfx;
> +using namespace mozilla::gfx;
>  using namespace mozilla::image;

Oops! Thanks - fixed.

(I think that was me cutting&pasting a template for the legitimately-new "using" decl later, and I accidentally pasted the original decl back twice.)

::: layout/generic/nsFrame.cpp:26
(Diff revision 1)
> +#include "nsIBaseWindow.h"
>  #include "nsIContent.h"
>  #include "nsIContentInlines.h"
>  #include "nsContentUtils.h"
> +#include "nsCSSFrameConstructor.h"
>  #include "nsCSSPseudoElements.h"
> +#include "nsCSSRendering.h"
>  #include "nsAtom.h"

nsIBaseWindow.h is needed for:
==
nsFrame.cpp:11120:13: error: member access into incomplete type 'nsIBaseWindow'
 0:10.23   baseWindow->GetMainWidget(getter_AddRefs(mainWidget));
 0:10.23             ^
==

nsCSSFrameConstructor.h is needed for:
==
nsFrame.cpp:255:15: error: member access into incomplete type 'nsCSSFrameConstructor'
 0:10.22               ->DestroyAnonymousContent(Move(aContent));
 0:10.22               ^
==
...plus a call to presContext->FrameConstructor()->GetRootElementStyleFrame() at 11133.

nsCSSRendering.h is needed for:
==
nsFrame.cpp:2717:9: error: use of undeclared identifier 'nsCSSRendering'
 0:10.22         nsCSSRendering::ImageLayerClipState clipState;
 0:10.22         ^
==

Is that OK? I don't know why adding these particular #includes would be problematic enough to merit spending time finding was around them.  (Though if it is problematic, we could [strawman] address it by adding a layer of nsLayoutUtils::DoTheThingForMe(args) indirection, for example. But I don't think that's great either.)

::: layout/generic/nsFrame.cpp:95
(Diff revision 1)
> +#include "gfxPrefs.h"
>  #include "nsAbsoluteContainingBlock.h"

We need gfxPrefs.h for:
==
nsFrame.cpp:3107:31: error: use of undeclared identifier 'gfxPrefs'
 0:10.23   if (hasOverrideDirtyRect && gfxPrefs::LayoutDisplayListShowArea()) {
 0:10.23                               ^
==

Plus there's another call later on:
  } else if (modifiedFrames->Length() > gfxPrefs::LayoutRebuildFrameLimit()) {
https://dxr.mozilla.org/mozilla-central/rev/6d8f470b2579e7570f14e3db557264dc075dd654/layout/generic/nsFrame.cpp#1085

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8950422 [details]
Bug 1437623 part 2: (layout/base) Add missing includes/namespaces to preemptively fix unified bustage.

https://reviewboard.mozilla.org/r/219630/#review225682

::: layout/base/ServoRestyleManager.cpp:20
(Diff revision 1)
> +#include "nsAccessibilityService.h"
>  #include "nsBlockFrame.h"

Thanks! Indeed, looks like we have #ifdefs at other callsites. (presumably for --disable-accessibility builds)

Fixed locally.

Comment 10

[Daniel Holbert [:dholbert]]

RE your concern about adding the nsFrame.cpp headers -- if it helps, consider the fact that nsFrame.cpp effectively *already gets these headers in its compilation unit*, by virtue of unified builds.  If it didn't, we'd be hitting the build errors that I quoted even in our (default) unified build config.

So the fact that I'm typing those #includes into nsFrame.cpp isn't actually changing anything about its compilation unit -- it's not increasing the code-size that the compiler sees or anything.  It's just making the dependencies official.  And in a unified build, it'll have zero effect [since the header's #ifndef will trigger and skip the whole header].  The only effect it'll have is that someday it may save us from compile errors, when unified bundling gets reshuffled due to e.g. files being added/deleted].

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 8950421 [details]
Bug 1437623 part 1: (layout/generic) Add missing includes/namespaces to preemptively fix unified bustage.

https://reviewboard.mozilla.org/r/219628/#review225684

Sorry, I was mistakenly thinking that you added them to nsFrame.h, not nsFrame.cpp,
which is what alarmed me a bit.

Still, some of these seems a bit fishy to me.  I don't think the code in
GetWindowWidget() / nsIFrame::UpdateWidgetProperties() needs to live here
at all, for example.

Anyway, this is just making explicit the already existing dependencies so it's fine.

Comment 12

[Daniel Holbert [:dholbert]]

Ah, gotcha. Thanks for the review!

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8950421 [details]
Bug 1437623 part 1: (layout/generic) Add missing includes/namespaces to preemptively fix unified bustage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219628/diff/1-2/

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8950422 [details]
Bug 1437623 part 2: (layout/base) Add missing includes/namespaces to preemptively fix unified bustage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219630/diff/1-2/

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8950423 [details]
Bug 1437623 part 3: (layout/painting) Add missing includes/namespaces to preemptively fix unified bustage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219632/diff/1-2/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7b77328d11f
part 1: (layout/generic) Add missing includes/namespaces to preemptively fix unified bustage. r=mats
https://hg.mozilla.org/integration/autoland/rev/0085ad323bd6
part 2: (layout/base) Add missing includes/namespaces to preemptively fix unified bustage. r=mats
https://hg.mozilla.org/integration/autoland/rev/5acf2f6a2b5b
part 3: (layout/painting) Add missing includes/namespaces to preemptively fix unified bustage. r=mattwoodrow

Comment 17

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/c7b77328d11f
https://hg.mozilla.org/mozilla-central/rev/0085ad323bd6
https://hg.mozilla.org/mozilla-central/rev/5acf2f6a2b5b