Closed Bug 898209 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(14 files)

17.07 KB, patch
heycam
: review+
Details | Diff | Splinter Review
23.55 KB, patch
Details | Diff | Splinter Review
35.40 KB, patch
heycam
: review+
Details | Diff | Splinter Review
10.47 KB, patch
heycam
: review+
Details | Diff | Splinter Review
7.99 KB, patch
heycam
: review+
Details | Diff | Splinter Review
21.25 KB, patch
heycam
: review+
Details | Diff | Splinter Review
1.54 KB, patch
heycam
: review+
Details | Diff | Splinter Review
18.76 KB, patch
heycam
: review+
Details | Diff | Splinter Review
15.11 KB, patch
heycam
: review+
Details | Diff | Splinter Review
12.27 KB, patch
heycam
: review+
Details | Diff | Splinter Review
15.24 KB, patch
heycam
: review+
Details | Diff | Splinter Review
12.53 KB, patch
heycam
: review+
Details | Diff | Splinter Review
17.64 KB, patch
heycam
: review+
Details | Diff | Splinter Review
53.85 KB, patch
Details | Diff | Splinter Review
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.
Blocks: 898329
Depends on: 896138
Note that step 2 is bug 898329.
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). ...
Attachment #781340 - Flags: review?(cam) → review+
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.
Attachment #781341 - Flags: review?(cam)
Attachment #781342 - Flags: review?(cam) → review+
Attachment #781343 - Flags: review?(cam) → review+
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?
Attachment #781345 - Flags: review?(cam) → review+
Attachment #781346 - Flags: review?(cam) → review+
Attachment #781347 - Flags: review?(cam) → review+
Attachment #781348 - Flags: review?(cam) → review+
Attachment #781349 - Flags: review?(cam) → review+
Attachment #781350 - Flags: review?(cam) → review+
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:".
Attachment #781352 - Flags: review?(cam) → review+
Attachment #781353 - Flags: review?(cam) → review+
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.
Attachment #781354 - Flags: review?(cam) → review+
(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.
Flags: needinfo?(cam)
(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.)
Flags: needinfo?(cam)
(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?
Flags: needinfo?(cam)
That works.
Flags: needinfo?(cam)
Depends on: 900284
You need to log in before you can comment on or make changes to this bug.