Closed Bug 1260310 Opened 4 years ago Closed 4 years ago

Generalize nsStyleContext to allow ServoComputedValues as a style source

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
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");
Attachment #8735646 - Flags: review?(cam) → review+
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?
Attachment #8735647 - Flags: review?(cam) → review-
(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.
Attachment #8735646 - Attachment is obsolete: true
Attachment #8735647 - Attachment is obsolete: true
(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.
Attachment #8736045 - Flags: review?(cam) → review+
https://hg.mozilla.org/mozilla-central/rev/d926ec33189d
https://hg.mozilla.org/mozilla-central/rev/de0c81292e18
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1262823
You need to log in before you can comment on or make changes to this bug.