1447483 - Merge nsStyleContext and ServoStyleContext, rename to ComputedStyle.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

Per https://public.etherpad-mozilla.org/p/post-stylo-stuff.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: The patch.

This is the patch without clang-format, for reference.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: The patch.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Mozreview is timing out when uploading the patches, so will push using splinter.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 1 - s/StyleContext/ComputedStyle.

Note that the unwanted changes under widget/gtk and devtools/ go away in following parts. I'll land the patches squashed.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 2 - s/nsComputedStyle/ComputedStyle.

Also completely automated using find + sed. This in combination with the previous patch make nsStyleContext -> ComputedStyle.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 3 - s/ServoComputedStyle/ComputedStyle.

This leaves ServoStyleContext and nsStyleContext using the same name, since they're going to be merged in following parts.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 4 - Move files from nsStyleContext* to ComputedStyle*.

Just moving the files, not build config touched or anything.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 4b - Actually undo a moz.build change in part 4.

This was supposed to be in the patch in part 4.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 5 - Merge what used to be ServoStyleContext and into ComputedStyle.

This part is manual, though should be mostly straight-forward.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> Created attachment 8961101 [details] [diff] [review]
> Part 5 - Merge what used to be ServoStyleContext and into ComputedStyle.
> 
> This part is manual, though should be mostly straight-forward.

This also makes ComputedStyle be in the mozilla::namespace, btw.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 6 - s/include "ComputedStyle/include "mozilla\/ComputedStyle/g.

Automatic include fixup for part 5.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 7 - Keep some dummy methods so that I get less C++ build errors.

This will be cleaned in followups to this bug, to make this bug a bit more manageable.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 8 - Minor namespacing fixes and manual cleanups / removals.

Just some manual fixes while I was trying to get this to build.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #13)
> Created attachment 8961104 [details] [diff] [review]
> Part 8 - Minor namespacing fixes and manual cleanups / removals.
> 
> Just some manual fixes while I was trying to get this to build.

This also renames nsIFrame::ComputedStyle() to nsIFrame::Style(), since (apart that it's nicer IMO), that prevents name ambiguity.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 9 - s/Frame->ComputedStyle/Frame->Style/g.

Automated change to fix a bunch of callers of nsIFrame::ComputedStyle to start calling nsIFrame::Style().

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 10 - Fix frame creation functions.

This patch is autogenerated. Three replacements under the layout directory in order:

  s/ComputedStyle\* aContext/mozilla::ComputedStyle* aStyle/g

To fix most function signatures.

  s/Frame(aContext,/Frame(aStyle,/g

To fix most delegated constructors.

  s/mozilla::mozilla::ComputedStyle/mozilla::ComputedStyle/g

To fix other callers that I had manually fixed already.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 11 - Remove ComputedStyle usage in nsStyleTransformMatrix.

These arguments are unused now, and can go away. The patches in Part 10 left the signatures very garbled.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 12 - s/DidSetComputedStyle(ComputedStyle/DidSetComputedStyle(mozilla::ComputedStyle/g

Automatically fix another large amount of users of ComputedStyle.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 13 - Some minor manual fixups.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 14 - s/Frame(aContext/Frame(aStyle/g.

Automatically fix most construction functions and similar.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 15 - Merge ServoStyleContextInlines into ComputedStyleInlines.

Had missed this before.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 16 - Some manual fixups and removing dead code that no longer builds in ComputedStyle.cpp

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 17 - Get layout/svg building.

The forward declarations removed are unneeded since all those files include ComputedStyle.h via nsIFrame.h.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 18 - Get some more parts of layout/generic building.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 19 - Fix callers of nsIFrame::Style in layout/

This was done semi-automatically, replacing s/ComputedStyle()/Style()/g and then removing the non-relevant replacements, like those mentioning getComputedStyle(), for example.

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 20 - Get layout/generic to build.

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 21 - Get layout/forms building.

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 22 - Get layout/mathml to build.

Comment 29

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 23 - Get layout/style building.

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 24 - Get layout/inspector building.

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 25 - Get layout/xul and layout/painting building.

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 26 - Get dom/animation building.

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 27 - Get dom/smil to build.

Comment 34

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 28 - Revert unwanted changes under widget/gtk from part 1 and following.

Comment 35

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 29 - get dom/svg to build.

Comment 36

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 30 - Fix the rust bits.

Comment 37

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 31 - Move ComputedStyle.cpp definitions into the namespace to fix -Wpermissive errors.

Comment 38

[Jonathan Watt [:jwatt]]

Comment on attachment 8961082 [details] [diff] [review]
The patch.

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

::: devtools/client/inspector/rules/test/browser_rules_add-rule-with-menu.js
@@ +20,4 @@
>  function* addNewRuleFromContextMenu(inspector, view) {
>    info("Waiting for context menu to be shown");
>  
> +  let allMenuItems = openStyleContextMenuAndGetAllItems(view, view.element);

Revert the changes to this file.

::: devtools/client/inspector/test/shared-head.js
@@ +580,4 @@
>   *        The instance of the rule-view panel
>   * @return An array of MenuItems
>   */
> +function openStyleContextMenuAndGetAllItems(view, target) {

And this one.

::: dom/animation/EffectCompositor.cpp
@@ +567,4 @@
>    Maybe<NonOwningAnimationTarget> result;
>  
>    CSSPseudoElementType pseudoType =
> +    aFrame->Style()->GetPseudoType();

I'm a bit disappointed you didn't take the time to put this on a single line. ;)

Comment 39

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 32 (not in the squashed patch since it adds noise) - Run clang-format-diff on these patches.

Not terribly happy about the output, I can fix nits manually instead.

Comment 40

[Emilio Cobos Álvarez (:emilio)]

Attachment: Squashed patch.

In case it's easier to review. Includes part 31, but not 32.

Comment 41

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=112d6b763d582419143f1ef592b531b3a6e5094f

Comment 42

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #41)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=112d6b763d582419143f1ef592b531b3a6e5094f

Rust SpiderMonkey bustage is expected (this is on top of inbound, and bug 888600 broke it).

Comment 43

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 33 - Fix KeyframeEffectReadOnly to not overflow the stack.

This should fix the orange of the try run. Should've seen it coming :-)

Comment 44

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jonathan Watt [:jwatt] from comment #38)
> Comment on attachment 8961082 [details] [diff] [review]
> The patch.
> 
> Review of attachment 8961082 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/inspector/rules/test/browser_rules_add-rule-with-menu.js
> @@ +20,4 @@
> >  function* addNewRuleFromContextMenu(inspector, view) {
> >    info("Waiting for context menu to be shown");
> >  
> > +  let allMenuItems = openStyleContextMenuAndGetAllItems(view, view.element);
> 
> Revert the changes to this file.
> 
> ::: devtools/client/inspector/test/shared-head.js
> @@ +580,4 @@
> >   *        The instance of the rule-view panel
> >   * @return An array of MenuItems
> >   */
> > +function openStyleContextMenuAndGetAllItems(view, target) {
> 
> And this one.

These are reverted in part 28, so not sure why they ended up there, it's definitely not in the patch from comment 40... Anyway, done.

> ::: dom/animation/EffectCompositor.cpp
> @@ +567,4 @@
> >    Maybe<NonOwningAnimationTarget> result;
> >  
> >    CSSPseudoElementType pseudoType =
> > +    aFrame->Style()->GetPseudoType();
> 
> I'm a bit disappointed you didn't take the time to put this on a single
> line. ;)

Yeah, the clang-format bit should take care of it... But I'm not sure if we should land it since it's super-noisy. Maybe I'll fix it manually.

Xidorn is on PTO today, so if you want to steal some reviews given you've looked at it already, please go ahead :P

Comment 45

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ceec106c62dfebf6d6006c60ee878116532686b1 is much greener :)

Comment 46

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 34 - Remove unnecessary mozilla:: qualifications.

As requested by jwatt over IRC. This adds a typedef to nsCSSFrameConstructor and nsIFrame to avoid most of the mozilla:: qualifications.

Comment 47

[Jonathan Watt [:jwatt]]

Comment on attachment 8961152 [details] [diff] [review]
Squashed patch.

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

I'm about two thirds through. Here are my comments so far for you to start on.

::: dom/smil/nsSMILCSSProperty.h
@@ +35,4 @@
>     * Constructs a new nsSMILCSSProperty.
>     * @param  aPropID   The CSS property we're interested in animating.
>     * @param  aElement  The element whose CSS property is being animated.
> +   * @param  aBaseComputedStyle  The style context to use when getting the base

Fix indent.

::: dom/smil/nsSMILCompositor.cpp
@@ +64,1 @@
>                                                             nullptr);

Indent.

::: dom/xbl/nsXBLResourceLoader.cpp
@@ +23,4 @@
>  #include "nsIURI.h"
>  #include "nsNetUtil.h"
>  #include "nsGkAtoms.h"
> +#include "mozilla/ComputedStyle.h"

It would be nice to have the new includes in the correct order in files that already have them ordered. Feel like searching through the patch and fixing that up before committing? :)

::: layout/base/ServoRestyleManager.h
@@ +325,1 @@
>                                ServoStyleSet& aStyleSet);

Indent.

::: layout/base/nsBidiPresUtils.h
@@ +29,5 @@
>  template<class T> class nsTHashtable;
>  namespace mozilla {
> +class ComputedStyle;
> +class WritingMode;
> +class LogicalMargin;

Could sort this while touching.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +8615,4 @@
>                                               bool              aIsFluid)
>  {
>    nsIPresShell*              shell = aPresContext->PresShell();
> +  ComputedStyle*            styleContext = aFrame->Style();

Indent.

::: layout/base/nsCSSFrameConstructor.h
@@ +436,4 @@
>     * @param aParentFrame the parent frame for the generated frame
>     * @param aAttrNamespace the namespace of the attribute in question
>     * @param aAttrName the localname of the attribute
> +   * @param aComputedStyle the style context to use

Fix comment (still using "style context").

@@ +469,1 @@
>     * style context

fix

@@ +587,4 @@
>  
>       @param nsIPresShell the presshell whose arena should be used to allocate
>                           the frame.
> +     @param ComputedStyle the style context to use for the frame. */

fix

@@ +595,5 @@
>    /* A function that can be used to get a FrameConstructionData.  Such
>       a function is allowed to return null.
>  
>       @param nsIContent the node for which the frame is being constructed.
> +     @param mozilla::ComputedStyle the style context to be used for the frame.

fix

@@ +1406,4 @@
>     *        adjusted to point to the right parent frame.
>     * @param aFCData the FrameConstructionData that would be used for frame
>     *        construction.
> +   * @param aComputedStyle the style context for aChildContent

fix

@@ +1414,4 @@
>    // be kept track of in the state...
>    void AdjustParentFrame(nsContainerFrame**           aParentFrame,
>                           const FrameConstructionData* aFCData,
> +                         mozilla::ComputedStyle*              aComputedStyle);

indent

@@ +1464,4 @@
>                            nsFrameConstructorState& aState,
>                            nsIContent*              aContent,
>                            nsContainerFrame*        aParentFrame,
> +                          mozilla::ComputedStyle*          aComputedStyle,

indent

@@ +1542,4 @@
>                                           nsAtom*                 aTag,
>                                           int32_t                  aNameSpaceID,
>                                           bool                     aSuppressWhiteSpaceOptimizations,
> +                                         mozilla::ComputedStyle*          aComputedStyle,

indent

@@ +1756,4 @@
>     */
>    void ProcessChildren(nsFrameConstructorState& aState,
>                         nsIContent*              aContent,
> +                       mozilla::ComputedStyle*          aComputedStyle,

indent

@@ +1789,4 @@
>    void
>    BuildScrollFrame(nsFrameConstructorState& aState,
>                     nsIContent*              aContent,
> +                   mozilla::ComputedStyle*          aContentStyle,

indent

@@ +1799,3 @@
>    BeginBuildingScrollFrame(nsFrameConstructorState& aState,
>                             nsIContent*              aContent,
> +                           mozilla::ComputedStyle*          aContentStyle,

indent

@@ +1819,4 @@
>                          nsContainerFrame*        aScrolledFrame,
>                          nsIContent*              aContent,
>                          nsContainerFrame*        aParentFrame,
> +                        mozilla::ComputedStyle*          aComputedStyle,

indent

@@ +1906,4 @@
>                        nsIContent*              aContent,
>                        nsContainerFrame*        aParentFrame,
>                        nsContainerFrame*        aContentParentFrame,
> +                      mozilla::ComputedStyle*          aComputedStyle,

indent

@@ +1979,5 @@
>                              nsIContent*              aTextContent,
>                              nsIFrame*                aTextFrame,
>                              nsContainerFrame*        aParentFrame,
> +                            mozilla::ComputedStyle*          aParentComputedStyle,
> +                            mozilla::ComputedStyle*          aComputedStyle,

indent

::: layout/base/nsLayoutUtils.h
@@ +676,4 @@
>     * attached to it; returns false otherwise.
>     *
>     * @param aContent the content node we're looking at
> +   * @param aComputedStyle aContent's style context

fix comment

@@ +1834,4 @@
>     *   @param aRenderingContext Where to draw the image, set up with an
>     *                            appropriate scale and transform for drawing in
>     *                            app units.
> +   *   @param aComputedStyle     The style context of the nsIFrame (or

fix comment

::: layout/forms/nsButtonFrameRenderer.cpp
@@ -42,5 @@
> -#ifdef DEBUG
> -  if (mInnerFocusStyle) {
> -    mInnerFocusStyle->FrameRelease();
> -  }
> -#endif

For the record for anyone doing bug archeology, this is being removed because FrameRelease() is now an empty method.

::: layout/forms/nsRangeFrame.cpp
@@ +920,5 @@
>  }
>  
>  void
> +nsRangeFrame::SetAdditionalComputedStyle(int32_t aIndex,
> +                                        ComputedStyle* aComputedStyle)

indent

::: layout/generic/nsIFrame.h
@@ +799,5 @@
>      }
>    }
>  
>    /**
> +   * SetComputedStyleWithoutNotification is for changes to the style

fix comment

::: layout/inspector/InspectorUtils.cpp
@@ +949,1 @@
>                                                 nsAtom* aPseudo)

indent

::: layout/mathml/nsMathMLmencloseFrame.cpp
@@ +739,5 @@
>  }
>  
>  void
> +nsMathMLmencloseFrame::SetAdditionalComputedStyle(int32_t          aIndex,
> +                                                 ComputedStyle*  aComputedStyle)

indent

::: layout/style/ComputedStyle.cpp
@@ +82,5 @@
> +    CSSPseudoElementType aPseudoType,
> +    ServoComputedDataForgotten aComputedValues)
> +  : mPresContext(aPresContext)
> +  , mSource(aComputedValues)
> +  , mPseudoTag(aPseudoTag)

Hmm, I'm not in love with having the parameters and member initializers align. Personally I'd align the parameters two spaces past the "::".

@@ +114,3 @@
>                                      uint32_t* aEqualStructs,
>                                      uint32_t* aSamePointerStructs,
>                                      bool aIgnoreVariables)

indent

::: layout/style/ComputedStyle.h
@@ +38,2 @@
>  /**
> + * An ComputedStyle represents the computed style data for an element.

This should now be "A", not "An".

Comment 48

[Jonathan Watt [:jwatt]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #46)
> Created attachment 8961352 [details] [diff] [review]
> Part 34 - Remove unnecessary mozilla:: qualifications.
> 
> As requested by jwatt over IRC. This adds a typedef to nsCSSFrameConstructor
> and nsIFrame to avoid most of the mozilla:: qualifications.

Looks like you did it for everything, not just nsCSSFrameConstructor and nsIFrame.

Thanks for doing this. I know it was a bit annoying to do, but it does make the code a bit more pleasant to read. :)

Comment 49

[Jonathan Watt [:jwatt]]

Comment on attachment 8961352 [details] [diff] [review]
Part 34 - Remove unnecessary mozilla:: qualifications.

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

::: layout/xul/tree/nsTreeColumns.cpp
@@ -11,4 @@
>  #include "nsTreeColumns.h"
>  #include "nsTreeUtils.h"
>  #include "mozilla/ComputedStyle.h"
> -#include "nsDOMClassInfoID.h"

Bit of an unrelated change here at the end, but I guess it doesn't matter too much. ;)

Comment 50

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jonathan Watt [:jwatt] from comment #49)
> Comment on attachment 8961352 [details] [diff] [review]
> Part 34 - Remove unnecessary mozilla:: qualifications.
> 
> Review of attachment 8961352 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/xul/tree/nsTreeColumns.cpp
> @@ -11,4 @@
> >  #include "nsTreeColumns.h"
> >  #include "nsTreeUtils.h"
> >  #include "mozilla/ComputedStyle.h"
> > -#include "nsDOMClassInfoID.h"
> 
> Bit of an unrelated change here at the end, but I guess it doesn't matter
> too much. ;)

This was removed and is rebase fallout, it's gone from the squashed patch.

Comment 51

[Emilio Cobos Álvarez (:emilio)]

Attachment: Squashed patch with more nits addressed.

Fixed the include order and removes most references to style contexts in layout/base, layout/style, servo/, and layout/generic

Comment 52

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8961374 [details] [diff] [review]
Squashed patch with more nits addressed.

Err, has some changes from my queue. will remove.

Comment 53

[Emilio Cobos Álvarez (:emilio)]

Attachment: Actually squashed patch with nits adressed.

Comment 54

[Jonathan Watt [:jwatt]]

Comment on attachment 8961152 [details] [diff] [review]
Squashed patch.

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

Here's the rest of it, starting at layout/style/ServoBindings.h

::: dom/smil/nsSMILCSSProperty.h
@@ +35,4 @@
>     * Constructs a new nsSMILCSSProperty.
>     * @param  aPropID   The CSS property we're interested in animating.
>     * @param  aElement  The element whose CSS property is being animated.
> +   * @param  aBaseComputedStyle  The style context to use when getting the base

Fix indent.

::: dom/smil/nsSMILCompositor.cpp
@@ +64,1 @@
>                                                             nullptr);

Indent.

::: dom/xbl/nsXBLResourceLoader.cpp
@@ +23,4 @@
>  #include "nsIURI.h"
>  #include "nsNetUtil.h"
>  #include "nsGkAtoms.h"
> +#include "mozilla/ComputedStyle.h"

It would be nice to have the new includes in the correct order in files that already have them ordered. Feel like searching through the patch and fixing that up before committing? :)

::: layout/base/ServoRestyleManager.h
@@ +325,1 @@
>                                ServoStyleSet& aStyleSet);

Indent.

::: layout/base/nsBidiPresUtils.h
@@ +29,5 @@
>  template<class T> class nsTHashtable;
>  namespace mozilla {
> +class ComputedStyle;
> +class WritingMode;
> +class LogicalMargin;

Could sort this while touching.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +8615,4 @@
>                                               bool              aIsFluid)
>  {
>    nsIPresShell*              shell = aPresContext->PresShell();
> +  ComputedStyle*            styleContext = aFrame->Style();

Indent.

::: layout/base/nsCSSFrameConstructor.h
@@ +436,4 @@
>     * @param aParentFrame the parent frame for the generated frame
>     * @param aAttrNamespace the namespace of the attribute in question
>     * @param aAttrName the localname of the attribute
> +   * @param aComputedStyle the style context to use

Fix comment (still using "style context").

@@ +469,1 @@
>     * style context

fix

@@ +587,4 @@
>  
>       @param nsIPresShell the presshell whose arena should be used to allocate
>                           the frame.
> +     @param ComputedStyle the style context to use for the frame. */

fix

@@ +595,5 @@
>    /* A function that can be used to get a FrameConstructionData.  Such
>       a function is allowed to return null.
>  
>       @param nsIContent the node for which the frame is being constructed.
> +     @param mozilla::ComputedStyle the style context to be used for the frame.

fix

@@ +1406,4 @@
>     *        adjusted to point to the right parent frame.
>     * @param aFCData the FrameConstructionData that would be used for frame
>     *        construction.
> +   * @param aComputedStyle the style context for aChildContent

fix

@@ +1414,4 @@
>    // be kept track of in the state...
>    void AdjustParentFrame(nsContainerFrame**           aParentFrame,
>                           const FrameConstructionData* aFCData,
> +                         mozilla::ComputedStyle*              aComputedStyle);

indent

@@ +1464,4 @@
>                            nsFrameConstructorState& aState,
>                            nsIContent*              aContent,
>                            nsContainerFrame*        aParentFrame,
> +                          mozilla::ComputedStyle*          aComputedStyle,

indent

@@ +1542,4 @@
>                                           nsAtom*                 aTag,
>                                           int32_t                  aNameSpaceID,
>                                           bool                     aSuppressWhiteSpaceOptimizations,
> +                                         mozilla::ComputedStyle*          aComputedStyle,

indent

@@ +1756,4 @@
>     */
>    void ProcessChildren(nsFrameConstructorState& aState,
>                         nsIContent*              aContent,
> +                       mozilla::ComputedStyle*          aComputedStyle,

indent

@@ +1789,4 @@
>    void
>    BuildScrollFrame(nsFrameConstructorState& aState,
>                     nsIContent*              aContent,
> +                   mozilla::ComputedStyle*          aContentStyle,

indent

@@ +1799,3 @@
>    BeginBuildingScrollFrame(nsFrameConstructorState& aState,
>                             nsIContent*              aContent,
> +                           mozilla::ComputedStyle*          aContentStyle,

indent

@@ +1819,4 @@
>                          nsContainerFrame*        aScrolledFrame,
>                          nsIContent*              aContent,
>                          nsContainerFrame*        aParentFrame,
> +                        mozilla::ComputedStyle*          aComputedStyle,

indent

@@ +1906,4 @@
>                        nsIContent*              aContent,
>                        nsContainerFrame*        aParentFrame,
>                        nsContainerFrame*        aContentParentFrame,
> +                      mozilla::ComputedStyle*          aComputedStyle,

indent

@@ +1979,5 @@
>                              nsIContent*              aTextContent,
>                              nsIFrame*                aTextFrame,
>                              nsContainerFrame*        aParentFrame,
> +                            mozilla::ComputedStyle*          aParentComputedStyle,
> +                            mozilla::ComputedStyle*          aComputedStyle,

indent

::: layout/base/nsLayoutUtils.h
@@ +676,4 @@
>     * attached to it; returns false otherwise.
>     *
>     * @param aContent the content node we're looking at
> +   * @param aComputedStyle aContent's style context

fix comment

@@ +1834,4 @@
>     *   @param aRenderingContext Where to draw the image, set up with an
>     *                            appropriate scale and transform for drawing in
>     *                            app units.
> +   *   @param aComputedStyle     The style context of the nsIFrame (or

fix comment

::: layout/forms/nsButtonFrameRenderer.cpp
@@ -42,5 @@
> -#ifdef DEBUG
> -  if (mInnerFocusStyle) {
> -    mInnerFocusStyle->FrameRelease();
> -  }
> -#endif

For the record for anyone doing bug archeology, this is being removed because FrameRelease() is now an empty method.

::: layout/forms/nsRangeFrame.cpp
@@ +920,5 @@
>  }
>  
>  void
> +nsRangeFrame::SetAdditionalComputedStyle(int32_t aIndex,
> +                                        ComputedStyle* aComputedStyle)

indent

::: layout/generic/nsIFrame.h
@@ +799,5 @@
>      }
>    }
>  
>    /**
> +   * SetComputedStyleWithoutNotification is for changes to the style

fix comment

::: layout/inspector/InspectorUtils.cpp
@@ +949,1 @@
>                                                 nsAtom* aPseudo)

indent

::: layout/mathml/nsMathMLmencloseFrame.cpp
@@ +739,5 @@
>  }
>  
>  void
> +nsMathMLmencloseFrame::SetAdditionalComputedStyle(int32_t          aIndex,
> +                                                 ComputedStyle*  aComputedStyle)

indent

::: layout/style/ComputedStyle.cpp
@@ +82,5 @@
> +    CSSPseudoElementType aPseudoType,
> +    ServoComputedDataForgotten aComputedValues)
> +  : mPresContext(aPresContext)
> +  , mSource(aComputedValues)
> +  , mPseudoTag(aPseudoTag)

Hmm, I'm not in love with having the parameters and member initializers align. Personally I'd align the parameters two spaces past the "::".

@@ +114,3 @@
>                                      uint32_t* aEqualStructs,
>                                      uint32_t* aSamePointerStructs,
>                                      bool aIgnoreVariables)

indent

::: layout/style/ComputedStyle.h
@@ +38,2 @@
>  /**
> + * An ComputedStyle represents the computed style data for an element.

This should now be "A", not "An".

::: layout/style/ServoBindings.h
@@ +162,5 @@
>  nsTArray<nsIContent*>* Gecko_GetAnonymousContentForElement(RawGeckoElementBorrowed element);
>  void Gecko_DestroyAnonymousContentList(nsTArray<nsIContent*>* anon_content);
>  
> +void Gecko_ComputedStyle_Init(mozilla::ComputedStyle* context,
> +                                  ComputedStyleBorrowedOrNull parent_context,

indent

::: layout/style/ServoStyleSet.cpp
@@ +1660,5 @@
>  
> +already_AddRefed<ComputedStyle>
> +ServoStyleSet::ReparentComputedStyle(ComputedStyle* aComputedStyle,
> +                                    ComputedStyle* aNewParent,
> +                                    ComputedStyle* aNewParentIgnoringFirstLine,

indent

::: layout/style/ServoStyleSet.h
@@ +467,5 @@
>     */
> +  already_AddRefed<ComputedStyle>
> +  ReparentComputedStyle(ComputedStyle* aComputedStyle,
> +                       ComputedStyle* aNewParent,
> +                       ComputedStyle* aNewParentIgnoringFirstLine,

indent

::: layout/style/nsComputedDOMStyle.cpp
@@ +468,1 @@
>                                      nsAtom* aPseudo,

indent

@@ +498,1 @@
>                                               nsAtom* aPseudo,

indent

@@ +820,1 @@
>                                              uint64_t aGeneration)

indent

@@ +832,1 @@
>                                           uint64_t aGeneration)

indent

::: layout/style/nsFontFaceUtils.cpp
@@ +19,1 @@
>                           const gfxUserFontSet* aUserFontSet,

indent

::: layout/xul/tree/nsTreeBodyFrame.cpp
@@ +4332,3 @@
>  {
> +  return mStyleCache.GetComputedStyle(PresContext(), mContent,
> +                                     mComputedStyle, aPseudoElement,

indent

::: layout/xul/tree/nsTreeStyleCache.cpp
@@ +38,1 @@
>                                    nsIContent* aContent,

indent

Comment 55

[Jonathan Watt [:jwatt]]

Comment on attachment 8961379 [details] [diff] [review]
Actually squashed patch with nits adressed.

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

Given that this is a 1.10 MB patch, I'm going to assume that you squashed correctly and just rubber stamp this one. Same for the remaining handful of indent nits I pointed out that you haven't applied yet.

Comment 56

[Emilio Cobos Álvarez (:emilio)]

Attachment: Servo PR

Comment 57

[Emilio Cobos Álvarez (:emilio)]

Attachment: Rolled up Gecko patch.

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b683bb3f22a1
Merge nsStyleContext and ServoStyleContext, rename to ComputedStyle. r=jwatt

Comment 59

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0016368787a4
Merge nsStyleContext and ServoStyleContext, rename to ComputedStyle. r=jwatt on a CLOSED TREE

Comment 60

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b7c7195b99f4
Back out changeset b683bb3f22a1 for not landing with all the files. r=me on a CLOSED TREE

Comment 61

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/0016368787a4

Comment 62

[Jonathan Watt [:jwatt]]

Xidorn, I meant to ask you to be a second pair of eyes on the main changes in the following files:

layout/style/ComputedStyle.cpp
layout/style/ComputedStyle.h
layout/style/ComputedStyleInlines.h
layout/style/ServoStyleContext.cpp
layout/style/ServoStyleContext.h
layout/style/ServoStyleContextInlines.h

Can you take a look through the diff of those files in the final push and make sure I didn't miss anything?

Comment 63

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8961379 [details] [diff] [review]
Actually squashed patch with nits adressed.

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

Mostly looks fine. Some small nits.

::: layout/style/ComputedStyle.cpp
@@ +30,4 @@
>  #include "mozilla/ArenaObjectID.h"
>  #include "mozilla/StyleSetHandle.h"
>  #include "mozilla/StyleSetHandleInlines.h"
> +#include "mozilla/ComputedStyle.h"

This include is redundant.

@@ +159,4 @@
>    // whether to return a struct or not, since this->mBits might not yet
>    // be correct (due to not calling ResolveSameStructsAs on it yet).
>  #define PEEK(struct_)                                                         \
> +   ComputedData()->GetStyle##struct_()                                        \

The trailing backslash can be removed.

::: layout/style/ComputedStyle.h
@@ +370,5 @@
> +    mCachedInheritingStyles.AddSizeOfIncludingThis(aSizes, aCVsSize);
> +  }
> +
> +  // Needs to be public so that we can call it from Servo.
> +  ~ComputedStyle() = default;

Please keep the destructor protected / private, and add Gecko_ComputedStyle_Destroy as a friend function of this class, instead of making it public.

I don't think we want to accidentally release it in C++ code elsewhere.

::: layout/style/ComputedStyleInlines.h
@@ +16,3 @@
>  
> +#include "mozilla/ComputedStyle.h"
> +#include "mozilla/ComputedStyle.h"

Redundant include.

Comment 64

[Xidorn Quan [:xidorn] UTC+10]

See comment 63.

Comment 65

[Emilio Cobos Álvarez (:emilio)]

I can't make an extern function friend (the compiler complains), unless I'm missing something obvious. I can add an explicit API to ComputedStyle, maybe, how does that sound?

Other comments are addressed in bug 1448559.

Comment 66

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/abac8caba161
and bug 1448850 followup to fix unification-hidden and now-revealed build bustage causing a CLOSED TREE.  r=bkelly

Comment 67

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/abac8caba161