Comment on attachment 8735646 [details] [diff] [review] Part 1 - Generalize nsStyleContext to support resolving styles from either nsRuleNode or ServoComputedValues. v1 Review of attachment 8735646 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsStyleContext.cpp @@ +70,5 @@ > // Whether to perform expensive assertions in the nsStyleContext destructor. > static bool sExpensiveStyleStructAssertionsEnabled; > #endif > > +nsStyleContext::nsStyleContext(nsStyleContext* aParent, OwningStyleContextSource&& aSource, aSource on a new line, to keep to 80 columns. @@ +94,5 @@ > + nsIAtom* aPseudoTag, > + CSSPseudoElementType aPseudoType, > + already_AddRefed<nsRuleNode> aRuleNode, > + bool aSkipParentDisplayBasedStyleFixup) > + : nsStyleContext(aParent, OwningStyleContextSource(Move(aRuleNode)), aPseudoTag, aPseudoType) 80 columns @@ +123,5 @@ > + nsIAtom* aPseudoTag, > + CSSPseudoElementType aPseudoType, > + already_AddRefed<ServoComputedValues> aComputedValues, > + bool aSkipParentDisplayBasedStyleFixup) > + : nsStyleContext(aParent, OwningStyleContextSource(Move(aComputedValues)), aPseudoTag, aPseudoType) 80 columns @@ +128,5 @@ > +{ > +#ifdef MOZ_STYLO > + mPresContext = aPresContext; > +#endif > +} Should we be calling FinishConstruction in here? @@ +130,5 @@ > + mPresContext = aPresContext; > +#endif > +} > + > +void nsStyleContext::FinishConstruction(bool aSkipParentDisplayBasedStyleFixup) Newline after "void". @@ +183,5 @@ > } > #endif > > + nsPresContext *presContext = PresContext(); > + nsStyleSet* geckoStyleSet = presContext->PresShell()->StyleSet()->AsGecko(); GetAsGecko? I'm not really sure why you made the changes to this bit of moved code. Also you are indented by one space too many. @@ +427,5 @@ > { > const void* cachedData = GetCachedStyleData(aSID); > if (cachedData) > return cachedData; // We have computed data stored on this node in the context tree. > // Our rule node will take care of it for us. Maybe s/rule node/style source/? @@ +1443,4 @@ > } > if (!aNewContext->mCachedResetData) { > aNewContext->mCachedResetData = > + new (PresContext()) nsResetStyleData; Probably fits on one line now. ::: layout/style/nsStyleContext.h @@ +26,5 @@ > +// root of the rule tree, and the most specific rule matched is the > +// |mRule| member of the rule node. > +// > +// In the Servo case, we hold an atomically-refcounted handle to a > +// Servo ComputedValue struct, which is more or less the Servo equivalent ComputedValues @@ +29,5 @@ > +// In the Servo case, we hold an atomically-refcounted handle to a > +// Servo ComputedValue struct, which is more or less the Servo equivalent > +// of an nsStyleContext. > + > +// Underling pointer without any strong ownership semantics. Underlying @@ +30,5 @@ > +// Servo ComputedValue struct, which is more or less the Servo equivalent > +// of an nsStyleContext. > + > +// Underling pointer without any strong ownership semantics. > +struct NonOwningStyleContextSource { { on new line. I think this and OwningStyleContextSource should be in a separate StyleContextSource.h header. (If they do stay in this file, please add them below the CSSPseudoElementType forward declaration.) @@ +34,5 @@ > +struct NonOwningStyleContextSource { > + MOZ_IMPLICIT NonOwningStyleContextSource(nsRuleNode* aRuleNode) > + : mBits(reinterpret_cast<uintptr_t>(aRuleNode)) {} > + explicit NonOwningStyleContextSource(ServoComputedValues* aComputedValues) > + : mBits(reinterpret_cast<uintptr_t>(aComputedValues) | 1) {} As mentioned on IRC, it would be nice if we could just use one of the nsStyleContext::mBits to tag the style source, but I guess that's difficult without giving up on OwningStyleContextSource's automatic refcounting. @@ +41,5 @@ > + MOZ_ASSERT(IsServoComputedValues() == aOther.IsServoComputedValues(), > + "Comparing Servo to Gecko - probably a bug"); > + return mBits == aOther.mBits; > + } > + bool operator!=(const NonOwningStyleContextSource& aOther) const { return !(*this == aOther); } 80 columns @@ +46,5 @@ > + > + // We intentionally avoid exposing IsGeckoRuleNode() here, because that would > + // encourage callers to do: > + // > + // if (source.IsGecko()) { IsGeckoRuleNode? @@ +62,5 @@ > + return false; > +#endif > + } > + > + nsRuleNode* AsGeckoRuleNode() const { Should we be having const and non-const methods? const nsRuleNode* AsGeckoRuleNode() const { ... } nsRuleNode* AsGeckoRuleNode() { ... } @@ +76,5 @@ > + bool MatchesNoRules() const { > + if (IsGeckoRuleNodeOrNull()) { > + return AsGeckoRuleNode()->IsRoot(); > + } else { > + MOZ_CRASH("Not implemented"); MOZ_CRASH("stylo: not implemented") just for consistency (and searchability) with other similar crashes we've added. @@ +80,5 @@ > + MOZ_CRASH("Not implemented"); > + } > + } > + > + uintptr_t mBits; private: ? @@ +85,5 @@ > +}; > + > +// Higher-level struct that owns a strong reference to the source. The source > +// is never null. > +struct OwningStyleContextSource { { on new line. @@ +90,5 @@ > + OwningStyleContextSource(already_AddRefed<nsRuleNode> aRuleNode) > + : mRaw(aRuleNode.take()) { MOZ_ASSERT(!mRaw.IsNull()); }; > + OwningStyleContextSource(already_AddRefed<ServoComputedValues> aComputedValues) > + : mRaw(aComputedValues.take()) { MOZ_ASSERT(!mRaw.IsNull()); } > + OwningStyleContextSource(OwningStyleContextSource&& aOther) : mRaw(aOther.mRaw) { aOther.mRaw = nullptr; } 80 columns @@ +104,5 @@ > + } > + } > + > + bool operator==(const OwningStyleContextSource& aOther) const { return mRaw == aOther.mRaw; } > + bool operator!=(const OwningStyleContextSource& aOther) const { return !(*this == aOther); } 80 columns @@ +106,5 @@ > + > + bool operator==(const OwningStyleContextSource& aOther) const { return mRaw == aOther.mRaw; } > + bool operator!=(const OwningStyleContextSource& aOther) const { return !(*this == aOther); } > + > + bool IsNull() const { May as well put this on one line if IsServoComputedvalues() is too. @@ +117,5 @@ > + bool IsServoComputedValues() const { return mRaw.IsServoComputedValues(); } > + > + NonOwningStyleContextSource AsRaw() const { return mRaw; } > + nsRuleNode* AsGeckoRuleNode() const { return mRaw.AsGeckoRuleNode(); } > + ServoComputedValues* AsServoComputedValues() const { return mRaw.AsServoComputedValues(); } non-const method needed, as above? @@ +124,5 @@ > + > +private: > + // Not implemented. > + OwningStyleContextSource& operator=(OwningStyleContextSource&); > + OwningStyleContextSource(OwningStyleContextSource&); It's fine these days to leave these up in public: and use "= delete". @@ +186,5 @@ > mozilla::CSSPseudoElementType aPseudoType, > + already_AddRefed<nsRuleNode> aRuleNode, > + bool aSkipParentDisplayBasedStyleFixup); > + > + nsStyleContext(nsStyleContext* aParent, Please add a comment that mentions how this differs from the other constructor. @@ +275,4 @@ > // Find, if it already exists *and is easily findable* (i.e., near the > // start of the child list), a style context whose: > // * GetPseudo() matches aPseudoTag > // * RuleNode() matches aRules Please update this comment. @@ +607,5 @@ > > void ApplyStyleFixups(bool aSkipParentDisplayBasedStyleFixup); > > + const void* StyleStructFromServoComputedValues(nsStyleStructID aSID) { > + MOZ_CRASH("Not implemented"); MOZ_CRASH("stylo: not implemented");

Comment on attachment 8735647 [details] [diff] [review] Part 2 - Create servo style contexts from ServoStyleSet. v1 Review of attachment 8735647 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/ServoStyleSet.cpp @@ +71,5 @@ > ServoStyleSet::ResolveStyleFor(Element* aElement, > nsStyleContext* aParentContext) > { > + // XXXbholley: Will this always work? nsStyleSet pulls it off the rule tree. > + nsPresContext* presContext = aElement->OwnerDoc()->GetShell()->GetPresContext(); I *think* you'll always have a pres shell if you have a style set. We should assert that before getting the pres context. @@ +74,5 @@ > + // XXXbholley: Will this always work? nsStyleSet pulls it off the rule tree. > + nsPresContext* presContext = aElement->OwnerDoc()->GetShell()->GetPresContext(); > + MOZ_ASSERT(presContext); > + > + RefPtr<ServoComputedValues> computedValues = dont_AddRef(Servo_GetComputedValues(aElement)); Maybe we should add comments to the prototypes in ServoBindings.h to make it clear which methods return an already_AddRefed thing as a raw pointer. @@ +126,3 @@ > > + // XXXbholley: Doe ResolveAnonymousBoxStyle always have a non-null > + // aParentContext? If not, we need some other way to get the presContext. The root frame is an anonymous box so aParentContext will be null then. I think we should just store the pres context that's passed in to Init() on the ServoStyleSet. @@ +129,5 @@ > + nsPresContext* presContext = aParentContext->PresContext(); > + MOZ_ASSERT(presContext); > + > + RefPtr<ServoComputedValues> computedValues = > + dont_AddRef(Servo_GetComputedValuesForAnonymousBox(aPseudoTag)); Servo_GetComputedValuesForAnonymousBox will need to take a parent so that properties can inherit into the anonymous box, won't it?

(In reply to Cameron McCormack (:heycam) from comment #3) Sorry for all the nit-picking that was required. This patch went through a bunch of iterations and rebases and got kind of messy along the way. :\ > Comment on attachment 8735646 [details] [diff] [review] > Part 1 - Generalize nsStyleContext to support resolving styles from either > nsRuleNode or ServoComputedValues. v1 > > Review of attachment 8735646 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/style/nsStyleContext.cpp > @@ +70,5 @@ > > // Whether to perform expensive assertions in the nsStyleContext destructor. > > static bool sExpensiveStyleStructAssertionsEnabled; > > #endif > > > > +nsStyleContext::nsStyleContext(nsStyleContext* aParent, OwningStyleContextSource&& aSource, > > aSource on a new line, to keep to 80 columns. Fixed. > > @@ +94,5 @@ > > + nsIAtom* aPseudoTag, > > + CSSPseudoElementType aPseudoType, > > + already_AddRefed<nsRuleNode> aRuleNode, > > + bool aSkipParentDisplayBasedStyleFixup) > > + : nsStyleContext(aParent, OwningStyleContextSource(Move(aRuleNode)), aPseudoTag, aPseudoType) > > 80 columns Fixed. > > @@ +123,5 @@ > > + nsIAtom* aPseudoTag, > > + CSSPseudoElementType aPseudoType, > > + already_AddRefed<ServoComputedValues> aComputedValues, > > + bool aSkipParentDisplayBasedStyleFixup) > > + : nsStyleContext(aParent, OwningStyleContextSource(Move(aComputedValues)), aPseudoTag, aPseudoType) > > 80 columns Fixed. > > @@ +128,5 @@ > > +{ > > +#ifdef MOZ_STYLO > > + mPresContext = aPresContext; > > +#endif > > +} > > Should we be calling FinishConstruction in here? Good catch! That was lost in a rebase. > > @@ +130,5 @@ > > + mPresContext = aPresContext; > > +#endif > > +} > > + > > +void nsStyleContext::FinishConstruction(bool aSkipParentDisplayBasedStyleFixup) > > Newline after "void". Fixed. > > @@ +183,5 @@ > > } > > #endif > > > > + nsPresContext *presContext = PresContext(); > > + nsStyleSet* geckoStyleSet = presContext->PresShell()->StyleSet()->AsGecko(); > > GetAsGecko? I'm not really sure why you made the changes to this bit of > moved code. Also you are indented by one space too many. Fixed. > > @@ +427,5 @@ > > { > > const void* cachedData = GetCachedStyleData(aSID); > > if (cachedData) > > return cachedData; // We have computed data stored on this node in the context tree. > > // Our rule node will take care of it for us. > > Maybe s/rule node/style source/? Fixed, along with some others. > > @@ +1443,4 @@ > > } > > if (!aNewContext->mCachedResetData) { > > aNewContext->mCachedResetData = > > + new (PresContext()) nsResetStyleData; > > Probably fits on one line now. Fixed. > > ::: layout/style/nsStyleContext.h > @@ +26,5 @@ > > +// root of the rule tree, and the most specific rule matched is the > > +// |mRule| member of the rule node. > > +// > > +// In the Servo case, we hold an atomically-refcounted handle to a > > +// Servo ComputedValue struct, which is more or less the Servo equivalent > > ComputedValues Fixed. > > @@ +29,5 @@ > > +// In the Servo case, we hold an atomically-refcounted handle to a > > +// Servo ComputedValue struct, which is more or less the Servo equivalent > > +// of an nsStyleContext. > > + > > +// Underling pointer without any strong ownership semantics. > > Underlying Fixed. > > @@ +30,5 @@ > > +// Servo ComputedValue struct, which is more or less the Servo equivalent > > +// of an nsStyleContext. > > + > > +// Underling pointer without any strong ownership semantics. > > +struct NonOwningStyleContextSource { > > { on new line. Fixed. > > I think this and OwningStyleContextSource should be in a separate > StyleContextSource.h header. (If they do stay in this file, please add them > below the CSSPseudoElementType forward declaration.) Moved them. > > @@ +34,5 @@ > > +struct NonOwningStyleContextSource { > > + MOZ_IMPLICIT NonOwningStyleContextSource(nsRuleNode* aRuleNode) > > + : mBits(reinterpret_cast<uintptr_t>(aRuleNode)) {} > > + explicit NonOwningStyleContextSource(ServoComputedValues* aComputedValues) > > + : mBits(reinterpret_cast<uintptr_t>(aComputedValues) | 1) {} > > As mentioned on IRC, it would be nice if we could just use one of the > nsStyleContext::mBits to tag the style source, but I guess that's difficult > without giving up on OwningStyleContextSource's automatic refcounting. Yeah. > > @@ +41,5 @@ > > + MOZ_ASSERT(IsServoComputedValues() == aOther.IsServoComputedValues(), > > + "Comparing Servo to Gecko - probably a bug"); > > + return mBits == aOther.mBits; > > + } > > + bool operator!=(const NonOwningStyleContextSource& aOther) const { return !(*this == aOther); } > > 80 columns Fixed. > > @@ +46,5 @@ > > + > > + // We intentionally avoid exposing IsGeckoRuleNode() here, because that would > > + // encourage callers to do: > > + // > > + // if (source.IsGecko()) { > > IsGeckoRuleNode? Fixed. > > @@ +62,5 @@ > > + return false; > > +#endif > > + } > > + > > + nsRuleNode* AsGeckoRuleNode() const { > > Should we be having const and non-const methods? I don't think so. The OwningStyleContext is const in the way that const RefPtr<T> is const - the reference may not be reassigned, but non-const methods on the underlying object may still be invoked. It might be worthwhile to make the referent const too at some point, but that would require const-correcting a bunch of methods on nsRuleNode among other things, so I'm not going to worry about it for now. > > const nsRuleNode* AsGeckoRuleNode() const { ... } > nsRuleNode* AsGeckoRuleNode() { ... } > > @@ +76,5 @@ > > + bool MatchesNoRules() const { > > + if (IsGeckoRuleNodeOrNull()) { > > + return AsGeckoRuleNode()->IsRoot(); > > + } else { > > + MOZ_CRASH("Not implemented"); > > MOZ_CRASH("stylo: not implemented") just for consistency (and searchability) > with other similar crashes we've added. > > @@ +80,5 @@ > > + MOZ_CRASH("Not implemented"); > > + } > > + } > > + > > + uintptr_t mBits; > > private: ? Fixed. > > @@ +85,5 @@ > > +}; > > + > > +// Higher-level struct that owns a strong reference to the source. The source > > +// is never null. > > +struct OwningStyleContextSource { > > { on new line. Fixed. > > @@ +90,5 @@ > > + OwningStyleContextSource(already_AddRefed<nsRuleNode> aRuleNode) > > + : mRaw(aRuleNode.take()) { MOZ_ASSERT(!mRaw.IsNull()); }; > > + OwningStyleContextSource(already_AddRefed<ServoComputedValues> aComputedValues) > > + : mRaw(aComputedValues.take()) { MOZ_ASSERT(!mRaw.IsNull()); } > > + OwningStyleContextSource(OwningStyleContextSource&& aOther) : mRaw(aOther.mRaw) { aOther.mRaw = nullptr; } > > 80 columns Fixed. > > @@ +104,5 @@ > > + } > > + } > > + > > + bool operator==(const OwningStyleContextSource& aOther) const { return mRaw == aOther.mRaw; } > > + bool operator!=(const OwningStyleContextSource& aOther) const { return !(*this == aOther); } > > 80 columns Fixed. > > @@ +106,5 @@ > > + > > + bool operator==(const OwningStyleContextSource& aOther) const { return mRaw == aOther.mRaw; } > > + bool operator!=(const OwningStyleContextSource& aOther) const { return !(*this == aOther); } > > + > > + bool IsNull() const { > > May as well put this on one line if IsServoComputedvalues() is too. Fixed. > > @@ +117,5 @@ > > + bool IsServoComputedValues() const { return mRaw.IsServoComputedValues(); } > > + > > + NonOwningStyleContextSource AsRaw() const { return mRaw; } > > + nsRuleNode* AsGeckoRuleNode() const { return mRaw.AsGeckoRuleNode(); } > > + ServoComputedValues* AsServoComputedValues() const { return mRaw.AsServoComputedValues(); } > > non-const method needed, as above? See above. > > @@ +124,5 @@ > > + > > +private: > > + // Not implemented. > > + OwningStyleContextSource& operator=(OwningStyleContextSource&); > > + OwningStyleContextSource(OwningStyleContextSource&); > > It's fine these days to leave these up in public: and use "= delete". Fixed. > > @@ +186,5 @@ > > mozilla::CSSPseudoElementType aPseudoType, > > + already_AddRefed<nsRuleNode> aRuleNode, > > + bool aSkipParentDisplayBasedStyleFixup); > > + > > + nsStyleContext(nsStyleContext* aParent, > > Please add a comment that mentions how this differs from the other > constructor. Fixed. > > @@ +275,4 @@ > > // Find, if it already exists *and is easily findable* (i.e., near the > > // start of the child list), a style context whose: > > // * GetPseudo() matches aPseudoTag > > // * RuleNode() matches aRules > > Please update this comment. Fixed. > > @@ +607,5 @@ > > > > void ApplyStyleFixups(bool aSkipParentDisplayBasedStyleFixup); > > > > + const void* StyleStructFromServoComputedValues(nsStyleStructID aSID) { > > + MOZ_CRASH("Not implemented"); > > MOZ_CRASH("stylo: not implemented"); Fixed.

(In reply to Cameron McCormack (:heycam) from comment #4) > Comment on attachment 8735647 [details] [diff] [review] > Part 2 - Create servo style contexts from ServoStyleSet. v1 > > Review of attachment 8735647 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/style/ServoStyleSet.cpp > @@ +71,5 @@ > > ServoStyleSet::ResolveStyleFor(Element* aElement, > > nsStyleContext* aParentContext) > > { > > + // XXXbholley: Will this always work? nsStyleSet pulls it off the rule tree. > > + nsPresContext* presContext = aElement->OwnerDoc()->GetShell()->GetPresContext(); > > I *think* you'll always have a pres shell if you have a style set. We > should assert that before getting the pres context. We're just storing the member now, so we can use that directly. > @@ +74,5 @@ > > + // XXXbholley: Will this always work? nsStyleSet pulls it off the rule tree. > > + nsPresContext* presContext = aElement->OwnerDoc()->GetShell()->GetPresContext(); > > + MOZ_ASSERT(presContext); > > + > > + RefPtr<ServoComputedValues> computedValues = dont_AddRef(Servo_GetComputedValues(aElement)); > > Maybe we should add comments to the prototypes in ServoBindings.h to make it > clear which methods return an already_AddRefed thing as a raw pointer. With the new binding generator, I _think_ we can just return already_AddRefed and UniquePtr directly (possible with a 'simple' replacement type). It's on my list, but let's not worry about it for this patch. > > @@ +126,3 @@ > > > > + // XXXbholley: Doe ResolveAnonymousBoxStyle always have a non-null > > + // aParentContext? If not, we need some other way to get the presContext. > > The root frame is an anonymous box so aParentContext will be null then. I > think we should just store the pres context that's passed in to Init() on > the ServoStyleSet. Ok. Weak, I assume? > > @@ +129,5 @@ > > + nsPresContext* presContext = aParentContext->PresContext(); > > + MOZ_ASSERT(presContext); > > + > > + RefPtr<ServoComputedValues> computedValues = > > + dont_AddRef(Servo_GetComputedValuesForAnonymousBox(aPseudoTag)); > > Servo_GetComputedValuesForAnonymousBox will need to take a parent so that > properties can inherit into the anonymous box, won't it? Good call.

Comment on attachment 8736045 [details] [diff] [review] Create servo style contexts from ServoStyleSet. v2 Review of attachment 8736045 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d926ec33189d https://hg.mozilla.org/integration/mozilla-inbound/rev/de0c81292e18

https://hg.mozilla.org/mozilla-central/rev/d926ec33189d https://hg.mozilla.org/mozilla-central/rev/de0c81292e18