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

RESOLVED FIXED in Firefox 60

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: zjz, Assigned: zjz)

Tracking

58 Branch
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

a year ago
I am going to upload the patch for a while
(Assignee)

Updated

a year ago
Summary: Remove several #include directives referring to unused headers in table/layout files → Remove several #include directives referring to unused headers in layout files
Comment hidden (mozreview-request)
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.
Attachment #8952698 - Flags: review?(bugs) → review+
(Assignee)

Comment 4

a year ago
(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.
(Assignee)

Updated

a year ago
Keywords: checkin-needed
The review board shows the patch is only r?, it has to be r+ for landing.
Flags: needinfo?(bugs)
Keywords: checkin-needed
(Assignee)

Comment 7

a year ago
(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
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Comment 8

a year ago
(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.
Component: Untriaged → Layout
Product: Firefox → Core

Comment 9

a year ago
mozreview-review
Comment on attachment 8952698 [details]
Bug 1439882 - Removes several redundant #include directives in layout files

https://reviewboard.mozilla.org/r/221926/#review230100
Thanks for pushing on this. LGTM. r+
Assignee: nobody → zjz
Flags: needinfo?(bugs)
(Assignee)

Comment 11

a year ago
mozreview-review-reply
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+.
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.
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
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

a year ago
mozreview-review
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.)
Flags: needinfo?(zjz)
(Assignee)

Comment 16

a year ago
(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!
Flags: needinfo?(zjz)
(Assignee)

Updated

a year ago
Status: UNCONFIRMED → RESOLVED
Last Resolved: a year ago
Resolution: --- → WONTFIX
(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 hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8952698 - Attachment is obsolete: true
(Assignee)

Comment 19

a year ago
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.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---

Comment 20

a year ago
mozreview-review
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.
Attachment #8955285 - Flags: review?(dholbert) → review+

Comment 21

a year ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a1af2d03c54
Removes several redundant #include directives in layout files r=dholbert

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7a1af2d03c54
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.