Closed Bug 1328420 Opened 8 years ago Closed 8 years ago

stylo: implement child stylesheets

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

Details

Attachments

(8 files)

Bug 1328420 Part 1: Uplift parent pointer and accessor to StyleSheet class.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1328420 Part 2: Uplift mNext into StyleSheet.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1328420 Part 3: Uplift the first child to StyleSheet via a new method, abstracting out the inner sheet concept (which is not present in Stylo sheets).
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1328420 Part 4: Uplift SetOwningDocument into StyleSheet.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1328420 Part 5: Uplift AppendStyleSheet to StyleSheet.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1328420 Part 6: Implement ServoStyleSheet destructor.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1328420 Part 7: Uplift List debug method into StyleSheet.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1328420 Part 8: Uplift SizeOfIncludingThis into StyleSheet, with override in CSSStyleSheet.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
ServoStyleSheet::GetParentSheet currently executes MOZ_CRASH, and this method is hit when bringing up the Style Inspector. Implement GetParentSheet thoroughly enough to eliminate this point of failure when bringing up the Style Inspector. Additionally port over any existing tests of child stylesheet support in Gecko style system.
Attachment #8823887 - Flags: review?(cam)
Attachment #8823888 - Flags: review?(cam)
Attachment #8823889 - Flags: review?(cam)
This pretty much obsoletes bug 1326242, which was about fixing this too.

Brad, can you remove the IsServo() check for the css Loader? (http://searchfox.org/mozilla-central/source/layout/style/Loader.cpp#2224)
See Also: → 1326242, 1317427
(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> Brad, can you remove the IsServo() check for the css Loader?
> (http://searchfox.org/mozilla-central/source/layout/style/Loader.cpp#2224)

er, the IsGecko() check, I mean.

And thanks for fixing this :)
It looks like most of these method implementations on ServoStyleSheet are the same as, or similar to, those on CSSStyleSheet.  Instead of having separate methods, can you hoist as much as you can up to StyleSheet itself (including the mNext/mParent/mNextChild, making them StyleSheet rather than ServoStyleSheet pointers)?  (See StyleSheet::SetEnabled for an example of one method that is mostly the same, but defers some processing in the middle to a CSSStyleSheet / ServoStyleSheet.)
Flags: needinfo?(bwerth)
Attachment #8828178 - Flags: review?(cam)
Attachment #8828179 - Flags: review?(cam)
Comment on attachment 8823887 [details]
Bug 1328420 Part 1: Uplift parent pointer and accessor to StyleSheet class.

https://reviewboard.mozilla.org/r/102352/#review106574

::: layout/style/CSSStyleSheet.cpp:410
(Diff revision 2)
> +  NS_ASSERTION(!aParentToUse || aParentToUse->IsGecko(),
> +    "Parent sheet must be a CSS sheet.");

We don't need this check, since aParentToUse is declared to be a CSSStyleSheet.
Attachment #8823887 - Flags: review?(cam) → review+
Comment on attachment 8823888 [details]
Bug 1328420 Part 2: Uplift mNext into StyleSheet.

https://reviewboard.mozilla.org/r/102354/#review106590

::: layout/style/CSSStyleSheet.cpp:235
(Diff revision 2)
> -    s = s->mNext;
> +    NS_ASSERTION(!s->mNext || s->mNext->IsGecko(),
> +      "Next sheet should be a CSS sheet.");
> +    s = static_cast<CSSStyleSheet*>(s->mNext.get());

It would be good to use AsGecko() and rely on the assertion inside it.  So can we rewrite these two statements as:

  s = s->mNext ? s->mNext.AsGecko() : nullptr;

::: layout/style/CSSStyleSheet.cpp:251
(Diff revision 2)
> -  ChildSheetListBuilder builder = { &mFirstChild, aPrimarySheet };
> +  RefPtr<StyleSheet> firstChild(mFirstChild);
> +  ChildSheetListBuilder builder = { &firstChild, aPrimarySheet };

Is this necessary?  It looks like you've changed mFirstChild to be a RefPtr<StyleSheet> already in this patch, so I think we don't need this local variable.

::: layout/style/CSSStyleSheet.cpp:479
(Diff revision 2)
> -  RefPtr<CSSStyleSheet> child;
> -  child.swap(mInner->mFirstChild);
> +  RefPtr<StyleSheet> child(mInner->mFirstChild.get());
> +  mInner->mFirstChild.forget();

Same here.  Instead, I think we can just change |child|'s type and leave the existing swap() call.

Note that the forget() call here leaks, since it returns an already_AddRefed, which we don't use.  (We should get an assertion in already_AddRefed's destructor complaining that we didn't take its value.)

::: layout/style/CSSStyleSheet.cpp:741
(Diff revision 2)
> +    NS_ASSERTION(child->IsGecko(),
> +      "First and next sheets should be CSS sheets.");
> +    CSSStyleSheet* childAsCSS = static_cast<CSSStyleSheet*>(child);
> +
> +    aArray.AppendElement(childAsCSS);

Rely on AsGecko()'s assertion:

  aArray.AppendElement(child->AsGecko());

::: layout/style/StyleSheet.h:235
(Diff revision 2)
>  
>    const StyleBackendType mType;
>    bool                  mDisabled;
> +
> +  friend struct mozilla::ChildSheetListBuilder;
> +  friend class mozilla::CSSStyleSheet;

This this friend declaration needed?  Looks like most StyleSheet's members are protected.

::: layout/style/nsCSSRuleProcessor.cpp:3669
(Diff revision 2)
> +    NS_ASSERTION(!aSheet->mInner->mFirstChild ||
> +      aSheet->mInner->mFirstChild->IsGecko(),
> +      "First sheet should be a CSS sheet.");
> +    CSSStyleSheet* child = static_cast<CSSStyleSheet*>(
> +      aSheet->mInner->mFirstChild.get());
>      while (child) {
>        CascadeSheet(child, aData);
> -      child = child->mNext;
> +
> +      NS_ASSERTION(!child->mNext || child->mNext->IsGecko(),
> +        "Next sheet should be a CSS sheet.");
> +      child = static_cast<CSSStyleSheet*>(child->mNext.get());
>      }

Similarly, I think this can just be:

  StyleSheet* child = aSheet->mInner->mFirstChild;
  while (child) {
    CascadeSheet(child->AsGecko(), aData);
    child = child->mNext;
  }
Attachment #8823888 - Flags: review?(cam) → review+
Comment on attachment 8823889 [details]
Bug 1328420 Part 3: Uplift the first child to StyleSheet via a new method, abstracting out the inner sheet concept (which is not present in Stylo sheets).

https://reviewboard.mozilla.org/r/102356/#review106600

For mNext, I think we should hoist this up to StyleSheetInfo instead of StyleSheet, although we should still have an accessor method StyleSheet.  (StyleSheetInfo is the class that is used for data that is present on both ServoStyleSheets and CSSStyleSheetInners.)

See for example StyleSheet::IsComplete() for an example of an accessor that sets data on the StyleSheetInfo.
Attachment #8823889 - Flags: review?(cam) → review-
Comment on attachment 8823889 [details]
Bug 1328420 Part 3: Uplift the first child to StyleSheet via a new method, abstracting out the inner sheet concept (which is not present in Stylo sheets).

https://reviewboard.mozilla.org/r/102356/#review106604

::: layout/style/ServoStyleSheet.h:80
(Diff revision 2)
>    nsIDOMCSSRule* GetDOMOwnerRule() const final;
>  
>    void WillDirty() {}
>    void DidDirty() {}
>  
> +  RefPtr<StyleSheet>* GetFirstChild() final;

Also, with the move of mFirstChild to StyleSheetInfo, we can just have a single accessor on StyleSheet that just returns |SheetInfo().mFirstChild| as a StyleSheet*, rather than returning a pointer to the RefPtr.
Comment on attachment 8828178 [details]
Bug 1328420 Part 4: Uplift SetOwningDocument into StyleSheet.

https://reviewboard.mozilla.org/r/105656/#review106606

::: layout/style/StyleSheet.cpp:371
(Diff revision 1)
> +  for (RefPtr<StyleSheet>* child = GetFirstChild();
> +       *child; child = &(*child)->mNext) {
> +    if ((*child)->mParent == this) {
> +      (*child)->SetOwningDocument(aDocument);
> +    }
> +  }

With the suggested changes to the previous patch, I guess this will become:

  for (StyleSheet* child = GetFirstChild(); ...
Attachment #8828178 - Flags: review?(cam) → review+
Comment on attachment 8828179 [details]
Bug 1328420 Part 5: Uplift AppendStyleSheet to StyleSheet.

https://reviewboard.mozilla.org/r/105658/#review106610

::: layout/style/CSSStyleSheet.cpp:650
(Diff revision 1)
>    WillDirty();
> -  RefPtr<StyleSheet>* tail = &mInner->mFirstChild;
> -  while (*tail) {
> -    tail = &(*tail)->mNext;
> +
> +  StyleSheet::AppendStyleSheet(aSheet);
> +
> -  }
> -  *tail = aSheet;
> -
> -  // This is not reference counted. Our parent tells us when
> -  // it's going away.
> -  aSheet->mParent = this;
> -  aSheet->mDocument = mDocument;
>    DidDirty();

Ah, it looks like at the moment that we already forward on WillDirty / DidDirty to the CSSStyleSheet version, and do nothing for ServoStyleSheets.  (See the definitions in StyleSheetInlines.h.)  So I think we can simplify this and just have a single AppendStyleSheet method up on StyleSheet, which unconditionally calls WillDirty / DidDirty.
Attachment #8828179 - Flags: review?(cam) → review-
Comment on attachment 8823889 [details]
Bug 1328420 Part 3: Uplift the first child to StyleSheet via a new method, abstracting out the inner sheet concept (which is not present in Stylo sheets).

https://reviewboard.mozilla.org/r/102356/#review106604

> Also, with the move of mFirstChild to StyleSheetInfo, we can just have a single accessor on StyleSheet that just returns |SheetInfo().mFirstChild| as a StyleSheet*, rather than returning a pointer to the RefPtr.

I tried this initially and found it difficult to implement AppendStyleSheet without introducing an algorithm change. I'll go ahead and make a raw pointer accessor.
See Also: → 1332353
Comment on attachment 8823888 [details]
Bug 1328420 Part 2: Uplift mNext into StyleSheet.

https://reviewboard.mozilla.org/r/102354/#review106590

> This this friend declaration needed?  Looks like most StyleSheet's members are protected.

For reasons I don't understand, compilation fails on CSSStyleSheet access of mNext and mParent and mDocument without this directive. I'll keep poking at it and look for another fix that doesn't require this unintutive directive. Failing that, I'll add a comment.
Comment on attachment 8823888 [details]
Bug 1328420 Part 2: Uplift mNext into StyleSheet.

https://reviewboard.mozilla.org/r/102354/#review106590

> For reasons I don't understand, compilation fails on CSSStyleSheet access of mNext and mParent and mDocument without this directive. I'll keep poking at it and look for another fix that doesn't require this unintutive directive. Failing that, I'll add a comment.

Ah, the problem is that CSSStyleSheet methods are trying to access the mNext pointers of OTHER objects which are StyleSheets. Not allowed without a friend declaration:
https://isocpp.org/wiki/faq/classes-and-objects#directly-access-private-in-other-instances

I'll keep the declaration in and add a comment.
Attachment #8828594 - Flags: review?(cam)
Attachment #8828595 - Flags: review?(cam)
Attachment #8828596 - Flags: review?(cam)
Addressed all the issues in the previous review (thank you, heycam), and implemented a few more missing methods. The implementation of the destructor in attachment 8828179 [details] does not uplift the common logic because C++ destructor order forces the base class code to execute after the subclass, and that code needs to execute first.
Flags: needinfo?(bwerth)
Note that this patch will conflict with bug 1332353, so whoever lands second will have to do some rebasing.
Comment on attachment 8823889 [details]
Bug 1328420 Part 3: Uplift the first child to StyleSheet via a new method, abstracting out the inner sheet concept (which is not present in Stylo sheets).

https://reviewboard.mozilla.org/r/102356/#review106942

r=me with these changes.

::: layout/style/CSSStyleSheet.cpp:149
(Diff revision 3)
>    }
>    return true;
>  }
>  
>  struct ChildSheetListBuilder {
> -  RefPtr<StyleSheet>* sheetSlot;
> +  StyleSheet* sheetSlot;

Shouldn't this remain a RefPtr<StyleSheet>*?  Otherwise how can we write to CSSStyleSheet::mFirstChild from the mOrderedRules.EnumerateForwards() call?

::: layout/style/CSSStyleSheet.cpp:500
(Diff revision 3)
>    // because otherwise there are no JS wrappers for anything in the inner.
>    if (mInner->mSheets.Length() != 1) {
>      return;
>    }
>  
> -  RefPtr<StyleSheet>* childSheetSlot = &mInner->mFirstChild;
> +  StyleSheet* childSheetSlot = GetFirstChild();

Probably can rename this to childSheet (since I think "slot" here is used to suggest that we had a pointer to the RefPtr that contains the StyleSheet* pointer).

::: layout/style/CSSStyleSheet.cpp:656
(Diff revision 3)
> -  RefPtr<StyleSheet>* tail = &mInner->mFirstChild;
> -  while (*tail) {
> -    tail = &(*tail)->mNext;
> +  StyleSheet* tail = GetFirstChild();
> +  if (!tail) {
> +    // Use aSheet as the first child.
> +    SheetInfo().mFirstChild = aSheet;
> +  } else {
> +    StyleSheet* lastChild = nullptr;
> +    while (tail) {
> +      lastChild = tail;
> +      tail = tail->mNext;
> +    }
> +    lastChild->mNext = aSheet;
>    }
> -  *tail = aSheet;

I think this change is OK, but I guess we could have left the previous (shorter) code if we just rewrote the first line to:

  RefPtr<StyleSheet>* tail = &SheetInfo().mFirstChild

?

::: layout/style/ServoStyleSheet.h:91
(Diff revision 3)
> +  RefPtr<StyleSheet> mFirstChild;
> +

We shouldn't add this member, right?  Since we have one in mSheetInfo?
Attachment #8823889 - Flags: review?(cam) → review+
Comment on attachment 8828179 [details]
Bug 1328420 Part 5: Uplift AppendStyleSheet to StyleSheet.

https://reviewboard.mozilla.org/r/105658/#review106944
Attachment #8828179 - Flags: review?(cam) → review+
Comment on attachment 8828594 [details]
Bug 1328420 Part 6: Implement ServoStyleSheet destructor.

https://reviewboard.mozilla.org/r/105924/#review106946

r=me with that.

::: layout/style/ServoStyleSheet.cpp:32
(Diff revision 1)
> +  for (StyleSheet* child = GetFirstChild();
> +       child;
> +       child = child->mNext) {
> +    if (child->mParent == this) {
> +      child->mParent = nullptr;
> +      child->mDocument = nullptr;
> +    }
> +  }

Instead of duplicating the code, can you add a protected method to StyleSheet that both ~CSSStyleSheet and ~ServoStyleSheet can call, which does this child sheet clearing?
Attachment #8828594 - Flags: review?(cam) → review+
Comment on attachment 8828595 [details]
Bug 1328420 Part 7: Uplift List debug method into StyleSheet.

https://reviewboard.mozilla.org/r/105926/#review106948
Attachment #8828595 - Flags: review?(cam) → review+
Comment on attachment 8828594 [details]
Bug 1328420 Part 6: Implement ServoStyleSheet destructor.

https://reviewboard.mozilla.org/r/105924/#review106950

::: layout/style/CSSStyleSheet.cpp
(Diff revision 1)
> -    // XXXbz this is a little bogus; see the XXX comment where we
> -    // declare mFirstChild.

Is this comment no longer accurate?  If it is still accurate, please keep it.
Comment on attachment 8828596 [details]
Bug 1328420 Part 8: Uplift SizeOfIncludingThis into StyleSheet, with override in CSSStyleSheet.

https://reviewboard.mozilla.org/r/105928/#review106952
Attachment #8828596 - Flags: review?(cam) → review+
Comment on attachment 8823887 [details]
Bug 1328420 Part 1: Uplift parent pointer and accessor to StyleSheet class.

https://reviewboard.mozilla.org/r/102350/#review108050

Ready for push.
Keywords: checkin-needed
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8fe43f283d6c
Part 1: Uplift parent pointer and accessor to StyleSheet class. r=heycam
https://hg.mozilla.org/integration/autoland/rev/99afe80d3abe
Part 2: Uplift mNext into StyleSheet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/f9ccc7d902d2
Part 3: Uplift the first child to StyleSheet via a new method, abstracting out the inner sheet concept (which is not present in Stylo sheets). r=heycam
https://hg.mozilla.org/integration/autoland/rev/3d49a652a180
Part 4: Uplift SetOwningDocument into StyleSheet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/3ceb5cad92df
Part 5: Uplift AppendStyleSheet to StyleSheet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e32841ab90d0
Part 6: Implement ServoStyleSheet destructor. r=heycam
https://hg.mozilla.org/integration/autoland/rev/99b47f5198fc
Part 7: Uplift List debug method into StyleSheet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/b3bfcc29a579
Part 8: Uplift SizeOfIncludingThis into StyleSheet, with override in CSSStyleSheet. r=heycam
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: