229883 - reorganize CalcDominantBorder

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Bernd]

this function takes 14 arguments, the meaning of some of these arguments are
only obvious to chris karnaze, for mere mortals this thing needs 1.
documentation 2. a reduced number of arguments. Some of these arguments are
booleans indicating that this function is in reality two functions.

Comment 1

[Bernd]

Attachment: reorg

Comment 2

[Bernd]

Comment on attachment 138299 [details] [diff] [review]
reorg

Boris, I am open for any further suggestions to get a readable code. The
primary idea in this patch is by integrating the owner variable into the
BCCellBorder  the cellborder has all the info necessary to compute the dominant
border

Comment 3

[fantasai]

If 'index' is only used for "y indexes", then perhaps you should call it 'yIndex'.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 138299 [details] [diff] [review]
reorg

>Index: nsTableFrame.cpp
>diff -u -r3.543 nsTableFrame.cpp

Please use more context next time!  -u6 is about the smallest context amount
that's really readable...  -u8 is better.  Also, using the -p option goes a
long way to making the diff more readable...

>+struct BCCellBorder

This struct could use a nice paragraph of documentation explaining what it
represents, exactly.


>+  void Reset(PRUint32 aIndex = 0, PRUint32 aSpan = 1);

I'd avoid default values here unless they are really, really needed.... 
Default values are just asking to be ignored by callers (and hence passed
incorrectly).

>+  nscolor        color;

Comment that this is the color of the border

>+  PRUint16       index;   // y index, not used for vertical borders

What does that mean?   What does this member do?  Document it, please (and
maybe call it yIndex as fantasai suggested).

>+  PRUint16       span;    // row span

What does that mean?  Imo the documentation of a class should be readable
without knowing how the class is used by other code.  In fact, said
documentation should explain how it's used.

>+  PRUint16       width;

Of the border, right?  In what units?  (px, I presume?	Document that).

>+  PRUint8        style;

Comment with a pointer to where valid values are defined, please?

>+  BCBorderOwner  owner;

What does that member mean/do?

>+BCCellBorder::Reset(PRUint32 aIndex,
>+  index = (PRUint16)aIndex;
>+  span  = (PRUint16)aSpan;

I'm not that happy with those casts... is there a reason not to make the args
PRUint16?  I know you just copied this code, but still...  If you want to spin
off a separate bug on the BCCellBorder non-comment issues, go for it.

>+// Compare two border segments, this comparison depends whether the two
>+// segments meet at a corner and if the second segment is horzontal.

"whether the second segment...."

So if both are horizontal, we would pass PR_FALSE for aIsCorner and PR_TRUE for
aSecondIsHorizontal?  If both are vertical we pass PR_FALSE for both?  If one
is vertical and one horizontal, we pass PR_TRUE for aIsCorner and the second
bool determines which is horizontal?

>+static BCCellBorder
> CalcDominantBorder(PRBool          aIsCorner,
>+                   BCCellBorder    aBorder1,
>+                   BCCellBorder    aBorder2,
>+                   PRBool          aSecondIsHorizontal,
>+                   PRBool*         aFirstDominates = nsnull)

Why pass by value here?  I'd do:

static 
const BCCellBorder & CalcDominantBorder(PRBool		    aIsCorner,
					const BCCellBorder& aBorder1,
					const BCCellBorder& aBorder2,
					PRBool		   
aSecondIsHorizontal,
					PRBool* 	    aFirstDominates =
nsnull)

or something along those lines...

> // calc the dominate border by considering the table, row/col group, row/col, cell, 

"dominant"

>+// at the table edges the rules should be ignored as they are only inner borders

Is this part of the same sentence?  Should there be a period after "cell"
above?

>+// depending wether the side is vertical or horizontal and the adjacent frames are
>+// considered the ownership of an single border segment is defined.

"whether".  "a single border".	I'm not sure what this comment is trying to
say, really....  Are you saying that the ownership of a border segment is
determined based on which frame the border that "wins" belonged to?

>+static BCCellBorder
> CalcDominantBorder(const nsIFrame*  aTableFrame,

>   // start with the table as dominate if present

"dominant"

>     GetStyleInfo(*aTableFrame, aSide, style, color, aIgnoreIfRules, &width, &aTwipsToPixels);

Why not pass the fields of the struct directly to this instead of copying into
the struct later?

>   // see if the col row group is dominate

"dominant"

>   if (aColGroupFrame) {
>     GetStyleInfo(*aColGroupFrame, aSide, style, color, aIgnoreIfRules, &width, &aTwipsToPixels);
>     if ((NS_STYLE_BORDER_STYLE_HIDDEN == style) ||
>+        (width > border.width) ||
>+        ((width == border.width) &&
>+         (styleToPriority[style] >= styleToPriority[border.style]))) {
>+      border.style = style;
>+      border.width = width;
>+      border.color = color;
>+      border.owner = (aAja && !horizontal) ? eAjaColGroupOwner : eColGroupOwner;
>       if (NS_STYLE_BORDER_STYLE_HIDDEN == style) {
>-        return;
>+        return border;

Wouldn't it be easier to declare a second struct, GetStyleInfo into that, set
its owner, use the already existing function to compare two structs to decide
which is dominant, then return if the dominant style is HIDDEN?  It seems like
we're duplicating the priority-calculating logic in a lot of places....

>   if (aColFrame) {
>     GetStyleInfo(*aColFrame, aSide, style, color, aIgnoreIfRules, &width, &aTwipsToPixels);

Same.

>   if (aRowGroupFrame) {
>     GetStyleInfo(*aRowGroupFrame, aSide, style, color, aIgnoreIfRules, &width, &aTwipsToPixels);

Same.

>   if (aRowFrame) {
>     GetStyleInfo(*aRowFrame, aSide, style, color, aIgnoreIfRules, &width, &aTwipsToPixels);

Same.

>   if (aCellFrame) {
>     GetStyleInfo(*aCellFrame, aSide, style, color, aIgnoreIfRules, &width, &aTwipsToPixels);

Same.

> BCCornerInfo::Update(PRUint8       aSide,

I'd make both Set and Update take const references, not copies, of
BCCellBorder.

>+    oldBorder.owner  = (BCBorderOwner) ownerElem;
>+    oldBorder.style =  ownerStyle;
>+    oldBorder.width =  ownerWidth;
>+    oldBorder.color =  ownerColor;

Looking at this, BCCornerInfo and BCCellBorder are crying out to inherit from a
single struct that holds those 4 items of data... separate patch, if we decide
to do that.

In any case, this is looking like we want a constructor that takes those 4 args
on BCCellBorder.

>+        BCCellBorder subBorder;
>+        subBorder.owner  = (BCBorderOwner) ownerElem;
>+        subBorder.style =  ownerStyle;
>+        subBorder.width =  ownerWidth;

Comment that you're purposefully leaving color at 0?

> static PRBool
>+SetBorder(BCCellBorder   aNewBorder,
>           BCCellBorder&  aBorder)

Take a const ref for the first one?

>+  PRBool changed = (aNewBorder.style != aBorder.style) || (aNewBorder.width != aBorder.width) || 

Why not use BorderAppearanceIsDifferent() here?

This didn't use to copy the owner... is that desired behavior?

>+static PRBool BorderAppearanceIsDifferent(BCCellBorder border1,
>+                                          BCCellBorder border2)

Again, take const refs.

> static PRBool
>+SetHorBorder(BCCellBorder        aNewBorder,

Const ref.

> nsTableFrame::CalcBCBorders(nsIPresContext& aPresContext)
>+        currentBorder = CalcDominantBorder(this, info.cg, info.leftCol, info.rg, rowFrame, info.cell, 
>+                                          TABLE_EDGE, NS_SIDE_LEFT, !ADJACENT, t2p);

Weird indent.

>@@ -5942,21 +5930,21 @@
>-                                          colX, brCorner.ownerSide, brCorner.subWidth, brCorner.bevel, PR_TRUE);  
>+                                          colX, brCorner.ownerSide, brCorner.subWidth, brCorner.bevel, PR_FALSE);  

Why this change?

The rest looks fine, but please make the above changes (and answer that last
question).

Comment 5

[Bernd]

Attachment: revised patch untested yet

Comment 6

[Bernd]

Attachment: next revision

Comment 7

[fantasai]

 +// return the border style, border color for a given frame and side
  GetStyleInfo

Rename to GetColorAndStyle or somesuch; there are too many functions
with the same name in this corner of layout.

Rename NS_STYLE_BORDER_STYLE_RULES_MASK to NS_STYLE_BORDER_STYLE_RULES_MARKER

If aIgnoreIfRules means "this is a table edge and therefore we should ignore any
borders set for the purpose of satisfying the rules attribute", then let's make
it a little clearer.

+  PRBool transparent, foreground;
+  styleData->GetBorderColor(aSide, aColor, transparent, foreground);
+  if (foreground) {
+    aColor = aFrame.GetStyleColor()->mColor;
+  }

It looks like we're losing any transparentness here. You told me it's not
supported in the code at all, so we need a comment.

+BCCellBorder::Reset(PRUint32 aRowIndex,
+                    PRUint32 aRowSpan)
+{
+  style = color = width = 0;

Set style to NS_STYLE_BORDER_STYLE_NONE instead of 0.

+// Compare two border segments, this comparison depends whether the two
+// segments meet at a corner and whether the second segment is horizontal.

In which cases and why do we care whether the second segment is horizontal?

> CalcDominantBorder

Rename to CompareBorders

+// cell. At the table edges the rules should be ignored as they are only inner
+// borders depending wether the side is vertical or horizontal and the adjacent
+// frames are considered the ownership of an single border segment is defined.

I don't understand this sentence.

+  border.style = NS_STYLE_BORDER_STYLE_NONE;
+  border.width = 0;
+  border.color = 0;
+  border.owner = eTableOwner;

This is already done in the constructor.

   if (aTableFrame) {
+    GetStyleInfo(*aTableFrame, aSide, border.style, border.color,
aIgnoreIfRules, border.width, aTwipsToPixels);

If the table frame is present, then aIgnoreIfRules is true, otherwise its
false, right? If so, can we drop the aIgnoreIfRules parameter from this
function and just pass in PR_TRUE and PR_FALSE?

+    oldBorder.owner  = (BCBorderOwner) ownerElem;

If ownerElem is a BCBorderOwner, shouldn't it be declared as such?
(You should delete that extra space, too.)

+    ownerElem  = tempBorder.owner;
+    ownerStyle = tempBorder.style;
+    ownerWidth = tempBorder.width;
+    ownerColor = tempBorder.color;
+    if (existingWins) { // existing corner is dominant

Shouldn't we only be making these assignments if the existing border
does /not/ win?

         // see if the new sub info replaces the old
+        subBorder.owner  = (BCBorderOwner) ownerElem;
+        subBorder.style =  ownerStyle;
+        subBorder.width =  ownerWidth;

Shouldn't you be assigning subStyle etc. instead of ownerStyle etc.?

+        tempBorder = CalcDominantBorder(CELL_CORNER, subBorder, aBorder,
horizontal, &firstWins);
+        subElem  = tempBorder.owner;
+        subStyle = tempBorder.style;
+        subWidth = tempBorder.width;
         if (firstWins) {
           subSide = aSide;
         }
Is this saying that, if the existing border wins (i.e. not the argument),
we switch the side to to the argument side??

Also, you should only make the assignment if something's changed.

+static PRBool BorderAppearanceIsDifferent(const BCCellBorder& border1,

Is there a reason to make this a separate function?
*thinks it should be inlined*

+SetBorder(const BCCellBorder&   aNewBorder,
           BCCellBorder&  aBorder)

Make those arguments line up.

+
 static PRBool
-SetHorBorder

Is this newline intentional?

-          tableCellMap->SetBCBorderCorner(eTopLeft, *info.cellMap, 0, 0, colX, 
-                                          tlCorner.ownerSide,
tlCorner.subWidth, tlCorner.bevel);
+          tlCorner.Update(NS_SIDE_RIGHT, currentBorder);
+          tableCellMap->SetBCBorderCorner(eTopLeft, *info.cellMap, 0, 0, colX,
+                                           tlCorner.ownerSide,
tlCorner.subWidth, tlCorner.bevel);
-------------------------------------------^
The new code should line up evenly.

-      lastVerBorders[colX].Reset();
+      lastVerBorders[colX].Reset(0,1);

I think this is a place where default arguments would make more sense.

r-

Comment 8

[Bernd]

Attachment: rev6

Comment 9

[Bernd]

Comment on attachment 159701 [details] [diff] [review]
rev6

I did not follow fantasai's recommendations for the CornerUpdate,as the
proposed changes will result in incorrect rendering.  There is another patch in
bug 43178 which also covers this area, it will when committed remove the rules
handling, but I need this cleanup here before proceeding with that change.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 159701 [details] [diff] [review]
rev6

>Index: html/table/src/nsTableFrame.cpp
>+// priority rules follow CSS 2.1 spec
>+// double', 'solid', 'dashed', 'dotted', 'ridge', 'outset', 'groove', and the
>+// lowest: 'inset'. none is even weaker

I'd say this more like:

  // priority rules follow CSS 2.1 spec, from highest to lowest:
  // 'hidden', 'double', 'solid', 'dashed', 'dotted', 'ridge',
  // 'outset', 'groove', 'inset', 'none'

(note the addition of 'hidden' and the addition of the starting quote on
'double').  I realize that 'hidden' is special-cased in the code later, but
saying it this way makes it clear why 'hidden' has top priority in this list...

> GetColorAndStyle(const nsIFrame&  aFrame

Why not nsIFrame*?  I'd stick to passing pointers, not references, here.

>+  if ((NS_STYLE_BORDER_STYLE_NONE == aStyle) ||
>+      (NS_STYLE_BORDER_STYLE_HIDDEN == aStyle)) {
>     return;

We don't set aColor here.  I assume that's because callers won't look at it? 
Could we set aColor to black instead, just in case?

>+  styleData->GetBorderColor(aSide, aColor, transparent, foreground);
>+  if (foreground) {
>+    // XXX_Bernd we are loosing transparency here

"losing".  This is actually quite bad, because GetBorderColor won't set aColor
if transparent is true.  So it looks like the color will simply end up
uninitialized by some of the codepaths here, ending up with random color
values.  Set it to 0 for "transparent == true" and file a followup bug to fix
this?  Maybe even add an NS_WARNING for the transparent case?

> +GetPaintStyleInfo(const nsIFrame&  aFrame,

Again, nsIFrame*.

> +GetColorAndStyle(const nsIFrame&  aFrame,

And here.

>+/* BCCellBorder represents a border segment which can be either a horizontal
>+ * or a vertical segment

Period after "segment" here.

>+ * Ownership of these segment

"segments"

I'm a little curious why we have a single struct for two purposes.  The
rowIndex and rowSpan is unused in a lot of places; wouldn't it make more sense
to have two structs, one inheriting from the other?  Perhaps a followup patch?

>+BCCellBorder::Reset(PRUint32 aRowIndex,

>+  color = NS_STYLE_COLOR_TRANSPARENT;

That's not actually an nscolor value... just use 0 directly, or NS_RGB(0, 0,
0)?  (Though note that NS_RGB(0,0,0) is an opaque black while 0 is a
transparent black for nscolor... not like it matters, since we don't handle
alpha channel for nscolors anywhere, really.)

>+// Compare two border segments, this comparison depends whether the two
>+// segments meet at a corner and whether the second segment is horizontal.

Document that the return value is whichever of aBorder1 or aBorder2 dominates?

> At the table edges the rules should be ignored

"At the table edges borders coming from the 'rules' attribute should be
ignored"

> Depending wether

"Depending on whether"

> the ownership of an single border
>+// segment is defined.

"a single border segment"

>+// it will be a adjacent owner aka eAjaCellOwner.

"an adjacent owner"

>+CompareBorders(const nsIFrame*  aTableFrame,
...
>+               PRBool           aAja,

What does that argument mean?  If it's set, does that mean that the owners we
could end up with would all be adjacent owners (whenever that makes sense)?  If
so, please document that.

>+  // see if the col row group is dominant

s/col row group/col group/

>+    tempBorder.owner = (aAja && !horizontal) ? eAjaColGroupOwner : eColGroupOwner;
>+    border = CompareBorders(!CELL_CORNER, border, tempBorder, PR_FALSE);

Shouldn't the last arg to CompareBorders be |horizontal|?

>+    tempBorder.owner = (aAja && !horizontal) ? eAjaColOwner : eColOwner;
>+    border = CompareBorders(!CELL_CORNER, border, tempBorder, PR_FALSE);

Same here.

>+    tempBorder.owner = (aAja && horizontal) ? eAjaRowGroupOwner : eRowGroupOwner;
>+    border = CompareBorders(!CELL_CORNER, border, tempBorder, PR_FALSE);

And again.

>+    tempBorder.owner = (aAja && horizontal) ? eAjaRowOwner : eRowOwner;
>+    border = CompareBorders(!CELL_CORNER, border, tempBorder, PR_FALSE);

And here.

>+    tempBorder.owner = (aAja) ? eAjaCellOwner : eCellOwner;
>+    border = CompareBorders(!CELL_CORNER, border, tempBorder, PR_FALSE);

And here.

> Perpendicular(PRUint8 aSide1, 
>               PRUint8 aSide2)

I was looking at this method, and it seemed to me that something like:

  |return (aSide1 % 2) != (aSide2 % 2);|

would be faster.  Perhaps combined with a compile-time check like at
http://lxr.mozilla.org/seamonkey/source/content/html/style/src/nsCSSStruct.cpp#
326 ?  Just a random comment, not really relevant to this patch.

> BCCornerInfo::Update(PRUint8       aSide,

>         // see if the new sub info replaces the old
>-          CalcDominantBorder(PR_TRUE, (BCBorderOwner)subElem, subStyle, subWidth, color, 
>-                             (BCBorderOwner)aBorderOwner, aOwnerBStyle, aOwnerWidth, aOwnerColor, 
>-                             tempBorderOwner, tempStyle, subWidth, color, horizontal);
....
>+        BCCellBorder subBorder;
>+        subBorder.owner  = (BCBorderOwner) ownerElem;
>+        subBorder.style =  ownerStyle;
>+        subBorder.width =  ownerWidth;

Those should be subElem, subStyle, subWidth, no?

Also, we're never initing the color here.  We probably should...  I know the
existing code didn't init it either, but...

If the color really can't affect things, please comment to that effect
explaining why (I assume because color doesn't influence border-dominance
calculations?)

>         if (firstWins) {
>           subSide = aSide; 
>         }

I know you just left this, but why does this make sense?  If firstWins, then
we've just sorta summarily ignored aBorder.  Why are we changing subSide in
that case and not when the subStyle/Elem/Width come from aBorder?  That is, I
think this check needs to be reversed....

> static PRBool
>+SetBorder(const BCCellBorder&   aNewBorder,
>+          BCCellBorder&         aBorder)

Wouldn't this make more sense as a member method of BCCellBorder?  Something
like 

  PRBool SetTo(const BCCellBorder& aNewBorder);

(or call it SetToBorder?) That eliminates the question about what aBorder is in
the existing SetBorder method.	It'd also be good to document what this method
returns...

> static PRBool
>+SetHorBorder(const BCCellBorder& aNewBorder,

Again, good to document what the return value here means.

>@@ -5724,106 +5749,106 @@ nsTableFrame::CalcBCBorders(nsPresContex
>+    // find the dominant border considernig the cell's top border and the table, row group, row

s/considernig/considering/

>+        currentBorder = CompareBorders(this, cgFrame, colFrame, info.rg, info.topRow, info.cell, 
>+                                           TABLE_EDGE, NS_SIDE_TOP, !ADJACENT, t2p);

Weird indent here.  Please fix it.

>         if (colFrame) {
>+          currentBorder = CompareBorders(this, cgFrame, colFrame, 

No need for the colFrame null check, I think -- it's checked at the top of the
loop and ABORT0() called if it's null.	Or is it reset somewhere in between?

>+                                             info.topRow, nsnull, TABLE_EDGE,
>+                                             NS_SIDE_TOP, !ADJACENT, t2p);

Weird indent.

>+            currentBorder = CompareBorders(this, cgFrame, colFrame, nsnull,
>+                                               nsnull, nsnull, TABLE_EDGE,
>+                                               NS_SIDE_RIGHT, !ADJACENT, t2p);

Weird indent.


>+            currentBorder = CompareBorders(nsnull, cgFrame, colFrame, nsnull,
>+                                               nsnull, nsnull, !TABLE_EDGE,
>+                                               NS_SIDE_RIGHT, !ADJACENT, t2p);

Weird indent.

>+        currentBorder = CompareBorders(this, nsnull, nsnull, info.rg,
>+                                           info.topRow, nsnull, TABLE_EDGE,
>+                                             NS_SIDE_TOP, !ADJACENT, t2p);

Weird indent.

>+        currentBorder = CompareBorders(this, info.cg, nsnull, info.rg,
>+                                           info.topRow, nsnull, TABLE_EDGE,
>+                                           NS_SIDE_TOP, !ADJACENT, t2p);
>+        info.cg->SetContinuousBCBorderWidth(NS_SIDE_TOP, currentBorder.width);

Weird indent.

>+        currentBorder = CompareBorders(this, info.cg, info.leftCol, nsnull,
>+                                           nsnull, nsnull, TABLE_EDGE,
>+                                           NS_SIDE_LEFT, !ADJACENT, t2p);

Weird indent.

>@@ -5831,137 +5856,137 @@ nsTableFrame::CalcBCBorders(nsPresContex
>+    // find the dominant border considernig the cell's left border and the table, col group, col  

"considering"

>+        currentBorder = CompareBorders(this, info.cg, info.leftCol, info.rg, rowFrame, info.cell, 
>+                                           TABLE_EDGE, NS_SIDE_LEFT, !ADJACENT, t2p);

Weird indent.

>+          currentBorder = CompareBorders(this, info.cg, info.leftCol,
>+                                             info.rg, rowFrame, nsnull,
>+                                             TABLE_EDGE, NS_SIDE_LEFT, !ADJACENT, t2p);

Weird indent.

>+        currentBorder = CompareBorders(this, info.cg, info.leftCol, info.rg, nsnull,
>+                                           nsnull,  TABLE_EDGE, NS_SIDE_LEFT, !ADJACENT, t2p);

Weird indent.

>+    // find the dominant border considernig the cell's right border, adjacent cells and the table, row group, row

"considering"

>+        currentBorder = CompareBorders(this, info.cg, info.rightCol, info.rg, rowFrame, info.cell, 
>+                                           TABLE_EDGE, NS_SIDE_RIGHT, ADJACENT, t2p);

Weird indent.

>+          currentBorder = CompareBorders(this, info.cg, info.rightCol, info.rg,
>+                                             rowFrame, nsnull, TABLE_EDGE,
>+                                             NS_SIDE_RIGHT, ADJACENT, t2p);

Weird indent.

>+        currentBorder = CompareBorders(this, info.cg, info.rightCol, info.rg, 
>+                                           nsnull, nsnull, TABLE_EDGE,
>+                                           NS_SIDE_RIGHT, ADJACENT, t2p);

Weird indent.


>+        currentBorder = CompareBorders(nsnull, cg, info.rightCol, nsnull, nsnull, info.cell,
>+                                           !TABLE_EDGE, NS_SIDE_RIGHT, ADJACENT, t2p);

Weird indent.

>+        adjacentBorder = CompareBorders(nsnull, cg, ajaInfo.leftCol, nsnull, nsnull, ajaInfo.cell, 
>+                                            !TABLE_EDGE, NS_SIDE_LEFT, !ADJACENT, t2p);

Weird indent.

>+          currentBorder = CompareBorders(nsnull, nsnull, nsnull, rg, priorAjaInfo.bottomRow, priorAjaInfo.cell,
>+                                             !TABLE_EDGE, NS_SIDE_BOTTOM, ADJACENT, t2p);

Weird indent.

>+          adjacentBorder = CompareBorders(nsnull, nsnull, nsnull, rg, ajaInfo.topRow, ajaInfo.cell,
>+                                              !TABLE_EDGE, NS_SIDE_TOP, !ADJACENT, t2p);

Weird indent.

>+    // find the dominant border considernig the cell's bottom border, adjacent cells and the table, row group, row

"considering"

>+        currentBorder = CompareBorders(this, cgFrame, colFrame, info.rg, info.bottomRow, info.cell,
>+                                           TABLE_EDGE, NS_SIDE_BOTTOM, ADJACENT, t2p);

Weird indent.

>+          currentBorder = CompareBorders(this, cgFrame, colFrame, info.rg, info.bottomRow,
>+                             nsnull, TABLE_EDGE, NS_SIDE_BOTTOM, ADJACENT, t2p);

Weird indent.

>+        currentBorder = CompareBorders(this, nsnull, nsnull, info.rg, info.bottomRow,
>+                           nsnull, TABLE_EDGE, NS_SIDE_BOTTOM, ADJACENT, t2p);

Weird indent.

>+        currentBorder = CompareBorders(this, info.cg, nsnull, info.rg, info.bottomRow,
>+                           nsnull, TABLE_EDGE, NS_SIDE_BOTTOM, ADJACENT, t2p);

Weird indent.

>+        currentBorder = CompareBorders(nsnull, nsnull, nsnull, rg, info.bottomRow, info.cell, 
>+                                           !TABLE_EDGE, NS_SIDE_BOTTOM, ADJACENT, t2p);

Weird indent.

>+        adjacentBorder = CompareBorders(nsnull, nsnull, nsnull, rg, ajaInfo.topRow, ajaInfo.cell, 
>+                                            !TABLE_EDGE, NS_SIDE_TOP, !ADJACENT, t2p);

Weird indent.

>+        currentBorder = CompareBorders(nsnull, nsnull, nsnull, rg, info.bottomRow,
>+                           nsnull, !TABLE_EDGE, NS_SIDE_BOTTOM, ADJACENT, t2p);

Weird indent.

>+        adjacentBorder = CompareBorders(nsnull, nsnull, nsnull, rg, ajaInfo.topRow,
>+                           nsnull, !TABLE_EDGE, NS_SIDE_TOP, !ADJACENT, t2p);

Weird indent.

r+sr=bzbarsky with these changes, though if you want me to give the patch
another once-over once you change all this stuff I'd be happy to (and it should
take a lot less time the second time  ;) ).

Comment 11

[Bernd]

Attachment: next rev.

Comment 12

[Bernd]

Comment on attachment 161294 [details] [diff] [review]
next rev.

Boris could you please look again on it

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 161294 [details] [diff] [review]
next rev.

>+// cell. At the table edges borders coming from the 'rules' attribute should
>+// beignored

"be ignored"

>+CompareBorders(const nsIFrame*  aTableFrame,
>+               PRBool           aAja,

That argument still needs documenting...

>+  // see if the col row group is dominant

This comment still needs fixing.

>+SetBorder(const BCCellBorder&   aNewBorder,
>+          BCCellBorder&         aBorder)

I still think this would make more sense as a member on BCCellBorder, but it's
OK for now.

>+// this function will set the horizontal border. It will return true if the 
>+// existing segment will not be continued. Having a vetical

"vertical"

>+    // find the dominant border considernig the cell's top border and the table, 

"considering"

>         if (colFrame) {

Again, as far as I can tell this check is not needed..

>+    // find the dominant border considernig the cell's left border and the table, 

"considering"

>+    // find the dominant border considernig the cell's right border, adjacent 

"considering"

>+          currentBorder = CompareBorders(nsnull, nsnull, nsnull, rg, priorAjaInfo.bottomRow, priorAjaInfo.cell,
>+                                             !TABLE_EDGE, NS_SIDE_BOTTOM, ADJACENT, t2p);

Weird indent.

>+          adjacentBorder = CompareBorders(nsnull, nsnull, nsnull, rg, ajaInfo.topRow, ajaInfo.cell,
>+                                              !TABLE_EDGE, NS_SIDE_TOP, !ADJACENT, t2p);

Again.

>+    // find the dominant border considernig the cell's bottom border, adjacent 

"considering"

Most of these are issues from last time that just didn't get addressed...
Please do fix them before checking in.

Comment 14

[Bernd]

fix checked in