stylo: Cache servo stylesheets

RESOLVED FIXED in Firefox 54

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: Bobby Holley (parental leave - send mail for anything urgent), Assigned: bradwerth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(13 attachments, 8 obsolete attachments)

59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
In css:Loader::DoSheetComplete, we skip the caching logic in the servo case. This presumably has a negative impact on performance and memory usage.
Priority: -- → P3

Comment 1

11 months ago
If there is anything we can do to support a global parsed stylesheet cache (bug 219276) that would be nice.  It has some significant wins in e10s mode when navigating around within a site.
(Assignee)

Comment 2

9 months ago
Investigating...
Assignee: nobody → bwerth
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8832710 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8832711 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8835111 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8835112 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8832212 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8832708 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8835113 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8832213 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8835174 - Flags: review?(cam)
Attachment #8835175 - Flags: review?(cam)
Attachment #8835176 - Flags: review?(cam)
Attachment #8835177 - Flags: review?(cam)
Attachment #8835178 - Flags: review?(cam)
Attachment #8835179 - Flags: review?(cam)
Attachment #8835180 - Flags: review?(cam)
Attachment #8835181 - Flags: review?(cam)
Attachment #8835182 - Flags: review?(cam)
Attachment #8832709 - Flags: review?(cam)
Comment on attachment 8835174 [details]
Bug 1290218 Part 1: Uplift IsModified into StyleSheet from CSSStyleSheet, and provide empty implementation for ServoStyleSheet.

https://reviewboard.mozilla.org/r/110856/#review113258

::: layout/style/CSSStyleSheet.h:138
(Diff revision 2)
>    already_AddRefed<CSSStyleSheet> Clone(CSSStyleSheet* aCloneParent,
>                                          css::ImportRule* aCloneOwnerRule,
>                                          nsIDocument* aCloneDocument,
>                                          nsINode* aCloneOwningNode) const;
>  
> -  bool IsModified() const { return mDirty; }
> +  bool IsModified() const final override { return mDirty; }

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style says this should just be "final" rather than "final override".

::: layout/style/ServoStyleSheet.h:71
(Diff revision 2)
>    css::Rule* GetDOMOwnerRule() const final;
>  
>    void WillDirty() {}
>    void DidDirty() {}
>  
> +  bool IsModified() const final override { return false; }

Here too.
Attachment #8835174 - Flags: review?(cam) → review+
Comment on attachment 8835175 [details]
Bug 1290218 Part 2: Uplift mInner pointer from CSSStyleSheetInner into StyleSheetInfo.

https://reviewboard.mozilla.org/r/110858/#review113266

r=me with these addressed.

::: layout/style/CSSStyleSheet.h:132
(Diff revision 2)
>    css::ImportRule* GetOwnerRule() const { return mOwnerRule; }
>    // Workaround overloaded-virtual warning in GCC.
>    using StyleSheet::GetOwnerRule;
>  
> -  nsXMLNameSpaceMap* GetNameSpaceMap() const { return mInner->mNameSpaceMap; }
> +  nsXMLNameSpaceMap* GetNameSpaceMap() const {
> +    return static_cast<CSSStyleSheetInner*>(mInner)->mNameSpaceMap;

Instead of having static_asserts throughout various methods on CSSStyleSheet, how about we add a (private) method

  CSSStyleSheetInner* Inner()
  {
    return static_cast<CSSStyleSheetInner*>(mInner);
  }

so we can avoid repeating it?  Then it'll be clear enough not to have a separate |inner| local variable where we need it, and we can just operate directly on Inner().

::: layout/style/CSSStyleSheet.cpp
(Diff revision 2)
> -    mInner(aCopy.mInner),
>      mRuleProcessors(nullptr)
>  {
>    mParent = aParentToUse;
> +  mInner = aCopy.mInner;

Any reason we have to do this?  Probably better to keep it in the initializer list, if possible.

::: layout/style/StyleSheet.h:262
(Diff revision 2)
>    // the sense that if it's known-live then we're known-live).  Always
>    // NotOwnedByDocument when mDocument is null.
>    DocumentAssociationMode mDocumentAssociationMode;
>  
> +  // Core information we get from parsed sheets, which are shared amongst
> +  // Stylesheet clones.

Nit: "StyleSheet".

::: layout/style/StyleSheet.cpp:46
(Diff revision 2)
>    , mType(aCopy.mType)
>    , mDisabled(aCopy.mDisabled)
>      // We only use this constructor during cloning.  It's the cloner's
>      // responsibility to notify us if we end up being owned by a document.
>    , mDocumentAssociationMode(NotOwnedByDocument)
> +  , mInner(nullptr)

It might make sense to copy the mInner pointer here, and just leave the AddSheet call in CSSStyleSheet instead.  If not, then let's add a comment in here pointing out that we copy mInner in the concrete class.
Attachment #8835175 - Flags: review?(cam) → review+
Comment on attachment 8835175 [details]
Bug 1290218 Part 2: Uplift mInner pointer from CSSStyleSheetInner into StyleSheetInfo.

https://reviewboard.mozilla.org/r/110858/#review113282

::: layout/style/CSSStyleSheet.cpp
(Diff revision 2)
> -    mInner(aCopy.mInner),
>      mRuleProcessors(nullptr)
>  {
>    mParent = aParentToUse;
> +  mInner = aCopy.mInner;

I realize of course that it's because it's on the superclass now.
Comment on attachment 8835176 [details]
Bug 1290218 Part 3: Transition ServoStyleSheet to use mInner pointer, instead of maintaining a static StyleSheetInfo.

https://reviewboard.mozilla.org/r/110860/#review113284
Attachment #8835176 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8835175 [details]
Bug 1290218 Part 2: Uplift mInner pointer from CSSStyleSheetInner into StyleSheetInfo.

https://reviewboard.mozilla.org/r/110858/#review113540

::: layout/style/CSSStyleSheet.h:132
(Diff revision 3)
>    css::ImportRule* GetOwnerRule() const { return mOwnerRule; }
>    // Workaround overloaded-virtual warning in GCC.
>    using StyleSheet::GetOwnerRule;
>  
> -  nsXMLNameSpaceMap* GetNameSpaceMap() const { return mInner->mNameSpaceMap; }
> +  nsXMLNameSpaceMap* GetNameSpaceMap() const {
> +    return static_cast<CSSStyleSheetInner*>(mInner)->mNameSpaceMap;

This one can use Inner() too.

::: layout/style/nsCSSRuleProcessor.cpp:3676
(Diff revision 3)
> -    if (!aSheet->mInner->mOrderedRules.EnumerateForwards(CascadeRuleEnumFunc,
> +    CSSStyleSheetInner* inner = static_cast<CSSStyleSheetInner*>(aSheet->mInner);
> +    if (!inner->mOrderedRules.EnumerateForwards(CascadeRuleEnumFunc,

Use aSheet->Inner() here too.
Comment on attachment 8835177 [details]
Bug 1290218 Part 4: Implement shared mInners for ServoStyleSheets, and standardize calling of AddSheet into CSSStyleSheet and ServoStyleSheet constructors.

https://reviewboard.mozilla.org/r/110862/#review113550

This generally looks good but a couple of things need fixing up.

::: layout/style/CSSStyleSheet.cpp:265
(Diff revision 3)
> -  mSheets.AppendElement(aSheet);
> -}
> +  // Get some information now that we'll use if we survive the superclass
> +  // implementation of RemoveSheet.
> +  bool removingFirstOfSeveral = (aSheet == mSheets.ElementAt(0)) &&
> +                                (mSheets.Length() > 1);
>  
> -void
> -CSSStyleSheetInner::RemoveSheet(CSSStyleSheet* aSheet)
> -{
> +  StyleSheetInfo::RemoveSheet(aSheet);
> +
> +  if (removingFirstOfSeveral) {

I'm uneasy about duplicating the logic here of when RemoveSheet will end up deleting our object, and then doing work on the object after this call.

What do you think about structuring the function like this:

  if (mSheets[0] == aSheet && mSheets.Length() > 1) {
    mOrderedRules.EnumerateForwards(..., mSheets[1]);
    ...::ReparentChildList(mSheets[1], ...);
  }
  StyleSheetInfo::RemoveSheet(aSheet);
  // WARNING: RemoveSheet might delete this object!

::: layout/style/CSSStyleSheet.cpp:398
(Diff revision 3)
>      mInRuleProcessorCache(false),
>      mScopeElement(nullptr),
>      mRuleProcessors(nullptr)
>  {
> +  MOZ_ASSERT(mInner, "We should have an mInner after copy.");
> +  MOZ_ASSERT(mInner->mSheets.Contains(this), "Our mInner should include us.");

Is this assertion correct?  I think we'll do a shallow copy of the mInner pointer up in the StyleSheet and do the AddSheet() call in the StyleSheet copy constructor.

::: layout/style/CSSStyleSheet.cpp:402
(Diff revision 3)
> +  MOZ_ASSERT(mInner, "We should have an mInner after copy.");
> +  MOZ_ASSERT(mInner->mSheets.Contains(this), "Our mInner should include us.");
> +
>    mParent = aParentToUse;
>  
>    Inner()->AddSheet(this);

This shouldn't be needed if we do the AddSheet in the StyleSheet copy constructor now.

::: layout/style/StyleSheet.cpp:167
(Diff revision 3)
> +StyleSheetInfo::StyleSheetInfo(StyleSheetInfo& aCopy,
> +                               StyleSheet* aPrimarySheet)
> +  : mSheetURI(aCopy.mSheetURI)
> +  , mOriginalSheetURI(aCopy.mOriginalSheetURI)
> +  , mBaseURI(aCopy.mBaseURI)
> +  , mPrincipal(aCopy.mPrincipal)
> +  , mCORSMode(aCopy.mCORSMode)
> +  , mReferrerPolicy(aCopy.mReferrerPolicy)
> +  , mIntegrity(aCopy.mIntegrity)
> +  , mComplete(aCopy.mComplete)
> +  , mFirstChild()  // We don't rebuild the child because we're making a copy
> +                   // without children.
> +{

We should #ifdef DEBUG copy over mPrincipalSet too.

::: layout/style/StyleSheetInfo.h:60
(Diff revision 3)
>  #ifdef DEBUG
>    bool                   mPrincipalSet;
>  #endif
> +
> +  AutoTArray<StyleSheet*, 8> mSheets;

Nit: This is not too important, but if there's no other reason I'd keep the DEBUG-only bool at the end, so that mSheets is in the same position in the object in both debug and release builds.
Attachment #8835177 - Flags: review?(cam) → review-
Comment on attachment 8835178 [details]
Bug 1290218 Part 5: Subclass StyleSheetInfo to move the ServoStyleSheet raw sheet into its mInner.

https://reviewboard.mozilla.org/r/110864/#review113558

r=me with that.

::: layout/style/ServoStyleSheet.h:70
(Diff revision 3)
> -  RawServoStyleSheet* RawSheet() const { return mSheet; }
> +  RawServoStyleSheet* RawSheet() const {
> +    ServoStyleSheetInner* inner = static_cast<ServoStyleSheetInner*>(mInner);
> +    return inner->mSheet;
> +  }
>    void SetSheetForImport(RawServoStyleSheet* aSheet) {
> -    MOZ_ASSERT(!mSheet);
> +    ServoStyleSheetInner* inner = static_cast<ServoStyleSheetInner*>(mInner);

Here too I would have a private Inner() function that does the cast (and use it throughout the .cpp file).
Attachment #8835178 - Flags: review?(cam) → review+
Comment on attachment 8835179 [details]
Bug 1290218 Part 6: Generalize nsXULPrototypeCache to hold StyleSheet instead of CSSStyleSheet.

https://reviewboard.mozilla.org/r/110866/#review113570
Attachment #8835179 - Flags: review?(cam) → review+
Comment on attachment 8835180 [details]
Bug 1290218 Part 7: Bypass XULPrototypeCache (which is a singleton) for non-Gecko Loaders.

https://reviewboard.mozilla.org/r/110868/#review113564

r=me with that.

::: layout/style/Loader.cpp:1117
(Diff revision 3)
> -    if (IsChromeURI(aURI)) {
> +    // The XUL cache is a singleton that only holds Gecko-style sheets, so
> +    // only use the cache if the loader is also Gecko.
> +    if (IsChromeURI(aURI) &&
> +        mStyleBackendType.isSome() &&
> +        mStyleBackendType.value() == StyleBackendType::Gecko) {

Can you file a followup bug to allow ServoStyleSheets to be stored in the nsXULPrototypeCache too?  (It is probably important for performance.)  I guess we can just have two hash tables for the two types of sheets.

::: layout/style/Loader.cpp:1120
(Diff revision 3)
> +        mStyleBackendType.isSome() &&
> +        mStyleBackendType.value() == StyleBackendType::Gecko) {

Use GetStyleBackendType() instead (which will call through to mDocument to find the type, when mStyleBackendType is Nothing).

::: layout/style/Loader.cpp:1966
(Diff revision 3)
> +              mStyleBackendType.isSome() &&
> +              mStyleBackendType.value() == StyleBackendType::Gecko) {

Same here.

::: layout/style/Loader.cpp:1966
(Diff revision 3)
> +              mStyleBackendType.isSome() &&
> +              mStyleBackendType.value() == StyleBackendType::Gecko) {

Here too.
Attachment #8835180 - Flags: review?(cam) → review+
Comment on attachment 8835181 [details]
Bug 1290218 Part 8: Implement ServoStyleSheet Clone.

https://reviewboard.mozilla.org/r/110870/#review113572

r=me with this.

::: layout/style/ServoStyleSheet.h:24
(Diff revision 3)
>  class ServoCSSRuleList;
>  
>  namespace css {
>  class Loader;
>  class Rule;
> +class ImportRule;

Nit: two lines up to keep this (short) list sorted.

::: layout/style/ServoStyleSheet.h:92
(Diff revision 3)
> +  already_AddRefed<ServoStyleSheet> Clone(ServoStyleSheet* aCloneParent,
> +                                          css::ImportRule* aCloneOwnerRule,
> +                                          nsIDocument* aCloneDocument,
> +                                          nsINode* aCloneOwningNode) const;

Should we just make Clone() virtual (and pure virtual on StyleSheet) and return already_AddRefed<StyleSheet>?  That would make the calls to it simpler in CreateSheet in the next patch.
Attachment #8835181 - Flags: review?(cam) → review+
Comment on attachment 8835182 [details]
Bug 1290218 Part 9: Generalize stylesheet Loader to cache Servo sheets in the same way that it caches Gecko sheets.

https://reviewboard.mozilla.org/r/110872/#review113574
Attachment #8835182 - Flags: review?(cam) → review+
Comment on attachment 8832709 [details]
Bug 1290218 Part 10: Add asserts to ServoStyleSets.

https://reviewboard.mozilla.org/r/108900/#review113578
Attachment #8832709 - Flags: review?(cam) → review+
Please also file a followup bug to handle CSSOM sheet modifications, doing something like the mDirty handling on CSSStyleSheet, otherwise we can end up having two sheets sharing an inner, and using CSSOM to modify one will end up modifying both.
(Assignee)

Updated

8 months ago
See Also: → bug 1339627
(Assignee)

Updated

8 months ago
See Also: → bug 1339629
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 74

8 months ago
mozreview-review
Comment on attachment 8835181 [details]
Bug 1290218 Part 8: Implement ServoStyleSheet Clone.

https://reviewboard.mozilla.org/r/110870/#review114240

::: layout/style/ServoStyleSheet.h:92
(Diff revision 3)
> +  already_AddRefed<ServoStyleSheet> Clone(ServoStyleSheet* aCloneParent,
> +                                          css::ImportRule* aCloneOwnerRule,
> +                                          nsIDocument* aCloneDocument,
> +                                          nsINode* aCloneOwningNode) const;

Did this, and it led to several new cases of new static_casts. I think the trade off is still worth it.
(Assignee)

Comment 75

8 months ago
mozreview-review
Comment on attachment 8835177 [details]
Bug 1290218 Part 4: Implement shared mInners for ServoStyleSheets, and standardize calling of AddSheet into CSSStyleSheet and ServoStyleSheet constructors.

https://reviewboard.mozilla.org/r/110862/#review113800

::: layout/style/CSSStyleSheet.cpp:265
(Diff revision 3)
> -  mSheets.AppendElement(aSheet);
> -}
> +  // Get some information now that we'll use if we survive the superclass
> +  // implementation of RemoveSheet.
> +  bool removingFirstOfSeveral = (aSheet == mSheets.ElementAt(0)) &&
> +                                (mSheets.Length() > 1);
>  
> -void
> -CSSStyleSheetInner::RemoveSheet(CSSStyleSheet* aSheet)
> -{
> +  StyleSheetInfo::RemoveSheet(aSheet);
> +
> +  if (removingFirstOfSeveral) {

I'll do this. I was being sensitive about reversing the order of operations between the reparenting and the removal of aSheet from mSheets. But I'll ensure that the code will still work with the operations in this order, and structure the code in the clearer way you propose.

::: layout/style/CSSStyleSheet.cpp:398
(Diff revision 3)
>      mInRuleProcessorCache(false),
>      mScopeElement(nullptr),
>      mRuleProcessors(nullptr)
>  {
> +  MOZ_ASSERT(mInner, "We should have an mInner after copy.");
> +  MOZ_ASSERT(mInner->mSheets.Contains(this), "Our mInner should include us.");

At this point, the StyleSheet copy constructor has been called, and it calls AddSheet, so this assertion holds.

::: layout/style/CSSStyleSheet.cpp:402
(Diff revision 3)
> +  MOZ_ASSERT(mInner, "We should have an mInner after copy.");
> +  MOZ_ASSERT(mInner->mSheets.Contains(this), "Our mInner should include us.");
> +
>    mParent = aParentToUse;
>  
>    Inner()->AddSheet(this);

Yes, this was an erroneous carryover. AddSheet should stay in the StyleSheet copy constructor, so we don't have to override it in subclasses just to get this behavior.

::: layout/style/StyleSheetInfo.h:60
(Diff revision 3)
>  #ifdef DEBUG
>    bool                   mPrincipalSet;
>  #endif
> +
> +  AutoTArray<StyleSheet*, 8> mSheets;

Good idea.
(Assignee)

Comment 76

8 months ago
mozreview-review
Comment on attachment 8835175 [details]
Bug 1290218 Part 2: Uplift mInner pointer from CSSStyleSheetInner into StyleSheetInfo.

https://reviewboard.mozilla.org/r/110858/#review113394

::: layout/style/CSSStyleSheet.h:132
(Diff revision 2)
>    css::ImportRule* GetOwnerRule() const { return mOwnerRule; }
>    // Workaround overloaded-virtual warning in GCC.
>    using StyleSheet::GetOwnerRule;
>  
> -  nsXMLNameSpaceMap* GetNameSpaceMap() const { return mInner->mNameSpaceMap; }
> +  nsXMLNameSpaceMap* GetNameSpaceMap() const {
> +    return static_cast<CSSStyleSheetInner*>(mInner)->mNameSpaceMap;

Much better solution; thank you for the suggestion.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8837877 - Flags: review?(cam)
Comment on attachment 8835177 [details]
Bug 1290218 Part 4: Implement shared mInners for ServoStyleSheets, and standardize calling of AddSheet into CSSStyleSheet and ServoStyleSheet constructors.

https://reviewboard.mozilla.org/r/110862/#review114756

::: layout/style/CSSStyleSheet.cpp:265
(Diff revisions 3 - 5)
> -  // Get some information now that we'll use if we survive the superclass
> +  if ((aSheet == mSheets.ElementAt(0)) && (mSheets.Length() > 1)) {
> -  // implementation of RemoveSheet.
> -  bool removingFirstOfSeveral = (aSheet == mSheets.ElementAt(0)) &&
> -                                (mSheets.Length() > 1);
> -
> -  StyleSheetInfo::RemoveSheet(aSheet);
> -
> -  if (removingFirstOfSeveral) {
>      mOrderedRules.EnumerateForwards(SetStyleSheetReference,
> -                                    mSheets.ElementAt(0));
> +                                    mSheets.ElementAt(1));
>  
> -    ChildSheetListBuilder::ReparentChildList(mSheets[0], mFirstChild);
> +    ChildSheetListBuilder::ReparentChildList(mSheets[1], mFirstChild);
>    }

Nit: if you like you can standardize on using [] instead of ElementAt().  (Probably this array used to be an nsCOMArray, which used not to have an operator[].)
Attachment #8835177 - Flags: review?(cam) → review+
Comment on attachment 8837877 [details]
Bug 1290218 Part 4b: Fixup cycle collector declarations for mInner moving into StyleSheetInfo.

https://reviewboard.mozilla.org/r/112872/#review114758
Attachment #8837877 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 101

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=68e662ec0369
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8839715 - Flags: review?(cam)
Comment on attachment 8839715 [details]
Bug 1290218 Part 11: Update mochitest failure count, and mark a reftest as failing.

https://reviewboard.mozilla.org/r/114292/#review115766
Attachment #8839715 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Blocks: 1342289
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8840690 - Flags: review?(cam)
Comment on attachment 8840690 [details]
Bug 1290218 Part 12: Turn off a crashtest that leaks memory, see Bug 1342289.

https://reviewboard.mozilla.org/r/115120/#review116598
Attachment #8840690 - Flags: review?(cam) → review+

Comment 141

8 months ago
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1b319a801847
Part 1: Uplift IsModified into StyleSheet from CSSStyleSheet, and provide empty implementation for ServoStyleSheet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/ea165fd4359a
Part 2: Uplift mInner pointer from CSSStyleSheetInner into StyleSheetInfo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/5e9b9569c297
Part 3: Transition ServoStyleSheet to use mInner pointer, instead of maintaining a static StyleSheetInfo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/b433bbbd6228
Part 4: Implement shared mInners for ServoStyleSheets, and standardize calling of AddSheet into CSSStyleSheet and ServoStyleSheet constructors. r=heycam
https://hg.mozilla.org/integration/autoland/rev/f42cbc7439d0
Part 4b: Fixup cycle collector declarations for mInner moving into StyleSheetInfo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/bc83f0653f30
Part 5: Subclass StyleSheetInfo to move the ServoStyleSheet raw sheet into its mInner. r=heycam
https://hg.mozilla.org/integration/autoland/rev/6cb592ba64ec
Part 6: Generalize nsXULPrototypeCache to hold StyleSheet instead of CSSStyleSheet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/cd6d1b2b9b6c
Part 7: Bypass XULPrototypeCache (which is a singleton) for non-Gecko Loaders. r=heycam
https://hg.mozilla.org/integration/autoland/rev/a72b5585f19c
Part 8: Implement ServoStyleSheet Clone. r=heycam
https://hg.mozilla.org/integration/autoland/rev/da2bee86bcb2
Part 9: Generalize stylesheet Loader to cache Servo sheets in the same way that it caches Gecko sheets. r=heycam
https://hg.mozilla.org/integration/autoland/rev/fd354b31d6e3
Part 10: Add asserts to ServoStyleSets. r=heycam
https://hg.mozilla.org/integration/autoland/rev/5b9d8ea1d448
Part 11: Update mochitest failure count, and mark a reftest as failing. r=heycam
https://hg.mozilla.org/integration/autoland/rev/8a73e70ee40a
Part 12: Turn off a crashtest that leaks memory, see Bug 1342289. r=heycam

Comment 142

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1b319a801847
https://hg.mozilla.org/mozilla-central/rev/ea165fd4359a
https://hg.mozilla.org/mozilla-central/rev/5e9b9569c297
https://hg.mozilla.org/mozilla-central/rev/b433bbbd6228
https://hg.mozilla.org/mozilla-central/rev/f42cbc7439d0
https://hg.mozilla.org/mozilla-central/rev/bc83f0653f30
https://hg.mozilla.org/mozilla-central/rev/6cb592ba64ec
https://hg.mozilla.org/mozilla-central/rev/cd6d1b2b9b6c
https://hg.mozilla.org/mozilla-central/rev/a72b5585f19c
https://hg.mozilla.org/mozilla-central/rev/da2bee86bcb2
https://hg.mozilla.org/mozilla-central/rev/fd354b31d6e3
https://hg.mozilla.org/mozilla-central/rev/5b9d8ea1d448
https://hg.mozilla.org/mozilla-central/rev/8a73e70ee40a
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Updated

8 months ago
Duplicate of this bug: 1317427
Depends on: 1350789
You need to log in before you can comment on or make changes to this bug.