985336 - Reduce unnecessary #includes in .h files within /layout/style

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

During downtime at the work week, I spun up a patch to drop some unnecessary #includes in layout/style (in some cases replacing them with a forward-declare or a more targeted #include).

I tested to make sure my patch doesn't remove any potentially-hidden-dependencies by creating a .cpp file layout/style/tmp.cpp, and added that to moz.build, and recompiling with that file *only* containing a single .h file. (for each .h file in this patch)

The command I used (after creating/adding tmp.cpp and qimporting my patch) was:

hg qstat | cut -f3 -d'/' > /tmp/files.txt
for x in `cat /tmp/files.txt`; do
  echo "*** TESTING $x ***" ;
  echo "#include \"$x\"" > layout/style/tmp.cpp && make -s -C ../obj/layout/;
done

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

did you test with unified builds disabled?

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

er, I guess the above does so

Comment 4

[Daniel Holbert [:dholbert]]

Yeah, I added tmp.cpp to SOURCES in moz.build, not UNIFIED_SOURCES.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: patch to add tmp.cpp (for testing this)

(for the record, here's the patch to add tmp.cpp to moz.build and create the empty tmp.cpp file)

(Reposting the actual patch in a minute. Previous version included a tweak to tmp.cpp, which I'm removing now, and also adding one more include-removal.)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v2

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8393381 [details] [diff] [review]
fix v2

>+++ b/layout/style/nsDOMCSSDeclaration.h
> #include "nsICSSDeclaration.h"
> 
> #include "mozilla/Attributes.h"
>+#include "nsIURI.h"

NOTE: In this file, I'm *adding* an include. This file was previously depending on getting that #include indirectly, via nsICSSDeclaration.h, and I'm removing it from nsICSSDeclaration.h, so I have to add it here.


* (We need the include here because this file has nsCOMPtr<nsIURI>, which doesn't compile unless it knows that nsIURI inherits from nsISupports, which it can only tell by seeing the header file)

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8393381 [details] [diff] [review]
fix v2

r=dbaron.  Worth a try run or a test on one of the builds that does non-unified builds before landing.

Comment 9

[Daniel Holbert [:dholbert]]

Thanks! All-builds try run, for good measure: https://tbpl.mozilla.org/?tree=Try&rev=7d877f1798ec

Comment 10

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2c462515397

Comment 11

[Daniel Holbert [:dholbert]]

(FWIW, I re-ran the command at the end of comment 0, to be sure that all the .h files touched by the patch end up being compilable on their own.)

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d2c462515397