Closed Bug 1292618 Opened 8 years ago Closed 8 years ago

stylo: Incremental restyle for pseudo elements

Categories

(Core :: CSS Parsing and Computation, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: bholley, Assigned: emilio)

References

Details

Attachments

(5 files, 3 obsolete files)

Bug 1292618: Support basic pseudo-element restyling.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1292618: Tidy up nsStyleContent's API now allocations are infallible.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1292618: Add Gecko_ClearPODTArray to clear arrays of types without destructors.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1292618: Specialize ServoStyleSet::ResolveStyleForText to take into account generated nodes.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1292618: Add Gecko_CopyStyleContentsFrom.
58 bytes, text/x-review-board-request
heycam
: review+
Details 
Similar to bug 1292609, traversing the DOM rather than the frame tree makes it nontrivial to find and update pseudo-elements during incremental restyle.

There are effectively 4 types of PEs to worry about:
(1) Anonymous boxes: This is 1292609.
(2) Element-specific PEs like the HTML5 form-control PEs. These should be straightforward to find, and we can just include them in the switch statement described in bug 1292609 comment 0.
(3) ::first-line and ::first-letter. This may require some tricky detection, emilio and dbaron met yesterday to work out the details. Hopefully emilio can brain-dump the results here.
(4) ::before and ::after. Similar to (3).

In Servo, we say a PE is "eagerly cascaded" if we calculate the computed values for the PE on any node that the left-of-the-PE-part matches a PE-selector. ::before and ::after are both eagerly cascaded, and I think we decided that we should do the same for ::first-line and ::first-letter.

Once all the PEs in (3) and (4) are eagerly-cascaded, we'll want to compare the maps before or after restyle, which logically give us data of the form Option<ComputedValues> for each PE type. For ::before and ::after, most difference will probably require retriggering frame construction. I'm less sure about ::first-line and ::first-letter.
Another thing I'm not entirely sure about is Native Anonymous Content. Cameron, do you know if there are cases other than the PEs in (2) where we need to update the style for native anonymous content?
Flags: needinfo?(cam)
Blocks: stylo-incremental
No longer blocks: stylo
Yes, native anonymous content doesn't have to get style contexts with a pseudo-element set on them.  Also, some HTML form control pseudo-elements are (intentionally) exposed to content, like ::-moz-meter-bar.

To get all native anonymous content for an element, we can get its frame, QI it to nsIAnonymousContentCreator, and then call AppendAnonymousContentTo on it.  I'm not sure that we have to distinguish native anonymous content that are pseudo-elements from those that aren't.  There might be cases where we can prove that certain PEs will never need selector matching re-performed, like with the anonymous boxes, but it's probably easier just to treat the uniformly.
Flags: needinfo?(cam)
Oh I see, it looks like AllChildrenIterator already handles both NAC and XBL. So if we use that during our DOM post-traversal, we shouldn't have trouble locating those kinds of anonymous content.
So I think as long as we:
* handle the traversal of AC/NAC in bug 1292662
* match UA rules only for NAC (bug 1293809)
* Use AllChildrenIterator during style context fixup (bug 1293812)

We should be good for both XBL AC and NAC.

So the work left to do in this bug is to compare the pseudo maps and generate appropriate change hints, and then process those during style context fixup.
Attachment #8780682 - Attachment is obsolete: true
Attachment #8780682 - Flags: review?(cam)
Attachment #8780683 - Attachment is obsolete: true
Attachment #8780683 - Flags: review?(cam)
Attachment #8780685 - Attachment is obsolete: true
Attachment #8780685 - Flags: review?(cam)
Comment on attachment 8780684 [details]
Bug 1292618: Support basic pseudo-element restyling.

https://reviewboard.mozilla.org/r/71332/#review69366

::: layout/base/ServoRestyleManager.h:80
(Diff revision 2)
> +  /**
> +   * Gets the appropriate frame given a content and a pseudo-element tag.
> +   *
> +   * Right now only supports a null tag, before or after.
> +   */

Please document in the comment that aContent must be an Element if aPseudoTagOrNull is not null.

::: layout/base/ServoRestyleManager.cpp:149
(Diff revision 2)
>      }
>  
> +    // Update pseudo-elements state if appropriate.
> +    if (aContent->IsElement()) {
> +      Element* aElement = aContent->AsElement();
> +      CSSPseudoElementType pseudosToRestyle[] = {

Make this |static const|.

::: layout/base/ServoRestyleManager.cpp:153
(Diff revision 2)
> +      Element* aElement = aContent->AsElement();
> +      CSSPseudoElementType pseudosToRestyle[] = {
> +        CSSPseudoElementType::before, CSSPseudoElementType::after,
> +      };
> +
> +      for (uint32_t i = 0; i < MOZ_ARRAY_LENGTH(pseudosToRestyle); ++i) {

for (CSSPseudoElementType pseudoType : pseudosToRestyle)

::: layout/base/ServoRestyleManager.cpp:158
(Diff revision 2)
> +      for (uint32_t i = 0; i < MOZ_ARRAY_LENGTH(pseudosToRestyle); ++i) {
> +        nsIAtom* pseudoTag =
> +          nsCSSPseudoElements::GetPseudoAtom(pseudosToRestyle[i]);
> +        if (nsIFrame* pseudoFrame =
> +              FrameForPseudoElement(aElement, pseudoTag)) {
> +          // TODO: we could probably make this more performant.

It's implicit in the MOZ_ASSERT_IF, but can you add a comment in here explaining that if the ::before or ::after is going away because content is now none, for example, that we will generate a ReconstructFrame for the parent and we don't have new style to set here?

(Also what's slow about this?)

::: layout/base/ServoRestyleManager.cpp:165
(Diff revision 2)
> +            aStyleSet->ProbePseudoElementStyle(aElement, pseudosToRestyle[i],
> +                                               newContext);
> +          MOZ_ASSERT_IF(!pseudoContext,
> +                        changeHint & nsChangeHint_ReconstructFrame);
> +          if (pseudoContext) {
> +            pseudoFrame->SetStyleContext(pseudoContext.get());

The |.get()| shouldn't be needed.

(As an aside, maybe nsIFrame::SetStyleContext should take a RefPtr<nsStyleContext>&& or an already_AddRefed<nsStyleContext>...)

::: layout/base/ServoRestyleManager.cpp:221
(Diff revision 2)
>  }
>  
> -void
> +/* static */ nsIFrame*
> +ServoRestyleManager::FrameForPseudoElement(nsIContent* aContent,
> +                                           nsIAtom* aPseudoTagOrNull)
> +{

Please assert somewhere in this function that we don't get unexpected aPseudoTagOrNull values.

::: layout/base/ServoRestyleManager.cpp:223
(Diff revision 2)
> -void
> +/* static */ nsIFrame*
> +ServoRestyleManager::FrameForPseudoElement(nsIContent* aContent,
> +                                           nsIAtom* aPseudoTagOrNull)
> +{
> +  MOZ_ASSERT_IF(aPseudoTagOrNull, aContent->IsElement());
> +  nsIFrame* primaryFrame = aContent->GetPrimaryFrame();

Passing aContent->GetPrimaryFrame() to the nsLayoutUtils methods might not be correct for display:contents (see that RestyleManager passes in the frame of the display:contents node's parent in that case).  Add a comment in here saying we'll need to fix this?

::: layout/style/ServoBindings.cpp:192
(Diff revision 2)
>  }
>  
>  nsStyleContext*
>  Gecko_GetStyleContext(RawGeckoNode* aNode, nsIAtom* aPseudoTagOrNull)
>  {
> -  MOZ_ASSERT(aNode->IsContent());
> +  using mozilla::ServoRestyleManager;

Just add a |using namespace mozilla;| at the top of the file, just below the #includes.

::: layout/style/ServoStyleSet.cpp:117
(Diff revision 2)
>    // XXXbholley: Figure out the correct thing to pass here. Does this fixup
>    // duplicate something that servo already does?
>    bool skipFixup = false;
>  
>    return NS_NewStyleContext(aParentContext, mPresContext, aPseudoTag,
> -                            aPseudoType,
> +                            aPseudoType, Move(aComputedValues), skipFixup);

Sure, though please try to keep cleanups in separate patches. :-)
Attachment #8780684 - Flags: review?(cam) → review+
Comment on attachment 8780753 [details]
Bug 1292618: Tidy up nsStyleContent's API now allocations are infallible.

https://reviewboard.mozilla.org/r/71394/#review69380

::: layout/style/nsStyleStruct.h:3180
(Diff revision 1)
>      MOZ_ASSERT(!mImageTracked,
>                 "Setting a new image without untracking the old one!");
>      MOZ_ASSERT(mType == eStyleContentType_Image, "Wrong type!");
>      NS_IF_ADDREF(mContent.mImage = aRequest);
>    }
> -private:
> +  nsStyleContentData(const nsStyleContentData&);

Move this declaration up with the other constructor?

::: layout/style/nsStyleStruct.h:3237
(Diff revision 1)
> +    if (aCount == mContents.Length()) {
> +      mContents.ClearAndRetainStorage();
> +    } else {
> +      mContents.Clear();
> -  }
> +    }
> -
> -  nsresult  AllocateCounterIncrements(uint32_t aCount) {
> +    mContents.EnsureLengthAtLeast(aCount);
> +    mContents.SetLength(aCount);

To be honest it's probably not worth managing the length in case it matches like this.  Let's just do:

  mContents.Clear();
  mContents.SetLength(aCount);

::: layout/style/nsStyleStruct.h:3246
(Diff revision 1)
> -        if (! mIncrements) {
> -          mIncrementCount = 0;
> -          return NS_ERROR_OUT_OF_MEMORY;
> -        }
> +  }
> +
> +  uint32_t  CounterIncrementCount() const { return mIncrements.Length(); }  // [reset]

Nit: fix the double space after "uint32_t" while touching this line.

::: layout/style/nsStyleStruct.h:3253
(Diff revision 1)
> +      mIncrements.EnsureLengthAtLeast(aCount);
> +      mIncrements.SetLength(aCount);

The EnsureLengthAtLeast call isn't needed.  It just expands to something equivalent to:

  if (mIncrements.Length() < aCount) {
    mIncrements.SetLength(aCount);
  }

::: layout/style/nsStyleStruct.h:3268
(Diff revision 1)
> -  nsresult  AllocateCounterResets(uint32_t aCount) {
> -    if (aCount != mResetCount) {
> -      DELETE_ARRAY_IF(mResets);
> -      if (aCount) {
> +  void AllocateCounterResets(uint32_t aCount) {
> +    if (aCount != mResets.Length()) {
> +      mIncrements.EnsureLengthAtLeast(aCount);
> +      mIncrements.SetLength(aCount);
> -        mResets = new nsStyleCounterData[aCount];
> -        if (! mResets) {
> -          mResetCount = 0;
> -          return NS_ERROR_OUT_OF_MEMORY;
> -        }
> -      }
> +    }
> -      mResetCount = aCount;
> -    }
> -    return NS_OK;
>    }

That should be mResets, not mIncrements.

But let's just call mResets.SetLength(aCount); regardless of what its current length is.

::: layout/style/nsStyleStruct.cpp:3563
(Diff revision 1)
> -  if (mContentCount != aNewData.mContentCount ||
> -      mIncrementCount != aNewData.mIncrementCount ||
> -      mResetCount != aNewData.mResetCount) {
> +  if (ContentCount() != aNewData.ContentCount() ||
> +      CounterIncrementCount() != aNewData.CounterIncrementCount() ||
> +      CounterResetCount() != aNewData.CounterResetCount()) {
>      return nsChangeHint_ReconstructFrame;
>    }
>  
> -  uint32_t ix = mContentCount;
> +  if (mContents != aNewData.mContents) {
> -  while (0 < ix--) {
> -    if (mContents[ix] != aNewData.mContents[ix]) {
> -      // Unfortunately we need to reframe here; a simple reflow
> +    // Unfortunately we need to reframe here; a simple reflow
> -      // will not pick up different text or different image URLs,
> +    // will not pick up different text or different image URLs,
> -      // since we set all that up in the CSSFrameConstructor
> +    // since we set all that up in the CSSFrameConstructor
> -      return nsChangeHint_ReconstructFrame;
> +    return nsChangeHint_ReconstructFrame;
> -    }
> +  }
> -  }
> -  ix = mIncrementCount;
> +
> +  if (mIncrements != aNewData.mIncrements) {
> -  while (0 < ix--) {
> -    if ((mIncrements[ix].mValue != aNewData.mIncrements[ix].mValue) ||
> -        (mIncrements[ix].mCounter != aNewData.mIncrements[ix].mCounter)) {
> -      return nsChangeHint_ReconstructFrame;
> +    return nsChangeHint_ReconstructFrame;
> -    }
> +  }
> -  }
> -  ix = mResetCount;
> +
> +  if (mResets != aNewData.mResets) {
> -  while (0 < ix--) {
> -    if ((mResets[ix].mValue != aNewData.mResets[ix].mValue) ||
> -        (mResets[ix].mCounter != aNewData.mResets[ix].mCounter)) {
> -      return nsChangeHint_ReconstructFrame;
> +    return nsChangeHint_ReconstructFrame;
> -    }
> +  }

Let's just combine these into one:

  if (mContents != aNewData.mContents ||
      mIncrements != ... ||
      mResets != ...)

and then reword the comment about the unfortunate need to reframe to be specifically about same-length mContents.
Attachment #8780753 - Flags: review?(cam) → review+
Comment on attachment 8780754 [details]
Bug 1292618: Add Gecko_ClearPODTArray to clear arrays of types without destructors.

https://reviewboard.mozilla.org/r/71396/#review69674

I'm probably not the right person to review changes to nsTArray.h, but it looks fine to me and just does more of the same of what Manish added, so r=me.

::: layout/style/ServoBindings.cpp:724
(Diff revision 1)
>  void
>  Gecko_EnsureTArrayCapacity(void* aArray, size_t aCapacity, size_t aElemSize)
>  {
> -  auto base = reinterpret_cast<nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils> *>(aArray);
> +  auto base =
> +    reinterpret_cast<nsTArray_base<nsTArrayInfallibleAllocator,
> +                                   nsTArray_CopyWithMemutils> *>(aArray);

Nit: can drop the space before the "*".

::: layout/style/ServoBindings.cpp:734
(Diff revision 1)
>  void
> +Gecko_ClearPODTArray(void* aArray, size_t aElementSize, size_t aElementAlign)
> +{
> +  auto base =
> +    reinterpret_cast<nsTArray_base<nsTArrayInfallibleAllocator,
> +                                   nsTArray_CopyWithMemutils> *>(aArray);

Nit: and here.
Attachment #8780754 - Flags: review?(cam) → review+
Comment on attachment 8780755 [details]
Bug 1292618: Specialize ServoStyleSet::ResolveStyleForText to take into account generated nodes.

https://reviewboard.mozilla.org/r/71398/#review69684

::: layout/style/ServoStyleSet.cpp:156
(Diff revision 3)
> +  if (parentName == nsGkAtoms::mozgeneratedcontentbefore ||
> +      parentName == nsGkAtoms::mozgeneratedcontentafter) {

I think we can produce nodes with these names in regular content, e.g. by creating content like:

  <div>
    <_moz_generated_content_before>
      some text
    </_moz_generated_content_before>
  </div>

(The HTML parser doesn't allow elements starting with "_" it seems, but you can create them with document.createElement().)

So we should check probably check parent->IsInNativeAnonymousSubtree() too.
Attachment #8780755 - Flags: review?(cam) → review+
Comment on attachment 8781722 [details]
Bug 1292618: Add Gecko_CopyStyleContentsFrom.

https://reviewboard.mozilla.org/r/72058/#review69704
Attachment #8781722 - Flags: review?(cam) → review+
FYI I'll add some methods to nsIContent to check whether it's a generated content container, in bug 1295852:

https://hg.mozilla.org/try/rev/9af8d617ba0ff6914323a9ec602ac3b34da7a865
Comment on attachment 8780684 [details]
Bug 1292618: Support basic pseudo-element restyling.

https://reviewboard.mozilla.org/r/71332/#review70350

::: layout/base/ServoRestyleManager.cpp:158
(Diff revision 4)
> +          // TODO: we could maybe make this more performant via calling into
> +          // Servo just once to know which pseudo-elements we've got to restyle?

Yeah, this sounds good - I'm kind of concerned about the amount of unconditional work we're doing here.

Another good thing would be to use a change hint bit to indicate whether there are any pseudos to restyle at all. Can you file a bug for both of those against stylo-perf?

::: layout/style/ServoBindings.cpp:726
(Diff revision 4)
> +Gecko_ClearStyleContents(nsStyleContent* aContent)
> +{
> +  aContent->AllocateContents(0);
> +}

Can you add MOZ_COUNT_{C,D}TOR to nsStyleContentData so that we can catch leaks like this?
See Also: → 1296432
Comment on attachment 8780684 [details]
Bug 1292618: Support basic pseudo-element restyling.

https://reviewboard.mozilla.org/r/71332/#review70350

> Yeah, this sounds good - I'm kind of concerned about the amount of unconditional work we're doing here.
> 
> Another good thing would be to use a change hint bit to indicate whether there are any pseudos to restyle at all. Can you file a bug for both of those against stylo-perf?

I think the way to go here is an Element bit (ELEMENT_HAS_PSEUDO_ELEMENT), or similar. Adding it to the change hints will mean special-casing a lot for that specific change hint, in order for, for example, not push it to the changeList.

Filled 1296432.

> Can you add MOZ_COUNT_{C,D}TOR to nsStyleContentData so that we can catch leaks like this?

Sure, will upload once this is ready to land, I have the rebase due to bug 1275913 ready to push once the Servo changes are reviewed.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c347701d343
Tidy up nsStyleContent's API now allocations are infallible. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e274c66ae7f
Add Gecko_ClearPODTArray to clear arrays of types without destructors. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/928dd363efa0
Support basic pseudo-element restyling. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6034e58efe4
Specialize ServoStyleSet::ResolveStyleForText to take into account generated nodes. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d767147e160
Add Gecko_CopyStyleContentsFrom. r=heycam
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/43689d56cbf6 for masses of "!mImageTracked (nsStyleContentData being destroyed while still tracking image!)" assertion failures mostly, but not entirely, in devtools tests, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=34211111&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=34211201&repo=mozilla-inbound
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c46096fb15c
Tidy up nsStyleContent's API now allocations are infallible. r=heycam
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30f7696fea2d
Add Gecko_ClearPODTArray to clear arrays of types without destructors. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/d04597bd1109
Support basic pseudo-element restyling. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/17dfe8bc5f76
Specialize ServoStyleSet::ResolveStyleForText to take into account generated nodes. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/14733a383b4c
Add Gecko_CopyStyleContentsFrom. r=heycam
I had to back this out because merge conflicts from it was preventing me from pushing a merge from mozilla-central. Feel free to rebase and re-push.

https://hg.mozilla.org/integration/mozilla-inbound/rev/51a479a3feba
Flags: needinfo?(ealvarez)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70b776aa432e
Add Gecko_ClearPODTArray to clear arrays of types without destructors. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/1976d34e92f8
Support basic pseudo-element restyling. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/0830c52ff6c7
Specialize ServoStyleSet::ResolveStyleForText to take into account generated nodes. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/409352a758dd
Add Gecko_CopyStyleContentsFrom. r=heycam
No problem, just rebased, re-checked, and pushed, thanks for your work Wes :)
Flags: needinfo?(ealvarez)
> So we should check probably check parent->IsInNativeAnonymousSubtree() too.

Is there a reason we're reinventing nsIContent::IsGeneratedContentContainerForBefore/IsGeneratedContentContainerForAfter to start with?
Flags: needinfo?(ealvarez)
Oh, I see.  Those were just added in bug 1295852 a few days ago.  But still, shouldn't we have used them here once they got added?
This bug was the reason IsGeneratedContentContainerForBefore was added in the first place (Cameron noticed that the checks were wrong basically everywhere).

This landed on inbound what the former was still on autoland, will push a followup to use those.
Flags: needinfo?(ealvarez)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5608fd58157c
followup: Use IsGeneratedContentContainerForBefore/After in ServoStyleSet. r=me
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: