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: