1285474 - stylo: Add dirtiness tracking support to Servo

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

This is the first part (and the simplest, arguably) of the incremental restyle stuff.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1285474: stylo: Reuse some element restyle-related flags for Servo-styled nodes.

Review commit: https://reviewboard.mozilla.org/r/63034/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63034/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1285474: stylo: Add nsINode method for knowing if the current document is using Servo's style back-end.

Review commit: https://reviewboard.mozilla.org/r/63036/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63036/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1285474: stylo: Give a name to the node flags to ease rust binding generation.

Review commit: https://reviewboard.mozilla.org/r/63038/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63038/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1285474: stylo: Add dirtiness-tracking hooks for Servo and convenient methods.

Review commit: https://reviewboard.mozilla.org/r/63040/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63040/

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1285474: stylo: Partially implement some restyling APIs to take rid of some gecko-only code paths.

Review commit: https://reviewboard.mozilla.org/r/63042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63042/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4849cd3ca4d2

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769066 [details]
Bug 1285474: stylo: Partially implement some restyling APIs to take rid of some gecko-only code paths.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63042/diff/1-2/

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8769062 [details]
Bug 1285474: stylo: Reuse some element restyle-related flags for Servo-styled nodes.

https://reviewboard.mozilla.org/r/63034/#review60108

::: dom/base/nsINode.h:184
(Diff revision 1)
>  
>    NODE_CHROME_ONLY_ACCESS =               NODE_FLAG_BIT(19),
>  
>    NODE_IS_ROOT_OF_CHROME_ONLY_ACCESS =    NODE_FLAG_BIT(20),
>  
> +  // Wether this node is dirty for Servo's style system.

"Whether"

::: dom/base/nsINode.h:189
(Diff revision 1)
> +  // Wether this node is dirty for Servo's style system.
> +  //
> +  // These two flags are shared with Gecko's ELEMENT_HAS_PENDING_RESTYLE and
> +  // ELEMENT_IS_POTENTIAL_RESTYLE_ROOT flags.
> +  //
> +  // This meens that every access to this flags in assertions need to be

It's not just about assertions - every access to these flags _in general_ needs to go through the accessors we were talking about:

bool
nsINode::IsDirtyForServo() const {
  MOZ_ASSERT(IsStyledByServo());
  return GetFlags() & NODE_IS_DIRTY_FOR_SERVO;
}

bool
Element::HasPendingRestyle const {
  MOZ_ASSERT(!IsStyledByServo());
  return GetFlags() & ELEMENT_HAS_PENDING_RESTYLE;
}

And so on.

The idea is that every callsite in the tree that checks the flags should go through an accessor that checks to make sure the flags are being interpretted correctly.

::: dom/base/nsINode.h:192
(Diff revision 1)
> +  // ELEMENT_IS_POTENTIAL_RESTYLE_ROOT flags.
> +  //
> +  // This meens that every access to this flags in assertions need to be
> +  // previously checked to see if the document is being styled by Servo's style
> +  // system.
> +  NODE_IS_DIRTY_FOR_SERVO =               NODE_FLAG_BIT(21),

I think we should define these as SHARED_DIRTY_BIT_1 and SHARED_DIRTY_BIT_2, with a comment indicating that they're used for different purposes between the Gecko and Servo style systems.

Then we can define NODE_IS_DIRTY_FOR_SERVO and NODE_HAS_DIRTY_DESCENDANTS_FOR_SERVO as aliases for those bits.

::: dom/base/nsINode.h:194
(Diff revision 1)
> +  // This meens that every access to this flags in assertions need to be
> +  // previously checked to see if the document is being styled by Servo's style
> +  // system.
> +  NODE_IS_DIRTY_FOR_SERVO =               NODE_FLAG_BIT(21),
> +
> +  // Wether this node has dirty descendants for Servo's style system.

"Whether" here, and "means" above.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8769063 [details]
Bug 1285474: stylo: Add nsINode method for knowing if the current document is using Servo's style back-end.

https://reviewboard.mozilla.org/r/63036/#review60110

::: dom/base/nsIDocument.h:1085
(Diff revision 1)
>      return mCSSLoader;
>    }
>  
>    mozilla::StyleBackendType GetStyleBackendType() const;
>  
> +  bool UsingStylo() const {

Lets call this IsStyledByServo to be consistent?

::: dom/base/nsINode.h:971
(Diff revision 1)
>  
>    virtual nsPIDOMWindowOuter* GetOwnerGlobalForBindings() override;
>    virtual nsIGlobalObject* GetOwnerGlobal() const override;
>  
>    /**
> +   * Return wether this node has been restyled by Servo's style system.

""whether".

But I think the comment is a bit misleading - it's not about whether the node has been restyled, it's about whether the document is using servo for styling.

So I'd say: "Returns true if this node belongs to a document that uses the Servo style system."

::: dom/base/nsINode.cpp:3071
(Diff revision 1)
>  }
> +
> +bool
> +nsINode::IsStyledByServo() const
> +{
> +  if (!nsPresContext::StyloEnabled()) {

The idea here is to optimize out the pointer-chase in non-stylo builds, right?

That's a nice idea, but we'll still have a non-inline call here. I think we should instead just #ifdef the definition in nsINode.h to inline return false in non-MOZ_STYLO builds, and be a non-inline version of the function below (without the nsPresContext check) in MOZ_STYLO builds.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8769064 [details]
Bug 1285474: stylo: Give a name to the node flags to ease rust binding generation.

https://reviewboard.mozilla.org/r/63038/#review60114

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8769065 [details]
Bug 1285474: stylo: Add dirtiness-tracking hooks for Servo and convenient methods.

https://reviewboard.mozilla.org/r/63040/#review60116

::: dom/base/nsINode.h:2031
(Diff revision 1)
>  
>    void SetServoNodeData(ServoNodeData* aData) {
>  #ifdef MOZ_STYLO
>    MOZ_ASSERT(!mServoNodeData);
> +  // Mark the node as dirty for the first restyle.
> +  SetFlags(NODE_IS_DIRTY_FOR_SERVO | NODE_HAS_DIRTY_DESCENDANTS_FOR_SERVO);

Hm, I thought we decided to set the bits eagerly?

I guess doing it in the nsINode constructor would be too soon, but we could probably get away with doing it anytime a node is inserted into the document (i.e. in nsINode::SetInDocument), as long as we handle the call in the nsIDocument constructor (which could probably just call SetBoolFlags(IsInDocument) directly).

And wherever we do make this call, I think we should invoke a helper called SetNodeDirtyWithDirtyDescendantsForServo(), which asserts IsStyledByServo.

::: layout/base/nsCSSFrameConstructor.cpp:10545
(Diff revision 1)
>      NS_ASSERTION(!creator || !creator->CreateFrameFor(content),
>                   "If you need to use CreateFrameFor, you need to call "
>                   "CreateAnonymousFrames manually and not follow the standard "
>                   "ProcessChildren() codepath for this frame");
>  #endif
> +    // Anything restyled by servo should already have the style data

Add a |.| after these comments.

::: layout/base/nsCSSFrameConstructor.cpp:10548
(Diff revision 1)
>                   "ProcessChildren() codepath for this frame");
>  #endif
> +    // Anything restyled by servo should already have the style data
> +    MOZ_ASSERT_IF(content->IsStyledByServo(), !!content->GetServoNodeData());
> +    // Gecko-styled nodes should have no pending restyle flags
> +    MOZ_ASSERT_IF(content->IsElement(),

Make this a MOZ_ASSERT_IF(content->IsStyledByServo()) for symmetry.

::: layout/style/ServoBindings.h:179
(Diff revision 1)
>                           ThreadSafeURIHolder* base_uri,
>                           ThreadSafeURIHolder* referrer,
>                           ThreadSafePrincipalHolder* principal);
>  void Gecko_CopyMozBindingFrom(nsStyleDisplay* des, const nsStyleDisplay* src);
>  
> +// Dirtyness tracking.

Surprisingly, this is spelled "Dirtiness". Here and in ServoBindings.cpp.

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8769066 [details]
Bug 1285474: stylo: Partially implement some restyling APIs to take rid of some gecko-only code paths.

https://reviewboard.mozilla.org/r/63042/#review60124

I think we should make a shared superclass called RestyleManagerBase, and hoist shared functionality to that. I know I previously suggested we should just duplicate code, but there's a lot of logic here that's not really related to the underlying style implementation itself, and I think we probably want to share it.

layout/Style/StyleSheet.h is a good example of how to do this. I'm also happy to write the patch if you prefer.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769062 [details]
Bug 1285474: stylo: Reuse some element restyle-related flags for Servo-styled nodes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63034/diff/1-2/

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769063 [details]
Bug 1285474: stylo: Add nsINode method for knowing if the current document is using Servo's style back-end.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63036/diff/1-2/

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769065 [details]
Bug 1285474: stylo: Add dirtiness-tracking hooks for Servo and convenient methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63040/diff/1-2/

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769066 [details]
Bug 1285474: stylo: Partially implement some restyling APIs to take rid of some gecko-only code paths.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63042/diff/2-3/

Comment 17

[Emilio Cobos Álvarez (:emilio)]

https://reviewboard.mozilla.org/r/63034/#review60120

::: dom/base/nsINode.h:192
(Diff revision 1)
> +  // ELEMENT_IS_POTENTIAL_RESTYLE_ROOT flags.
> +  //
> +  // This meens that every access to this flags in assertions need to be
> +  // previously checked to see if the document is being styled by Servo's style
> +  // system.
> +  NODE_IS_DIRTY_FOR_SERVO =               NODE_FLAG_BIT(21),

I don't know how to english, sorry. I meant "needs to be guarded by proper assertions".

Comment 18

[Emilio Cobos Álvarez (:emilio)]

https://reviewboard.mozilla.org/r/63040/#review60150

::: dom/base/nsINode.h:2031
(Diff revision 1)
>  
>    void SetServoNodeData(ServoNodeData* aData) {
>  #ifdef MOZ_STYLO
>    MOZ_ASSERT(!mServoNodeData);
> +  // Mark the node as dirty for the first restyle.
> +  SetFlags(NODE_IS_DIRTY_FOR_SERVO | NODE_HAS_DIRTY_DESCENDANTS_FOR_SERVO);

Yes, regarding this, we can't do it in SetInDocument, since the node data (the normal one, not the servo one) is not being set, so we can't prove that this is a servo-styled document.

Also, it seems that not every node executes this function (the most notable exception is the document), so if you want it there we should set the flags unconditionally (with possible gecko side-effects?), and also special-case the document.

So I'm leaving it here for now.

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 8769062 [details]
Bug 1285474: stylo: Reuse some element restyle-related flags for Servo-styled nodes.

https://reviewboard.mozilla.org/r/63034/#review60154

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8769065 [details]
Bug 1285474: stylo: Add dirtiness-tracking hooks for Servo and convenient methods.

https://reviewboard.mozilla.org/r/63040/#review60158

This looks good, but we _also_ need similar accesses for the Gecko bits on Element, and to route all the access through those accessors. Otherwise we still might accidentally read invalid state from Gecko codepaths.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1285474: Decide style system backend type for documents earlier.

Review commit: https://reviewboard.mozilla.org/r/63334/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63334/

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769065 [details]
Bug 1285474: stylo: Add dirtiness-tracking hooks for Servo and convenient methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63040/diff/2-3/

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769066 [details]
Bug 1285474: stylo: Partially implement some restyling APIs to take rid of some gecko-only code paths.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63042/diff/3-4/

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1285474: stylo: Conditionally compile UpdateStyleBackendType

This silences NS_ASSERTION messages than don't make sense in a non-stylo build.

Review commit: https://reviewboard.mozilla.org/r/63362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63362/

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1285474: Add missing include in nsCSSRenderingBorders.

This compiled before by chance, but was uncovered by the layout/base/moz.build
modifications of the next commit.

Review commit: https://reviewboard.mozilla.org/r/63364/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63364/

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1285474: Add mozilla::RestyleManagerBase to share logic between RestyleManager and ServoRestyleManager

Review commit: https://reviewboard.mozilla.org/r/63366/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63366/

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769062 [details]
Bug 1285474: stylo: Reuse some element restyle-related flags for Servo-styled nodes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63034/diff/2-3/

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769063 [details]
Bug 1285474: stylo: Add nsINode method for knowing if the current document is using Servo's style back-end.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63036/diff/2-3/

Comment 29

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769065 [details]
Bug 1285474: stylo: Add dirtiness-tracking hooks for Servo and convenient methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63040/diff/3-4/

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769066 [details]
Bug 1285474: stylo: Partially implement some restyling APIs to take rid of some gecko-only code paths.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63042/diff/4-5/

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769406 [details]
Bug 1285474: Decide style system backend type for documents earlier.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63334/diff/1-2/

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769458 [details]
Bug 1285474: stylo: Conditionally compile UpdateStyleBackendType

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63362/diff/1-2/

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769459 [details]
Bug 1285474: Add missing include in nsCSSRenderingBorders.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63364/diff/1-2/

Comment 34

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769460 [details]
Bug 1285474: Add mozilla::RestyleManagerBase to share logic between RestyleManager and ServoRestyleManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63366/diff/1-2/

Comment 35

[Bobby Holley (:bholley)]

Comment on attachment 8769063 [details]
Bug 1285474: stylo: Add nsINode method for knowing if the current document is using Servo's style back-end.

https://reviewboard.mozilla.org/r/63036/#review60156

::: dom/base/nsINode.cpp:3073
(Diff revision 3)
> +#ifdef MOZ_STYLO
> +bool
> +nsINode::IsStyledByServo() const
> +{
> +  nsIDocument* doc = OwnerDoc();
> +  return doc && doc->IsStyledByServo();

I think this can just be OwnerDoc()->IsStyledByServo.

Comment 36

[Bobby Holley (:bholley)]

Comment on attachment 8769065 [details]
Bug 1285474: stylo: Add dirtiness-tracking hooks for Servo and convenient methods.

https://reviewboard.mozilla.org/r/63040/#review60420

::: dom/base/nsDocument.cpp:1463
(Diff revision 4)
>      mPartID(0),
>      mDidFireDOMContentLoaded(true),
>      mHasScrollLinkedEffect(false),
>      mUserHasInteracted(false)
>  {
> -  SetInDocument();
> +  SetIsDocument();

Let's rename SetInDocument to SetIsInDocument to make this distinction a bit clearer.

Comment 37

[Bobby Holley (:bholley)]

Comment on attachment 8769406 [details]
Bug 1285474: Decide style system backend type for documents earlier.

https://reviewboard.mozilla.org/r/63334/#review60426

Thanks for upstreaming this. :-)

Comment 38

[Bobby Holley (:bholley)]

Comment on attachment 8769458 [details]
Bug 1285474: stylo: Conditionally compile UpdateStyleBackendType

https://reviewboard.mozilla.org/r/63362/#review60428

Can you squash this into the other?

Comment 39

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769063 [details]
Bug 1285474: stylo: Add nsINode method for knowing if the current document is using Servo's style back-end.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63036/diff/3-4/

Comment 40

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769065 [details]
Bug 1285474: stylo: Add dirtiness-tracking hooks for Servo and convenient methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63040/diff/4-5/

Comment 41

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769406 [details]
Bug 1285474: Decide style system backend type for documents earlier.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63334/diff/2-3/

Comment 42

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8769460 [details]
Bug 1285474: Add mozilla::RestyleManagerBase to share logic between RestyleManager and ServoRestyleManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63366/diff/2-3/

Comment 43

[Bobby Holley (:bholley)]

Comment on attachment 8769460 [details]
Bug 1285474: Add mozilla::RestyleManagerBase to share logic between RestyleManager and ServoRestyleManager

https://reviewboard.mozilla.org/r/63366/#review60452

::: layout/base/RestyleManager.h:15
(Diff revision 3)
>  
>  #ifndef mozilla_RestyleManager_h
>  #define mozilla_RestyleManager_h
>  
>  #include "mozilla/RestyleLogging.h"
> +#include "RestyleManagerBase.h"

Namespace this.

::: layout/base/RestyleManagerBase.h:41
(Diff revision 3)
> +      mObservingRefreshDriver = aObserving;
> +  }
> +
> +  void Disconnect() { mPresContext = nullptr; }
> +
> +protected:

Can we make the data private and add some protected accessors as needed?

::: layout/base/RestyleManagerBase.cpp:7
(Diff revision 3)
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "RestyleManagerBase.h"

Namespace this.

::: layout/base/ServoRestyleManager.h:11
(Diff revision 3)
>  
>  #ifndef mozilla_ServoRestyleManager_h
>  #define mozilla_ServoRestyleManager_h
>  
>  #include "mozilla/EventStates.h"
> +#include "RestyleManagerBase.h"

Namespace this.

::: layout/base/ServoRestyleManager.h:78
(Diff revision 3)
>  
> -  uint64_t mRestyleGeneration;
> +private:
> +  nsPresContext* PresContext() const { return mPresContext; }
> +  nsCSSFrameConstructor* FrameConstructor() const {
> +    return mPresContext->FrameConstructor();
> +  }

These should be removed.

::: layout/base/ServoRestyleManager.cpp:36
(Diff revision 3)
> +  if (!mObservingRefreshDriver) {
> +    mObservingRefreshDriver = mPresContext->RefreshDriver()->
> +      AddStyleFlushObserver(presShell);
> +  }

This should go through the accessors.

::: layout/base/ServoRestyleManager.cpp:109
(Diff revision 3)
> +  Element* aElement = aContent->AsElement();
> +  nsIFrame* primaryFrame = aElement->GetPrimaryFrame();
> +  if (primaryFrame) {
> +    primaryFrame->ContentStatesChanged(aStateMask);
> +  }
> +
> +  if (aStateMask.HasState(NS_EVENT_STATE_HOVER)) {
> +    ++mHoverGeneration;
> +  }

Hoist this into protected RestyleManagerBase::ContentStateChangedInternal?

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41a8a926e9b9
stylo: Reuse some element restyle-related flags for Servo-styled nodes. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa86942f16b
stylo: Add nsINode method for knowing if the current document is using Servo's style back-end. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/74c8690be96d
stylo: Add dirtiness-tracking hooks for Servo and convenient methods. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/f25ce46d4eac
Decide style system backend type for documents earlier. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/769a86b5787d
Add mozilla::RestyleManagerBase to share logic between RestyleManager and ServoRestyleManager. r=bholley

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/41a8a926e9b9
https://hg.mozilla.org/mozilla-central/rev/cfa86942f16b
https://hg.mozilla.org/mozilla-central/rev/74c8690be96d
https://hg.mozilla.org/mozilla-central/rev/f25ce46d4eac
https://hg.mozilla.org/mozilla-central/rev/769a86b5787d

Comment 46

[Bobby Holley (:bholley)]

Note that some patches from bug 1281393 landed with this bug number by accident.