898209 - refactor ReResolveStyleContext, step 1: create ElementRestyler and convert parameters to member variables

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

In order to make ReResolveStyleContext more comprehensible, I want to split it into multiple functions.  These functions, however, will need access to the same data -- the arguments to ReResolveStyleContext and a few of its local variables that have large scope across the function.

Therefore, I want to essentially turn it into a class:  an ElementRestyler, which is constructed, and then has its Restyle method called.  Then (in another bug) I will split the Restyle method into functions representing the logical pieces of work done.  (And then, for bug 828312, I can make us change the logic to walk continuations and special siblings during restyling the element so that we can propagate the "hints not handled for descendants" better.)

This patch series contains the part turning it into the ElementRestyler class, but not the splitting of the Restyle method.  Note that there are intermediate states in this patch series with odd indentation of function parameters, but it should all be cleaned up by the end of the patch (since the parameters all, or almost all, go away).  However, the intermediate states do all compile.

Comment 1

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

Attachment: patch 1: Add ElementRestyler class to begin refactoring ReResolveStyleContext.

Comment 2

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

Attachment: patch 2: Create and use ElementRestyler::mMinChange.

Comment 3

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

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

Comment 4

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

Attachment: patch 4: Remove aPresContext parameter to ElementRestyler::Restyle.

Comment 5

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

Attachment: patch 5: Remove the localContent variable in favor of writing mFrame->GetContent().

Comment 6

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

Attachment: patch 6: Create and use ElementRestyler::mParentContent and mContent.

Comment 7

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

Attachment: patch 7: Fix remaining variable shadowing warning.

Comment 8

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

Attachment: patch 8: Create and use ElementRestyler::mChangeList.

Comment 9

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

Attachment: patch 9: Create and use ElementRestyler::mParentFrameHintsNotHandledForDescendants and mHintsNotHandledForDescendants.

Comment 10

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

Attachment: patch 10: Create and use ElementRestyler::mRestyleTracker.

Comment 11

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

Attachment: patch 11: Create and use member variables for accessibility notifications.

Comment 12

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

Attachment: patch 12: Create and use ElementRestyler::mVisibleKidsOfHiddenElement.

Comment 13

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

Attachment: patch 13: Create and use ElementRestyler::mTreeMatchContext.

Comment 14

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

Attachment: diff showing the combined effect of all 13 patches

Comment 15

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

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

Comment 16

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

Note that step 2 is bug 898329.

Comment 17

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

Try run without bug 898329:
https://tbpl.mozilla.org/?tree=Try&rev=e6cd977e1c79

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 781340 [details] [diff] [review]
patch 1:  Add ElementRestyler class to begin refactoring ReResolveStyleContext.

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

r=me with comment tweaks.

::: layout/base/RestyleManager.h
@@ +281,5 @@
> +
> +  // Construct for an element whose parent is being restyled.
> +  ElementRestyler(const ElementRestyler& aParentRestyler);
> +
> +  // Construct for an frame whose parent is being restyled, but which

"a frame"

@@ +282,5 @@
> +  // Construct for an element whose parent is being restyled.
> +  ElementRestyler(const ElementRestyler& aParentRestyler);
> +
> +  // Construct for an frame whose parent is being restyled, but which
> +  // provides the parent style context for its parent frame.  (This is

"provides a different parent style context"?

@@ +286,5 @@
> +  // provides the parent style context for its parent frame.  (This is
> +  // only used for table frames, which are the parent style context for
> +  // the outer table frame (table wrapper frame).  We should probably
> +  // try to get rid of this exception and have the inheritance go the
> +  // other way.)

There's something about that first sentence in the parentheses that's hard to parse.  How about:

  // (This is only used for a table frame, whose style context is
  // is used as the parent style context for the outer table frame
  // (table wrapper frame). ...

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 781341 [details] [diff] [review]
patch 2:  Create and use ElementRestyler::mMinChange.

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

::: layout/base/RestyleManager.cpp
@@ +2579,5 @@
>                do {
>                  ElementRestyler oofRestyler(*this);
> +                oofRestyler.mMinChange =
> +                  NS_SubtractHint(oofRestyler.mMinChange,
> +                                  nsChangeHint_AllReflowHints);

I feel like this modification of mMinChange should be handled in the constructor somehow.  This looks to be the only time you're directly manipulating the state of an ElementRestyler from outside of it.

::: layout/base/RestyleManager.h
@@ +326,5 @@
>                 DesiredA11yNotifications aDesiredA11yNotifications,
>                 nsTArray<nsIContent*>& aVisibleKidsOfHiddenElement,
>                 TreeMatchContext &aTreeMatchContext);
>  
> +  nsChangeHint MinChange() { return mMinChange; }

Is MinChange the right name for the accessor?  Just after the object has been constructed, mMinChange has the minimum style change that's already going to be handled.  But during Restyle, and afterwards once we call MinChange, the value is the set of changes which are now going to be handled by the restyle that just happened.

I think it's confusing because what the change hint is the minimum relative to changes after calling Restyle.

Maybe for the moment just add a comment above MinChange describing what the returned value means, and we can worry about renaming it to something more obvious later.

Comment 20

[Cameron McCormack (:heycam)]

Comment on attachment 781345 [details] [diff] [review]
patch 5:  Remove the localContent variable in favor of writing mFrame->GetContent().

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

r=me with comment tweak.

::: layout/base/RestyleManager.cpp
@@ +2020,5 @@
>  {
>    // It would be nice if we could make stronger assertions here; they
>    // would let us simplify the ?: expressions below setting |content|
>    // and |pseudoContent| in sensible ways as well as making what
> +  // |mFrame->GetContent()|, |content|, and |pseudoContent| mean make more

Not sure it makes sense any longer to mention |mFrame->GetContent()| as one of the things that could be made to "make more sense".  Remove?

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 781352 [details] [diff] [review]
patch 11:  Create and use member variables for accessibility notifications.

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

r=me with this.

::: layout/base/RestyleManager.h
@@ +297,5 @@
>    ElementRestyler(ParentContextFromChildFrame,
>                    const ElementRestyler& aParentFrameRestyler,
>                    nsIFrame* aFrame);
>  
> +public:

Don't need this "public:".

Comment 22

[Cameron McCormack (:heycam)]

Comment on attachment 781354 [details] [diff] [review]
patch 13:  Create and use ElementRestyler::mTreeMatchContext.

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

r=me with this.

::: layout/base/RestyleManager.h
@@ +281,5 @@
>                    nsIFrame* aFrame,
>                    nsStyleChangeList* aChangeList,
>                    nsChangeHint aMinChange,
>                    RestyleTracker& aRestyleTracker,
> +                  TreeMatchContext &aTreeMatchContext,

"&" next to type.

Comment 23

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

(In reply to Cameron McCormack (:heycam) from comment #18)
> Comment on attachment 781340 [details] [diff] [review]
> patch 1:  Add ElementRestyler class to begin refactoring
> ReResolveStyleContext.
> 
> @@ +282,5 @@
> > +  // Construct for an element whose parent is being restyled.
> > +  ElementRestyler(const ElementRestyler& aParentRestyler);
> > +
> > +  // Construct for an frame whose parent is being restyled, but which
> > +  // provides the parent style context for its parent frame.  (This is
> 
> "provides a different parent style context"?

I think it makes more sense as is.  The parent frame's parent style context *is* the child frame's style context.  This constructor is used only for that case.  Maybe "which provides" -> "whose style context is"?

(In reply to Cameron McCormack (:heycam) from comment #19)
> Comment on attachment 781341 [details] [diff] [review]
> patch 2:  Create and use ElementRestyler::mMinChange.
> 
> Review of attachment 781341 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/RestyleManager.cpp
> @@ +2579,5 @@
> >                do {
> >                  ElementRestyler oofRestyler(*this);
> > +                oofRestyler.mMinChange =
> > +                  NS_SubtractHint(oofRestyler.mMinChange,
> > +                                  nsChangeHint_AllReflowHints);
> 
> I feel like this modification of mMinChange should be handled in the
> constructor somehow.  This looks to be the only time you're directly
> manipulating the state of an ElementRestyler from outside of it.

I guess I could add a flags word to the constructor.  Do you mind if I do that in a separate patch on top of the series to save the time rebuilding the whole patch stack?

> ::: layout/base/RestyleManager.h
> @@ +326,5 @@
> >                 DesiredA11yNotifications aDesiredA11yNotifications,
> >                 nsTArray<nsIContent*>& aVisibleKidsOfHiddenElement,
> >                 TreeMatchContext &aTreeMatchContext);
> >  
> > +  nsChangeHint MinChange() { return mMinChange; }
> 
> Is MinChange the right name for the accessor?  Just after the object has
> been constructed, mMinChange has the minimum style change that's already
> going to be handled.  But during Restyle, and afterwards once we call
> MinChange, the value is the set of changes which are now going to be handled
> by the restyle that just happened.
> 
> I think it's confusing because what the change hint is the minimum relative
> to changes after calling Restyle.
> 
> Maybe for the moment just add a comment above MinChange describing what the
> returned value means, and we can worry about renaming it to something more
> obvious later.

Perhaps rename the getter from MinChange() to Change()?  And maybe eventually rename the member variable as well?  What it means changes over time, since it essentially represents the change hint that's already taken care of, which changes over time.  Maybe that suggests a name like ChangeDone() or ChangeCompleted(), though I'm not a fan of either.

(In reply to Cameron McCormack (:heycam) from comment #20)
> Comment on attachment 781345 [details] [diff] [review]
> patch 5:  Remove the localContent variable in favor of writing
> mFrame->GetContent().
> 
> ::: layout/base/RestyleManager.cpp
> @@ +2020,5 @@
> >  {
> >    // It would be nice if we could make stronger assertions here; they
> >    // would let us simplify the ?: expressions below setting |content|
> >    // and |pseudoContent| in sensible ways as well as making what
> > +  // |mFrame->GetContent()|, |content|, and |pseudoContent| mean make more
> 
> Not sure it makes sense any longer to mention |mFrame->GetContent()| as one
> of the things that could be made to "make more sense".  Remove?

I think it does still make sense since it would make their relationship make more sense, which is largely what the comment is about.


Other than the above, I agree with the suggested changes.

Comment 24

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #23)
> (In reply to Cameron McCormack (:heycam) from comment #18)
> > Comment on attachment 781340 [details] [diff] [review]
> > patch 1:  Add ElementRestyler class to begin refactoring
> > ReResolveStyleContext.
> > 
> > @@ +282,5 @@
> > > +  // Construct for an element whose parent is being restyled.
> > > +  ElementRestyler(const ElementRestyler& aParentRestyler);
> > > +
> > > +  // Construct for an frame whose parent is being restyled, but which
> > > +  // provides the parent style context for its parent frame.  (This is
> > 
> > "provides a different parent style context"?
> 
> I think it makes more sense as is.  The parent frame's parent style context
> *is* the child frame's style context.  This constructor is used only for
> that case.  Maybe "which provides" -> "whose style context is"?

Yes that sounds good.

> (In reply to Cameron McCormack (:heycam) from comment #19)
> > Comment on attachment 781341 [details] [diff] [review]
> > patch 2:  Create and use ElementRestyler::mMinChange.
> > 
> > Review of attachment 781341 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: layout/base/RestyleManager.cpp
> > @@ +2579,5 @@
> > >                do {
> > >                  ElementRestyler oofRestyler(*this);
> > > +                oofRestyler.mMinChange =
> > > +                  NS_SubtractHint(oofRestyler.mMinChange,
> > > +                                  nsChangeHint_AllReflowHints);
> > 
> > I feel like this modification of mMinChange should be handled in the
> > constructor somehow.  This looks to be the only time you're directly
> > manipulating the state of an ElementRestyler from outside of it.
> 
> I guess I could add a flags word to the constructor.  Do you mind if I do
> that in a separate patch on top of the series to save the time rebuilding
> the whole patch stack?

Separate patch is fine.

> > ::: layout/base/RestyleManager.h
> > @@ +326,5 @@
> > >                 DesiredA11yNotifications aDesiredA11yNotifications,
> > >                 nsTArray<nsIContent*>& aVisibleKidsOfHiddenElement,
> > >                 TreeMatchContext &aTreeMatchContext);
> > >  
> > > +  nsChangeHint MinChange() { return mMinChange; }
> > 
> > Is MinChange the right name for the accessor?  Just after the object has
> > been constructed, mMinChange has the minimum style change that's already
> > going to be handled.  But during Restyle, and afterwards once we call
> > MinChange, the value is the set of changes which are now going to be handled
> > by the restyle that just happened.
> > 
> > I think it's confusing because what the change hint is the minimum relative
> > to changes after calling Restyle.
> > 
> > Maybe for the moment just add a comment above MinChange describing what the
> > returned value means, and we can worry about renaming it to something more
> > obvious later.
> 
> Perhaps rename the getter from MinChange() to Change()?  And maybe
> eventually rename the member variable as well?  What it means changes over
> time, since it essentially represents the change hint that's already taken
> care of, which changes over time.  Maybe that suggests a name like
> ChangeDone() or ChangeCompleted(), though I'm not a fan of either.

What about naming the constructor argument aHintsHandledByAncestors, the member variable mHintsHandledForFrame and the getter HintsHandledForFrame?  Is that a more accurate description?

(The naming of change hint related variables sometimes uses "change" and sometimes "hint"; it'd be good to make that uniform at some point.)

> (In reply to Cameron McCormack (:heycam) from comment #20)
> > Comment on attachment 781345 [details] [diff] [review]
> > patch 5:  Remove the localContent variable in favor of writing
> > mFrame->GetContent().
> > 
> > ::: layout/base/RestyleManager.cpp
> > @@ +2020,5 @@
> > >  {
> > >    // It would be nice if we could make stronger assertions here; they
> > >    // would let us simplify the ?: expressions below setting |content|
> > >    // and |pseudoContent| in sensible ways as well as making what
> > > +  // |mFrame->GetContent()|, |content|, and |pseudoContent| mean make more
> > 
> > Not sure it makes sense any longer to mention |mFrame->GetContent()| as one
> > of the things that could be made to "make more sense".  Remove?
> 
> I think it does still make sense since it would make their relationship make
> more sense, which is largely what the comment is about.

I understand.  I read the sentence as saying that it would help, among other things, make "mFrame->GetContent()" make more sense.  In isolation, of course that makes sense; it's the content node for the frame.  What about

  as well as making the relationship between |mFrame->GetContent()|,
  |content|, and |pseudoContent| make more sense

or

  as well as making |content| and |pseudoContent| and their relationship to
  |mFrame->GetContent()| make more sense.

(Sorry, I didn't want to get too far into changing existing things you're just moving.)

Comment 25

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

(In reply to Cameron McCormack (:heycam) from comment #24)
> What about naming the constructor argument aHintsHandledByAncestors, the
> member variable mHintsHandledForFrame and the getter HintsHandledForFrame? 
> Is that a more accurate description?

Not quite, since the meaning of the member variable changes over time.  The argument and the getter probably work, though.  Maybe just mHintsHandled for the member variable?

Comment 26

[Cameron McCormack (:heycam)]

That works.

Comment 27

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/49b374b0b7c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d26163c01c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b330b441fcd
https://hg.mozilla.org/integration/mozilla-inbound/rev/13cc87d5b2db
https://hg.mozilla.org/integration/mozilla-inbound/rev/38528a20bcf4
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cfd498adf33
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6ee9b412a8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba7321484801
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab7b779a4087
https://hg.mozilla.org/integration/mozilla-inbound/rev/dba4bd2585cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/89f45106a984
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bffd84e325c
https://hg.mozilla.org/integration/mozilla-inbound/rev/03bc6da23257
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9101cca26dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/78e90f762a3e

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/49b374b0b7c7
https://hg.mozilla.org/mozilla-central/rev/3d26163c01c9
https://hg.mozilla.org/mozilla-central/rev/6b330b441fcd
https://hg.mozilla.org/mozilla-central/rev/13cc87d5b2db
https://hg.mozilla.org/mozilla-central/rev/38528a20bcf4
https://hg.mozilla.org/mozilla-central/rev/5cfd498adf33
https://hg.mozilla.org/mozilla-central/rev/a6ee9b412a8b
https://hg.mozilla.org/mozilla-central/rev/ba7321484801
https://hg.mozilla.org/mozilla-central/rev/ab7b779a4087
https://hg.mozilla.org/mozilla-central/rev/dba4bd2585cf
https://hg.mozilla.org/mozilla-central/rev/89f45106a984
https://hg.mozilla.org/mozilla-central/rev/4bffd84e325c
https://hg.mozilla.org/mozilla-central/rev/03bc6da23257
https://hg.mozilla.org/mozilla-central/rev/b9101cca26dd
https://hg.mozilla.org/mozilla-central/rev/78e90f762a3e

Comment 29

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/cab84ad2c07c

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cab84ad2c07c