Closed Bug 1157569 Opened 9 years ago Closed 9 years ago

Convert BCBorder calculation to use logical direction wording

Categories

(Core :: Layout: Tables, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: xidorn, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(18 files, 1 obsolete file)

39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
3.58 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.72 KB, patch
roc
: review+
Details | Diff | Splinter Review
87.45 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
134.28 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
6.17 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
53.33 KB, patch
Details | Diff | Splinter Review
The border-collapse calculation seems to be a part which can be converted relatively independently.
Depends on: 1159990
Attached file MozReview Request: bz://1157569/xidorn (obsolete) —
/r/7979 - bug 1157569 part 1 - Rename BC_BORDER_{TOP,RIGHT,BOTTOM,LEFT}_HALF* to BC_BORDER_{START,END}_HALF*.
/r/7981 - Bug 1157569 part 2 - Convert output parameters of GetColorAndStyle/GetPaintStyleInfo in nsTableFrame to use pointers.
/r/7983 - Bug 1157569 part 3 - Merge two GetColorAndStyle functions in nsTableFrame.
/r/7985 - Bug 1157569 part 4 - Replace mTableIsLTR with mTableWM in BCMapCellInfo & BCPaintBorderIterator.
/r/7987 - Bug 1157569 part 5 - Convert GetColorAndStyle, GetPaintStyleInfo, and CompareBorders in nsTableFrame to accept writing mode and logical side.
/r/7989 - Bug 1157569 part 6 - Rename methods and fields in BCMapCellInfo from physicals to logicals.
/r/7991 - Bug 1157569 part 7 - Rename methods and fields in nsTableColFrame from physicals to logicals.
/r/7993 - Bug 1157569 part 8 - Move some code in BCPaintBorderIterator::SetDamageArea for less computation.
/r/7995 - Bug 1157569 part 9 - Rename methods and fields in nsTableRowFrame from physicals to logicals.
/r/7997 - Bug 1157569 part 10 - Add operator+=/-= for LogicalMargin.
/r/7999 - Bug 1157569 part 11 - Rename methods and fields in nsTableCellFrame from physicals to logicals.
/r/8001 - Bug 1157569 part 12 - Remove useless m{Start,End}Side from BCMapCellInfo.

Pull down these commits:

hg pull -r 86d59959feff45c595becfd1bb3e832fba075a57 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8600234 - Flags: review?(roc)
Attachment #8600234 - Flags: review?(jfkthame)
This is part of the work. Since I'm going to delay the work on this bug for a while, I'd like to land the patches I've finished first.
Keywords: leave-open
Most of these patches shouldn't change any behavior. Part 5, though, changes the behavior of border collapse on RTL tables, but should be fine.
https://reviewboard.mozilla.org/r/7997/#review6765

::: layout/generic/WritingModes.h:1288
(Diff revision 1)
> +    return *this;

Can't we just write

  CHECK_WRITING_MODE(...);
  mMargin += aMargin.mMargin;
  return *this;

here (and similarly for -= below)?
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #3)
> Most of these patches shouldn't change any behavior. Part 5, though, changes
> the behavior of border collapse on RTL tables, but should be fine.

Could you describe the behavior change you expect here? If it's an improvement, we should really have some tests that fail before the change, and start passing after it.
https://reviewboard.mozilla.org/r/7997/#review6767

> Can't we just write
> 
>   CHECK_WRITING_MODE(...);
>   mMargin += aMargin.mMargin;
>   return *this;
> 
> here (and similarly for -= below)?

Yes I guess so. I was probably misled by operator+/- on which we cannot do this.

BTW, nsMargin doesn't have operator-=, which probably means we don't need that at all. I wonder whether I should add one to BaseMargin and simplify operator-=() here, or remove this operator here either.
(In reply to Jonathan Kew (:jfkthame) from comment #6)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #3)
> > Most of these patches shouldn't change any behavior. Part 5, though, changes
> > the behavior of border collapse on RTL tables, but should be fine.
> 
> Could you describe the behavior change you expect here? If it's an
> improvement, we should really have some tests that fail before the change,
> and start passing after it.

TBH I don't know. I haven't had an overall idea about how the border collapse works. As it still mixes physical and logical code, it is hard to say whether it would be an improvement or a regression. Probably just change the way how it is broken. It doesn't break existing tests, though.

I agree that we absolutely need tests once this conversion finishes, but not now.
https://reviewboard.mozilla.org/r/7995/#review6841

r+ with that fixed

::: layout/tables/nsTableFrame.cpp:2678
(Diff revision 1)
> +    pCollapseBorder = &collapseBorder;

It looks like his change doesn't belong in this patch.
https://reviewboard.mozilla.org/r/7995/#review6853

> It looks like his change doesn't belong in this patch.

This change, along with the change to nsTableRowGroupFrame::GetBCBorderWidth, could be either in this patch, or in a separate patch. I'm fine with either way. If you prefer them be in a separate patch, I can do that.
Comment on attachment 8600234 [details]
MozReview Request: bz://1157569/xidorn

/r/7979 - bug 1157569 part 1 - Rename BC_BORDER_{TOP,RIGHT,BOTTOM,LEFT}_HALF* to BC_BORDER_{START,END}_HALF*. r=roc
/r/7981 - Bug 1157569 part 2 - Convert output parameters of GetColorAndStyle/GetPaintStyleInfo in nsTableFrame to use pointers. r=roc
/r/7983 - Bug 1157569 part 3 - Merge two GetColorAndStyle functions in nsTableFrame. r=roc
/r/7985 - Bug 1157569 part 4 - Replace mTableIsLTR with mTableWM in BCMapCellInfo & BCPaintBorderIterator. r=roc
/r/7987 - Bug 1157569 part 5 - Convert GetColorAndStyle, GetPaintStyleInfo, and CompareBorders in nsTableFrame to accept writing mode and logical side. r=roc
/r/7989 - Bug 1157569 part 6 - Rename methods and fields in BCMapCellInfo from physicals to logicals. r=roc
/r/7991 - Bug 1157569 part 7 - Rename methods and fields in nsTableColFrame from physicals to logicals. r=roc
/r/7993 - Bug 1157569 part 8 - Move some code in BCPaintBorderIterator::SetDamageArea for less computation. r=roc
/r/7995 - Bug 1157569 part 9 - Rename methods and fields in nsTableRowFrame from physicals to logicals. r=roc
/r/7997 - Bug 1157569 part 10 - Add operator+= for LogicalMargin.
/r/7999 - Bug 1157569 part 11 - Rename methods and fields in nsTableCellFrame from physicals to logicals. r=roc
/r/8001 - Bug 1157569 part 12 - Remove useless m{Start,End}Side from BCMapCellInfo. r=roc

Pull down these commits:

hg pull -r 4eaceb5e2c628800f605de7cc5fec5417e2a2104 https://reviewboard-hg.mozilla.org/gecko/
I've updated part 10, could you review again, jfkthame?
Flags: needinfo?(jfkthame)
Flags: needinfo?(jfkthame)
Attachment #8600234 - Flags: review?(roc)
Attachment #8600234 - Flags: review?(jfkthame)
Depends on: 1170419
It seems all related code in layout/tables/ could be converted to logical direction. The final conversion between logical and physical should happen in BC{Horizontal,Vertical}Seg::Paint(), before calling nsCSSRendering::DrawTableBorderSegment().
Depends on: 1171328
Attachment #8600234 - Attachment is obsolete: true
The patches so far have slightly broken the rendering of RTL tables with border-collapse; see reftest, which fails with current trunk code (https://treeherder.mozilla.org/#/jobs?repo=try&revision=671d3d53cbdd).
The problem is that the values in BCPropertyData are computed and stored physically, but returned by Get[Included]OuterBCBorder in a LogicalMargin as though they were logical values. It's not clear to me at this stage whether we want/need to convert the computation entirely to logical coordinates, but for the time being at least, we need to convert properly from physical to logical when returning these values as a LogicalMargin. This fixes the rendering of the above testcase (https://treeherder.mozilla.org/#/jobs?repo=try&revision=a48aad03e6d3).
Attachment #8623550 - Flags: review?(roc)
Attachment #8623548 - Flags: review?(roc)
Blocks: 1020208
A lot of this is just mechanical name changes; but also it switches code from using physical Side constants to LogicalSides.
Attachment #8626460 - Flags: review?(dholbert)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
And this finishes the conversion sufficiently that collapsed borders render properly in vertical tables. The computation of which borders are to be drawn is entirely done in logical-coord terms, and the final border segments are converted to physical only for the actual painting stage. Reftest to follow. While we're here, we should also do a purely cosmetic pass over this code to fix the most glaring style-guide violations, I think; I've tried to resist doing much of that within the patches that are actually changing code.
Attachment #8626463 - Flags: review?(dholbert)
Blocks: 1177690
Comment on attachment 8626460 [details] [diff] [review]
part 13 - More conversion of physical to logical terminology in border-collapse calculations

>+++ b/layout/tables/celldata.h
>-inline bool BCData::IsLeftStart() const
>+inline bool BCData::IsIStartStart() const
> {
>-  return (bool)mLeftStart;
>+  return (bool)mIStartStart;
> }
[...]
>-  unsigned mLeftStart:     1; // set if this is the start of a vertical border segment
>+  unsigned mIStartStart:   1; // set if this is the start of a block-dir border segment

To the extent that this code lives on, we should rename these methods/variables at some point to avoid "StartStart" confusingness.  (Without having talked to you & seen the old code, I'd have a much harder time figuring out what this means.)

(Maybe it doesn't matter since this border-collapsing code is getting reimplemented soon, though.)
Here's a basic test for border-collapse in vertical mode. On OS X, we get a small discrepancy between the testcase and the reference in the bevelling of some of the corners, but I think we can just annotate that as fuzz.
Attachment #8626815 - Flags: review?(dholbert)
Comment on attachment 8626460 [details] [diff] [review]
part 13 - More conversion of physical to logical terminology in border-collapse calculations

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

r=me with the following nits.

I'm pretty sure none of these are addressed by the next patch; apologies if any of them are.

::: layout/tables/nsCellMap.cpp
@@ +966,4 @@
>    }
>  }
>  
>  // store the aSide border segment at coord = (aRowIndex, aColIndex). For top/left, store

This comment (before nsTableCellMap::SetBCBorderEdge) has various mentions of top, left, bottom, right that need logicalization.

@@ +1037,2 @@
>        }
>        else NS_ERROR("Cellmap: Top edge not found");

s/Top/BStart/

@@ +1044,1 @@
>      // since top, bottom borders were set, there should already be a cellData entry

top/bottom need logicalization in this comment.

@@ +1057,2 @@
>          }
>          else NS_ERROR("Cellmap: Left edge not found");

"left" needs logicalization

::: layout/tables/nsCellMap.h
@@ +199,5 @@
>  
>  public:
>    void ExpandZeroColSpans();
>  
> +  void ResetTopStart(mozilla::LogicalSide    aSide,

s/Top/BStart/ in the name of this function.

(You may want to hand-edit the next patch to fix this, too; there's a mention of this function in the next patch.)

::: layout/tables/nsTableFrame.cpp
@@ +4973,5 @@
>    nscolor   ownerColor;     // color of borderOwner
>    uint16_t  ownerWidth;     // pixel width of borderOwner
>    uint16_t  subWidth;       // pixel width of the largest border intersecting the border perpendicular
>                              // to ownerSide
> +  uint32_t  ownerSide:2;    // LogicalSide (e.g NS_SIDE_TOP, NS_SIDE_RIGHT, etc) of the border

Insufficient replacement here -- "LogicalSide (e.g NS_SIDE_TOP...)" makes no sense. :)

So -- convert the "e.g." text.
Attachment #8626460 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #52)
> > +  uint32_t  ownerSide:2;    // LogicalSide (e.g NS_SIDE_TOP, NS_SIDE_RIGHT, etc) of the border
> 
> Insufficient replacement here -- "LogicalSide (e.g NS_SIDE_TOP...)" makes no
> sense. :)

Sorry, looks like you do fix this up in the next patch (part 14) -- so never mind about this part.
Comment on attachment 8626463 [details] [diff] [review]
part 14 - Finish conversion of border-collapse code in nsTableFrame to logical coordinates

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

Looks good! Nits below, almost entirely about places where things are becoming inconsistently-named (& where comments need adjusting to keep up with the code they're describing).

If you'd like, I'd be OK with you landing both parts as they currently stand, & addressing my review comments in a followup in the next few days.  (If the patches here ride the trains, we should backport the followup to the same train too.)

::: layout/tables/nsCellMap.cpp
@@ +99,5 @@
>  }
>  
>  // Get the bcData holding the border segments of the right edge of the table
>  BCData*
>  nsTableCellMap::GetRightMostBorder(int32_t aRowIndex)

This function (and the comment right before it) looks like it needs renaming. ("IEndMost"?)

Same for GetBottomMostBorder() and :DeleteRightBottomBorders() [both logicalized but not renamed in this patch].

@@ +728,2 @@
>    if (nullptr != mBCInfo) {
>      printf("***** bottom borders *****\n");

s/bottom/BEnd/ (or something), here in nsTableCellMap::Dump

@@ +1069,1 @@
>  nsTableCellMap::SetBCBorderCorner(Corner      aCorner,

This method's documentation-comment (just before it, outside of the patch-context here) still has stale mentions of physical Corner values (which don't exist anymore) -- e.g. "eTopLeft", "eTopRight". Please convert those over.

@@ +1103,5 @@
>    BCCellData* cellData = nullptr;
>    BCData*     bcData   = nullptr;
>    if (GetColCount() <= xPos) {
>      NS_ASSERTION(xPos == GetColCount(), "program error");
>      // at the right edge of the table as we checked the corner before

s/right/BEnd/ in this comment.

::: layout/tables/nsTableFrame.cpp
@@ +5145,5 @@
>  
>    return changed;
>  }
>  
>  // this function will set the horizontal border. It will return true if the

This comment needs s/horizontal/inline-dir/, and s/vertical/block-dir/ on the next line.

(This is the documentation for the function you're renaming to SetInlineDirBorder in this patch.)

@@ +5478,2 @@
>  {
>    // update the left/right first cell border

Replace "left/right" with "IStart" here, at the beginning of SetTableIStartBorderWidth.
(IIUC, "left/right" here is considering LTR vs RTL, and in either case it's talking about IStart.)

@@ +5489,2 @@
>  {
>    // update the left/right first cell border

Same here (in SetTableIEndBorderWidth), though here we should say IEnd.

@@ +5975,3 @@
>      // cells and the table, row group, row
>      if (info.mNumTableRows == info.GetCellEndRowIndex() + 1) {
>        // touches bottom edge of table

s/bottom/BEnd/

@@ +6222,4 @@
>  
>    void Start(BCPaintBorderIterator& aIter,
>               BCBorderOwner          aBorderOwner,
> +             BCPixelSize            aBEndBlockSegWidth,

This variable "aBEndBlockSegWidth" needs s/SegWidth/SegISize/  (in BCInlineDirSeg's Start() function signature, and function-definition, and in the documentation for the function definition)

Alternately: if you want to call this "width" in an axis-independent sense: then, please make that a similar change to the other Start method here, on BCBlockDirSeg. (That one also has "height"/"width" flavored args in the old code, which you've renamed to BSize/ISize).

@@ +6224,5 @@
>               BCBorderOwner          aBorderOwner,
> +             BCPixelSize            aBEndBlockSegWidth,
> +             BCPixelSize            aInlineSegBSize);
> +   void GetIEndCorner(BCPaintBorderIterator& aIter,
> +                      BCPixelSize            aIStartSegWidth);

Same here in this GetIEndCorner method -- you're keeping a "Width"-flavored variable name here, whereas in GetBEndCorner(), you've renamed the old variable from "Height" to "BSize".  These methods (GetIEndCorner & GetBEndCorner) should be consistent (with each other & with the Start() methods) on naming strategy.

So: please rename the parameter in one or the other of these methods.

@@ +6355,3 @@
>                                              // respect to the table
> +  nscoord               mNextOffsetB;       // offsetB of the next segment
> +  BCBlockDirSeg*        mVerInfo; // this array is used differently when

s/mVerInfo/mBlockDirInfo/, I think? (Ver is for Vertical, and we've done s/Ver/BlockDir/ on the type name, so we should do so on this variable name as well.)

@@ +6467,5 @@
>    int32_t leftCol, rightCol; // columns are in the range [leftCol, rightCol)
>  
>    LogicalMargin childAreaOffset = mTable->GetChildAreaOffset(mTableWM, nullptr);
> +
> +  // x position of first col in damage area

s/x position/inline position/

@@ +6470,5 @@
> +
> +  // x position of first col in damage area
> +  mInitialOffsetI = childAreaOffset.IStart(mTableWM);
> +  leftCol = 0;
> +  rightCol = mNumTableCols;

The leftCol & rightCol variables should be removed entirely here; they're abstractions from when we had RTL vs LTR branches here. (You're collapsing those branches, so we don't need them anymore.)

@@ +6476,3 @@
>    nscoord x = 0;
>    int32_t colX;
> +  for (colX = leftCol; colX != rightCol; colX++) {

So "leftCol" can just be replaced with 0 and rightCol can be replaced with mNumTableCols here.

Also: consider s/colX/colIdx/

@@ +6837,3 @@
>                                                  topBevel);
>  
> +  mBStartBevelOffset = topBevel ?

(Here you're renaming mTopBevelOffset to mBStartBevelOffset)

Rename local-var "topBevel" to "bStartBevel", too.

@@ +6857,5 @@
>   * vertical segment in this column
>   * @param aIter - iterator containing the structural information
>   */
>  void
> +BCBlockDirSeg::Initialize(BCPaintBorderIterator& aIter)

The documentation before this method says "vertical" several times. ("Initialize the vertical segments ...")

Update to "block-dir".

@@ +6869,2 @@
>    }
>    // set colX for the next column

In the old code, I think this comment meant to say "mOffsetX". (There is no "colX" here.)

So, please update it to say "mOffsetI" (which we are indeed about to set for the next column).

@@ +6883,4 @@
>   *                        at the start
>   */
>  void
> +BCBlockDirSeg::GetBEndCorner(BCPaintBorderIterator& aIter,

This method's documentation still says "vertical" and "horizontal"; those need updating.

@@ +6908,4 @@
>   *                        at the start
>   */
>  void
> +BCBlockDirSeg::Paint(BCPaintBorderIterator& aIter,

Same here. (This method's documentation still says "vertical" and "horizontal"; those need updating.)

@@ +7040,5 @@
>    nscoord cornerSubWidth  = (aIter.mBCData) ?
>                               aIter.mBCData->GetCorner(cornerOwnerSide,
>                                                         bevel) : 0;
>  
> +  bool    leftBevel = (aInlineSegBSize > 0) ? bevel : false;

s/leftBevel/iStartBevel/, I think? (similar to "topBevel" mentioned above)

@@ +7065,4 @@
>   *                        at the start
>   */
>  void
> +BCInlineDirSeg::GetIEndCorner(BCPaintBorderIterator& aIter,

This method's documentation still says "vertical" and "horizontal"; those need updating.

@@ +7170,1 @@
>    if (aIter.mTableWM.IsBidiLTR()) {

I assume this LTR check can be collapsed here -- drop the RTL special-case code, which is the entire "else" clause here.

(This is in BCHorizontalSeg::Paint. You already did this collapsing in the analogous VerticalSeg class.)

@@ +7195,5 @@
>  /**
>   * Advance the start point of a segment
>   */
>  void
> +BCInlineDirSeg::AdvanceOffsetI(int32_t aIncrement)

Drop the "aIncrement" arg here.

This is only called in one place, and that one place passes in the value "1" (now that we don't have LTR/RTL special cases). So we can just assume this is 1 and drop the arg.

@@ +7210,5 @@
>    mLength += aIter.mVerInfo[aIter.GetRelativeColIndex()].mColWidth;
>  }
>  
>  /**
>   * store the column width information while painting horizontal segment

s/horizontal/inline-dir/

@@ +7225,5 @@
>      mVerInfo[aIndex].mColWidth = col->ISize(mTableWM);
>    }
>  }
>  /**
>   * Determine if a vertical segment owns the corder

s/vertical/block-dir/, and s/corder/corner/ while you're here.

@@ +7273,5 @@
> +    mInlineSeg.Start(*this, borderOwner, iStartSegISize, bStartSegBSize);
> +  }
> +
> +  if (!IsDamageAreaIStartMost() && (isSegStart || IsDamageAreaIEndMost() ||
> +                                  BlockDirSegmentOwnsCorner())) {

Indent "BlockDirSegmentOwnsCorner" here by 2 more spaces to keep it inside its parenthesized clause (which opens on the previous line).

@@ +7283,2 @@
>        }
> +      mInlineSeg.AdvanceOffsetI(1);

(As noted above, drop the "1" here; this is the only call to AdvanceOffsetI, and it always passes 1. So AdvanceOffsetI() should just become a no-arg function, like AdvanceOffsetB is.)

@@ +7290,4 @@
>    mVerInfo[relColIndex].mLastCell = mCell;
>  }
>  /**
> + * Paint if necessary a block-dir segment, otherwise  it

While you're here, fill in the missing word -- "otherwise accumulate it". (This is AccumulateOrPaintBlockDirSegment).

@@ +7307,4 @@
>      mBCData ? mBCData->GetBStartEdge(ignoreBorderOwner, ignoreSegStart) : 0;
>  
>    int32_t relColIndex = GetRelativeColIndex();
> +  BCBlockDirSeg& verSeg = mVerInfo[relColIndex];

s/verSeg/blockDirSeg/, I think?

@@ +7318,1 @@
>                                   IsAfterRepeatedHeader() ||

Please reindent this line and the next (in AccumulateOrPaintBlockDirSegment), to keep these lines inside their parenthesized expression.

::: layout/tables/nsTableFrame.h
@@ +299,5 @@
>     */
>    nsMargin GetDeflationForBackground(nsPresContext* aPresContext) const;
>  
>    /** Get width of table + colgroup + col collapse: elements that
>     *  continue along the length of the whole left side.

s/left side/IStart side/.

(This is documentation for the renamed-in-this-patch "GetContinuousIStartBCBorderWidth")

::: layout/tables/nsTablePainter.cpp
@@ +321,5 @@
>      LogicalMargin border(wm);
>      /* BC left borders aren't stored on cols, but the previous column's
>         right border is the next one's left border.*/
>      //Start with table's left border.
> +    nscoord lastLeftBorder = aTableFrame->GetContinuousIStartBCBorderWidth();

"left" in "lastLeftBorder" needs logicalizing, I think?

(Could be done in a followup about TableBackgroundPainter; but this patch here is where you're dropping "left" from the thing that's being assigned into this variable, so it also makes sense to do it here...)
Attachment #8626463 - Flags: review?(dholbert) → review+
Attachment #8626815 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #54)

> @@ +7170,1 @@
> >    if (aIter.mTableWM.IsBidiLTR()) {
> 
> I assume this LTR check can be collapsed here -- drop the RTL special-case
> code, which is the entire "else" clause here.
> 
> (This is in BCHorizontalSeg::Paint. You already did this collapsing in the
> analogous VerticalSeg class.)

Unfortunately, we can't do this yet; I initially did so, but it results in bevels being applied to the wrong ends of horizontal segments when RTL direction is in effect. In simple cases (where the two ends are symmetrical) this is invisible; but when a segment is only bevelled at one end, it becomes an ugly glitch. AFAICS, we need to keep the start/end swapping for RTL here until nsCSSRendering::DrawTableBorderSegment is also converted to work with logical coordinates.

Aside from this, I think I've addressed all the nits (and fixed a couple more names that caught my eye) in a followup patch that I'll push together with parts 13 and 14 here.
FTR, here's the review-nits followup I plan to include with the push.
Keywords: leave-open
Depends on: 1180166
You need to log in before you can comment on or make changes to this bug.