898329 - Split ElementRestyler::Restyle into multiple functions

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

As the second step of the refactoring begun in bug 898209, I'd like to split ElementRestyler::Restyle into multiple functions.  Probably something like:

  /**
   * First half of Restyle().
   */
  void RestyleSelf();

  /**
   * Restyle the children of this frame (and, in turn, their children).
   *
   * Second half of Restyle().
   */
  void RestyleChildren();

  /**
   * Helpers for RestyleChildren().
   */
  void RestyleBeforeChild();
  void RestyleAfterChild();
  void RestyleContentChildren();
  void InitializeAccessibilityNotifications();
  void SendAccessibilityNotifications();

This will make the function easier to understand and to modify.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: Remove null-checks on oldContext and newContext and avoid manual reference counting.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: Fix indentation in ReParentStyleContext from patch 1.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3: Create and use ElementRestyler::mWasFrameVisible.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 4: Avoid using newContext out of what will be in RestyleManager::RestyleSelf.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 5: Create and use ElementRestyler::mResolvedChild.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 6: Avoid using pseudoTag out of what will be in RestyleManager::RestyleSelf.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 7: Split ElementRestyler::Restyle into multiple functions.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 8: Use for loops instead of while loops for iterating extra style contexts.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 9: Fix indentation from previous patches.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 10: Fix wrapping of comment that I just reintended.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that some of the patches in this sequence (1, 6, and 8, I think) make changes that should require reindentation, but I didn't actually do the reindentation until patches 2 and 9 (which are *only* reindentation).  This makes it easier for me to maintain the patch queue (since it's easy to rewrite the reindentation patches if needed).

https://tbpl.mozilla.org/?tree=Try&rev=9097f32da328

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(And for what it's worth, I suspect the accessibility notifications code is messier than it needs to be in terms of location within this code and need for state, and I suspect that if it had been done after this refactoring, it might not be.  But I'm not the right person to clean it up.)

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3.5: Don't check undisplayed content if we're going to reframe.

This avoids doing work that is unnecessary; if we're going to reframe
anyway, the frame destruction will clear the undisplayed content list,
and we'll then rebuild it when we recreate the frame (if needed).

It is also a prerequisite for patch 4, which replaces some uses of
newContext with mFrame->StyleContext().  These are the same as long as
we don't have a framechange hint, since when we do have a framechange
hint we leave the old style context on the frame to avoid violating
invariants.  This patch ensures that all such uses replaced in patch 4
are actually inside a check that we don't have a framechange hint.
(Most of them already were.)

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 4: Avoid using newContext outside of what will be in RestyleManager::RestyleSelf.

This replaces newContext with mFrame->StyleContext(), which is a valid
replacement since all of the replacements are inside a test that we
don't have a framechange hint.  If we have a framechange hint, then
mFrame still has its old style context.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

New try run with the above revisions:
https://tbpl.mozilla.org/?tree=Try&rev=f3228e1541bd

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 782074 [details] [diff] [review]
patch 1:  Remove null-checks on oldContext and newContext and avoid manual reference counting.

Review of attachment 782074 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/RestyleManager.cpp
@@ -1757,5 @@
>    // DO NOT verify the style tree before reparenting.  The frame
>    // tree has already been changed, so this check would just fail.
>    nsStyleContext* oldContext = aFrame->StyleContext();
> -  // XXXbz can oldContext really ever be null?
> -  if (oldContext) {

I want to understand how this condition holds.

All frames inherit from nsFrame, and its constructor takes an nsStyleContext* argument, assigns it to mStyleContext, and calls AddRef on it.  So we know at least that when a frame is created it has a style context.  However, SetStyleContext supports being called with null, and ~nsFrame checks if mStyleContext is null before calling Release on it.  Will SetStyleContext actually ever be called with null?  Looking at all the calls to SetStyleContext{,WithoutNotification}, I don't think so.

(nsStyleSet::GetContext can return null if NS_NewStyleContext does, but I guess now that new is infallible this will never actually happen.)

We should remove those checks from SetStyleContext{,WithoutNotification} -- and from many places that call nsStyleSet::GetContext/ResolveStyleForBlah -- if that's right.

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 782079 [details] [diff] [review]
patch 6:  Avoid using pseudoTag out of what will be in RestyleManager::RestyleSelf.

Review of attachment 782079 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/RestyleManager.cpp
@@ +2358,5 @@
>      // DocElementContainingBlock.
>      bool checkUndisplayed;
>      nsIContent* undisplayedParent;
>      nsCSSFrameConstructor* frameConstructor = mPresContext->FrameConstructor();
> +    if (mFrame->StyleContext()->GetPseudo()) {

Before, pseudoTag came from the old style context.  Here it's coming from the new one.  What guarantees that it will be the same?

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 782080 [details] [diff] [review]
patch 7:  Split ElementRestyler::Restyle into multiple functions.

Review of attachment 782080 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/RestyleManager.h
@@ +330,5 @@
> +  void RestyleBeforePseudo();
> +  void RestyleAfterPseudo();
> +  void RestyleContentChildren(nsRestyleHint aChildRestyleHint);
> +  void InitializeAccessibilityNotifications();
> +  void SendAccessibilityNotifications();

Is there a reason RestyleSelf, RestyleChildren and the helpers for RestyleChildren aren't private?

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 782199 [details] [diff] [review]
patch 3.5:  Don't check undisplayed content if we're going to reframe.

Review of attachment 782199 [details] [diff] [review]:
-----------------------------------------------------------------

> It is also a prerequisite for patch 4, which replaces some uses of
> newContext with mFrame->StyleContext(). These are the same as long as
> we don't have a framechange hint, since when we do have a framechange
> hint we leave the old style context on the frame to avoid violating
> invariants.

Is this invariant documented or enforced anywhere?  There's a comment "// if frame gets regenerated, let it keep old context", but that sounds like we're not setting new style context as an optimisation rather than something we need to do.

Comment 20

[Cameron McCormack (:heycam)]

Comment on attachment 782074 [details] [diff] [review]
patch 1:  Remove null-checks on oldContext and newContext and avoid manual reference counting.

Review of attachment 782074 [details] [diff] [review]:
-----------------------------------------------------------------

> I want to understand how this condition holds.

I think I've satisfied myself that SetStyleContext will never be called with null.  And I'll file some bugs to ensure that.

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 782080 [details] [diff] [review]
patch 7:  Split ElementRestyler::Restyle into multiple functions.

Review of attachment 782080 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if you make those functions private per comment 18.

Comment 22

[Cameron McCormack (:heycam)]

Comment on attachment 782079 [details] [diff] [review]
patch 6:  Avoid using pseudoTag out of what will be in RestyleManager::RestyleSelf.

Pending an answer to comment 17.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) from comment #17)
> Before, pseudoTag came from the old style context.  Here it's coming from
> the new one.  What guarantees that it will be the same?

The pseudo type/tag is one of the pieces of data that is input into constructing a style context.  It's for things like ::before, ::first-letter, etc.  When we create new style contexts in this function, we simply copy the old pseudoTag to the new style context, when we pass it to ResolveAnonymousBoxStyle, ProbePseudoElementStyle, or ResolvePseudoElementStyle.  (See the code that ends up in RestyleSelf.)

Comment 24

[Cameron McCormack (:heycam)]

Comment on attachment 782079 [details] [diff] [review]
patch 6:  Avoid using pseudoTag out of what will be in RestyleManager::RestyleSelf.

I see now.  Is it worth asserting at the end of RestyleSelf that oldContext->GetPseudo() == newContext->GetPseudo()?

By the way, does a given pseudo tag atom always imply a particular pseudo type?  Like, nsCSSPseudoElements::before is always of type nsCSSPseudoElements::ePseudo_before, nsCSSPseudoElements::mozProgressBar is always nsCSSPseudoElements::ePseudo_AnonBox?

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) from comment #16)
> Comment on attachment 782074 [details] [diff] [review]
> patch 1:  Remove null-checks on oldContext and newContext and avoid manual
> reference counting.
> 
> Review of attachment 782074 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/RestyleManager.cpp
> @@ -1757,5 @@
> >    // DO NOT verify the style tree before reparenting.  The frame
> >    // tree has already been changed, so this check would just fail.
> >    nsStyleContext* oldContext = aFrame->StyleContext();
> > -  // XXXbz can oldContext really ever be null?
> > -  if (oldContext) {
> 
> I want to understand how this condition holds.
> 
> All frames inherit from nsFrame, and its constructor takes an
> nsStyleContext* argument, assigns it to mStyleContext, and calls AddRef on
> it.  So we know at least that when a frame is created it has a style
> context.  However, SetStyleContext supports being called with null, and
> ~nsFrame checks if mStyleContext is null before calling Release on it.  Will
> SetStyleContext actually ever be called with null?  Looking at all the calls
> to SetStyleContext{,WithoutNotification}, I don't think so.
> 
> (nsStyleSet::GetContext can return null if NS_NewStyleContext does, but I
> guess now that new is infallible this will never actually happen.)
> 
> We should remove those checks from SetStyleContext{,WithoutNotification} --
> and from many places that call nsStyleSet::GetContext/ResolveStyleForBlah --
> if that's right.

Anything that null check's a frame's style context should have the null-check removed.  In order to construct a frame, we have to check values on the style context we've created for it, so we'd already have crashed due to null-deref before we even got to the point of constructing the frame.  In the new world of having infallible allocators, allocating a style context should use infallible allocators so that it crashes immediately.

(In reply to Cameron McCormack (:heycam) from comment #18)
> Is there a reason RestyleSelf, RestyleChildren and the helpers for
> RestyleChildren aren't private?

No.  I intended to make them private when I originally sketched out the design; see 
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/96fcbffca321
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/6db4b82243e3
but then I forgot when I built the patch series step-by-step.

(In reply to Cameron McCormack (:heycam) from comment #19)
> Comment on attachment 782199 [details] [diff] [review]
> patch 3.5:  Don't check undisplayed content if we're going to reframe.
> 
> Review of attachment 782199 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > It is also a prerequisite for patch 4, which replaces some uses of
> > newContext with mFrame->StyleContext(). These are the same as long as
> > we don't have a framechange hint, since when we do have a framechange
> > hint we leave the old style context on the frame to avoid violating
> > invariants.
> 
> Is this invariant documented or enforced anywhere?  There's a comment "// if
> frame gets regenerated, let it keep old context", but that sounds like we're
> not setting new style context as an optimisation rather than something we
> need to do.

So bug 828312 patch 11 (which may move to bug 898333) expands on that comment a good bit:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/7d701c42f50f/dont-generate-change-hints-for-continuations

I'd forgotten that that patch was ahead of this one in the needs-to-land-in order; it was written earlier.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) from comment #24)
> I see now.  Is it worth asserting at the end of RestyleSelf that
> oldContext->GetPseudo() == newContext->GetPseudo()?

I think the best place to put the assertion is in CaptureChange, actually, since that catches all the places where we replace one context with another.

Comment 27

[Cameron McCormack (:heycam)]

Great, thanks for the replies.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c647818eb586
https://hg.mozilla.org/integration/mozilla-inbound/rev/27914b6ab3b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/17084bc158aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/c500d0a58eac
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1f2ea65f846
https://hg.mozilla.org/integration/mozilla-inbound/rev/54617e0ac453
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac0d803f2112
https://hg.mozilla.org/integration/mozilla-inbound/rev/95ec9086d91a
https://hg.mozilla.org/integration/mozilla-inbound/rev/37cf2915f42a
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d8b3781d2bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/65450675b8cc

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c647818eb586
https://hg.mozilla.org/mozilla-central/rev/27914b6ab3b4
https://hg.mozilla.org/mozilla-central/rev/17084bc158aa
https://hg.mozilla.org/mozilla-central/rev/c500d0a58eac
https://hg.mozilla.org/mozilla-central/rev/b1f2ea65f846
https://hg.mozilla.org/mozilla-central/rev/54617e0ac453
https://hg.mozilla.org/mozilla-central/rev/ac0d803f2112
https://hg.mozilla.org/mozilla-central/rev/95ec9086d91a
https://hg.mozilla.org/mozilla-central/rev/37cf2915f42a
https://hg.mozilla.org/mozilla-central/rev/3d8b3781d2bc
https://hg.mozilla.org/mozilla-central/rev/65450675b8cc