1439882 - Remove several #include directives referring to unused headers in layout files

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[張俊芝(ZHANG Junzhi)]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180123231643

Comment 1

[張俊芝(ZHANG Junzhi)]

I am going to upload the patch for a while

Comment 2

[張俊芝(ZHANG Junzhi)]

Attachment: Bug 1439882 - Removes several redundant #include directives in layout files

Some included headers for source code in layout directory are left unused. This
patch merely removes these redundant headers. All of these headers are still
found in use for other code, so all of them and their related cpp files are kept
still.

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

Comment 3

[Jet Villegas (inactive)]

Comment on attachment 8952698 [details]
Bug 1439882 - Removes several redundant #include directives in layout files

LGTM assuming you've got a green Try build across all the supported platforms.

Comment 4

[張俊芝(ZHANG Junzhi)]

(In reply to Jet Villegas (:jet) from comment #3)
> Comment on attachment 8952698 [details]
> Bug 1439882 - Removes several redundant #include directives in layout files
> 
> LGTM assuming you've got a green Try build across all the supported
> platforms.

I don't think I have an access to try servers, because I am a beginner. Could you do me a favour? Thanks.

Comment 5

[Adrian Wielgosik [:adrian17]]

Build-only Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=edb7731dabd7b163fb90aea7b541e195f8df69be

Comment 6

[Cristina Coroiu [:ccoroiu]]

The review board shows the patch is only r?, it has to be r+ for landing.

Comment 7

[張俊芝(ZHANG Junzhi)]

(In reply to Jet Villegas (:jet) from comment #3)
> Comment on attachment 8952698 [details]
> Bug 1439882 - Removes several redundant #include directives in layout files
> 
> LGTM assuming you've got a green Try build across all the supported
> platforms.

Could you give it a r+? Thanks. It has already passed try builds

Comment 8

[張俊芝(ZHANG Junzhi)]

(In reply to Jet Villegas (:jet) from comment #3)
> Comment on attachment 8952698 [details]
> Bug 1439882 - Removes several redundant #include directives in layout files
> 
> LGTM assuming you've got a green Try build across all the supported
> platforms.

The attachment has been given a r+, but the mozreview request is still in r?. So it can be checked in. Please give it a r+, thank you.

Comment 9

[Jet Villegas (inactive)]

Comment on attachment 8952698 [details]
Bug 1439882 - Removes several redundant #include directives in layout files

https://reviewboard.mozilla.org/r/221926/#review230100

Comment 10

[Jet Villegas (inactive)]

Thanks for pushing on this. LGTM. r+

Comment 11

[張俊芝(ZHANG Junzhi)]

Comment on attachment 8952698 [details]
Bug 1439882 - Removes several redundant #include directives in layout files

https://reviewboard.mozilla.org/r/221926/#review230100

Thank you. But I don't know why this review request is still in r? while the attachment is in r+.

Comment 12

[Daniel Holbert [:dholbert]]

I'd guess that's because MozReview only trusts r+ from people who have L3 commit access, and Jet's commit access might've auto-expired since he doesn't write very much code.

Anyway -- having said this, the fact that this compiles doesn't actually mean it's correct, and I ran into at least one issue with it.  Specifically, we "cheat" at building right now by concatenating huge bunches of C++ files together, which we call "Unified Builds". And this can hide the fact that a .cpp file is dependent on a given .h file, because it benefits from some other concatenated .cpp file's #include of the same header.  So, it might look like you're safe to remove these headers, but really the cpp file might use something from them and we just "get lucky" and pull in the header from another file by virtue of concatenation.

I try to do periodic waves to detect this sort of problem and clean it up, e.g. bug 1437623.  The only way to be sure that a patch like this is really fine is to:
 (1) Open the affected .cpp's moz.build file, and edit that file so that the .cpp file is in "SOURCES" rather than "UNIFIED_SOURCES". (The quick-and-easy way to do this is: just drop the "UNIFIED_" prefix from UNIFIED_SOURCES in that moz.build file. This makes *all* of the .cpp files build in non-unified mode.)

 (2) Try to build. Hopefully you don't hit problems. If you do, fix them (like I did in bug 1437623).
 (3) After you've got the file building in non-unified mode, *then* remove the header, and see if you can still build.

I just tried this for nsContainerFrame.cpp (fortunately (2) was trivial since this directory is still fine from bug 1437623), and unfortunately it looks like this patch introduces the following error:
====
 0:08.32 ../../../mozilla/layout/generic/nsContainerFrame.cpp: In member function ‘void nsContainerFrame::DestroyAbsoluteFrames(nsIFrame*, nsIFrame::PostDestroyData&)’:
 0:08.32 ../../../mozilla/layout/generic/nsContainerFrame.cpp:191:33: error: invalid use of incomplete type ‘class nsAbsoluteContainingBlock’
 0:08.32      GetAbsoluteContainingBlock()->DestroyFrames(this, aDestructRoot, aPostDestroyData);
 0:08.32                                  ^~
 0:08.32 In file included from ../../../mozilla/layout/xul/nsBox.h:11:0,
 0:08.32                  from ../../../mozilla/layout/generic/nsFrame.h:15,
 0:08.32                  from ../../../mozilla/layout/generic/nsSplittableFrame.h:16,
 0:08.32                  from ../../../mozilla/layout/generic/nsContainerFrame.h:13,
 0:08.32                  from ../../../mozilla/layout/generic/nsContainerFrame.cpp:9:
 0:08.32 ../../../mozilla/layout/generic/nsIFrame.h:88:7: note: forward declaration of ‘class nsAbsoluteContainingBlock’
 0:08.32  class nsAbsoluteContainingBlock;
 0:08.32        ^~~~~~~~~~~~~~~~~~~~~~~~~
 0:08.32 ../../../mozilla/layout/generic/nsContainerFrame.cpp:191:35: error: invalid use of incomplete type ‘class nsAbsoluteContainingBlock’
 0:08.33      GetAbsoluteContainingBlock()->DestroyFrames(this, aDestructRoot, aPostDestroyData);
 0:08.33                                    ^~~~~~~~~~~~~
 0:08.33 In file included from ../../../mozilla/layout/xul/nsBox.h:11:0,
 0:08.33                  from ../../../mozilla/layout/generic/nsFrame.h:15,
 0:08.33                  from ../../../mozilla/layout/generic/nsSplittableFrame.h:16,
 0:08.33                  from ../../../mozilla/layout/generic/nsContainerFrame.h:13,
 0:08.33                  from ../../../mozilla/layout/generic/nsContainerFrame.cpp:9:
 0:08.33 ../../../mozilla/layout/generic/nsIFrame.h:88:7: note: forward declaration of ‘class nsAbsoluteContainingBlock’
 0:08.33  class nsAbsoluteContainingBlock;
 0:08.33        ^~~~~~~~~~~~~~~~~~~~~~~~~
 0:08.33 /scratch/work/builds/mozilla-inbound/mozilla/config/rules.mk:1047: recipe for target 'nsContainerFrame.o' failed
 0:08.33 make[1]: *** [nsContainerFrame.o] Error 1
 0:08.33 make[1]: *** Waiting for unfinished jobs....

====

So you can't remove nsAbsoluteContainingBlock.h after all here.

Comment 13

[Daniel Holbert [:dholbert]]

Oh, also useful to know for doing stuff like the process described in comment 12 -- you can selectively rebuild *just* in the directory that you're tweaking, by doing e.g.:
  ./mach build layout/generic
  ./mach build layout/tables

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: sample patch to disable unified builds for just the .cpp files you're working with here

For illustration / testing purposes, here's a patch you could use to test the process that I outlined above.

In my local build, this patch (just on its own) causes build failures in layout/tables, if I do "./mach build layout/tables -j100".    But all of the build errors seem to be for Unified_cpp_layout_tables0.cpp (the unified .cpp files).  So I suspect that code is depending on things from nsTableCellFrame.cpp, and nsTableCellFrame.cpp itself is presumably fine (because no errors are reported for it, and I'm pretty sure we're getting to it because "-j100" should make us spawn a compile job for it.)

And with your patch applied, I still don't see any build errors for nsTableCellFrame.cpp. So I think that means it's probably fine. (It'd be nice to clean up layout/tables so that it all builds correctly in non-unified mode, if you're looking for a good next bug -- but I don't think that's necessary just to get this fix landed, given that it seems likely the file you're touching is fine on its own.)

So I think your patch is fine, as long as you add back the nsAbsoluteContainingBlock include in nsContainerFrame.cpp. (You can see that that's required if you test with your patch and my patch both applied, and you build in layout/generic.)

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8952698 [details]
Bug 1439882 - Removes several redundant #include directives in layout files

https://reviewboard.mozilla.org/r/221926/#review230164

::: layout/generic/nsContainerFrame.cpp
(Diff revision 1)
>  
>  #include "nsContainerFrame.h"
>  
> -#include "mozilla/dom/HTMLDetailsElement.h"
>  #include "mozilla/dom/HTMLSummaryElement.h"
> -#include "nsAbsoluteContainingBlock.h"

(so this is the one line you need to restore, and then this is good I think.)

Comment 16

[張俊芝(ZHANG Junzhi)]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> I'd guess that's because MozReview only trusts r+ from people who have L3
> commit access, and Jet's commit access might've auto-expired since he
> doesn't write very much code.
> 
> Anyway -- having said this, the fact that this compiles doesn't actually
> mean it's correct, and I ran into at least one issue with it.  Specifically,
> we "cheat" at building right now by concatenating huge bunches of C++ files
> together, which we call "Unified Builds". And this can hide the fact that a
> .cpp file is dependent on a given .h file, because it benefits from some
> other concatenated .cpp file's #include of the same header.  So, it might
> look like you're safe to remove these headers, but really the cpp file might
> use something from them and we just "get lucky" and pull in the header from
> another file by virtue of concatenation.
>
Thank your for your meticulous analysis, and your tip, Daniel. They are very helpful.

It's good to know that source files are concatenated into bunches at building time, which I wasn't aware of. From reading your comments(along with bug 1437623) I believe I understand the right way to fix this kind of clean-up bug, as well as finding the missing dependencies hidden in our daily unified compilation.

Right now I am going to work on some specific bugs, but I am alway interested in taking care of this kind of issue along the way(Actually, this bug was filed when I was writing code for bug 1259168). I will keep track of them, and file a bug when necessary or when they get to a certain amount.

Thank you!

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Zhang Junzhi from comment #16)
> Thank your for your meticulous analysis, and your tip, Daniel. They are very
> helpful.

sure!
 
> It's good to know that source files are concatenated into bunches at
> building time, which I wasn't aware of.

It's a pretty weird thing for us to do, but it apparently gives us a huge speedup in build times, which is nice. :)

Also, as noted in comment 15: if you like, this patch is probably still fine to land, as long as the #include for nsAbsoluteContainingBlock.h is added back.  But also no pressure.

Comment 18

[張俊芝(ZHANG Junzhi)]

Attachment: Bug 1439882 - Removes several redundant #include directives in layout files

Some included headers for source code in layout directory are left unused. This
patch merely removes these redundant headers. All of these headers are still
found in use for other code, so all of them and their related cpp files are kept
still.

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

Comment 19

[張俊芝(ZHANG Junzhi)]

Comment on attachment 8955285 [details]
Bug 1439882 - Removes several redundant #include directives in layout files

Okay. In order to double-check it, I updated the source to the newest revision in mozilla-central, and had the patch rebased on it and then tried ./mach build, and also tried ./mach build layout/generic, ./mach build layout/tables with your "UNIFIED_"-dropping method. It all compiles.

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8955285 [details]
Bug 1439882 - Removes several redundant #include directives in layout files

https://reviewboard.mozilla.org/r/224442/#review230420

Thanks! This seems fine -- I'll r+ and trigger autoland.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a1af2d03c54
Removes several redundant #include directives in layout files r=dholbert

Comment 22

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7a1af2d03c54