Preemptively fix unified bustage in several layout subdirectories

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(3 attachments)

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.
Priority: -- → P3
Summary: Fix unified bustage in several layout subdirectories → Preemptively fix unified bustage in several layout subdirectories
Depends on: 1437625
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
Attachment #8950423 - Flags: review?(matt.woodrow) → review+
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 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
Attachment #8950422 - Flags: review?(mats) → review+
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 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
Flags: needinfo?(mats)
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.
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 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.
Attachment #8950421 - Flags: review?(mats) → review+
Flags: needinfo?(mats)
Ah, gotcha. Thanks for the review!
Pushed by dholbert@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.