1173307 - convert nsTableCellFrame to work with logical coordinates

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Jonathan Kew [:jfkthame]]

Attachment: patch, convert nsTableCellFrame to logical coordinates

Another part of table logicalization; just attaching current WIP.

Comment 1

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11a27cf89fd6

Comment 2

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5442546f4d34

Comment 3

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0f6f4449a26

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Convert nsTableCellFrame to work with logical coordinates. try: -b do -p linux64 -u all

This should be the great majority of the work needed to make nsTableCellFrame work in vertical writing modes; there will be just a few further fixups as the other table classes get converted.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8622435 [details] [diff] [review]
Convert nsTableCellFrame to work with logical coordinates. try: -b do -p linux64 -u all

Review feedback on nsTableRowFrame.cpp changes here:

>+++ b/layout/tables/nsTableRowFrame.cpp
>@@ -340,17 +340,17 @@ nsTableRowFrame::DidResize()
>         cellFrame->SetSize(nsSize(cellRect.width, cellHeight));
>       // realign cell content based on the new height.  We might be able to
>       // skip this if the height didn't change... maybe.  Hard to tell.
>-      cellFrame->VerticallyAlignChild(mMaxCellAscent);
>+      cellFrame->BlockDirAlignChild(wm, mMaxCellAscent);

s/height/bsize/ in comment, I think?

> // Calculates the available width for the table cell based on the known
> // column widths taking into account column spans and column spacing
> static nscoord
>-CalcAvailWidth(nsTableFrame&     aTableFrame,
>+CalcAvailISize(nsTableFrame&     aTableFrame,
>                nsTableCellFrame& aCellFrame)
> {

The comment before this method needs s/width/isize/ in 2 spots.

>@@ -855,50 +855,49 @@ nsTableRowFrame::ReflowChildren(nsPresCo
>     if (doReflowChild) {
>       // Calculate the available width for the table cell using the known column widths
>-      nscoord availCellWidth =
>-        CalcAvailWidth(aTableFrame, *cellFrame);
>+      nscoord availCellISize =
>+        CalcAvailISize(aTableFrame, *cellFrame);

s/width/isize/ in comment

>       // If the avail width is not the same as last time we reflowed the cell or
>       // the cell wants to be bigger than what was available last time or
>       // it is a style change reflow or we are printing, then we must reflow the
>       // cell. Otherwise we can skip the reflow.
[...]
>-      if ((availCellWidth != cellFrame->GetPriorAvailWidth())       ||
>-          (cellDesiredSize.ISize(cellWM) > cellFrame->GetPriorAvailWidth()) ||
>+      if ((availCellISize != cellFrame->GetPriorAvailISize())       ||
>+          (cellDesiredSize.ISize(cellWM) > cellFrame->GetPriorAvailISize()) ||

s/width/isize/ in comment

>         // Reflow the child
>         kidReflowState.emplace(aPresContext, aReflowState, kidFrame,
>-                               LogicalSize(kidFrame->GetWritingMode(),
>-                                           kidAvailSize),
>+                               kidAvailSize.ConvertTo(kidFrame->GetWritingMode(), cellWM),

I don't understand the kidFrame/cellWM conversion here. I think kidFrame *is* the cell that we're working on (which cellWM comes from) -- so its writing mode should be cellWM.  Note that higher up, we have:
  nsTableCellFrame *cellFrame = do_QueryFrame(kidFrame);
so I think they're the same.

>@@ -1088,26 +1087,27 @@ nsTableRowFrame::Reflow(nsPresContext*  
> nscoord
> nsTableRowFrame::ReflowCellFrame(nsPresContext*          aPresContext,
[...]
> {
>+  WritingMode wm = aReflowState.GetWritingMode();
>+
[...]
>   nsTableCellReflowState
>     cellReflowState(aPresContext, aReflowState, aCellFrame,
>-                    LogicalSize(aCellFrame->GetWritingMode(),
>-                                availSize),
>+                    LogicalSize(wm, availSize),
>                     nsHTMLReflowState::CALLER_WILL_INIT);

I think this change is wrong -- the LogicalSize that we pass in to the reflow state should be in the child frame's WM, not in the parent frame's WM, right?

(You're changing it here from using the child's WM to using the parent's.)

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> I think this change is wrong -- the LogicalSize that we pass in to the
> reflow state should be in the child frame's WM, not in the parent frame's
> WM, right?
> 
> (You're changing it here from using the child's WM to using the parent's.)

Alternately: maybe this change is just an optimization, because we're assuming that all table-parts will have the same writing mode.

If that's the case, could you add an assertion that aCellFrame->GetWritingMode() == wm before we create the reflow state? Without that, this looks superficially like a mistake.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8622435 [details] [diff] [review]
Convert nsTableCellFrame to work with logical coordinates. try: -b do -p linux64 -u all

review feedback on nsTableCellFrame.*:

>diff --git a/layout/tables/nsTableCellFrame.cpp b/layout/tables/nsTableCellFrame.cpp
>+void nsTableCellFrame::BlockDirAlignChild(WritingMode aWM, nscoord aMaxAscent)
> {
>   /* It's the 'border-collapse' on the table that matters */
>-  nsMargin borderPadding = GetUsedBorderAndPadding();
>+  nscoord containerWidth = mRect.width;
>+  LogicalMargin borderPadding = GetLogicalUsedBorderAndPadding(aWM);

The new "containerWidth" line you're adding here is splitting a comment & the line that it describes. So: please either bump containerWidth to the very top (before the comment), or declare it further down where it's first used (probably better).

>   // if the content is larger than the cell height align from top
>-  kidYTop = std::max(0, kidYTop);
>+  kidBStart = std::max(0, kidBStart);

comment needs s/height/bsize/, s/top/bstart/

(and add a comma [after "bsize"] while you're here, to help with readability)

> static
> void DebugCheckChildSize(nsIFrame*            aChild,
>                          nsHTMLReflowMetrics& aMet,
>-                         nsSize&              aAvailSize)
>+                         LogicalSize&         aAvailSize)
> {

aAvailSize is entirely unused in this function; we should just remove that parameter.  (either here or in a followup)

I think there's only one caller, so it should be easy to remove.

> // the computed height for the cell, which descendants use for percent height calculations
> // it is the height (minus border, padding) of the cell's first in flow during its final
> // reflow without an unconstrained height.
> static nscoord
>-CalcUnpaginagedHeight(nsPresContext*        aPresContext,
>+CalcUnpaginatedBSize(nsPresContext*        aPresContext,
>                       nsTableCellFrame&     aCellFrame,
>                       nsTableFrame&         aTableFrame,

(1) comment needs s/height/bsize/ (x4 I think?)
(2) "aPresContext," needs an extra space of indentation to stay aligned with the other params

>@@ -867,93 +868,105 @@ nsTableCellFrame::Reflow(nsPresContext* 
>-  nscoord topInset = borderPadding.Top(wm);
>-  nscoord rightInset = borderPadding.Right(wm);
>-  nscoord bottomInset = borderPadding.Bottom(wm);
>-  nscoord leftInset = borderPadding.Left(wm);
>+  nscoord bStartInset = borderPadding.BStart(wm);
>+  nscoord iEndInset = borderPadding.IEnd(wm);
>+  nscoord bEndInset = borderPadding.BEnd(wm);
>+  nscoord iStartInset = borderPadding.IStart(wm);
[...]
>+  availSize.ISize(wm) -= iStartInset + iEndInset;
[...]
>+    availSize.BSize(wm) -= bStartInset + bEndInset;
[etc]

I think we should do away with these inset variables. Nearly all their usages are paired-up, & can be replaced with:
  borderPadding.BStartEnd(wm)
...or...
  borderPadding.IStartEnd(wm)
...which are much cleaner & more foolproof IMO.

>+  LogicalPoint kidOrigin(wm, iStartInset, bStartInset);

This is the one usage where we can't use StartEnd.  We can just grab the values directly here, though, like so:
  LogicalPoint kidOrigin(wm, borderPadding.IStart(wm),
                        borderPadding.BStart(wm));

>-    nscoord computedUnpaginatedHeight =
>-      CalcUnpaginagedHeight(aPresContext, (nsTableCellFrame&)*this,
>-                            *tableFrame, topInset + bottomInset);
[...]
>+    nscoord computedUnpaginatedBSize =
>+      CalcUnpaginatedBSize(aPresContext, (nsTableCellFrame&)*this,
>+                            *tableFrame, bStartInset + bEndInset);

*tableFrame is indented 1 space too far here.


>   // first, compute the height which can be set w/o being restricted by aMaxSize.height
>   LogicalSize cellSize(wm);
>-  LogicalMargin logicalInsets(wm, nsMargin(topInset, rightInset,
>-                                           bottomInset, leftInset));
>   cellSize.BSize(wm) = kidSize.BSize(wm);

Comment tweak:
  s/height/bsize/
  s/aMaxSize.height/available bsize/

(This comment dates back to 1999; available-height must've been called "aMaxSize.height" at that point.)


>   // next determine the cell's width
>   cellSize.ISize(wm) = kidSize.ISize(wm);      // at this point, we've factored in the cell's style attributes

s/width/isize/ in this comment, while you're here

>   // the overflow area will be computed when the child will be vertically aligned

I think the second half of this sentence should now be "...when BlockDirAlignChild() gets called", or something like that.

(This patch is renaming VerticallyAlignChild to BlockDirAlignChild; I think that's the function this comment is referring to.)

>   if (aReflowState.mFlags.mSpecialHeightReflow) {
>-    if (aDesiredSize.Height() > mRect.height) {
>+    if (aDesiredSize.BSize(wm) > BSize(wm)) {
>       // set a bit indicating that the pct height contents exceeded
>       // the height that they could honor in the pass 2 reflow
>-      SetHasPctOverHeight(true);
>+      SetHasPctOverBSize(true);

comment tweak: s/height/bsize/

>+++ b/layout/tables/nsTableCellFrame.h
>   /** return the available width given to this frame during its last reflow */
>-  inline nscoord GetPriorAvailWidth();
>+  inline nscoord GetPriorAvailISize();
> 
>   /** set the available width given to this frame during its last reflow */
>-  inline void SetPriorAvailWidth(nscoord aPriorAvailWidth);
>+  inline void SetPriorAvailISize(nscoord aPriorAvailISize);

Both comments need s/width/ISize/

>-  nscoord      mPriorAvailWidth;      // the avail width during the last reflow
>+  nscoord      mPriorAvailISize;      // the avail width during the last reflow

This comment too.

>-inline bool nsTableCellFrame::HasPctOverHeight()
>+inline bool nsTableCellFrame::HasPctOverBSize()
> {
>   return (mState & NS_TABLE_CELL_HAS_PCT_OVER_HEIGHT) ==
>          NS_TABLE_CELL_HAS_PCT_OVER_HEIGHT;
> }

Looks like this flag needs a rename. Can you file a bug on that, if that's not already covered somewhere?

r=me with this & previous 2 comments addressed. (unless you think any of the adjustments merits an additional sanity-check)

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> >-inline bool nsTableCellFrame::HasPctOverHeight()
> >+inline bool nsTableCellFrame::HasPctOverBSize()
> > {
> >   return (mState & NS_TABLE_CELL_HAS_PCT_OVER_HEIGHT) ==
> >          NS_TABLE_CELL_HAS_PCT_OVER_HEIGHT;
> > }
> 
> Looks like this flag needs a rename. Can you file a bug on that, if that's
> not already covered somewhere?

Indeed it does. I've got a patch in my queue (at the end of bug 1174711, not yet posted for review) that does phys-to-log renaming of several frame state flags whose interpretation is changing, so I'll include this one there.

Comment 9

[Jonathan Kew [:jfkthame]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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