Closed Bug 896138 Opened 6 years ago Closed 6 years ago

move style reresolution code into its own file

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(10 files, 2 obsolete files)

5.38 KB, patch
heycam
: review+
Details | Diff | Splinter Review
7.12 KB, patch
heycam
: review+
Details | Diff | Splinter Review
1.32 KB, patch
heycam
: review+
Details | Diff | Splinter Review
4.84 KB, patch
heycam
: review+
Details | Diff | Splinter Review
136.88 KB, patch
heycam
: review+
Details | Diff | Splinter Review
5.18 KB, patch
heycam
: review+
Details | Diff | Splinter Review
181.29 KB, patch
heycam
: review+
Details | Diff | Splinter Review
70.54 KB, text/plain
Details
59.61 KB, text/plain
Details
5.28 KB, patch
heycam
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → dbaron
The later patches in this series will move restyling code into this file
from nsCSSFrameConstructor and nsFrameManager.
Attachment #778790 - Flags: review?(bzbarsky)
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.
Attachment #778792 - Flags: review?(bzbarsky)
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).
Attachment #778793 - Flags: review?(bzbarsky)
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.
Attachment #778794 - Flags: review?(bzbarsky)
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.
Attachment #778791 - Flags: review?(bzbarsky)
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.
Attachment #778801 - Flags: review?(bzbarsky)
Attachment #778792 - Attachment is obsolete: true
Attachment #778792 - Flags: review?(bzbarsky)
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
Attachment #778791 - Flags: review?(bzbarsky)
This patch series reduces nsCSSFrameConstructor from 12550 lines to 11129 lines, and nsFrameManager.cpp from 2036 lines to 766 lines.
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.
Attachment #778803 - Flags: review?(cam)
Attachment #778801 - Attachment is obsolete: true
Attachment #778801 - Flags: review?(bzbarsky)
Attached file 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.
Attachment #778804 - Attachment mime type: text/x-patch → text/plain
Attachment #778789 - Flags: review?(bzbarsky) → review?(cam)
Attachment #778790 - Flags: review?(bzbarsky) → review?(cam)
Attachment #778791 - Flags: review?(bzbarsky) → review?(cam)
Attachment #778793 - Flags: review?(bzbarsky) → review?(cam)
Attachment #778794 - Flags: review?(bzbarsky) → review?(cam)
Attachment #778795 - Flags: review?(bzbarsky) → review?(cam)
Attachment #778804 - Attachment description: interdiff or patch 4 → interdiff for patch 4
Attached file interdiff for patch 6
generated the same way, showing the big chunks moved from nsFrameManager to RestyleManager
Attachment #778789 - Flags: review?(cam) → review+
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...
Attachment #778790 - Flags: review?(cam) → review+
Attachment #778791 - Flags: review?(cam) → review+
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().
Attachment #778803 - Flags: review?(cam) → review+
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.
Attachment #778793 - Flags: review?(cam) → review+
(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?
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 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.
Attachment #778794 - Flags: review?(cam) → review+
Attachment #778795 - Flags: review?(cam) → review+
(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.
(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.
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).
(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 on attachment 779598 [details] [diff] [review]
patch 10:  Move beginning and ending of ProcessPendingRestyles into helper functions on the RestyleManager.

Great, thanks!
Attachment #779598 - Flags: review?(cam) → review+
for patch 10:
https://tbpl.mozilla.org/?tree=Try&rev=729da4b00a6c
(and another one a few days ago with different other patches)
You need to log in before you can comment on or make changes to this bug.