Closed Bug 1438020 Opened 6 years ago Closed 6 years ago

Add missing #includes for "***Inlines.h" headers, in several layout subdirectories

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(2 files, 1 obsolete file)

warnings to be fixed here
20.88 KB, text/plain
Details
Bug 1438020: Add some includes for "Inlines" headers to address clang build warnings in non-unified build.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details 
As a followup to bug 1437623:

After bug 1437623's & bug 1437723's fixes for hidden-by-unified build errors in layout/generic, we're still left with a few build *warnings* for inline functions whose definitions are missing.

These functions all live in "***Inlines.h" files and we just need to include this "Inlines" header in the relevant .cpp client file.  Filing this bug on fixing these.
For reference during review, here are the build warnings that I'm fixing here.
Comment on attachment 8950754 [details]
Bug 1438020: Add some includes for "Inlines" headers to address clang build warnings in non-unified build.

https://reviewboard.mozilla.org/r/219996/#review225816

Sorry, just realized this isn't quite complete -- posting an updated version shortly.
Comment on attachment 8950754 [details]
Bug 1438020: Add some includes for "Inlines" headers to address clang build warnings in non-unified build.

https://reviewboard.mozilla.org/r/219996/#review225820
Attachment #8950754 - Flags: review?(mats) → review+
Comment on attachment 8950754 [details]
Bug 1438020: Add some includes for "Inlines" headers to address clang build warnings in non-unified build.

https://reviewboard.mozilla.org/r/219994/#review225822

::: layout/style/StyleSheet.cpp:9
(Diff revision 2)
> +#include "nsStyleContext.h"
> +#include "nsStyleContextInlines.h"
>  #include "mozilla/css/GroupRule.h"
>  #include "mozilla/dom/CSSImportRule.h"
>  #include "mozilla/dom/CSSRuleList.h"
>  #include "mozilla/dom/Element.h"
>  #include "mozilla/dom/MediaList.h"
>  #include "mozilla/dom/ShadowRoot.h"
>  #include "mozilla/dom/ShadowRootBinding.h"
> +#include "mozilla/GeckoStyleContext.h"
>  #include "mozilla/ServoCSSRuleList.h"
> +#include "mozilla/ServoStyleContext.h"
>  #include "mozilla/ServoStyleSet.h"
>  #include "mozilla/ServoStyleSheet.h"
>  #include "mozilla/StyleSheetInlines.h"
> +#include "mozilla/StyleSetHandleInlines.h"

Note: this file requires particularly invasive changes, due to side effects of adding a new include.

(1) I'm adding the last include here "StyleSetHandleInlines" to fix the proximal issue noted in my attached warning-log ("inline function 'mozilla::StyleSetHandle::Ptr::RuleChanged' is not define").

However, on its own, this change causes build *errors* due to StyleSetHandleInlines.h having some casts between {Gecko,Servo}StyleContext* and nsStyleContext* -- it needs the definitions of those types in order to know that this cast is valid. Hence:
(2) we need to also include the nsStyleContext.h and {Gecko,Servo}StyleContext.h headers.

And once we've done that...
(3) we need to *also* include nsStyleContextHeaders.h
... or else we introduce a build warning for GeckoStyleContext.h calling context->AsGecko() without that inline function being defined.

So, that's why there are so many new #includes here. Anyway, these are all (necessarily) already being included in our main builds via other .cpp files that get unified with this one (otherwise we'd have compile failures on trunk), so this is just formalizing the already-existing situation.
(Thanks for the review!)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
[ni=me to trigger landing after autoland has reopened & supporting bugs (bug 1437623 & bug 1437723) have landed & stuck]
Flags: needinfo?(dholbert)
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ab50bf89a3ac
Add some includes for "Inlines" headers to address clang build warnings in non-unified build. r=mats
Flags: needinfo?(dholbert)
https://hg.mozilla.org/mozilla-central/rev/ab50bf89a3ac
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(In reply to Daniel Holbert [:dholbert] from comment #6)
> ::: layout/style/StyleSheet.cpp:9
> However, on its own, this change causes build *errors* due to
> StyleSetHandleInlines.h having some casts between {Gecko,Servo}StyleContext*
> and nsStyleContext* -- it needs the definitions of those types in order to
> know that this cast is valid. Hence:
> (2) we need to also include the nsStyleContext.h and
> {Gecko,Servo}StyleContext.h headers.

In that case, shouldn't you be adding {Gecko,Servo}StyleContext.h to StyleSetHandleInlines.h instead, so that other files which try to include that file don't need to include all them again?

> And once we've done that...
> (3) we need to *also* include nsStyleContextHeaders.h
> ... or else we introduce a build warning for GeckoStyleContext.h calling
> context->AsGecko() without that inline function being defined.

Again, shouldn't this be added to GeckoStyleContext.h, then?
(In reply to Daniel Holbert [:dholbert] from comment #6)
> And once we've done that...
> (3) we need to *also* include nsStyleContextHeaders.h
> ... or else we introduce a build warning for GeckoStyleContext.h calling
> context->AsGecko() without that inline function being defined.

And, this is also not true. You only need nsStyleContext.h, which defined AsGecko() already.
... and GeckoStyleContext.h has been including nsStyleContext.h since long ago...

I don't know what is GCC's warning complaining about, but that sounds wrong.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #12)
> In that case, shouldn't you be adding {Gecko,Servo}StyleContext.h to
> StyleSetHandleInlines.h instead, so that other files which try to include
> that file don't need to include all them again?

Probably yes.

It can be more controversial (& can cause further bustage) to add new #includes to .h files, though, since there can be second/third-order effects (via headers including other headers, and entirely unrelated .cpp files including those headers).

I opted for the simpler & more direct solution of adding #includes directly to the .cpp file that was having problems, to fix the proximal issue that I was running into.  But yeah, it'd probably be ideal to fix up the Inlines file to include what it needs.

> > And once we've done that...
> > (3) we need to *also* include nsStyleContextHeaders.h
> > ... or else we introduce a build warning for GeckoStyleContext.h calling
> > context->AsGecko() without that inline function being defined.
> 
> Again, shouldn't this be added to GeckoStyleContext.h, then?

(oops, I meant to say nsStyleContextInlines.h here, not nsStyleContextHeaders.h)

I don't feel great #including "Inlines" files in other headers (since the whole point of having the "Inlines" file in the first place is to get the definitions *out* of header files, to avoid include-dependency-hell etc.)  Hence, I resolved this by adding an #include to the .cpp file rather than to the technically-dependent .h file.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #13)
> (In reply to Daniel Holbert [:dholbert] from comment #6)
> And, this is also not true. You only need nsStyleContext.h, which defined
> AsGecko() already.

I don't think so --  nsStyleContext.h isn't sufficient.

In current mozilla-central, if I disable unified builds in layout/style and remove the nsStyleContextInlines.h include from StyleSheet.cpp (and keep the nsStyleContext.h include), I get this build warning output:
 0:04.38 In file included from ../../../mozilla/layout/style/StyleSheet.cpp:9:
 0:04.38 ../../../mozilla/layout/style/nsStyleContext.h:66:3: warning: inline function 'nsStyleContext::AsGecko' is not defined [-Wundefined-inline]
 0:04.38   MOZ_DECL_STYLO_CONVERT_METHODS(mozilla::GeckoStyleContext, mozilla::ServoStyleContext);
 0:04.38   ^
 0:04.38 ../../dist/include/mozilla/ServoUtils.h:67:3: note: expanded from macro 'MOZ_DECL_STYLO_CONVERT_METHODS'
 0:04.38   MOZ_DECL_STYLO_CONVERT_METHODS_GECKO(geckotype_)
 0:04.38   ^
 0:04.38 ../../dist/include/mozilla/ServoUtils.h:59:22: note: expanded from macro 'MOZ_DECL_STYLO_CONVERT_METHODS_GECKO'
 0:04.38   inline geckotype_* AsGecko();                         \
 0:04.38                      ^
 0:04.38 ../../dist/include/mozilla/GeckoStyleContext.h:22:57: note: used here
 0:04.38     return already_AddRefed<GeckoStyleContext>(context->AsGecko());
 0:04.38                                                         ^
 0:04.38 In file included from ../../../mozilla/layout/style/StyleSheet.cpp:9:
 0:04.38 ../../../mozilla/layout/style/nsStyleContext.h:66:3: warning: inline function 'nsStyleContext::AsServo' is not defined [-Wundefined-inline]
 0:04.38   MOZ_DECL_STYLO_CONVERT_METHODS(mozilla::GeckoStyleContext, mozilla::ServoStyleContext);
 0:04.38   ^
 0:04.39 ../../dist/include/mozilla/ServoUtils.h:66:3: note: expanded from macro 'MOZ_DECL_STYLO_CONVERT_METHODS'
 0:04.39   MOZ_DECL_STYLO_CONVERT_METHODS_SERVO(servotype_) \
 0:04.39   ^
 0:04.39 ../../dist/include/mozilla/ServoUtils.h:53:22: note: expanded from macro 'MOZ_DECL_STYLO_CONVERT_METHODS_SERVO'
 0:04.39   inline servotype_* AsServo();                         \
 0:04.39                      ^
 0:04.39 ../../dist/include/mozilla/StyleSetHandleInlines.h:114:3: note: used here
 0:04.39   FORWARD_WITH_PARENT(ResolveStyleFor, aParentContext, (aElement, parent, aMayCompute));
 0:04.39   ^
 0:04.39 ../../dist/include/mozilla/StyleSetHandleInlines.h:31:39: note: expanded from macro 'FORWARD_WITH_PARENT'
 0:04.39     auto* parent = parent_ ? parent_->AsServo() : nullptr; \
 0:04.39                                       ^
 0:04.39 2 warnings generated.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #14)
> I don't know what is GCC's warning complaining about, but that sounds wrong.

*shrug*  This is clang, too, BTW.
BTW: if you like, you can play with this locally by applying attachment 8952879 [details] [diff] [review] (which disables unified builds to the extent that we can right now in current mozilla-central/layout/style -- it leaves two files unified to avoid triggering bug 1437727).

That's what I used to generate the warning output in comment 15.
Attached patch possible followup patch (obsolete) — — Splinter Review 
It looks like we're OK taking your first suggestion -- adding {Gecko,Servo}StyleContext.h to StyleSetHandleInlines.h instead of adding them to StyleSheet.cpp.

Here's a patch that I just tested locally (on top of attachment 8952879 [details] [diff] [review]) to verify that that was fine.

(As noted above, though, we do still need the nsStyleContextInlines.h header to avoid introducing build warnings. I tried removing it again after this patch, just to be sure, and the warnings did indeed crop up.  Like you said at the end of comment 12, that header should arguably be included from GeckoStyleContext.h since that's where we need it -- but I feel conflicted about adding Inlines includes to headers, as I noted above.)
Attachment #8952898 - Flags: feedback?(xidorn+moz)
Comment on attachment 8952898 [details] [diff] [review]
possible followup patch

Review of attachment 8952898 [details] [diff] [review]:
-----------------------------------------------------------------

I'm doing similar stuff in bug 1440141.
Attachment #8952898 - Flags: feedback?(xidorn+moz)
Depends on: 1440141
Cool, I suspected.  (Was just posting that patch for illustration/discussion purposes anyway.)
Attachment #8952898 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: