Closed Bug 1373018 Opened 7 years ago Closed 7 years ago

stylo: Split nsStyleContext into GeckoStyleContext and ServoStyleContext

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

Attachments

(11 files, 2 obsolete files)

Bug 1373018 - Part 1: stylo: Introduce ServoStyleContext and GeckoStyleContext subclasses;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1373018 - Part 2: stylo: Add stylo conversion methods for nsStyleContext; stop using arena;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1373018 - Part 3: stylo: Move mPresContext to ServoStyleContext;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1373018 - Part 4: stylo: Rename eArenaObjectID_nsStyleContext to eArenaObjectID_GeckoStyleContext;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1373018 - Part 5: stylo: Move child/sibling pointers to GeckoStyleContext;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1373018 - Part 6: stylo: Move most Gecko-specific methods into GeckoStyleContext;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1373018 - Part 7: stylo: Move nsStyleContext::mSource into subclasses;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1373018 - Part 8: stylo: Move nsStyleContext::SetStyle to GeckoStyleContext;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1373018 - Part 9: stylo: Make more things on nsStyleContext inlined;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1373018 - Part 10: stylo: Remove StyleSource;
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1373018 - Part 11: stylo: Move cached style structs to GeckoStyleContext;
59 bytes, text/x-review-board-request
bholley
: review+
Details 
Comment on attachment 8877733 [details]
Bug 1373018 - Part 1: stylo: Introduce ServoStyleContext and GeckoStyleContext subclasses;

https://reviewboard.mozilla.org/r/149160/#review153640
Attachment #8877733 - Flags: review?(bobbyholley) → review+
Comment on attachment 8877734 [details]
Bug 1373018 - Part 2: stylo: Add stylo conversion methods for nsStyleContext; stop using arena;

https://reviewboard.mozilla.org/r/149162/#review153644
Attachment #8877734 - Flags: review?(bobbyholley) → review+
Comment on attachment 8877735 [details]
Bug 1373018 - Part 3: stylo: Move mPresContext to ServoStyleContext;

https://reviewboard.mozilla.org/r/149164/#review153646

::: commit-message-3c491:1
(Diff revision 1)
> +Bug 1373018 - Part 3: stylo: Move mPresContext to ServoStyleContext; stop using arena; r?bholley
> +
> +MozReview-Commit-ID: 2BmRpIjxEO8

Nit: the "stop using arena" part happened in the previous patch.
Attachment #8877735 - Flags: review?(bobbyholley) → review+
Comment on attachment 8877736 [details]
Bug 1373018 - Part 4: stylo: Rename eArenaObjectID_nsStyleContext to eArenaObjectID_GeckoStyleContext;

https://reviewboard.mozilla.org/r/149166/#review153650
Attachment #8877736 - Flags: review?(bobbyholley) → review+
Comment on attachment 8877737 [details]
Bug 1373018 - Part 5: stylo: Move child/sibling pointers to GeckoStyleContext;

https://reviewboard.mozilla.org/r/149168/#review153656

r=me with those fixes.

::: layout/style/GeckoStyleContext.cpp:63
(Diff revision 1)
> +void
> +GeckoStyleContext::AddChild(GeckoStyleContext* aChild)
> +{
> +  NS_ASSERTION(aChild->mPrevSibling == aChild &&
> +               aChild->mNextSibling == aChild,

I'm assuming all these were just moved, and am not reviewing them.

::: layout/style/nsStyleContext.cpp:256
(Diff revision 1)
> -  if (mChild) {
> -    const nsStyleContext* child = mChild;
> +  if (const GeckoStyleContext* gecko = GetAsGecko()) {
> +    if (gecko->mChild) {
> +      const GeckoStyleContext* child = gecko->mChild;
> -    do {
> +      do {
> -      child->AssertStructsNotUsedElsewhere(aDestroyingContext, aLevels - 1);
> +        child->AssertStructsNotUsedElsewhere(aDestroyingContext, aLevels - 1);
> -      child = child->mNextSibling;
> +        child = child->mNextSibling;
> -    } while (child != mChild);
> +      } while (child != gecko->mChild);
> -  }
> +    }
>  
> -  if (mEmptyChild) {
> -    const nsStyleContext* child = mEmptyChild;
> +    if (gecko->mEmptyChild) {
> +      const GeckoStyleContext* child = gecko->mEmptyChild;
> -    do {
> +      do {
> -      child->AssertStructsNotUsedElsewhere(aDestroyingContext, aLevels - 1);
> +        child->AssertStructsNotUsedElsewhere(aDestroyingContext, aLevels - 1);
> -      child = child->mNextSibling;
> +        child = child->mNextSibling;
> -    } while (child != mEmptyChild);
> +      } while (child != gecko->mEmptyChild);
> +    }

We should have the caller of this function (~nsStyleContext) only invoke it in gecko mode, then MOZ_ASSERT(IsGecko()) at the beginning of this function, and drop the conditionals.

::: layout/style/nsStyleContext.cpp:389
(Diff revision 1)
>  nsStyleContext::GetUniqueStyleData(const nsStyleStructID& aSID)
>  {
>    MOZ_ASSERT(!mSource.IsServoComputedValues(),
>               "Can't COW-mutate servo values from Gecko!");
>  
>    // If we already own the struct and no kids could depend on it, then
>    // just return it.  (We leak in this case if there are kids -- and this
>    // function really shouldn't be called for style contexts that could
>    // have kids depending on the data.  ClearStyleData would be OK, but
>    // this test for no mChild or mEmptyChild doesn't catch that case.)
>    const void *current = StyleData(aSID);
> -  if (!mChild && !mEmptyChild &&
> +  GeckoStyleContext *child = nullptr, *emptyChild = nullptr;
> +  if (const GeckoStyleContext* gecko = GetAsGecko()) {
> +    child = gecko->mChild;
> +    emptyChild = gecko->mEmptyChild;
> +  }
> +  if (!child && !emptyChild &&

You should be able to MOZ_ASSERT(IsGecko()) in this function and drop the conditional.

::: layout/style/nsStyleContext.cpp:442
(Diff revision 1)
>  nsStyleContext::CreateEmptyStyleData(const nsStyleStructID& aSID)
>  {
> -  MOZ_ASSERT(!mChild && !mEmptyChild &&
> -             !(mBits & nsCachedStyleData::GetBitForSID(aSID)) &&
> +  if (const GeckoStyleContext* gecko = GetAsGecko()) {
> +    MOZ_ASSERT(!gecko->mChild && !gecko->mEmptyChild, "This style should not have been computed");
> +  }
> +  MOZ_ASSERT(!(mBits & nsCachedStyleData::GetBitForSID(aSID)) &&
>               !GetCachedStyleData(aSID),
>               "This style should not have been computed");

Same here.

::: layout/style/nsStyleContext.cpp:1463
(Diff revision 1)
>  {
>    if (mBits & NS_STYLE_INELIGIBLE_FOR_SHARING) {
>      return;
>    }
>    mBits |= NS_STYLE_INELIGIBLE_FOR_SHARING;
> -  if (mChild) {
> +  if (const GeckoStyleContext* gecko = GetAsGecko()) {

Same here.

::: layout/style/nsStyleContext.cpp:1585
(Diff revision 1)
> +
> +nsPresContext*
> +nsStyleContext::PresContext() const
> +{
> +    MOZ_STYLO_FORWARD(PresContext, ())
> +}

Doesn't this belong in the previous patch?
Attachment #8877737 - Flags: review?(bobbyholley) → review+
Attachment #8877740 - Attachment is obsolete: true
Attachment #8877740 - Flags: review?(bobbyholley)
Comment on attachment 8877738 [details]
Bug 1373018 - Part 6: stylo: Move most Gecko-specific methods into GeckoStyleContext;

https://reviewboard.mozilla.org/r/149170/#review153662

Oh I see, you did that in this patch.
Attachment #8877738 - Flags: review?(bobbyholley) → review+
Comment on attachment 8877739 [details]
Bug 1373018 - Part 7: stylo: Move nsStyleContext::mSource into subclasses;

https://reviewboard.mozilla.org/r/149172/#review153664

::: layout/style/GeckoStyleContext.h:98
(Diff revision 2)
>    // meaningful.
>    GeckoStyleContext* mChild;
>    GeckoStyleContext* mEmptyChild;
>    GeckoStyleContext* mPrevSibling;
>    GeckoStyleContext* mNextSibling;
> +  RefPtr<nsRuleNode> mSource;

We should rename this mRuleNode.

::: layout/style/GeckoStyleContext.cpp:74
(Diff revision 2)
>  {
>    NS_ASSERTION(aChild->mPrevSibling == aChild &&
>                 aChild->mNextSibling == aChild,
>                 "child already in a child list");
>  
> -  GeckoStyleContext **listPtr = aChild->mSource.MatchesNoRules() ? &mEmptyChild : &mChild;
> +  GeckoStyleContext **listPtr = &mChild;

Presumably you can remove the MatchesNoRules stuff from NonOwningStyleSource now, as well as the entirety of OwningStyleContextSource.

::: layout/style/GeckoStyleContext.cpp:102
(Diff revision 2)
>  GeckoStyleContext::RemoveChild(GeckoStyleContext* aChild)
>  {
>    NS_PRECONDITION(nullptr != aChild && this == aChild->mParent, "bad argument");
>  
> -  GeckoStyleContext **list = aChild->mSource.MatchesNoRules() ? &mEmptyChild : &mChild;
> +  GeckoStyleContext **list = &mChild;
> +  if (const nsRuleNode* source = aChild->mSource) {

You don't need to null-check this - we didn't before I added the OwningStyleContextSource stuff [1]. Please just MOZ_ASSERT the existence of the rule node in the construct and remove the null-check here.

[1] https://hg.mozilla.org/releases/mozilla-esr45/file/tip/layout/style/nsStyleContext.cpp#l264

::: layout/style/GeckoStyleContext.cpp:204
(Diff revision 2)
>  GeckoStyleContext::FindChildWithRules(const nsIAtom* aPseudoTag,
>                                     NonOwningStyleContextSource aSource,
>                                     NonOwningStyleContextSource aSourceIfVisited,

You can just make these nsRuleNode pointers now, and drop the usage of StyleContextSource here. Really, all the StyleContextSource stuff can just go away.

::: layout/style/GeckoStyleContext.cpp:361
(Diff revision 2)
>        child = child->mNextSibling;
>      } while (mEmptyChild != child);
>    }
>  }
>  
> +#ifdef DEBUG

I think this should be #ifdef RESTYLE_LOGGING.

::: layout/style/nsStyleContext.h:481
(Diff revision 2)
> +  ~nsStyleContext() {}
> +  // Where the actual destructor lives
> +  // We use this instead of a real destructor because we need
> +  // this to be called *before* the subclass fields are destroyed
> +  // by the subclass destructor
> +  void Destructor();

Nit: By convention, this should be called Destroy().

::: layout/style/nsStyleContext.h:556
(Diff revision 2)
>            return nullptr;                                               \
>          }                                                               \
>          /* Have the rulenode deal */                                    \
>          AUTO_CHECK_DEPENDENCY(eStyleStruct_##name_);                    \
>          const nsStyle##name_ * newData =                                \
> -          mSource.AsGeckoRuleNode()->                                   \
> +          StyleSource().AsGeckoRuleNode()->                             \

It would be faster to do GetAsGecko directly at the top, rather than branching a second time here to create the StyleSource() and then a third time to cast it back to a GeckoRuleNode.

::: layout/style/nsStyleContext.h:598
(Diff revision 2)
>        if (!aComputeData && needToCompute) {                             \
>          return nullptr;                                                 \
>        }                                                                 \
>                                                                          \
>        const nsStyle##name_* data =                                      \
> -        Servo_GetStyle##name_(mSource.AsServoComputedValues());         \
> +        Servo_GetStyle##name_(StyleSource().AsServoComputedValues());   \

Similarly, we should just operate directly on a ServoStyleContext* here.

::: layout/style/nsStyleContext.h:613
(Diff revision 2)
>      }
>  
>    #define STYLE_STRUCT_RESET(name_, checkdata_cb_)                      \
>      template<bool aComputeData>                                         \
>      const nsStyle##name_ * DoGetStyle##name_() {                        \
> -      if (mSource.IsGeckoRuleNode()) {                                  \
> +      if (IsGecko()) {                                                  \

Same here and below.
Attachment #8877739 - Flags: review?(bobbyholley) → review+
Comment on attachment 8877733 [details]
Bug 1373018 - Part 1: stylo: Introduce ServoStyleContext and GeckoStyleContext subclasses;

https://reviewboard.mozilla.org/r/149160/#review153706

::: layout/style/GeckoStyleContext.cpp:30
(Diff revision 1)
> +#endif
> +
> +  if (aParent) {
> +#ifdef DEBUG
> +    nsRuleNode *r1 = mParent->RuleNode(), *r2 = mSource.AsGeckoRuleNode();
> +    while (r1->GetParent())

You have a `RuleTree()` function that does just this . I know this is probably pre-existing though.

::: layout/style/nsStyleContext.h:507
(Diff revision 1)
>    }
>  
>    mozilla::NonOwningStyleContextSource StyleSource() const { return mSource.AsRaw(); }
>  
> -private:
> +protected:
>    // Private destructor, to discourage deletion outside of Release():

nit: Update the comment?
Comment on attachment 8877739 [details]
Bug 1373018 - Part 7: stylo: Move nsStyleContext::mSource into subclasses;

https://reviewboard.mozilla.org/r/149172/#review153664

> Nit: By convention, this should be called Destroy().

we already have one of those.

> It would be faster to do GetAsGecko directly at the top, rather than branching a second time here to create the StyleSource() and then a third time to cast it back to a GeckoRuleNode.

Can't, GetAsGecko can't be used here yet. Will fix this in another patch (I'm not done with this bug yet)
Comment on attachment 8877811 [details]
fixup! Bug 1373018 - Part 7: stylo: Move nsStyleContext::mSource into subclasses;

https://reviewboard.mozilla.org/r/149226/#review153722
Attachment #8877811 - Flags: review?(bobbyholley) → review+
Attachment #8877811 - Attachment is obsolete: true
Comment on attachment 8877733 [details]
Bug 1373018 - Part 1: stylo: Introduce ServoStyleContext and GeckoStyleContext subclasses;

https://reviewboard.mozilla.org/r/149160/#review153706

> nit: Update the comment?

(gets fixed later)
Comment on attachment 8877799 [details]
Bug 1373018 - Part 8: stylo: Move nsStyleContext::SetStyle to GeckoStyleContext;

https://reviewboard.mozilla.org/r/149220/#review153728
Attachment #8877799 - Flags: review?(bobbyholley) → review+
Alright, this patch series is complete. I'll handle the mCachedFooData stuff at some point later, that's a bit trickier and doesn't need to be done now.

I'll work on bug 1367904 next
Comment on attachment 8877840 [details]
Bug 1373018 - Part 9: stylo: Make more things on nsStyleContext inlined;

https://reviewboard.mozilla.org/r/149270/#review153740

r=me assuming this is just moving code around.
Attachment #8877840 - Flags: review?(bobbyholley) → review+
Comment on attachment 8877841 [details]
Bug 1373018 - Part 10: stylo: Remove StyleSource;

https://reviewboard.mozilla.org/r/149272/#review154136
Attachment #8877841 - Flags: review?(bobbyholley) → review+
Comment on attachment 8878269 [details]
Bug 1373018 - Part 11: stylo: Move cached style structs to GeckoStyleContext;

https://reviewboard.mozilla.org/r/149604/#review154266
Attachment #8878269 - Flags: review?(bobbyholley) → review+
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e4ebccf607e3
Part 1: stylo: Introduce ServoStyleContext and GeckoStyleContext subclasses; r=bholley
https://hg.mozilla.org/integration/autoland/rev/52c1509f515e
Part 2: stylo: Add stylo conversion methods for nsStyleContext; stop using arena; r=bholley
https://hg.mozilla.org/integration/autoland/rev/20db5241e456
Part 3: stylo: Move mPresContext to ServoStyleContext; r=bholley
https://hg.mozilla.org/integration/autoland/rev/df118b3de43b
Part 4: stylo: Rename eArenaObjectID_nsStyleContext to eArenaObjectID_GeckoStyleContext; r=bholley
https://hg.mozilla.org/integration/autoland/rev/5492eb087406
Part 5: stylo: Move child/sibling pointers to GeckoStyleContext; r=bholley
https://hg.mozilla.org/integration/autoland/rev/3dcb1623e115
Part 6: stylo: Move most Gecko-specific methods into GeckoStyleContext; r=bholley
https://hg.mozilla.org/integration/autoland/rev/305613bee6fb
Part 7: stylo: Move nsStyleContext::mSource into subclasses; r=bholley
https://hg.mozilla.org/integration/autoland/rev/02462591f243
Part 8: stylo: Move nsStyleContext::SetStyle to GeckoStyleContext; r=bholley
https://hg.mozilla.org/integration/autoland/rev/b422d7f837f2
Part 9: stylo: Make more things on nsStyleContext inlined; r=bholley
https://hg.mozilla.org/integration/autoland/rev/3b5bae7326a2
Part 10: stylo: Remove StyleSource; r=bholley
https://hg.mozilla.org/integration/autoland/rev/64a2ba65f0d6
Part 11: stylo: Move cached style structs to GeckoStyleContext; r=bholley
Blocks: 1368290
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/120ac289ac48
followup: Fix indentation in nsStyleContext::Destroy. r=whitespace-only
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: