861746 - encapsulate nsHTMLReflowState member variables

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Cameron McCormack (:heycam)]

Attachment: patch

As with nsCSSOffsetState.

Most of the changes, like in bug 861593, are to use accessor functions rather than direct access to the member variables, which are now all private.  I added setters only where they were needed.  I changed how some of these state variables were modified:

* I added a SetAncestorReflowStates method that sets both mParentReflowState and mCBReflowState to the same value, since that's the only way these variables are set.
* I have a single nsMargin getter ComputedOffsets(), but two separate setters SetComputed{Top,Left}(), as the other sides were never assigned to.
* I moved the logic of checking whether we've already recorded a frame with clearance into a RecordDiscoveredClearance() method.
* I moved the logic of checking whether we have a percentage height and then invoking mPercentHeightObserver into a NotifyPercentHeightObserverIfNeeded() method.

I also moved CalcQuirkContainingBlockHeight up from a static helper function to a member function of nsHTMLReflowState.

Can you please check whether my choice of verbs at the start of the mFlags accessors are reasonable.  (For example, I wasn't exactly sure about "HasHResize".)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 737360 [details] [diff] [review]
patch

Going to try to pass this off to Daniel too.

Comment 2

[Daniel Holbert [:dholbert]]

I couldn't get this to apply cleanly (after updating to a the revision right before 'blink' landed and applying bug 861593's patch and then this patch).  That makes it harder to review -- 
for huge renaming patches like this that only change a character here & there, I prefer to apply it locally & review w/ 'hg meld', since that highlights the characters on each line that actually changed.)

Could you post a cset ID that this applies cleanly on top of? (with with bug 861593's patch in between, of course)

Alternately, could you repost w/ bitrot fixed and include the up-to-date cset ID that your bitrot-fixed version stacks on top of? (with the unbitrotted version of bug 861593's patch available for download somewhere, too)

Comment 3

[Daniel Holbert [:dholbert]]

For reference, the commands I used to try to apply this were:
====
# Update to the revision right before the blink-removal:
hg up -r 128728
# apply bug 861593's patch:
hg qimport https://bugzilla.mozilla.org/attachment.cgi?id=737210 && hg qpush
# apply this bug's patch:
hg qimport https://bugzilla.mozilla.org/attachment.cgi?id=737360 && hg qpush
====

Those commands end up saying:

patching file layout/generic/nsHTMLReflowState.h
Hunk #1 FAILED at 261
1 out of 6 hunks FAILED -- saving rejects to file layout/generic/nsHTMLReflowState.h.rej
patching file layout/tables/nsTableRowGroupFrame.cpp
Hunk #10 FAILED at 1278
1 out of 11 hunks FAILED -- saving rejects to file layout/tables/nsTableRowGroupFrame.cpp.rej

patch -F10 doesn't seem to fix it.  And updating to the parent of the last nsTableRowGroupFrame change doesn't help, either -- it makes things worse. (It makes bug 861593's patch no longer apply).

Comment 4

[Cameron McCormack (:heycam)]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Could you post a cset ID that this applies cleanly on top of? (with with bug
> 861593's patch in between, of course)

It should apply on top of a3dad9390a3098bf7c646c5197a303b3b993ce38, though you will also have to apply bug 861598's patch on top of bug 861593's.

Comment 5

[Daniel Holbert [:dholbert]]

Nice, that works to get the patch applied -- I was missing bug 861598's patch. Thanks!

FIRST BATCH OF COMMENTS (haven't looked in too much detail yet):

(In reply to Cameron McCormack (:heycam) from comment #0)
> * I added a SetAncestorReflowStates method that sets both mParentReflowState
> and mCBReflowState to the same value, since that's the only way these
> variables are set.

Well, they're only set in one place (nsFrame::BoxReflow), and that one place is pretty idiosyncratic.  I'd rather be verbose and use separate setters, I think - clients generally shouldn't be modifying these, and when they do, it's better for them to explicitly know what they're changing.

So -- I'd prefer we use separate setters for these pointers.

> * I have a single nsMargin getter ComputedOffsets(), but two separate
> setters SetComputed{Top,Left}(), as the other sides were never assigned to.

I don't think you actually need setters at all for these. The one place that sets these values (nsCSSFrameConstructor::RecomputePosition) doesn't actually need them to be persistently set in the reflow state -- it can instead make its own local copy of reflowState.ComputedOffsets(), and modify top & left in that local copy if it needs to, and then use that local copy instead of the reflowState's copy for positioning purposes.

That should completely remove the need for these SetComputed* setters.

Also: a significant chunk of this patch seems to be from moving all the nsHTMLReflowState member-vars (and their header-documentation) to a different spot in the class.

Instead of doing that, could you just label them as "private" and leave them in their current spot, and then later move them in a cut-n-paste just-code-reordering followup patch? (If you leave them in their current spot, they'll mostly automatically be treated as private, since you're changing from struct to class, FWIW.  So you should be able to mostly leave them alone, drop the 'private' keyword before mComputedWidth in the existing code, and move the subsequent "public" down a few lines to be for the methods instead of the member-vars).

Then you can move all the member-vars to the bottom of the class decl in a large-but-easy-to-verify cut-n-paste followup patch.

Both this mega-patch and the followup patch will be easier to sanity-check on their own, rather than combined, and it'll also make this mega-patch less likely to get bitrotted while it's going through review.

That's all for now; I'll look more tomorrow.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 737360 [details] [diff] [review]
patch

SECOND BATCH OF COMMENTS:

>+++ b/layout/base/nsCSSFrameConstructor.cpp
>     const nsMargin& parentBorder =
>-      parentReflowState.mStyleBorder->GetComputedBorder();
>+      parentReflowState.StyleBorder()->GetComputedBorder();

I'm not sure that it makes sense to expose public getters for StyleBorder() etc. on the reflow state.  We already have StyleFoo() methods on the style context, and StyleFoo() methods on the frame (which just forward to the style context), and I'd rather not introduce another set of those methods.

IIUC the reflow state just caches these pointers for its own convenience, and anyone external that's using them could just as easily be calling Frame()->StyleFoo() (or even just directly calling StyleFoo() on the "this" pointer when we're actually in an instance method on the frame, as is the case in many places in this patch)

So: rather than introducing these getters, maybe file a helper-bug on converting the existing external usages of nsHTMLReflowState::mStyleFoo to use the frame's StyleFoo() methods -- and then this patch (which will cut off external access to mStyleFoo) would layer on top of that bug.

>+++ b/layout/generic/nsBRFrame.cpp
>@@ -89,17 +89,17 @@ BRFrame::Reflow(nsPresContext* aPresContext,
>-  nsLineLayout* ll = aReflowState.mLineLayout;
>+  nsLineLayout* ll = aReflowState.LineLayout();
>   if (ll) {

LineLayout should be GetLineLayout(), since it can return null.

>+++ b/layout/generic/nsBlockFrame.cpp
>+    while (aLastRS->ParentReflowState() &&
>+           aLastRS->ParentReflowState()->Frame()->GetContent() == frame->GetContent()) {
>       lastButOneRS = aLastRS;
>-      aLastRS = aLastRS->parentReflowState;
>+      aLastRS = aLastRS->ParentReflowState();

ParentReflowState() should be named GetParentReflowState(), since it can return null.

>@@ -1061,17 +1061,17 @@ nsBlockFrame::Reflow(nsPresContext*           aPresContext,

>-  if (reflowState->mFlags.mHResize)
>+  if (reflowState->HasHResize())

I'd prefer IsHResize() ("is" instead of "has"), and similar for VResize. This flag basically maps to "Does this reflow constitute a horizontal resize, in some sense", so I think that naming makes sense.

While you're at it, we might want to rename the flags themselves, too (e.g. mVResize --> mIsVResize), for consistency, but that's maybe less important.

> nsBlockFrame::PropagateFloatDamage(nsBlockReflowState& aState,
>                                    nsLineBox* aLine,
>                                    nscoord aDeltaY)
> {
>-  nsFloatManager *floatManager = aState.mReflowState.mFloatManager;
[...]
>+  nsFloatManager *floatManager = aState.mReflowState.FloatManager();

Does this need to be GetFloatManager()? (i.e. can it return null?)

Looks like this is *usually* non-null, but e.g. one of the constructors initializes it to null and looks like it might not reassign it to something else... On the other hand, most (all?) of its usages don't seem to bother to null-check it.  So I'm not sure offhand whether this should have a Get or not.

But if we keep it as "FloatManager()" (no Get), then at least add an assertion in that accessor impl, to verify that it's returning something non-null.

Also, while we're on the FloatManager topic: your patch adds a method "SetFloatManager", but we only invoke it in 2 places, both of which are inside nsFloatManager.cpp.

I'm not sure that merits having an explicit setter, public to everyone... Might be better/simpler to just declare nsFloatManager and nsAutoFloatManager as being friend classes of nsHTMLReflowState, since they kind of are helper-classes for it anyway.

>@@ -2026,17 +2026,18 @@ nsBlockFrame::ReflowDirtyLines(nsBlockReflowState& aState)
>     // may be placed incorrectly), but that's OK because we'll mark the
>-    // line dirty below under "if (aState.mReflowState.mDiscoveredClearance..."
>+    // line dirty below under
>+    // "if (aState.mReflowState.WillReflowAgainForClearance()..."

I don't see the "if (aState.mReflowState.WillReflowAgainForClearance()) check that this comment refers to, anywhere below it.

I *do* see something like what the *old* code mentions -- there's a check for
   if ([stuff] &&
       aState.mReflowState.mDiscoveredClearance) {
which is approximately what the old comment says to look for.  But your patch changes that to a check for DiscoveredClearancePointer(), not WillReflowAgainForClearance().  So I think your comment-tweak here might want to say DiscoveredClearancePointer()...?

I might hold off on further review at this point, and circle back after you've addressed these comments, since I think the patch may look a bit different after these have been addressed (and when layered on top of an updated patch for the offsetstate patch).

Comment 7

[Cameron McCormack (:heycam)]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> I'm not sure that it makes sense to expose public getters for StyleBorder()
> etc. on the reflow state.  We already have StyleFoo() methods on the style
> context, and StyleFoo() methods on the frame (which just forward to the
> style context), and I'd rather not introduce another set of those methods.
> 
> IIUC the reflow state just caches these pointers for its own convenience,
> and anyone external that's using them could just as easily be calling
> Frame()->StyleFoo() (or even just directly calling StyleFoo() on the "this"
> pointer when we're actually in an instance method on the frame, as is the
> case in many places in this patch)
> 
> So: rather than introducing these getters, maybe file a helper-bug on
> converting the existing external usages of nsHTMLReflowState::mStyleFoo to
> use the frame's StyleFoo() methods -- and then this patch (which will cut
> off external access to mStyleFoo) would layer on top of that bug.

That's fair.  Filed bug 864129 for that.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #7)
> (In reply to Daniel Holbert [:dholbert] from comment #6)
> > I'm not sure that it makes sense to expose public getters for StyleBorder()
> > etc. on the reflow state.
> That's fair.  Filed bug 864129 for that.

aaand that turned out to be a red herring. Sorry for that :-/  Looks like these are worth caching on the reflow state after all.

I think it'd probably be worth giving these methods a slightly different name (e.g. "CachedStyleBorder()") to indicate that they're doing something different (and cheaper) than what nsIFrame::StyleBorder() and nsStyleContext::StyleBorder() do.

Otherwise, it'd be less clear why our reflow code bothers to call reflowState.StyleBorder() instead of just calling StyleBorder() (on |this|, the frame pointer).  If instead the method were called reflowState.CachedStyleBorder(), then that'd be clearer.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 737360 [details] [diff] [review]
patch

THIRD BATCH OF COMMENTS
(I'm considering comment 8 as part of the second batch)

>+++ b/layout/generic/nsBlockFrame.cpp
>@@ -2913,29 +2914,27 @@ nsBlockFrame::ReflowBlockFrame(nsBlockReflowState& aState,
>-      // Only record the first frame that requires clearance
>-      if (!*aState.mReflowState.mDiscoveredClearance) {
>-        *aState.mReflowState.mDiscoveredClearance = frame;
>-      }
>+      // This only records the first frame that requires clearance
>+      aState.mReflowState.RecordDiscoveredClearance(frame);

So, you've refactored this null-check-and-set into a new method "RecordDiscoveredClearance". Do we really need that method? (This code here is its only caller, FWIW.)

If we don't need it, I'd prefer we avoid adding it, to avoid bloating the nsHTMLReflowState API.

It looks to me like we can just apply s/mDiscoveredClearance/DiscoveredClearancePointer()/ to the chunk of old code here - if so, then we don't need the special method.

>@@ -3099,24 +3098,24 @@ nsBlockFrame::ReflowBlockFrame(nsBlockReflowState& aState,
>-      blockHtmlRS.mDiscoveredClearance = aState.mReflowState.mDiscoveredClearance;
>+      blockHtmlRS.SetDiscoveredClearancePointer(aState.mReflowState.DiscoveredClearancePointer());
>     }

This line is pretty long now -- maybe bump aState.* down to the next line (& indent it by 2 spaces w.r.t. previous line)?

>+++ b/layout/generic/nsBlockReflowContext.cpp
>@@ -212,64 +212,67 @@ nsBlockReflowContext::ReflowBlock(const nsRect&       aSpace,
>                                   nsReflowStatus&     aFrameReflowStatus,
>                                   nsBlockReflowState& aState)
> {
>+    if ((mFrame->GetStateBits() & NS_BLOCK_FLOAT_MGR) == 0) {
>+      aFrameRS.SetBlockDelta(mOuterReflowState.BlockDelta() + ty
>+                             - aLine->mBounds.y);
>+    }

Don't start a line with an operator ("-" in this case). Either shift the "-" up to the previous line, or bump the "ty" down.

>@@ -7825,23 +7821,21 @@ nsFrame::BoxReflow(nsBoxLayoutState&        aState,
>-      reflowState.parentReflowState = outerReflowState;
>-      reflowState.mCBReflowState = outerReflowState;
>+      reflowState.SetAncestorReflowStates(*outerReflowState);
>     } else {
>-      reflowState.parentReflowState = &parentReflowState;
>-      reflowState.mCBReflowState = &parentReflowState;
>+      reflowState.SetAncestorReflowStates(parentReflowState);
>     }
>-    reflowState.mReflowDepth = aState.GetReflowDepth();
>+    reflowState.SetReflowDepth(aState.GetReflowDepth());

Hmm.  So as I noted at the beginning of comment 5, this makes me uncomfortable (both setters, actually - for the ancestor reflow states and the reflow depth). This code is basically duck-taping together a reflow state from scratch, and it's manually setting some variables that we don't need to set *anywhere* else [so we could abstract them away, if it weren't for this code right here].

(Ideally it'd be nice to use something like a special constructor or Init method to manage this for us, instead of exposing these setters to the world for use in this one spot. BUT that would basically amount to refactoring this chunk of BoxReflow(), and I'm not going to ask you to do that as part of this patch. :))

So: I still lean towards spliting up "SetAncestorReflowStates" as noted in comment 5, to make this less magical & more explicit -- and if you like, it might be worth adding a disclaimer above these setters in nsHTMLReflowSTate.h saying "WARNING: You probably don't want to call this (needed in BoxReflow, but generally shouldn't be needed elsewhere)", or something to that effect.

>+++ b/layout/generic/nsHTMLReflowState.cpp
>-static nscoord
>-CalcQuirkContainingBlockHeight(const nsHTMLReflowState* aCBReflowState)
>+nscoord
>+nsHTMLReflowState::CalcQuirkContainingBlockHeight() const

As discussed over on bug 861593, I don't think we need to promote this to a member function.

>+++ b/layout/generic/nsHTMLReflowState.h
>-struct nsHTMLReflowState : public nsCSSOffsetState {
>-  // the reflow states are linked together. this is the pointer to the
>-  // parent's reflow state
>-  const nsHTMLReflowState* parentReflowState;
>-
>-  // pointer to the float manager associated with this area
>-  nsFloatManager* mFloatManager;
[...]
>+  // the reflow states are linked together. this is the pointer to the
>+  // parent's reflow state
>+  const nsHTMLReflowState* mParentReflowState;
>+
>+  // pointer to the float manager associated with this area
>+  nsFloatManager* mFloatManager;

As in bug 861593 comment 16, this patch here has code-changes for adding getters and renaming member-vars, *combined with* a bunch of cut-and-paste reordering of nsHTMLReflowState.

Could you do the code-reordering stuff in a separate patch?

(So the first (main) patch here would have the variable-renames (e.g. s/parentReflowState/mParentReflowState/) and it'd create the accessors, but wouldn't reorder the class. And the second patch would have the reordering)

That'll make it much easier to see what's changing vs. not-changing here.

>+++ b/layout/generic/nsImageFrame.cpp
>@@ -104,33 +104,33 @@ static bool HaveFixedSize(const nsStylePosition* aStylePosition)
> inline bool HaveFixedSize(const nsHTMLReflowState& aReflowState)
> { 
>-  NS_ASSERTION(aReflowState.mStylePosition, "crappy reflowState - null stylePosition");
>+  NS_ASSERTION(aReflowState.StylePosition(), "crappy reflowState - null stylePosition");

Just drop this assertion -- it's unnecessary. The reflow state should already be making sure that these values aren't null (given the lack of a "Get" here), and clients don't need to re-check that.


...and with that, I'm finally all the way through this patch. :) Canceling review flag and toggling "feedback+" to indicate that I've looked at this; I'll hold off on r+ for now since that's a lot of changes)

Comment 10

[Daniel Holbert [:dholbert]]

This ended up being partly fixed by http://hg.mozilla.org/mozilla-central/rev/bcba7f58a8e7
( See particularly this chunk: http://hg.mozilla.org/mozilla-central/rev/bcba7f58a8e7#l2.80 )

Should we resolve this as FIXED by bug 735577, maybe?

Comment 11

[Cameron McCormack (:heycam)]

Yeah, let's do that.  I had a mind to remove all public fields from nsHTMLReflowState, but starting from the patches on the this bug won't be that useful.  Sorry I never got around to using your comments on my patches here; this bug never rose up to the top of my list again.

Comment 12

[Daniel Holbert [:dholbert]]

No worries; sorry again for Comment 8.