Closed Bug 1388625 Opened 7 years ago Closed 7 years ago

stylo: Restyle various wrapper anonymous boxes

Categories

(Core :: CSS Parsing and Computation, enhancement)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(7 files)

Worst-case performance testcase, of sorts
2.86 MB, text/html
Details
Bug 1388625 part 1. During a stylo restyle, update the style of the trailing anonymous colgroup of a table, if any.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1388625 part 2. During a stylo restyle, update the style of the anonymous cols in a colgroup, if any.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1388625 part 3. Add nsIFrame flags we will use in stylo post-traversal to keep track of wrapper anonymous boxes.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1388625 part 5. Implement wrapper anonymous box restyling in ServoRestyleManager.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1388625 part 6. Flag the in-flow frames of kids of various wrapper frames during frame construction, so we know to restyle the wrapper frames.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1388625 part 4. Add a concept of wrapper anon boxes to nsCSSAnonBoxes.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
This includes anonymous table and ruby boxes, as well as the block wrappers mathml creates.
This slows down 2x or so with this bug fixed, because we end up with lots of anon box kids of anon boxes and our anon box style caching doesn't work, etc.

That does make us slower than Gecko by 2x on this testcase, unfortunately.  If we decide this matters, well need to fix that caching bit.

I did test these patches on obama-noscript, myspace, amazon, youtube and they're all completely unnoticeable performance-wise on those.
Comment on attachment 8895523 [details]
Bug 1388625 part 1.  During a stylo restyle, update the style of the trailing anonymous colgroup of a table, if any.

https://reviewboard.mozilla.org/r/166730/#review172062

::: layout/tables/nsTableFrame.cpp:8038
(Diff revision 1)
>      OwnedAnonBox(wrapper, &UpdateStyleOfOwnedAnonBoxesForTableWrapper));
> +
> +  // We may also have an anonymous colgroup that we're responsible for.
> +  // Specifically, we can have three types of colgroup frames: (A) corresponding
> +  // to actual elements with "display: table-column-group", (B) wrapping runs of
> +  // "display: table-column" kinds, and (C) one colgroup frame added at the end

*kids

::: layout/tables/nsTableFrame.cpp:8049
(Diff revision 1)
> +  // eColGroupAnonymousCol, and type (C) to eColGroupAnonymousCell.  But we
> +  // never actually set eColGroupAnonymousCol on any colgroups right now; see
> +  // bug 1387568.  In any case, eColGroupAnonymousCell works correctly to detect
> +  // colgroups of type (C), which are the ones we want to restyle here.  Type
> +  // (A) will be restyled via their element, and type (B) via the machinery for
> +  // restyling wrapper anonymous frames..

Nit: one too many "."s.

::: layout/tables/nsTableFrame.cpp:8052
(Diff revision 1)
> +  // colgroups of type (C), which are the ones we want to restyle here.  Type
> +  // (A) will be restyled via their element, and type (B) via the machinery for
> +  // restyling wrapper anonymous frames..
> +  auto colGroupFrame =
> +    static_cast<nsTableColGroupFrame*>(mColGroups.LastChild());
> +  if (colGroupFrame && colGroupFrame->GetColType() == eColGroupAnonymousCell) {

For (B), is it that we never create an anonymous colgroup frame for it, or that we do but use a different coltype value?  If we never create one this is fine, but if we do, we should be careful this coltype check here is sufficient.
Attachment #8895523 - Flags: review?(cam) → review+
Comment on attachment 8895524 [details]
Bug 1388625 part 2.  During a stylo restyle, update the style of the anonymous cols in a colgroup, if any.

https://reviewboard.mozilla.org/r/166732/#review172108
Attachment #8895524 - Flags: review?(cam) → review+
Comment on attachment 8895525 [details]
Bug 1388625 part 3.  Add nsIFrame flags we will use in stylo post-traversal to keep track of wrapper anonymous boxes.

https://reviewboard.mozilla.org/r/166734/#review172092

::: layout/generic/nsIFrame.h:4165
(Diff revision 1)
> +   * We could compute this information directly when we need it, but it wouldn't
> +   * be all that cheap, and since this information is immutable for the lifetime
> +   * of the frame we might as well cache it.

That it's immutable for the lifetime of the frame sounds plausible to me, although I would need to check we never re-parent children of wrapper anonymous boxes... can/should we assert this somewhere, maybe in nsIFrame::SetParent?

::: layout/generic/nsIFrame.h:4171
(Diff revision 1)
> +   * be all that cheap, and since this information is immutable for the lifetime
> +   * of the frame we might as well cache it.
> +   *
> +   * Note that our parent may itself have mParentIsWrapperAnonBox set to true.
> +   */
> +  bool mParentIsWrapperAnonBox;

": 1" on this and the other field.
Attachment #8895525 - Flags: review?(cam) → review+
Comment on attachment 8895526 [details]
Bug 1388625 part 5.  Implement wrapper anonymous box restyling in ServoRestyleManager.

https://reviewboard.mozilla.org/r/166736/#review172094

I think this is OK, but a couple of questions.

::: layout/base/ServoRestyleManager.h:116
(Diff revision 1)
> +  // wrappers nested in it.  Returns the number of entries from
> +  // mPendingWrapperRestyles that we processed.

Maybe mention that this always returns at least 1?

::: layout/base/ServoRestyleManager.h:122
(Diff revision 1)
> +  // mPendingWrapperRestyles that we processed.
> +  size_t ProcessMaybeNestedWrapperRestyle(nsIFrame* aParent, size_t aIndex);
> +
>    ServoStyleSet& mStyleSet;
>    nsStyleChangeList& mChangeList;
> +  // A list of pending wrapper restyles.  Anonymous box wrapper frames that need

Nit: blank line before this comment.

::: layout/base/ServoRestyleManager.h:130
(Diff revision 1)
> +  // for these anonymous boxes.  The problem then becomes that we can have
> +  // multiple kids all with the same anonymous parent, and we don't want to
> +  // restyle it more than once.  We use mPendingWrapperRestyles to track which
> +  // anonymous wrapper boxes we've requested be restyled and which of them have
> +  // already been restyled.  We use a single array propagated through
> +  // ServoRestyleStates by reference, because in situation like this:

*situations

::: layout/base/ServoRestyleManager.h:160
(Diff revision 1)
>  #endif
> +
> +  // Whether we should assert in our destructor that we've processed all of the
> +  // relevant wrapper restyles.
> +#ifdef DEBUG
> +  const bool mAssertWrapperRestyleLength { true };

(Not sure I'm a fan of uniform initialization syntax for simple things like this...)

::: layout/base/ServoRestyleManager.cpp:129
(Diff revision 1)
> +  MOZ_ASSERT(aWrapperFrame->StyleContext()->GetPseudo() !=
> +             nsCSSAnonBoxes::cellContent,
> +             "Someone should be using TableAwareParentFor");

Should we check for table wrapper frames creeping in here too?

::: layout/base/ServoRestyleManager.cpp:144
(Diff revision 1)
> +
> +  // Make sure to queue up parents before children.  But don't queue up
> +  // ancestors of non-anonymous boxes here; those are handled when we traverse
> +  // their non-anonymous kids.
> +  if (aWrapperFrame->ParentIsWrapperAnonBox() &&
> +      IsAnonBox(*aWrapperFrame)) {

Do we want to check specifically whether this is an inheriting anonymous box?  Actually, haven't we basically asserted this at the top of the function?

Should mPendingWrapperRestyles only ever contain inheriting anonymous boxes?

::: layout/base/ServoRestyleManager.cpp:147
(Diff revision 1)
> +  // their non-anonymous kids.
> +  if (aWrapperFrame->ParentIsWrapperAnonBox() &&
> +      IsAnonBox(*aWrapperFrame)) {
> +    AddPendingWrapperRestyle(TableAwareParentFor(aWrapperFrame));
> +  }
> +  

Trailing space.

::: layout/base/ServoRestyleManager.cpp:158
(Diff revision 1)
> +  for (size_t i = mPendingWrapperRestyleOffset;
> +       i < mPendingWrapperRestyles.Length();
> +       /* i is incremented in the body */) {
> +    i += ProcessMaybeNestedWrapperRestyle(aParentFrame, i);
> +  }

Clearer IMO if written as a while loop, if you're happy for the i to be in scope after the loop...

::: layout/base/ServoRestyleManager.cpp:173
(Diff revision 1)
> +  MOZ_ASSERT(aIndex < mPendingWrapperRestyles.Length());
> +
> +  nsIFrame* cur = mPendingWrapperRestyles[aIndex];

Feel free to leave this assertion out, since the mPendingWrapperRestyles[aIndex] access here will do it.

::: layout/base/ServoRestyleManager.cpp:176
(Diff revision 1)
> +  // The frame at index aIndex is something we should restyle ourselves, but
> +  // following frames may need separate ServoRestyleStates to restyle.
> +  MOZ_ASSERT(aIndex < mPendingWrapperRestyles.Length());
> +
> +  nsIFrame* cur = mPendingWrapperRestyles[aIndex];
> +  MOZ_ASSERT(IsAnonBox(*cur));

Per above, should this check if it's an inheriting anonymous box?

::: layout/base/ServoRestyleManager.cpp:186
(Diff revision 1)
> +  MOZ_ASSERT(parent->FirstContinuation() == aParent ||
> +             (IsAnonBox(*parent) &&
> +              parent->GetContent() == aParent->GetContent()));

Can you explain this assertion a bit for me?

::: layout/base/ServoRestyleManager.cpp:218
(Diff revision 1)
> +        next->IsWrapperAnonBoxNeedingRestyle()) {
> +      // It might be nice if we could do better than nsChangeHint_Empty.  On
> +      // the other hand, presumably our mChangesHandled already has the bits
> +      // we really want here so in practice it doesn't matter.
> +      ServoRestyleState childState(*cur, curRestyleState, nsChangeHint_Empty,
> +                                   Type::InFlow, false);

/* aAssertWrapperRestyleLength = */ false

::: layout/base/ServoRestyleManager.cpp:879
(Diff revision 1)
>    // modify the styles of the kids, and the child traversal above would just
>    // clobber those modifications.
>    if (styleFrame) {
> +    // Process anon box wrapper frames before ::first-line bits.
> +    childrenRestyleState.ProcessWrapperRestyles(styleFrame);
> +  

Trailing space.
Comment on attachment 8895527 [details]
Bug 1388625 part 6.  Flag the in-flow frames of kids of various wrapper frames during frame construction, so we know to restyle the wrapper frames.

https://reviewboard.mozilla.org/r/166738/#review172096

::: layout/base/nsCSSFrameConstructor.cpp:3066
(Diff revision 1)
>    nsContainerFrame* frameAsContainer = do_QueryFrame(aFrame);
>    AddFCItemsForAnonymousContent(aState, frameAsContainer, anonymousItems, itemsToConstruct);
>  
>    nsFrameItems frameItems;
> -  ConstructFramesFromItemList(aState, itemsToConstruct, frameAsContainer, frameItems);
> +  ConstructFramesFromItemList(aState, itemsToConstruct, frameAsContainer,
> +                              false, frameItems);

Here (and elsewhere you're passing in false) could you write

  /* aParentIsWrapperAnonBox = */ false

to make it a bit clearer.

::: layout/base/nsCSSFrameConstructor.cpp:11025
(Diff revision 1)
>  inline void
>  nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState& aState,
>                                                     FrameConstructionItemList& aItems,
>                                                     nsContainerFrame* aParentFrame,
> +                                                   bool aParentIsWrapperAnonBox,
>                                                     nsFrameItems& aFrameItems)
>  {
>    CreateNeededPseudoContainers(aState, aItems, aParentFrame);

Is there any way in here we can assert that the aParentIsWrapperAnonBox value is correct by looking at aParentFrame?  I worry that it will be easy to pass in the wrong value.

::: testing/web-platform/tests/css/CSS2/tables/table-anonymous-text-indent.xht:8
(Diff revision 1)
> +    <script type="text/javascript"><![CDATA[
> +      function doTest() {
> +        var t = document.getElementById("target");
> +        t.style.textIndent = '20px';
> +      }
> +      ]]></script>
> +    <style><![CDATA[
> +    #target { text-indent: 0; display: table; }
> +    #target > * { display: table-cell; border: 1px solid black; }
> +    ]]></style>
> +  </head>

These lines are indented one level too much.
Attachment #8895527 - Flags: review?(cam) → review+
Comment on attachment 8895523 [details]
Bug 1388625 part 1.  During a stylo restyle, update the style of the trailing anonymous colgroup of a table, if any.

https://reviewboard.mozilla.org/r/166730/#review172062

> For (B), is it that we never create an anonymous colgroup frame for it, or that we do but use a different coltype value?  If we never create one this is fine, but if we do, we should be careful this coltype check here is sufficient.

For (B) we create them, but set them to eColGroupContent, as bug 1387568 describes.
Comment on attachment 8895525 [details]
Bug 1388625 part 3.  Add nsIFrame flags we will use in stylo post-traversal to keep track of wrapper anonymous boxes.

https://reviewboard.mozilla.org/r/166734/#review172092

> That it's immutable for the lifetime of the frame sounds plausible to me, although I would need to check we never re-parent children of wrapper anonymous boxes... can/should we assert this somewhere, maybe in nsIFrame::SetParent?

Hmm.  Some wrapper anonymous boxes (ruby) can have continuations, so we can get reparented, but only to another wrapper anon box.

I'll at least assert that in SetParent() if we have the flag set our new parent is an inheriting anon box:

    MOZ_ASSERT_IF(ParentIsWrapperAnonBox(),
                  aParent->StyleContext()->IsInheritingAnonBox());

with a comment explaining why.

> ": 1" on this and the other field.

Oh, nice catch!
Comment on attachment 8895526 [details]
Bug 1388625 part 5.  Implement wrapper anonymous box restyling in ServoRestyleManager.

https://reviewboard.mozilla.org/r/166736/#review172094

> *situations

I went with "a situation", but yes, good catch.

> (Not sure I'm a fan of uniform initialization syntax for simple things like this...)

I'm not usually, but in this case it avoids ugly ifdefs in the constructor that doesn't take this as an argument....  I can do those instead, if you prefer.

> Should we check for table wrapper frames creeping in here too?

Good idea.

> Do we want to check specifically whether this is an inheriting anonymous box?  Actually, haven't we basically asserted this at the top of the function?
> 
> Should mPendingWrapperRestyles only ever contain inheriting anonymous boxes?

Oh, good point.  I added this check when trying to sort out some test failures, but you're right that it's nonsensical, and that we already assert on entry to this function that aWrapperFrame is an inheriting anon box!  I've removed this IsAnonBox() check and made IsAnonBox() debug-only again.

mPendingWrapperRestyles does in fact only ever contain inheriting anon boxes.

> Clearer IMO if written as a while loop, if you're happy for the i to be in scope after the loop...

Yeah, that's fine.  Rewrote as a while loop.  I considered a for loop with an empty body (the increment step used to be a lot more complicated, but now it's just a single statement), but I really dislike those, so....

> Feel free to leave this assertion out, since the mPendingWrapperRestyles[aIndex] access here will do it.

I sort of liked the fact that it made it clear that we really did know it was ok.

> Can you explain this assertion a bit for me?

Well, the comment was trying to explain it.  It turns out that this is actually catching a bug, by the way, as I discovered this morning while debugging something slightly unrelated; it fires in this situation:

    <style>
      div::first-line { color: green }
    </style>
    <body style="width: 100px" onclick="this.style.width = 'auto'">
      <div>
        <span style="display: ruby-base-container">Click this</span>
        <span style="display: ruby-base-container">text that is fairly long</span>
      </div>
    </body>

because we land in this code with `aParent` being the block but `parent` being the first-line frame.   That last is wrong, because we don't want to restyle the anon box and its continuations with first-line styles.  Instead we should use the block and let first-line do its fixups later.  I'll add code for that and adjust the comments to make it clearer, and add that as a crashtest.
Comment on attachment 8895527 [details]
Bug 1388625 part 6.  Flag the in-flow frames of kids of various wrapper frames during frame construction, so we know to restyle the wrapper frames.

https://reviewboard.mozilla.org/r/166738/#review172096

> Here (and elsewhere you're passing in false) could you write
> 
>   /* aParentIsWrapperAnonBox = */ false
> 
> to make it a bit clearer.

OK, I can do that.

> Is there any way in here we can assert that the aParentIsWrapperAnonBox value is correct by looking at aParentFrame?  I worry that it will be easy to pass in the wrong value.

It's not trivial.  You sort of have to hardcode various special cases.  For example, for the table cell case, aParentFrame is the cell-inner.  But the wrapper anon box is the cell-outer, so we'd have to special-case the test for this situation.

In addition to that, we don't really have an exhaustive list of wrapper anon boxes.  I guess I could add one in nsCSSAnonBox.  Then I can also adjust the asserts in the ServoRestyleManager to assert that things are really wrapper anon boxes where they should be.
> I worry that it will be easy to pass in the wrong value.

You were totally right to worry!  We end up passing the wrong value when doing a dynamic insert that lands inside a wrapper box.  And I managed to write a testcase that fails as a result.
(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #14)
> > (Not sure I'm a fan of uniform initialization syntax for simple things like this...)
> 
> I'm not usually, but in this case it avoids ugly ifdefs in the constructor
> that doesn't take this as an argument....  I can do those instead, if you
> prefer.

Oh, by this I meant specifically the curly brace initialization syntax, i.e. I would prefer:

  bool mBlah = false;

instead of:

  bool mBlah { false };
Comment on attachment 8895952 [details]
Bug 1388625 part 4.  Add a concept of wrapper anon boxes to nsCSSAnonBoxes.

https://reviewboard.mozilla.org/r/167216/#review172546

::: layout/style/nsCSSAnonBoxList.h:8
(Diff revision 2)
>  /*
>   * This file contains the list of nsIAtoms and their values for CSS
>   * pseudo-element-ish things used internally for anonymous boxes.  It is

Please mention the new macro in this comment somewhere, and how although wrapper anon boxes do inherit style from their parent, that they aren't included in CSS_INHERITING_ANON_BOX.

::: servo/components/style/gecko/regen_atoms.py:112
(Diff revision 2)
>      def is_inheriting_anon_box(self):
> -        return self.macro == "CSS_ANON_BOX"
> +        return (self.macro == "CSS_ANON_BOX" or
> +                self.macro == "CSS_WRAPPER_ANON_BOX")

Whether something "is an inheriting anonymous box" is a bit confusing, since here, and in nsStyleContext::IsInheritingAnonBox, we check for wrapper and non-wrapper anon boxes that inherit.  But nsCSSAnonBoxes::IsInheritingAnonBox won't include wrapper anon boxes.

It might be a bit annoying to clearly separate these things, so I'd be OK just with comments (say in nsCSSAnonBoxList.h and on the IsInheritingAnonBox functions) pointing out whether they do or don't include wrappers.
Attachment #8895952 - Flags: review?(cam) → review+
Comment on attachment 8895526 [details]
Bug 1388625 part 5.  Implement wrapper anonymous box restyling in ServoRestyleManager.

https://reviewboard.mozilla.org/r/166736/#review172554

Looks good, thanks!
Attachment #8895526 - Flags: review?(cam) → review+
Comment on attachment 8895952 [details]
Bug 1388625 part 4.  Add a concept of wrapper anon boxes to nsCSSAnonBoxes.

https://reviewboard.mozilla.org/r/167216/#review172546

> Please mention the new macro in this comment somewhere, and how although wrapper anon boxes do inherit style from their parent, that they aren't included in CSS_INHERITING_ANON_BOX.

There is no CSS_INHERITING_ANON_BOX, just CSS_ANON_BOX.  But good catch; I added stuff to this comment to explain how things work now, and removed some old bits that never went away when they should have (about the fixup boolean).

> Whether something "is an inheriting anonymous box" is a bit confusing, since here, and in nsStyleContext::IsInheritingAnonBox, we check for wrapper and non-wrapper anon boxes that inherit.  But nsCSSAnonBoxes::IsInheritingAnonBox won't include wrapper anon boxes.
> 
> It might be a bit annoying to clearly separate these things, so I'd be OK just with comments (say in nsCSSAnonBoxList.h and on the IsInheritingAnonBox functions) pointing out whether they do or don't include wrappers.

nsCSSAnonBoxes::IsInheritingAnonBox does include wrapper anon boxes.  It includes CSS_ANON_BOX, and doesn't define CSS_WRAPPER_ANON_BOX, so that will default to CSS_ANON_BOX and be included.

I'll add commets on the IsInheritingAnonBox bits to make it clear that this includes wrappers.
> Oh, by this I meant specifically the curly brace initialization syntax

Oh!  Yeah, I can do the `= false` thing.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f631cb50bf53
part 1.  During a stylo restyle, update the style of the trailing anonymous colgroup of a table, if any.  r=heycam
https://hg.mozilla.org/integration/autoland/rev/e9fc7911b14f
part 2.  During a stylo restyle, update the style of the anonymous cols in a colgroup, if any.  r=heycam
https://hg.mozilla.org/integration/autoland/rev/d1460a3a9ab1
part 3.  Add nsIFrame flags we will use in stylo post-traversal to keep track of wrapper anonymous boxes.  r=heycam
https://hg.mozilla.org/integration/autoland/rev/76ba910b21ee
part 4.  Add a concept of wrapper anon boxes to nsCSSAnonBoxes.  r=heycam
https://hg.mozilla.org/integration/autoland/rev/fb891d5aa58f
part 5.  Implement wrapper anonymous box restyling in ServoRestyleManager.  r=heycam
https://hg.mozilla.org/integration/autoland/rev/1bb53448ce5a
part 6.  Flag the in-flow frames of kids of various wrapper frames during frame construction, so we know to restyle the wrapper frames.  r=heycam
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: