861593 - encapsulate nsCSSOffsetState member variables

Status: RESOLVED INCOMPLETE

(Core :: Layout, defect)

Description

[Cameron McCormack (:heycam)]

Attachment: patch

The use of public non-const member variables in nsCSSOffsetState isn't so great.

Comment 1

[Cameron McCormack (:heycam)]

(Ignore the s/m(Computed(Margin|Padding))/$1()/ changes that were made in the comments for Compute{Margin,Padding} in nsHTMLReflowState.h, they shouldn't have been made.)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Cameron, was this patch done by hand, or with a script?  It might be simpler to review the script.... ;)

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Either way, I'm going to see if I can foist this off on dholbert.

Daniel, please punt it back to me if you can't deal with it right now?

Comment 4

[Cameron McCormack (:heycam)]

It wasn't completely done by script.  Generally I made changes with `perl -i -pe 's{[>-]mComputedMargin}{ComputedMargin()}g` on files I selected after visually inspecting the result of grepping for say `[>-]mComputedMargin` under layout/, then compiling, and fixing up any compilation errors afterwards (e.g. to use SetComputedMargin() when it was an assignment).

Comment 5

[Daniel Holbert [:dholbert]]

Could you post a new patch that has Comment 1 addressed?  In the meantime, I'll take a look.

Comment 6

[Daniel Holbert [:dholbert]]

[Also, it looks like this has been quickly bitrotted by bug 857820. The bitrot is pretty easy to fix, though, especially if you qref on top of a pre-857820 with 3 lines of patch-context instead of 8, so that the patch-chunks are more granular.]

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> Could you post a new patch that has Comment 1 addressed?  In the meantime,
> I'll take a look.

Nevermind, I see what you mean now - those bogus-changes are more minimal than I was expecting.  (I'm assuming you've got those reverted locally, anyway.)

Comment 8

[Daniel Holbert [:dholbert]]

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

>@@ -7800,36 +7800,36 @@ nsFrame::BoxReflow(nsBoxLayoutState&        aState,
>     parentFrame->RemoveStateBits(~nsFrameState(0));
>     parentFrame->AddStateBits(savedState);
> 
>     // This may not do very much useful, but it's probably worth trying.
>     if (parentSize.width != NS_INTRINSICSIZE)
>       parentReflowState.SetComputedWidth(std::max(parentSize.width, 0));
>     if (parentSize.height != NS_INTRINSICSIZE)
>       parentReflowState.SetComputedHeight(std::max(parentSize.height, 0));
>-    parentReflowState.mComputedMargin.SizeTo(0, 0, 0, 0);
>+    parentReflowState.SetComputedMargin(nsMargin(0, 0, 0, 0));
>     // XXX use box methods
>-    parentFrame->GetPadding(parentReflowState.mComputedPadding);
>-    parentFrame->GetBorder(parentReflowState.mComputedBorderPadding);
>-    parentReflowState.mComputedBorderPadding +=
>-      parentReflowState.mComputedPadding;
>+    parentReflowState.SetComputedPadding(parentFrame->GetPadding());
>+    parentReflowState.SetComputedBorderPadding(
>+        parentFrame->GetBorder() +
>+        parentReflowState.ComputedPadding());
[...]
>+++ b/layout/generic/nsIFrame.h
>@@ -2739,16 +2739,30 @@ NS_PTR_TO_INT32(frame->Properties().Get(nsIFrame::ParagraphDepthProperty()))
>   NS_IMETHOD GetBorderAndPadding(nsMargin& aBorderAndPadding);
>   NS_IMETHOD GetBorder(nsMargin& aBorder)=0;
>   NS_IMETHOD GetPadding(nsMargin& aBorderAndPadding)=0;
>   NS_IMETHOD GetMargin(nsMargin& aMargin)=0;
>   virtual void SetLayoutManager(nsBoxLayout* aLayout) { }
>   virtual nsBoxLayout* GetLayoutManager() { return nullptr; }
>   NS_HIDDEN_(nsresult) GetClientRect(nsRect& aContentRect);
> 
>+  nsMargin GetPadding()
>+  {
>+    nsMargin padding; 
>+    GetPadding(padding);
>+    return padding;
>+  }
>+
>+  nsMargin GetBorder()
>+  {
>+    nsMargin border;
>+    GetBorder(border);
>+    return border;
>+  }

I'd rather not introduce these shims.  It looks like you only use them once, and I don't think it's worth polluting nsIFrame.h with additional convenience-methods that are only needed in one spot, especially when it's for a XUL-specific thing.

I'd prefer either of the following:
 (a) Move the shim code over to the code that invokes them, in nsFrame.cpp. (just as extra code in nsFrame::BoxReflow, or as static helper functions defined right above that method if you prefer)
...OR:
 (b) file a separate helper-bug on de-COM'ing these methods and just making them return their nsMargins directly, and layer this bug's patch on top of that.  (It looks like they only have one impl, in nsBox, and it only ever returns NS_OK, so there's no reason for them to need nsresult etc.)

Comment 9

[Daniel Holbert [:dholbert]]

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

>diff --git a/layout/generic/nsHTMLReflowState.h b/layout/generic/nsHTMLReflowState.h
>   // Callers using this constructor must call InitOffsets on their own.
>   nsCSSOffsetState(nsIFrame *aFrame, nsRenderingContext *aRenderingContext)
>-    : frame(aFrame)
>-    , rendContext(aRenderingContext)
>+    : mFrame(aFrame)
>+    , mRenderingContext(aRenderingContext)
>   {
>   }
[...]
>   nsCSSOffsetState(nsIFrame *aFrame, nsRenderingContext *aRenderingContext,
>                    nscoord aContainingBlockWidth)
>-    : frame(aFrame)
>-    , rendContext(aRenderingContext)
>+    : mFrame(aFrame)
>+    , mRenderingContext(aRenderingContext)
>   {
[...]
>+  nsIFrame* Frame() const
>+  { return mFrame; }
>+
>+  nsRenderingContext* RenderingContext() const
>+  { return mRenderingContext; }

Now that we've got Frame() and RenderingContext() accessor methods, which (by their naming) implicitly promise to return non-null, we should verify that this promise holds up, with assertions -- preferably in the nsCSSOffsetState constructors, or perhaps in the Frame() & RenderingContext() accessor methods.

>+  // the frame being reflowed
>+  nsIFrame* mFrame;
[...]
>+  // rendering context to use for measurement
>+  nsRenderingContext* mRenderingContext;

Can we make mFrame and mRenderingContext const-pointers, to ensure that they'll never be redirected to point to something else? This would strengthen our assurance that Frame() and RenderingContext() will never return non-null.

> nscoord 
>-GetVerticalMarginBorderPadding(const nsHTMLReflowState* aReflowState)
>+nsHTMLReflowState::GetVerticalMarginBorderPadding() const
> {
[...]
>+static nscoord
>+GetVerticalMarginBorderPadding(const nsHTMLReflowState* aReflowState)
>+{
>+  if (!aReflowState) {
>+    return 0;
>+  }
>+  return aReflowState->GetVerticalMarginBorderPadding();
>+}

I'm not sure it makes sense to move this into a public method on nsHTMLReflowState (exposed to the world in nsHTMLReflowState.h). This is only invoked in a single static helper-function, which happens to be directly below this code.  Seems like it'd make more sense to just keep this as a static helper function, rather than adding it to the giant list of public methods on nsHTMLReflowState.

>+protected:
>+  void InitOffsets(nscoord aHorizontalPercentBasis,
[...]
>+  nscoord ComputeHeightValue(nscoord aContainingBlockHeight,
>+                             uint8_t aBoxSizing,
>+                             const nsStyleCoord& aCoord);
[...]
> protected:
>-  void InitOffsets(nscoord aHorizontalPercentBasis,
[...]
>-  nscoord ComputeWidthValue(nscoord aContainingBlockWidth,
>-                            uint8_t aBoxSizing,
>-                            const nsStyleCoord& aCoord);

So it looks like you added a second "protected" clause here, and you moved the existing method-declarations upwards to it (or something like that). Was there a reason for that?

If there's no good reason to shuffle these around, it seems like we might as well leave those where they are (since they're already marked as 'protected'), and avoid messing with their hg blame.  (Also: seems like it'd be better to keep it at one "protected" chunk rather than introducing a second one, unless there's a good reason to have the two separate chunks.)

>@@ -529,16 +553,18 @@ public:
>+  nscoord GetVerticalMarginBorderPadding() const;
>+

(Per above, I don't think we want to promote this to a method -- so, this new decl in the .h file can be dropped.)

>--- a/layout/generic/nsLineLayout.h 
>   /**
>    * This can't be null. It usually returns a block frame but may return
>    * some other kind of frame when inline frames are reflowed in a non-block
>    * context (e.g. MathML or floating first-letter).
>    */
>-  nsIFrame* GetLineContainerFrame() const { return mBlockReflowState->frame; }
>+  nsIFrame* GetLineContainerFrame() const { return mBlockReflowState->Frame(); }

Observation: per the comment and the "Frame()" invocation, this method is guaranteed to return non-null; hence, we should drop the Get from its name.  Could you file a followup on that?

>+++ b/layout/generic/nsSimplePageSequence.cpp
>-    nsMargin pageCSSMargin = kidReflowState.mComputedMargin;
>+    nsMargin pageCSSMargin = kidReflowState.ComputedMargin();
>     y += pageCSSMargin.top;
>     const nscoord x = pageCSSMargin.left;

While you're here, could you change the pageCSSMargin decl to be a 'const nsMargin&' instead of a 'nsMargin'? It appears to be read-only, so there's no need for it to be a copy.

I think that's it for my review-feedback.  Not granting r+ yet since that's a decent number of things to change (plus there's been some bitrot as noted above), but I'll grant swift r+ when an updated patch is posted, with this comment & comment 8 & comment 1 addressed.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> > nscoord 
> >-GetVerticalMarginBorderPadding(const nsHTMLReflowState* aReflowState)
> >+nsHTMLReflowState::GetVerticalMarginBorderPadding() const
[...]
> I'm not sure it makes sense to move this into a public method on
> nsHTMLReflowState (exposed to the world in nsHTMLReflowState.h). This is
> only invoked in a single static helper-function, which happens to be
> directly below this code.

Ah, I see you're make the caller (CalcQuirkContainingBlockHeight) into a public method on nsHTMLReflowState, too, over in bug 861746's patch.

I don't see a strong motivation for that change, either, though, given that CalcQuirkContainingBlockHeight is only called from inside the .cpp file, and it only uses data that is already being made publicly accessible.

So I tend to think both of these (CalcQuirkContainingBlockHeight and GetVerticalMarginBorderPadding) should just stay as they are -- static helper-functions.

Comment 11

[Cameron McCormack (:heycam)]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> I'd rather not introduce these shims.  It looks like you only use them once,
> and I don't think it's worth polluting nsIFrame.h with additional
> convenience-methods that are only needed in one spot, especially when it's
> for a XUL-specific thing.
> 
> I'd prefer either of the following:
>  (a) Move the shim code over to the code that invokes them, in nsFrame.cpp.
> (just as extra code in nsFrame::BoxReflow, or as static helper functions
> defined right above that method if you prefer)
> ...OR:
>  (b) file a separate helper-bug on de-COM'ing these methods and just making
> them return their nsMargins directly, and layer this bug's patch on top of
> that.  (It looks like they only have one impl, in nsBox, and it only ever
> returns NS_OK, so there's no reason for them to need nsresult etc.)

I think (b) is reasonable.  (They do have more implementations than on nsBox, but none return anything other than NS_OK.)  Filed bug 863972 for that, and the next patch I'll attach to this bug will be on top of that one.

Comment 12

[Cameron McCormack (:heycam)]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> So it looks like you added a second "protected" clause here, and you moved
> the existing method-declarations upwards to it (or something like that). Was
> there a reason for that?

What I was attempting to do was to balance my desire to have all member variables at the end, while also keeping members in public-protected-private order.  What is the preferred style here?  Not interleave member functions and member variables, at expense of possibly duplicating visibility sections, or have at most one of each of public, protected and private sections, and allow member variables of different visibilities to be non-contiguous?

> If there's no good reason to shuffle these around, it seems like we might as
> well leave those where they are (since they're already marked as
> 'protected'), and avoid messing with their hg blame.  (Also: seems like it'd
> be better to keep it at one "protected" chunk rather than introducing a
> second one, unless there's a good reason to have the two separate chunks.)

(In general I'm loathe to not make changes to improve readability on account of "breaking blame".  I really feel that the blame reading tools should be improved to easily skip past such changes.)

> >--- a/layout/generic/nsLineLayout.h 
> >   /**
> >    * This can't be null. It usually returns a block frame but may return
> >    * some other kind of frame when inline frames are reflowed in a non-block
> >    * context (e.g. MathML or floating first-letter).
> >    */
> >-  nsIFrame* GetLineContainerFrame() const { return mBlockReflowState->frame; }
> >+  nsIFrame* GetLineContainerFrame() const { return mBlockReflowState->Frame(); }
> 
> Observation: per the comment and the "Frame()" invocation, this method is
> guaranteed to return non-null; hence, we should drop the Get from its name. 
> Could you file a followup on that?

Filed bug 864289.

Comment 13

[Cameron McCormack (:heycam)]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Ah, I see you're make the caller (CalcQuirkContainingBlockHeight) into a
> public method on nsHTMLReflowState, too, over in bug 861746's patch.
> 
> I don't see a strong motivation for that change, either, though, given that
> CalcQuirkContainingBlockHeight is only called from inside the .cpp file, and
> it only uses data that is already being made publicly accessible.
> 
> So I tend to think both of these (CalcQuirkContainingBlockHeight and
> GetVerticalMarginBorderPadding) should just stay as they are -- static
> helper-functions.

I think you are right about GetVerticalMarginBorderPadding being able to stay a static helper function.  But CalcQuirkContainingBlockHeight needs to become a member function if it is to access parentReflowState, which I think should be private.  So how about I make it a private member function instead?

Comment 14

[Cameron McCormack (:heycam)]

Attachment: patch (v1.1)

Patch with all comments addressed except for CalcQuirkContainingBlockHeight becoming a private member function, and without yet addressing the concerns about reordering the members in the class declaration.

Comment 15

[Cameron McCormack (:heycam)]

This new patch is on top of bug 863972's patch.

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #12)
> > If there's no good reason to shuffle these around, it seems like we might as
[...]
> (In general I'm loathe to not make changes to improve readability on account
> of "breaking blame".  I really feel that the blame reading tools should be
> improved to easily skip past such changes.)

(I'm less concerned with "breaking blame" and more concerned with making this bug's patch(es) reasonably granular & provably-correct.)

So currently, this bug's patch essentially consists of:
  (a) Add & use some accessors (i.e. single-line string replacements, all over the place)
  (b) Reshuffle a bunch of existing lines in a header-file.

I was questioning whether there was a reason for (b) -- it sounds like there is, good.  Still: those are orthogonal & different types of changes (and it takes a different type of analysis to review each one, see what changed, & make sure it's correct).  This becomes more complex when they're merged together.  It'd make it easier on future HG archeologists and on me as the reviewer if you could split (a) and (b) into two separate, atomic patches.

Comment 17

[Cameron McCormack (:heycam)]

(In reply to Daniel Holbert [:dholbert] from comment #16)
> I was questioning whether there was a reason for (b) -- it sounds like there
> is, good.  Still: those are orthogonal & different types of changes (and it
> takes a different type of analysis to review each one, see what changed, &
> make sure it's correct).  This becomes more complex when they're merged
> together.  It'd make it easier on future HG archeologists and on me as the
> reviewer if you could split (a) and (b) into two separate, atomic patches.

I can certainly split them up into separate patches.

Do you have an opinion on the order that the members should go in?

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #17)
> I can certainly split them up into separate patches.

Thanks!

> Do you have an opinion on the order that the members should go in?

I think the order you picked makes sense. (all methods go before all member-data, and protected methods go before private methods)

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #13)
> I think you are right about GetVerticalMarginBorderPadding being able to
> stay a static helper function.  But CalcQuirkContainingBlockHeight needs to
> become a member function if it is to access parentReflowState, which I think
> should be private.  So how about I make it a private member function instead?

Two things:
  a) parentReflowState is still public, as of *this* bug here. So that's not a reason to make CalcQuirkContainingBlockHeight into a member-function here.

  b) Even in the patch on bug 861746, where we hide parentReflowState, we add a public ParentReflowState() accessor.  So CalcQuirkContainingBlockHeight just call ParentReflowState(), right?  It doesn't need to be inside the class to get access to the parentReflowState.

(incidentally, I noticed that CalcQuirkContainingBlockHeight does have some shady const-removing casts, which appear to be unnecessary. I filed bug 864579 on dropping those.)

Comment 20

[Cameron McCormack (:heycam)]

Of course; I forgot I added a ParentReflowState() accessor.  I agree then it doesn't need to be a member function.

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 740254 [details] [diff] [review]
patch (v1.1)

>+++ b/layout/generic/nsHTMLReflowState.cpp
>-nscoord 
>+static nscoord
> GetVerticalMarginBorderPadding(const nsHTMLReflowState* aReflowState)
> {
[...]
> 
>+  nsMargin borderPadding = aReflowState->ComputedBorderPadding();
>   result += margin.top + margin.bottom;
>-  result += aReflowState->mComputedBorderPadding.top + 
>-            aReflowState->mComputedBorderPadding.bottom;
>+  result += borderPadding.top + borderPadding.bottom;

Simplify this to:
  result += aReflowState->ComputedBorderPadding().TopBottom();

(This means you don't have to introduce the |borderPadding| local var, which is nice.)

And while you're at it, might as well change the line above it to:
  result += margin.TopBottom();
for consistency in TopBottom usage there.

r=me with that, with CalcQuirkContainingBlockHeight staying as static helper-function (not a method), and with the header cut-n-pasting split out of this & into a separate patch per comment 16.

Thanks!

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 740254 [details] [diff] [review]
patch (v1.1)

One more nit:

>diff --git a/layout/tables/nsTableFrame.cpp b/layout/tables/nsTableFrame.cpp
> nsTableFrame::AncestorsHaveStyleHeight(const nsHTMLReflowState& aParentReflowState)
> {
>   for (const nsHTMLReflowState* rs = &aParentReflowState;
>-       rs && rs->frame; rs = rs->parentReflowState) {
>-    nsIAtom* frameType = rs->frame->GetType();
>+       rs && rs->Frame(); rs = rs->parentReflowState) {

As noted above (and as you're now verifying with an assertion IIRC), Frame() is guaranteed to return non-null -- so this null-check here isn't useful.

So, I'd suggest just dropping the null-check here. The old code could be forgiven for not being sure whether rs->frame might be null or non-null, but with your patch, we have our method-naming convention (backed by an assertion and a const pointer) to guarantee that it'll be non-null.

Comment 23

[Cameron McCormack (:heycam)]

Indeed; I'll remove that part of the condition.

Comment 24

[Cameron McCormack (:heycam)]

This is probably quite out of date.