Closed Bug 1336223 Opened 7 years ago Closed 7 years ago

Recent stylo changes were missing a bunch of includes (e.g. in PreloadedStyleSheet.cpp/h)

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(3 files)

Bug 1336223 part 1: Add some missing #includes to PreloadedStyleSheet.cpp and .h.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1336223 part 2: Add some missing #includes & namespaces to nsMediaList.cpp.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1336223 part 3: Add a missing #include to nsComputedDOMStyle.cpp.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
Just tried building in layout/style in non-unified mode (i.e. changing UNIFIED_SOURCES to SOURCES in its moz.build), and I hit a bunch of bustage in PreloadedStyleSheet.cpp/h which were added recently.

Filing this bug on fixing that. (Otherwise it's just a timebomb waiting to break the build when somebody adds a new file & reshuffles the unification.)
Summary: PreloadedStyleSheet.cpp/h are missing a bunch of includes/forward-declares → PreloadedStyleSheet.cpp/h are missing a bunch of includes
Explanation of why we need each of these new includes:

For the .h file:
================
#include "mozilla/StyleBackendType.h"
  - This type is used directly in this file (in function signatures).

#include "mozilla/StyleSheet.h"
  - This type is used as RefPtr<StyleSheet> in this file (which requires the type to be defined, so that the compiler can see that it has AddRef/Release methods when it expands the RefPtr definition).

#include "nsCOMPtr.h"
  - This type is used in this file.

#include "nsCycleCollectionParticipant.h"
  - to provide a macro used in this file (NS_DECL_CYCLE_COLLECTION_CLASS)


For the .cpp file:
===================
#include "mozilla/css/Loader.h"
#include "nsLayoutUtils.h"
 - Both of these ^^ types are used (by name) in this file -- "new css::Loader()", and nsLayoutUtils::Styloenabled().
Actually, I'm broadening the scope here a bit, to cover some fallout from other new Stylo code (e.g. bug 1290209 in nsMediaList.cpp which is also missing #includes)

2 more patches coming up (after which point layout/style builds nicely again, without being unified)
Blocks: 1290209
Summary: PreloadedStyleSheet.cpp/h are missing a bunch of includes → Recent stylo changes were missing a bunch of includes (e.g. in PreloadedStyleSheet.cpp/h)
(I'll tag xidorn to review, rather than heycam, since it looks like heycam's got quite a review queue at the moment.)
If you're curious, see comment 3 for an explanation of Part 1.

Part 2 explanation, for nsMediaList.cpp:

#include "nsCSSRules.h"
 - This provides the DocumentRule type, which is used in this file.

#include "nsMediaFeatures.h"
 - This provides the nsMediaFeature type, which is used in this file.

#include "nsRuleNode.h"
 - This provides nsRuleNode::CalcLengthWithInitialFont(...), used in this file.

And "using namespace mozilla" is there to make all the existing "css::DocumentRule" usage unambiguous. (Otherwise it complains that css::DocumentRule is not a type)

And I added one "dom::" prefix to a type, too (which must've been pulling a using namespace mozilla::dom from another file).  We could add using namespace mozilla::dom here, but... *shrug*. It's only for this one line, and we seem to be naming sub-namespaces explicitly elsewhere in this file. So, meh.

Part 3 explanation, for nsComputedDOMStyle.cpp:
#include "mozilla/RestyleManager.h"
 - This is to provide the RestyleManager definition so that we can call e.g. aPresContext->RestyleManager()->AsGecko() further down in the file, and so we can call methods on mRestyleManager.
Comment on attachment 8833084 [details]
Bug 1336223 part 1: Add some missing #includes to PreloadedStyleSheet.cpp and .h.

https://reviewboard.mozilla.org/r/109306/#review110420

::: layout/style/PreloadedStyleSheet.h:18
(Diff revision 3)
>  #include "mozilla/Result.h"
> +#include "mozilla/StyleBackendType.h"
> +#include "mozilla/StyleSheet.h"
> +#include "nsCOMPtr.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsIURI.h"

This can really be replaced with a `class nsIURI;` forward-declaration. But I guess it's fine.
Attachment #8833084 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8833093 [details]
Bug 1336223 part 2: Add some missing #includes & namespaces to nsMediaList.cpp.

https://reviewboard.mozilla.org/r/109316/#review110422
Attachment #8833093 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8833094 [details]
Bug 1336223 part 3: Add a missing #include to nsComputedDOMStyle.cpp.

https://reviewboard.mozilla.org/r/109318/#review110424
Attachment #8833094 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8833084 [details]
Bug 1336223 part 1: Add some missing #includes to PreloadedStyleSheet.cpp and .h.

https://reviewboard.mozilla.org/r/109306/#review110434

::: layout/style/PreloadedStyleSheet.h:18
(Diff revision 3)
>  #include "mozilla/Result.h"
> +#include "mozilla/StyleBackendType.h"
> +#include "mozilla/StyleSheet.h"
> +#include "nsCOMPtr.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsIURI.h"

It seems you're right!

I was thinking that we needed the full nsIURI class-definition, in order for the nsCOMPtr<nsIURI> further down in this file to compile nicely.  But it seems a forward-decl really is sufficient.

(For nsRefPtr, we really do need the class definition IIRC, but there must be something different about nsCOMPtr that makes it more forward-decl-tolerant.)

So: I'll take this suggestion before landing. Thanks for the review!
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/48a581014c8a
part 1: Add some missing #includes to PreloadedStyleSheet.cpp and .h. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/ecfa3ac80589
part 2: Add some missing #includes & namespaces to nsMediaList.cpp. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/e1a2f0216e03
part 3: Add a missing #include to nsComputedDOMStyle.cpp. r=xidorn
(In reply to Daniel Holbert [:dholbert] from comment #13)
> Comment on attachment 8833084 [details]
> (For nsRefPtr, we really do need the class definition IIRC, but there must
> be something different about nsCOMPtr that makes it more
> forward-decl-tolerant.)

A variable declaration with type of RefPtr doesn't need the definition of the inner class either. It needs that if you do any refcount manipulation, though.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #18)
> A variable declaration with type of RefPtr doesn't need the definition of
> the inner class either. It needs that if you do any refcount manipulation,
> though.

Huh! I could've sworn that I've observed the opposite before. Maybe I'm just recalling cases where the refcount was being subtly manipulated (e.g. via a nsRefPtr variable that goes out of scope)
Indeed -- RefPtr seems fine with a forward-decl, as you say.  So I technically could've used a forward-decl for the "StyleSheet" header in Part 1 as well. (just checked)

I'd push a followup, but autoland won't let me.  I won't worry about it; likely any caller that wants PreloadedStyleSheet.h will also want StyleSheet.h, so the technically-unnecessary header isn't a huge deal here.

Thanks for correcting my misconception!
Now that this has merged around to inbound & followups are easy to push, I pushed a followup to address the extra thing I noted in comment 20 (using a fwd-decl rather than an include for StyleSheet.h):
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa756da692bbb5bbf8fd8a290749725d7364db7f
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: