Closed Bug 898329 Opened 6 years ago Closed 6 years ago

Split ElementRestyler::Restyle into multiple functions

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(11 files, 1 obsolete file)

5.53 KB, patch
heycam
: review+
Details | Diff | Splinter Review
16.29 KB, patch
heycam
: review+
Details | Diff | Splinter Review
3.05 KB, patch
heycam
: review+
Details | Diff | Splinter Review
7.28 KB, patch
heycam
: review+
Details | Diff | Splinter Review
3.23 KB, patch
heycam
: review+
Details | Diff | Splinter Review
15.87 KB, patch
heycam
: review+
Details | Diff | Splinter Review
4.75 KB, patch
heycam
: review+
Details | Diff | Splinter Review
56.24 KB, patch
heycam
: review+
Details | Diff | Splinter Review
2.17 KB, patch
heycam
: review+
Details | Diff | Splinter Review
2.08 KB, patch
heycam
: review+
Details | Diff | Splinter Review
9.50 KB, patch
heycam
: review+
Details | Diff | Splinter Review
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.
Blocks: 898333
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
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
(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.)
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.)
Attachment #782199 - Flags: review?(cam)
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.
Attachment #782200 - Flags: review?(cam)
Attachment #782077 - Attachment is obsolete: true
Attachment #782077 - Flags: review?(cam)
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 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 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?
Attachment #782075 - Flags: review?(cam) → review+
Attachment #782076 - Flags: review?(cam) → review+
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.
Attachment #782199 - Flags: review?(cam) → review+
Attachment #782200 - Flags: review?(cam) → review+
Attachment #782078 - Flags: review?(cam) → review+
Attachment #782082 - Flags: review?(cam) → review+
Attachment #782083 - Flags: review?(cam) → review+
Attachment #782084 - Flags: review?(cam) → review+
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.
Attachment #782074 - Flags: review?(cam) → review+
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.
Attachment #782080 - Flags: review?(cam) → review+
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.
Attachment #782079 - Flags: review?(cam)
(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.)
Attachment #782079 - Flags: review?(cam)
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?
Attachment #782079 - Flags: review?(cam) → review+
(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.
(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.
Great, thanks for the replies.
You need to log in before you can comment on or make changes to this bug.