Misc. ComputedStyle cleanup.

RESOLVED FIXED in Firefox 62

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: emilio, Assigned: emilio)

Tracking

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

No description provided.
Comment on attachment 8980282 [details]
Bug 1464060: Remove a bunch of unused style bits.

https://reviewboard.mozilla.org/r/246444/#review252540

::: layout/style/ComputedStyle.h:50
(Diff revision 1)
>  #define NS_STYLE_SUPPRESS_LINEBREAK        0x080000000
> -// See ComputedStyle::IsInDisplayNoneSubtree
> -#define NS_STYLE_IN_DISPLAY_NONE_SUBTREE   0x100000000
> -// See ComputedStyle::FindChildWithRules
> -#define NS_STYLE_INELIGIBLE_FOR_SHARING    0x200000000
> -// See ComputedStyle::HasChildThatUsesResetStyle
> -#define NS_STYLE_HAS_CHILD_THAT_USES_RESET_STYLE 0x400000000
>  // See ComputedStyle::IsTextCombined
>  #define NS_STYLE_IS_TEXT_COMBINED          0x800000000
> -// Whether a ComputedStyle is a Gecko or Servo context

I think it's worth renumbering these to make it easier to see which bits are taken and so that we can just append new bits at the end in the future.
... and by "these" I mean NS_STYLE_SUPPRESS_LINEBREAK / NS_STYLE_IS_TEXT_COMBINED to be clear
Comment on attachment 8980282 [details]
Bug 1464060: Remove a bunch of unused style bits.

https://reviewboard.mozilla.org/r/246444/#review252780
Attachment #8980282 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8980283 [details]
Bug 1464060: Remove some unused templating and arguments.

https://reviewboard.mozilla.org/r/246446/#review252782

::: layout/style/nsStyleStruct.h:2476
(Diff revision 2)
>     * are based on the frame rather than the ComputedStyle (thus
>     * potentially returning a false positive).
> -   *
> -   * FIXME(stylo-everywhere): Pretty sure the template can go here.
>     */
> -  template<class ComputedStyleLike>
> +  inline bool IsAbsPosContainingBlockForAppropriateFrame(mozilla::ComputedStyle&) const;

This line is too long. Please put the parameter down to the next line.

::: layout/style/nsStyleStruct.h:2504
(Diff revision 2)
>     * are based on the frame rather than the ComputedStyle (thus
>     * potentially returning a false positive).
> -   *
> -   * FIXME(stylo-everywhere): Pretty sure the template can go here.
>     */
> -  template<class ComputedStyleLike>
> +  inline bool IsFixedPosContainingBlockForAppropriateFrame(mozilla::ComputedStyle&) const;

Ditto.

::: layout/style/nsStyleStruct.h:2522
(Diff revision 2)
>  
>  private:
>    // Helpers for above functions, which do some but not all of the tests
>    // for them (since transform must be tested separately for each).
> -  //
> -  // FIXME(stylo-everywhere): Pretty sure the template can go here.
> +  inline bool HasAbsPosContainingBlockStyleInternal() const;
> +  inline bool HasFixedPosContainingBlockStyleInternal(mozilla::ComputedStyle&) const;

Ditto.

::: layout/style/nsStyleStructInlines.h:157
(Diff revision 2)
>    return HasPerspectiveStyle() && aContextFrame->IsFrameOfType(nsIFrame::eSupportsCSSTransforms);
>  }
>  
> -template<class ComputedStyleLike>
>  bool
> -nsStyleDisplay::HasFixedPosContainingBlockStyleInternal(
> +nsStyleDisplay::HasFixedPosContainingBlockStyleInternal(mozilla::ComputedStyle& aStyle) const

Ditto.

::: layout/style/nsStyleStructInlines.h:175
(Diff revision 2)
> -  return aComputedStyle->ThreadsafeStyleEffects()->HasFilters();
> +  return aStyle.ThreadsafeStyleEffects()->HasFilters();
>  }
>  
> -template<class ComputedStyleLike>
>  bool
> -nsStyleDisplay::IsFixedPosContainingBlockForAppropriateFrame(
> +nsStyleDisplay::IsFixedPosContainingBlockForAppropriateFrame(mozilla::ComputedStyle& aStyle) const

Ditto.

::: layout/style/nsStyleStructInlines.h
(Diff revision 2)
> -  NS_ASSERTION(aComputedStyle->ThreadsafeStyleDisplay() == this,
> -               "unexpected aComputedStyle");

Maybe move this assertion to its callers.

::: layout/style/nsStyleStructInlines.h:206
(Diff revision 2)
>           (mWillChangeBitField & NS_STYLE_WILL_CHANGE_ABSPOS_CB);
>  }
>  
> -template<class ComputedStyleLike>
>  bool
> -nsStyleDisplay::IsAbsPosContainingBlockForAppropriateFrame(ComputedStyleLike* aComputedStyle) const
> +nsStyleDisplay::IsAbsPosContainingBlockForAppropriateFrame(mozilla::ComputedStyle& aStyle) const

Also too long.
Attachment #8980283 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8980354 [details]
Bug 1464060: Make the general setup for computed style bits nicer.

https://reviewboard.mozilla.org/r/246522/#review252784

I think you can merge part 3 and part 4 given they are both touching the same stuff.
Attachment #8980354 - Flags: review?(xidorn+moz)
Comment on attachment 8980355 [details]
Bug 1464060: Make the general setup for computed style bits nicer.

https://reviewboard.mozilla.org/r/246524/#review252786
Attachment #8980355 - Flags: review?(xidorn+moz)
Attachment #8980354 - Attachment is obsolete: true
Comment on attachment 8980355 [details]
Bug 1464060: Make the general setup for computed style bits nicer.

https://reviewboard.mozilla.org/r/246524/#review253186

r=me with nits below addressed

::: commit-message-d998d:3
(Diff revision 3)
> +Bug 1464060: Make the general setup for computed style bits nicer. r?xidorn
> +
> +This patch:

And it removes `NS_STYLE_IS_STYLE_IF_VISITED` which should probably have happened in the first part.

::: commit-message-d998d:15
(Diff revision 3)
> + * Makes mPseudoTag, mPseudoType and mBits const, since we don't want them to be
> +   mutated from C++, and we still need a few more refactorings (mostly getting
> +   rid of FinishStyle) to avoid mutating ComputedStyle instead.

If your target is to make `ComputedStyle` const, you can without more refactoring. You just need to make `mStructsRequested` a `mutate` field, then you'll be able to set it from `const` function.

That would probably be a bit better than marking every other field `const`. It might be a non-trivial refactoring itself (since you need to change lots of callsites of `ComputedStyle`), so it can be in a separate patch or bug.

::: layout/style/ComputedStyle.h:379
(Diff revision 3)
>    // If this ComputedStyle is for a pseudo-element or anonymous box,
>    // the relevant atom.
> -  RefPtr<nsAtom> mPseudoTag;
> +  const RefPtr<nsAtom> mPseudoTag;
>  
> -  // mBits stores a number of things:
> -  //  - It records (using the style struct bits) which structs have
> +  // A bitfield with the structs that have been requested so far.
> +  uint32_t mStructsRequested = 0;

I would suggest calling it `mRequestedStructs` instead. I think that matches the naming convention better.

::: layout/style/ServoBindings.cpp:354
(Diff revision 3)
>    MOZ_ASSERT(aOldStyle);
>    MOZ_ASSERT(aNewStyle);
>  
>    uint32_t equalStructs;
> -  nsChangeHint result = const_cast<mozilla::ComputedStyle*>(aOldStyle)->
> -    CalcStyleDifference(
> +  nsChangeHint result = const_cast<ComputedStyle*>(aOldStyle)->
> +    CalcStyleDifference(const_cast<ComputedStyle*>(aNewStyle), &equalStructs);

We probably should have `CalcStyleDifference` const at some point... I don't recall why it cannot be.

::: layout/style/nsStyleStructFwd.h:16
(Diff revision 3)
>  #ifndef nsStyleStructFwd_h_
>  #define nsStyleStructFwd_h_
>  
> -enum nsStyleStructID {
> +namespace mozilla {
>  
> +enum class StyleStructID : uint32_t {

Does it need to be `uint32_t`? I suppose `uint8_t` is more than enough for this?

::: layout/style/nsStyleStructFwd.h:80
(Diff revision 3)
> +#define STYLE_STRUCT_BIT(name_) \
> +  mozilla::StyleStructConstants::BitFor(mozilla::StyleStructID::name_)

I would prefer you either remove this macro (and use `StyleStructConstants::BitFor` directly), or keep the `NS_` prefix.

Name of macro is unscoped, and this macro is defined globally, so we probably should have a prefix for it, otherwise there may be conflict in the future.
Attachment #8980355 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8980733 [details]
Bug 1464060: Unify DoGetStyle* implementations.

https://reviewboard.mozilla.org/r/246890/#review253188
Attachment #8980733 - Flags: review?(xidorn+moz) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/906889473159
Remove a bunch of unused style bits. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/8197adfc55aa
Remove some unused templating and arguments. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/a28175c98d90
Make the general setup for computed style bits nicer. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/f44f6ecbc3fe
Unify DoGetStyle* implementations. r=xidorn
You need to log in before you can comment on or make changes to this bug.