896138 - move style reresolution code into its own file

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

I have a patch series to move style reresolution code into its own file, called RestyleManager.  This code was previously horribly scattered between nsFrameManager and two different big chunks in nsCSSFrameConstructor.cpp.

(I decided to leave the undisplayed map in nsFrameManager, although I considered moving it too.)

This is in preparation for another piece of refactoring that's needed to do bug 828312 comment 69 in a sane way.

Comment 1

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

https://tbpl.mozilla.org/?tree=Try&rev=77706b64f516

Comment 2

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

Attachment: patch 1: Move RestyleTracker and OverflowChangedTracker out of nested namespace mozilla::css (which shouldn't exist) into namespace mozilla.

Comment 3

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

Attachment: patch 2: Add a RestyleManager class.

The later patches in this series will move restyling code into this file
from nsCSSFrameConstructor and nsFrameManager.

Comment 4

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

Attachment: patch 3: Remove forward declaration of struct that hasn't been in this class since changeset e0df6dfdaaca (Bug 479655 part 3).

Comment 5

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

Attachment: patch 4: Move restyle management code from nsCSSFrameConstructor to RestyleManager.

This moves restyling management out of nsCSSFrameConstructor (thus
reducing its size), and keeps the restyling code closer together.

This is the first of two big chunks of code moved in this patch series.
A later patch in this series will move related code from nsFrameManager
into the same destination file.

Comment 6

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

Attachment: patch 5: Expose UndisplayedNode list from nsFrameManager.

This is needed for the following patch, which leaves the undisplayed map
in nsFrameManager rather than moving it to RestyleManager (I think one
could argue for either way, but it was easier not to move it).

Comment 7

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

Attachment: patch 6: Move restyle management code from nsFrameManager to RestyleManager.

This is the second of two big chunks of code moved into the new
RestyleManager class from another sources.

Note that the undisplayed map remains in nsFrameManager, although it
could perhaps have moved.

Comment 8

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

Attachment: patch 7: Remove unused aFrameManager parameters to some functions.

Comment 9

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

Comment on attachment 778791 [details] [diff] [review]
patch 3:  Remove forward declaration of struct that hasn't been in this class since changeset e0df6dfdaaca (Bug 479655 part 3).

Actually, I found another chunk that should have moved here.

Comment 10

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

Attachment: patch 4: Move restyle management code from nsCSSFrameConstructor to RestyleManager.

This moves restyling management out of nsCSSFrameConstructor (thus
reducing its size), and keeps the restyling code closer together.

This is the first of two big chunks of code moved in this patch series.
A later patch in this series will move related code from nsFrameManager
into the same destination file.

Comment 11

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

Comment on attachment 778791 [details] [diff] [review]
patch 3:  Remove forward declaration of struct that hasn't been in this class since changeset e0df6dfdaaca (Bug 479655 part 3).

er, I cancelled the review request on the wrong patch

Comment 12

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

This patch series reduces nsCSSFrameConstructor from 12550 lines to 11129 lines, and nsFrameManager.cpp from 2036 lines to 766 lines.

Comment 13

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

https://tbpl.mozilla.org/?tree=Try&rev=39228eb40710

Comment 14

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

Attachment: patch 4: Move restyle management code from nsCSSFrameConstructor to RestyleManager.

This moves restyling management out of nsCSSFrameConstructor (thus
reducing its size), and keeps the restyling code closer together.

This is the first of two big chunks of code moved in this patch series.
A later patch in this series will move related code from nsFrameManager
into the same destination file.

Comment 15

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

Attachment: interdiff for patch 4

This shows a full-context (i.e., diff -U10000) interdiff between the big chunks of minus lines and the big chunks of plus lines moving from nsCSSFrameConstructor to RestyleManager in patch 4.

(I just pulled the relevant chunks into a file, removed the leading + or - from the patch, and then ran diff.)

The full context is there to show you which chunks I'm diffing.

Comment 16

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

Attachment: interdiff for patch 6

generated the same way, showing the big chunks moved from nsFrameManager to RestyleManager

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 778790 [details] [diff] [review]
patch 2:  Add a RestyleManager class.

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

r=me with comments addressed and question answered either way.

::: layout/base/RestyleManager.cpp
@@ +11,5 @@
> +#include "RestyleManager.h"
> +
> +namespace mozilla {
> +
> +RestyleManager::RestyleManager(nsPresContext *aPresContext)

"*" next to type.

::: layout/base/RestyleManager.h
@@ +18,5 @@
> +namespace mozilla {
> +
> +class RestyleManager {
> +public:
> +

No blank line.

@@ +19,5 @@
> +
> +class RestyleManager {
> +public:
> +
> +  RestyleManager(nsPresContext *aPresContext);

"*" next to type.

@@ +27,5 @@
> +  void Disconnect() {
> +    mPresContext = nullptr;
> +  }
> +
> +  nsPresContext* PresContext() const { return mPresContext; }

Should we assert that mPresContext is non-null in here, like we do in nsPresContext::PresShell?

@@ +30,5 @@
> +
> +  nsPresContext* PresContext() const { return mPresContext; }
> +
> +private:
> +  nsPresContext *mPresContext; // weak, disconnected in Disconnect

"*" next to type.

::: layout/base/nsPresContext.cpp
@@ +931,5 @@
>    mTransitionManager = new nsTransitionManager(this);
>  
>    mAnimationManager = new nsAnimationManager(this);
>  
> +  // FIXME: Why is mozilla:: needed?

I couldn't say...

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 778803 [details] [diff] [review]
patch 4:  Move restyle management code from nsCSSFrameConstructor to RestyleManager.

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

Thanks for the diff between the moved code, that's useful.

r=me with comments addressed and if you can rename ContentStateChanged and Attribute{WillChange,Changed} to something better.

::: layout/base/RestyleManager.cpp
@@ +424,5 @@
> +  // processing restyles
> +  FrameConstructor()->BeginUpdate();
> +
> +  nsPresContext* presContext = PresContext();
> +  FramePropertyTable* propTable = presContext->PropertyTable();

Remove presContext and use mPresContext here and later in the function.

@@ +631,5 @@
> +                               nsIFrame       *aPrimaryFrame,
> +                               nsChangeHint   aMinHint,
> +                               RestyleTracker& aRestyleTracker,
> +                               bool            aRestyleDescendants)
> +{

Can you fix the indenting of the argument names (and maybe stick the "*"s next to the types) while moving this?

@@ +677,5 @@
> +  }
> +}
> +
> +nsresult
> +RestyleManager::ContentStateChanged(nsIContent* aContent,

Can we call this RestyleForContentStateChange, to be consistent with (most of) the other methods that call PostRestyleEvent?  And Attribute{WillChange,Changed} too, though the naming there is a bit harder.  RestyleForAttribute{AboutTo,}Change?  (I feel like it's not great to just use the same Attribute{WillChange,Changed} names from nsIMutationObserver.)

@@ +688,5 @@
> +  }
> +
> +  Element* aElement = aContent->AsElement();
> +
> +  nsPresContext *presContext = mPresContext;

Remove presContext and use mPresContext later in the function.

@@ +689,5 @@
> +
> +  Element* aElement = aContent->AsElement();
> +
> +  nsPresContext *presContext = mPresContext;
> +  nsStyleSet *styleSet = mPresContext->StyleSet();

"*" next to type.

@@ +711,5 @@
> +      hint = nsChangeHint_ReconstructFrame;
> +    } else {
> +      uint8_t app = primaryFrame->StyleDisplay()->mAppearance;
> +      if (app) {
> +        nsITheme *theme = presContext->GetTheme();

mPresContext

@@ +820,5 @@
> +    const nsStyleDisplay* disp = primaryFrame->StyleDisplay();
> +    if (disp->mAppearance) {
> +      nsPresContext* presContext = mPresContext;
> +      nsITheme *theme = presContext->GetTheme();
> +      if (theme && theme->ThemeSupportsWidget(presContext, primaryFrame, disp->mAppearance)) {

Remove presContext and use mPresContext here and later in the function.

@@ +926,5 @@
> +// eRestyle_LaterSiblings or nsRestyleHint(eRestyle_Subtree |
> +// eRestyle_LaterSiblings) as appropriate).
> +static void
> +RestyleSiblingsStartingWith(RestyleManager *aRestyleManager,
> +                            nsIContent *aStartingSibling /* may be null */)

"*"s next to types.

@@ +1124,5 @@
> +  mRebuildAllStyleData = false;
> +  NS_UpdateHint(aExtraHint, mRebuildAllExtraHint);
> +  mRebuildAllExtraHint = nsChangeHint(0);
> +
> +  nsIPresShell *presShell = mPresContext->GetPresShell();

"*" next to type.

@@ +1188,5 @@
> +  // must do this after the ProcessRestyledFrames call in case the
> +  // change list has frame reconstructs in it (since frames to be
> +  // reconstructed will still have their old style context pointers
> +  // until they are destroyed).
> +  mPresContext->PresShell()->StyleSet()->EndReconstruct();

Remove the "PresShell()->".

@@ +1199,5 @@
> +  NS_PRECONDITION(!nsContentUtils::IsSafeToRunScript(),
> +                  "Missing a script blocker!");
> +
> +  // Process non-animation restyles...
> +  nsPresContext *presContext = mPresContext;

Remove presContext and use mPresContext later in the function.

@@ +1275,5 @@
> +  // Make sure we're not in a style refresh; if we are, we still have
> +  // a call to ProcessPendingRestyles coming and there's no need to
> +  // add ourselves as a refresh observer until then.
> +  bool inRefresh = !aForLazyConstruction && mInStyleRefresh;
> +  nsIPresShell *presShell = mPresContext->PresShell();

"*" next to type.

::: layout/base/RestyleManager.h
@@ +105,5 @@
> +  // This function does not call ProcessAttachedQueue() on the binding manager.
> +  // If the caller wants that to happen synchronously, it needs to handle that
> +  // itself.
> +  void ProcessPendingRestyles();
> +  

Trailing white space.

@@ +140,5 @@
> +  {
> +    PostRestyleEventInternal(true);
> +  }
> +
> +  void FlushOverflowChangedTracker() 

Trailing white space.

@@ +167,5 @@
> +                              nsChangeHint aMinChangeHint,
> +                              bool aForAnimation);
> +  void PostRestyleEventInternal(bool aForLazyConstruction);
> +public:
> +

Move newline before "public:".

::: layout/base/RestyleTracker.cpp
@@ +141,2 @@
>  
> +  mRestyleManager->mInStyleRefresh = true;

I wonder if these two calls could be bundled up into a single mRestyleManager->BeginStyleRefresh() call.  Not necessary, but might be nice.  (The friend-ish changing member variable value irks me a bit.)

::: layout/base/nsCSSFrameConstructor.cpp
@@ +7762,1 @@
>    mPresShell->GetPresContext()->RefreshDriver()->

Change this to PresContext() while you're here?

::: layout/base/nsCSSFrameConstructor.h
@@ +78,5 @@
>    nsCSSFrameConstructor& operator=(const nsCSSFrameConstructor& aCopy) MOZ_DELETE;
>  
>  public:
> +  mozilla::RestyleManager* RestyleManager() const
> +    { return mPresShell->GetPresContext()->RestyleManager(); }

Use PresContext() instead.

::: layout/base/nsLayoutUtils.cpp
@@ +5075,5 @@
>    nsIDocument* doc = aElement->GetCurrentDoc();
>    if (doc) {
>      nsCOMPtr<nsIPresShell> presShell = doc->GetShell();
>      if (presShell) {
> +      presShell->GetPresContext()->RestyleManager()->PostRestyleEvent(

Use PresContext().

::: layout/base/nsPresContext.h
@@ +241,5 @@
>        return mDocument;
>    }
>  
>  #ifdef _IMPL_NS_LAYOUT
>    nsStyleSet* StyleSet() { return GetPresShell()->StyleSet(); }

Change this to PresShell() too.

::: layout/base/nsPresShell.cpp
@@ +2691,5 @@
>    changeList.AppendChange(nullptr, aContent, nsChangeHint_ReconstructFrame);
>  
>    // Mark ourselves as not safe to flush while we're doing frame construction.
>    ++mChangeNestCount;
> +  RestyleManager *restyleManager = mPresContext->RestyleManager();

"*" next to type.

@@ +3426,5 @@
>  void
>  PresShell::DispatchSynthMouseMove(nsGUIEvent *aEvent,
>                                    bool aFlushOnHoverChange)
>  {
> +  RestyleManager *restyleManager = mPresContext->RestyleManager();

"*" next to type.

@@ +4104,5 @@
>  
>    // Call this here so it only happens for real content mutations and
>    // not cases when the frame constructor calls its own methods to force
>    // frame reconstruction.
>    if (aContainer)

Can you stick braces around here?

@@ +4236,5 @@
>      // No content to restyle
>      return;
>    }
> +
> +  RestyleManager *restyleManager = mPresContext->RestyleManager();

"*" next to type.

@@ +8158,5 @@
>          // construction.
>          {
>            nsAutoScriptBlocker scriptBlocker;
>            ++mChangeNestCount;
> +          RestyleManager *restyleManager = mPresContext->RestyleManager();

"*" next to type.

::: layout/base/nsRefreshDriver.cpp
@@ +1158,5 @@
>            if (!mStyleFlushObservers.Contains(shell))
>              continue;
>            NS_ADDREF(shell);
>            mStyleFlushObservers.RemoveElement(shell);
> +          shell->GetPresContext()->RestyleManager()->mObservingRefreshDriver = false;

PresContext().

::: layout/generic/nsImageMap.cpp
@@ +841,5 @@
>    aArea->AddSystemEventListener(NS_LITERAL_STRING("blur"), this, false,
>                                  false);
>  
>    // This is a nasty hack.  It needs to go away: see bug 135040.  Once this is
>    // removed, the code added to nsCSSFrameConstructor::RestyleElement,

RestyleManager::RestyleElement

::: layout/style/nsHTMLStyleSheet.cpp
@@ +440,5 @@
>    // shouldn't happen often, so just rebuilding everything is ok.
>    if (mDocument && mDocument->GetShell()) {
>      Element* root = mDocument->GetRootElement();
>      if (root) {
> +      mDocument->GetShell()->GetPresContext()->RestyleManager()->

PresContext().

::: layout/style/nsTransitionManager.cpp
@@ +429,5 @@
>          primaryFrame->StyleContext()->GetParent(), changeList);
>      }
>    }
>  
> +  RestyleManager *restyleManager = mPresContext->RestyleManager();

"*" next to type.

::: layout/svg/nsSVGEffects.cpp
@@ +258,5 @@
>    // Don't need to request UpdateOverflow if we're being reflowed.
>    if (!(mFrame->GetStateBits() & NS_FRAME_IN_REFLOW)) {
>      NS_UpdateHint(changeHint, nsChangeHint_UpdateOverflow);
>    }
> +  mFramePresShell->GetPresContext()->RestyleManager()->PostRestyleEvent(

PresContext().

@@ +283,5 @@
>      nsSVGEffects::InvalidateRenderingObservers(mFrame);
>      // XXXSDL KILL THIS!!!
>      nsSVGUtils::ScheduleReflowSVG(mFrame);
>    }
> +  mFramePresShell->GetPresContext()->RestyleManager()->PostRestyleEvent(

PresContext().

@@ +324,5 @@
>  
>    // Repaint asynchronously in case the path frame is being torn down
>    nsChangeHint changeHint =
>      nsChangeHint(nsChangeHint_RepaintFrame | nsChangeHint_UpdateTextPath);
> +  mFramePresShell->GetPresContext()->RestyleManager()->PostRestyleEvent(

PresContext().

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 778793 [details] [diff] [review]
patch 5:  Expose UndisplayedNode list from nsFrameManager.

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

r=me with nit.

::: layout/base/nsFrameManager.h
@@ +46,5 @@
> +
> +    // Delete mNext iteratively to avoid blowing up the stack (bug 460461).
> +    UndisplayedNode *cur = mNext;
> +    while (cur) {
> +      UndisplayedNode *next = cur->mNext;

Move these two "*"s next to the type while moving this code.

Comment 20

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

(In reply to Cameron McCormack (:heycam) from comment #18)
> Can we call this RestyleForContentStateChange, to be consistent with (most
> of) the other methods that call PostRestyleEvent?  And
> Attribute{WillChange,Changed} too, though the naming there is a bit harder. 
> RestyleForAttribute{AboutTo,}Change?  (I feel like it's not great to just
> use the same Attribute{WillChange,Changed} names from nsIMutationObserver.)

I really prefer things that are just forwarding nsIMutationObserver methods around to use the same method names as nsIMutationObserver -- it makes it easy to find them when searching for those mutation observer method names (since mutation observer implementation spreads out a good bit in layout, with nsPresShell forwarding to other places).  Would a comment pointing out that they're mutation observer methods be sufficient?

Comment 21

[Cameron McCormack (:heycam)]

I think it's enough that the names match to know they're going to be called from actual mutation observer methods.

What about moving the body of Attribute{WillChange,Changed} to RestyleForAttribute{AboutTo,Change} methods and having the former just call the latter (and be inline)?  It seems like it might be useful to have all of the things which do a restyle because of content changes to be called RestyleForXXX.  But I'll leave it to you to decide.

Comment 22

[Cameron McCormack (:heycam)]

Comment on attachment 778794 [details] [diff] [review]
patch 6:  Move restyle management code from nsFrameManager to RestyleManager.

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

If you could adjust the closeness of "*" to types in the bits of code you moved but didn't modify, I'd be in favour of that.  r=me with nits addressed.

::: layout/base/RestyleManager.cpp
@@ +25,3 @@
>  #include "nsContainerFrame.h"
>  #include "nsPlaceholderFrame.h"
> +#include "nsBlockFrame.h"

I don't understand why you've chosen to insert the #includes at these positions. :)

@@ +2043,5 @@
> +    // frames that have the root element as their mContent, down to the
> +    // DocElementContainingBlock.
> +    bool checkUndisplayed;
> +    nsIContent *undisplayedParent;
> +    nsCSSFrameConstructor *frameConstructor = FrameConstructor();

"*" next to type.

@@ +2347,5 @@
> +}
> +
> +void
> +RestyleManager::ComputeStyleChangeFor(nsIFrame          *aFrame,
> +                                      nsStyleChangeList *aChangeList,

"*"s next to types.

@@ +2368,5 @@
> +  // We want to start with this frame and walk all its next-in-flows,
> +  // as well as all its special siblings and their next-in-flows,
> +  // reresolving style on all the frames we encounter in this walk.
> +
> +  FramePropertyTable *propTable = mPresContext->PropertyTable();

"*" next to type.

::: layout/base/RestyleManager.h
@@ +69,5 @@
>    // Get a counter that increments on every style change, that we use to
>    // track whether off-main-thread animations are up-to-date.
>    uint64_t GetAnimationGeneration() const { return mAnimationGeneration; }
>  
> +  /*

"/**".

@@ +79,5 @@
> +   * @param aFrame the root of the subtree to reparent.  Must not be null.
> +   */
> +  NS_HIDDEN_(nsresult) ReparentStyleContext(nsIFrame* aFrame);
> +
> +  /*

"/**".

@@ +172,5 @@
> +  // to mean "reresolve our style context and kids", and use
> +  // nsRestyleHint(0) to mean recompute a new style context for our
> +  // current parent and existing rulenode, and the same for kids.
> +  NS_HIDDEN_(nsChangeHint)
> +    ReResolveStyleContext(nsPresContext    *aPresContext,

Can you fix up the alignment of these arguments and stick the "*"s and "&"s next to their types.  (Well, aParentFrameHintsNotHandledForDescendants is already beyond 80 columns.  Correct how you see fit.)

@@ +175,5 @@
> +  NS_HIDDEN_(nsChangeHint)
> +    ReResolveStyleContext(nsPresContext    *aPresContext,
> +                          nsIFrame          *aFrame,
> +                          nsIContent        *aParentContent,
> +                          nsStyleChangeList *aChangeList, 

Trailing white space.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +440,4 @@
>                 nsIFrame* aNewParentFrame,
>                 const nsFrameList& aFrameList)
>  {
> +  RestyleManager *restyleManager = aFrameConstructor->RestyleManager();

"*" next to RestyleManager.

::: layout/generic/nsFirstLetterFrame.cpp
@@ +81,5 @@
>  NS_IMETHODIMP
>  nsFirstLetterFrame::SetInitialChildList(ChildListID  aListID,
>                                          nsFrameList& aChildList)
>  {
> +  RestyleManager *restyleManager = PresContext()->RestyleManager();

"*" next to type.

::: layout/generic/nsInlineFrame.cpp
@@ +283,5 @@
>  ReparentChildListStyle(nsPresContext* aPresContext,
>                         const nsFrameList::Slice& aFrames,
>                         nsIFrame* aParentFrame)
>  {
> +  RestyleManager *restyleManager = aPresContext->RestyleManager();

"*" next to type.

Comment 23

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

(In reply to Cameron McCormack (:heycam) from comment #21)
> I think it's enough that the names match to know they're going to be called
> from actual mutation observer methods.
> 
> What about moving the body of Attribute{WillChange,Changed} to
> RestyleForAttribute{AboutTo,Change} methods and having the former just call
> the latter (and be inline)?  It seems like it might be useful to have all of
> the things which do a restyle because of content changes to be called
> RestyleForXXX.  But I'll leave it to you to decide.

Though I think logical names for the attribute change methods would be StartRestyleForAttributeChange / FinishRestyleForAttributeChange.  I actually started o do a rename this way, but then realized that the latter propagates the notification to nsIFrame::AttributeChanged (another forwarded nsIMutationObserver method).

So I'm going to go with comments here.

Comment 24

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

(In reply to Cameron McCormack (:heycam) from comment #17)
> Comment on attachment 778790 [details] [diff] [review]
> patch 2:  Add a RestyleManager class.
> @@ +27,5 @@
> > +  void Disconnect() {
> > +    mPresContext = nullptr;
> > +  }
> > +
> > +  nsPresContext* PresContext() const { return mPresContext; }
> 
> Should we assert that mPresContext is non-null in here, like we do in
> nsPresContext::PresShell?

I added an assertion.

(In reply to Cameron McCormack (:heycam) from comment #18)
> Comment on attachment 778803 [details] [diff] [review]
> patch 4:  Move restyle management code from nsCSSFrameConstructor to
> RestyleManager.
> ::: layout/base/RestyleTracker.cpp
> @@ +141,2 @@
> >  
> > +  mRestyleManager->mInStyleRefresh = true;
> 
> I wonder if these two calls could be bundled up into a single
> mRestyleManager->BeginStyleRefresh() call.  Not necessary, but might be
> nice.  (The friend-ish changing member variable value irks me a bit.)

I addressed this by adding a patch 8 on top to do this, rather than putting more changes into this patch.


(In reply to Cameron McCormack (:heycam) from comment #22)
> Comment on attachment 778794 [details] [diff] [review]
> patch 6:  Move restyle management code from nsFrameManager to RestyleManager.
> ::: layout/base/RestyleManager.cpp
> @@ +25,3 @@
> >  #include "nsContainerFrame.h"
> >  #include "nsPlaceholderFrame.h"
> > +#include "nsBlockFrame.h"
> 
> I don't understand why you've chosen to insert the #includes at these
> positions. :)

I always put the corresponding .h (i.e., RestyleManager.h) first to ensure that it compiles on its own.  Then I tend to put more-closely-related code earlier than more-distantly-related code, and I was also grouping frame class headers togethr.

> @@ +172,5 @@
> > +  // to mean "reresolve our style context and kids", and use
> > +  // nsRestyleHint(0) to mean recompute a new style context for our
> > +  // current parent and existing rulenode, and the same for kids.
> > +  NS_HIDDEN_(nsChangeHint)
> > +    ReResolveStyleContext(nsPresContext    *aPresContext,
> 
> Can you fix up the alignment of these arguments and stick the "*"s and "&"s
> next to their types.  (Well, aParentFrameHintsNotHandledForDescendants is
> already beyond 80 columns.  Correct how you see fit.)

I think the best fix here was not bothering to keep lining up the names.

Comment 25

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

Also, there is no nsIPresShell::PresContext() method, only GetPresContext().  While I agree there should be, I'd rather leave that conversion for another day (rather than adding one here and using it in only a dozen places).

Comment 26

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6a65da0faed0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/73d414945f59
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/48769eacc0b8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b960146cc1ed
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/108208f9f817
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f7f2619fbcd9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a1c86c802e6c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4afe8edaedd0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f5baba863a18

I also added a patch 9 to move RecomputePosition within the file; it had been added to the end of nsCSSFrameConstructor.cpp, which wasn't particularly logical.


Thanks for getting to these reviews so quickly.

Comment 27

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #25)
> Also, there is no nsIPresShell::PresContext() method, only GetPresContext().
> While I agree there should be, I'd rather leave that conversion for another
> day (rather than adding one here and using it in only a dozen places).

Ah yes, was confusing it with PresShell() on nsPresContext.

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #24)
> (In reply to Cameron McCormack (:heycam) from comment #18)
> > Comment on attachment 778803 [details] [diff] [review]
> > patch 4:  Move restyle management code from nsCSSFrameConstructor to
> > RestyleManager.
> > ::: layout/base/RestyleTracker.cpp
> > @@ +141,2 @@
> > >  
> > > +  mRestyleManager->mInStyleRefresh = true;
> > 
> > I wonder if these two calls could be bundled up into a single
> > mRestyleManager->BeginStyleRefresh() call.  Not necessary, but might be
> > nice.  (The friend-ish changing member variable value irks me a bit.)
> 
> I addressed this by adding a patch 8 on top to do this, rather than putting
> more changes into this patch.

For this I was thinking more of having a BeginStyleRefresh() on the RestyleManager that did both the BeginUpdate() call and mInStyleRefresh assignment.

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6a65da0faed0
https://hg.mozilla.org/mozilla-central/rev/73d414945f59
https://hg.mozilla.org/mozilla-central/rev/48769eacc0b8
https://hg.mozilla.org/mozilla-central/rev/b960146cc1ed
https://hg.mozilla.org/mozilla-central/rev/108208f9f817
https://hg.mozilla.org/mozilla-central/rev/f7f2619fbcd9
https://hg.mozilla.org/mozilla-central/rev/a1c86c802e6c
https://hg.mozilla.org/mozilla-central/rev/4afe8edaedd0
https://hg.mozilla.org/mozilla-central/rev/f5baba863a18

Comment 29

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

Attachment: patch 10: Move beginning and ending of ProcessPendingRestyles into helper functions on the RestyleManager.

Comment 30

[Cameron McCormack (:heycam)]

Comment on attachment 779598 [details] [diff] [review]
patch 10:  Move beginning and ending of ProcessPendingRestyles into helper functions on the RestyleManager.

Great, thanks!

Comment 31

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

for patch 10:
https://tbpl.mozilla.org/?tree=Try&rev=729da4b00a6c
(and another one a few days ago with different other patches)

Comment 32

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

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

Comment 33

[Ed Morley [:emorley]]

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