452319 - [BC] Border collapse rewrite

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Julien "_FrnchFrgg_" RIVAUD]

I've decided to try to tackle this. We'll see if that finishes well.

Here is the bug to discuss actual code and patches when the time comes; Bug 203686 can be used for preliminary discussion.

Comment 1

[Julien "_FrnchFrgg_" RIVAUD]

The rewrite will need several preliminary patches and reftests (in CSS test suite format, I promiss).

The first preliminary patch is to change the way we paint backgrounds to have it more integrated with the display list system. For the record: the idea is to remove the GenericTraversal of the cells, and modify the table painter so that it creates display items instead of directly painting. Advantages: we traverse the cells only once, and the result is more able to manage optimizations or reorders for correctness.

Then the inevitable newbie questions:

a) The table painter calls PaintBackgroundWithSC with custom arguments, and the generic DisplayBackground item uses PaintBackground, and fetches the arguments from the given frame. IIUC, the arguments probably would end up identical, but what do we want ? I can create a specific display item wich would store all the arguments (kind of a DisplayCustomBackground, which would enable to paint bgs not corresponding at all to a real frame), or I can reuse the generic display item if we decide that the important information that the table painter has is the row/col ownership, and that caching the actual backgrounds/borders is less important, or I can do something inbetween.

Another idea would be to store enough info in the display item for it to go retrieve what it needs from the table painter (which we can keep alive until the end of painting), but is it worth it ? I'd say that it's interesting at least for the offsets instead of calling multiple times ToReferenceFrame...

b) I think we still want to pass the table's BorderBackground list to the cells but I'd like advice (see the comment in nsTableFrame::GenericTraversal)

Probably more questions to come, but for now I need sleep

Comment 2

[Julien "_FrnchFrgg_" RIVAUD]

Correction: In

Another idea would be to store enough info in the display item for it to go
retrieve what it needs from the table painter (which we can keep alive until
the end of painting), but is it worth it ? I'd say that it's interesting at
least for the offsets instead of calling multiple times ToReferenceFrame...

The last sentence should be at the end of the previous paragraph, I'm thinking it's probably worth it to store the offsets in the display lists.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

a) tough call. I would say, anything you can easily get from the frame or style should not be in the display item, everything else should be. We shouldn't need to refer back to the table painter during painting, that would be hard to manage.

b) yes.

Comment 4

[Bernd]

Attachment: refactoring #1

This is pure refactoring of code, no bug fix, just to get it readable.

I followed the guidelines from Fowlers refactoring book (http://martinfowler.com/books.html#refactoring).

Comment 5

[fantasai]

Looks pretty good from a quick scan, Bernd. Let me know if you want a review.

Comment 6

[Bernd]

Comment on attachment 357829 [details] [diff] [review]
refactoring #1

good idea, otherwise we might end with one big patch which is not very reviewable

Comment 7

[Bernd]

Attachment: variable renaming in BCMapBorderIterator

This renames all member variables of the BCMapBorderIterator class.

Further it clarifies what istopmost  etc. is doing. With this renaming it is obvious that the code is BCMapBorderIterator::SetNewData blatantly wrong.

This part proves:
1. why careful review is a good thing
2. why clear function naming is a good thing
3. the Fowler book exactly predicted that one will simply find wrong code once it is properly labeled.

This fixes bug 380200, bug 331631 and bug 267418

Comment 8

[fantasai]

Comment on attachment 357829 [details] [diff] [review]
refactoring #1

First set of comments (didn't get through it all)

+ * Ownership of these segments is  important to calculate which corners should

Double space "is  important".

   BCMapCellInfo();
+  void Initialize(nsTableFrame* aTableFrame);

Is there a reason why Initialize isn't folded into the constructor?

+  PRUint8               mFirstSide;
+  PRUint8               mSecondSide;

Call these mStartSide and mEndSide to be consistent with CSS terminology.
Shift these declarations to after mTableIsLTR.

+  PRInt32               mNumRows;
+  PRInt32               mNumCols;

Place these declarations (and the above LTR/RTL switches) together with the
mTableFrame declaration. They are all global to the table. Also mCellData
should shift down next to mCell. You should have basically two sections to
the declarations for this struct, one for table-global members, one for
context-specific members.

+  PRPackedBool          mRgTop;
+  PRPackedBool          mRgBottom;
+  PRPackedBool          mCgLeft;
+  PRPackedBool          mCgRight;

These should be named like mIsRgTop or mRgIsTop.

void BCMapCellInfo::Reset()

This function doesn't reset the mCurrent* members. Is that intended? If
so, please document why.

+  mCellData = nsnull;
+  mRowGroup = nsnull;
+  mTopRow    = nsnull;
+  mBottomRow = nsnull;
+  mColGroup  = nsnull;
+  mLeftCol   = nsnull;
+  mRightCol  = nsnull;
+  mCell      = nsnull;


You can shorten this by stringing the assignments together like the ones
below. If you'd rather not, at least line up the = signs. :)

-  BCMapCellIterator(nsTableFrame& aTableFrame,
+  BCMapCellIterator(nsTableFrame* aTableFrame,
                   const nsRect& aDamageArea);

While you're at it, fix the indentation here.

+  aCellInfo.mCell      = nsnull;
+  aCellInfo.mRowSpan   = 1;
+  aCellInfo.mColSpan  = 1;

And here.

+  PRInt32               mCellEndRowIndex;
+  PRInt32               mCellEndColIndex;
...
+  aCellInfo.mCellEndRowIndex = aCellInfo.mRowIndex + aCellInfo.mRowSpan - 1;
+  aCellInfo.mCellEndColIndex = aCellInfo.mColIndex + aCellInfo.mColSpan - 1;

This should be done with a function GetEndRowIndex() rather than with
a data variable. We have the data to compute it already. It's a simple
arithmetic operation. It's not worth complicating the setter code and
risking the data becoming out of sync to store it in a separate variable.

+BCMapCellInfo::GetTopEdgeBorder(PRInt32 aColX)
+{
+  BCCellBorder currentBorder;
+  mCurrentColFrame = mTableFrame->GetColFrame(aColX); 
+  if (!mCurrentColFrame) {
+    NS_ERROR("null mCurrentColFrame");
+    return currentBorder;
+  }
+  mCurrentColGroupFrame = mCurrentColFrame->GetParent(); 
+  if (!mCurrentColGroupFrame) {
+    NS_ERROR("null mCurrentColFrame");
+    return currentBorder;
+  }
+  return CompareBorders(mTableFrame, mCurrentColGroupFrame, mCurrentColFrame,
+                        mRowGroup, mTopRow, mCell, mTableIsLTR, TABLE_EDGE,
+                        NS_SIDE_TOP, !ADJACENT);
+}

A function called GetTopEdgeBorder shouldn't be having a side effect
like setting mCurrentColFrame and mCurrentColGroupFrame. Unless it's
doing some kind of caching, a function starting with Get should be
effectively const. What you probably want to do here is have two
functions, one to set mCurrentColFrame and mCurrentColGroupFrame
based on aColX and one to GetTopEdgeBorder once those are set correctly.

+  BCPropertyData* propData = 
+   (BCPropertyData*)nsTableFrame::GetProperty(mTableFrame, nsGkAtoms::tableBCProperty, PR_FALSE);  

You shouldn't be getting the propData for each function call here. Get
it once and store it. And fail properly if it gets returned as null.

+BCMapCellInfo::SetTableTopContBCBorder()
+{
...
+  if (0 == mColIndex) {
+    currentBorder = CompareBorders(mTableFrame, mColGroup, mLeftCol, nsnull,
+                                   nsnull, nsnull, mTableIsLTR, TABLE_EDGE,
+                                   NS_SIDE_LEFT, !ADJACENT);
+    mTableFrame->SetContinuousLeftBCBorderWidth(currentBorder.width);

Don't set a Left border inside a Set..Top..Border function.

Comment 9

[Bernd]

Comment on attachment 358217 [details] [diff] [review]
variable renaming in BCMapBorderIterator

I'am close to completing the paint refactoring, there will be bigger patch, ;-)

Comment 10

[Bernd]

Attachment: refactoring of the paint method

Comment 11

[Julien "_FrnchFrgg_" RIVAUD]

Bernd, I have three questions:

a) How does this work relates to the refactoring ? For now you seem to be just overhauling the current approach; do you plan to go further or do you consider it unnecessary if the current approach is corrected ? I was (and will still be during next two or three weeks) very busy so I didn't make any progress, but I'd like to know what my project will become, etc...

b) Do you want to take this bug ? I'm the assignee and you're the one doing the work...

c) You are beginning to take the shape of a mentor in the table painting code, would you mentor me (depending of the answers to the previous questions) :)

Comment 12

[Bernd]

How does this work relates to the refactoring?

Its just refactoring as described by Fowler. It tries to reorganize the code in a way that it becomes readable for one person (me). That is one person more than before.

From the refactoring it becomes obvious that the painting code is heavily dependent on the mLeftStart and mTopStart entries in the cellmap 
http://mxr.mozilla.org/mozilla-central/source/layout/tables/celldata.h#256

As I wrote in 2004 as soon as rowspans are involved there are problems
 http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableFrame.cpp&rev=3.725&mark=4669#4655

My current idea is to split the code from the CalcBCBorders that sets those flags into a seperate function. The first patch is  1/3 of the way. 

Further for this the BCMapBorderIterator is good vehicle to walk over every corner.

But before it can be used, one needs to make it readable, this is patch #2.

Yeah and while I was at it I reduced the painting function from a 400 line gorilla to a 20 line thing. The other code did not vanish it is just split into smaller pieces.

b) no
c) I am not a mentor. I am just the house keeper till roc finally takes some money to employ a full time layout dev for tables (aka THE wizzard), which would be the first one since 2002 (I am not lobbying for myself here, I like to keep it a hobby level.)

One of my ideas is that once this is done, converting the paint code to display lists should be a nobrainer.

Comment 13

[Julien "_FrnchFrgg_" RIVAUD]

By refactoring I meant the project to better integrate the table code into the newer more efficient layout system (display lists especially, which are able to manage probably better what the TablePainter and the DamageArea are managing), with probably the removal of what I see as premature optimisation (all this merging of border chunks together); moreover even if this optimisation was needed at the time, there is no proof that with the advent of cairo & thebes the bottlenecks are at the same place.

So the question was: are you planning to tackle that, or do you think it isn't necessary and/or damaging ? Since you say "once this is done, converting the paint code to display lists should be a nobrainer", I guess the question is really "how far is going to go your refactoring" ?

I don't think that having a big display item responsible for the painting of every borders at once can really be called "converting the paint code to display lists"; it's more or less what we have now.

And last but not least, why people think that housekeepers would make bad mentors :) ? Being my mentor wouldn't mean leaving the hobby level, just helping me when I cannot find a broom.

Comment 14

[Bernd]

I do not target the Tablepainter & DamageArea, as I do not know what to do for it. I do target bug 195299 and its siblings.

I believe that your analysis of the merging borders is more due to the fact that you can't understand the code, which the refactoring is partly about.

I believe that one can already today generate the required display list items based on the information that we store in the cellmap.

If you can come up with something that creates the display list items based on what you get from the cellmap today, cool.

Comment 15

[Bernd]

Based on the severity of the issues I propose a slightly different schedule:

0. leave the background painter as it is now (its close to bug free)
1. checkin in the reftests that I have
2. write more reftests
3. convert the BC painting into a display list user for table cells
3.1 create a technical drawing for the involved borders and corners that shows 
    all variables and there meaning.
3.1.1 Make a drawing (svg) for the rowspan=1 colspan=1 case
3.1.2 make a drawing (svg) that demonstrates the iteration for the rowspan="2" 
      colspan="3" case.
3.1.3 create reftests that demonstrate the use of those variables
3.2 use for painting the current information that we store in the cellmap
3.3 make it pass reftests
3.4 revisit which variables are really necessary in the cellmap
3.5 change the CalcBC code to store this information
3.6 change the paint code to use this
3.7 make it pass reftests

if this gets executed we would get rid of
- the scrolling problems,
- the problems with visibility: collapse
- and maybe a couple of the intermittent paint problems

If this a proposal that sounds reasonable:
I would like to distribute the work as following:

1,2 bernd
3.1.1, 3.1.2 Julien
3.1.3 bernd
3.2, 3.3 Julien
3.4 Julien, bz, roc, bernd
3.5 bernd
3.6, 3.7 Julien

assumption: the information stored in the cellmap is correct, if this does not hold I will fix it.

4. Revisit the background painting issue.

Comment 16

[Bernd]

Attachment: this is th revised patch addressing the comments for attachment #1

> Double space "is  important".
fixed
> Is there a reason why Initialize isn't folded into the constructor?
nope
>Call these mStartSide and mEndSide to be consistent with CSS terminology.
>Shift these declarations to after mTableIsLTR.
done

>Place these declarations (and the above LTR/RTL switches) together with the
>mTableFrame declaration.
done

>This function doesn't reset the mCurrent* members. Is that intended? If
>so, please document why.
function renamed to make tis clear

>You can shorten this by stringing the assignments together like the ones
>below. If you'd rather not, at least line up the = signs. :)
no you can't or you will need to cast the pointers, so alignment works better

>This should be done with a function GetEndRowIndex()
done

>A function called GetTopEdgeBorder shouldn't be having a side effect
>like setting mCurrentColFrame and mCurrentColGroupFrame. 
fixed

>You shouldn't be getting the propData for each function call here. Get
>it once and store it.
done

>Don't set a Left border inside a Set..Top..Border function.
function renamed.

Comment 17

[Bernd]

Attachment: border iterator refactoring

Comment 18

[Bernd]

Attachment: paint patch revised

fantasai: shall I merge the border iterator refactoring and the paint patch into one patch if not the paint applies on top of the iterator refactoring.

Comment 19

[Bernd]

Comment on attachment 366399 [details] [diff] [review]
paint patch revised

forget the last comment

Comment 20

[fantasai]

  *  BCMapCellIterator

Put an empty line between each series of functions, e.g. between the Set*BCBorder() functions and the Set*Widths() functions, and between the Set*() functions and the Get*() functions.

A quick summary of why BCMapCellInfo exists and an overview of how it works would be useful to have at the top here. Also, there should be some comments in the methods and data members sections. You don't need to comment every function and every variable, but you can comment on groups of related functions and variables. For example, the Set* functions: they take no arguments, so where do they get their data and where do they set it? The mLeftCol/mRightCol data members, what do they represent and how are they related to mCurrentColFrame?

Ideally, I shouldn't have to look at BCCellMapInfo's users to figure out how to use it myself.

+  void ResetCellReleatedInformation();

Releated is misspelled. But you should probably just remove it and call the method ResetCellInfo(). It's much shorter and still gets the point across. But do document why it doesn't reset mCurrent*, because that's still not clear.

+  void Initialize(nsTableFrame* aTableFrame);

Initialize() appears to be called only once, from the BCMapCellInfo constructor. Paste its contents into BCMapCellInfo's constructor and delete this method.

+  BCPropertyData*       mPropData;

This should probably be mTableBCData, because we don't particularly care how it's stored on the table here, but we do care that it's a BC-related data structure that's global to the table.

+  nsIFrame*             mCurrentColGroupFrame;

Why is this an nsIFrame* if we know for sure it's a ColGroupFrame?

+BCMapCellInfo::SetTableTopLeftContBCBorder()

Why is the implementation of BCMapCellInfo split with tons of unrelated code in between?

+BCMapCellInfo::SetBotttomBorderWidths(nscoord aWidth)

Botttom is spelled wrong.

+void 
+BCMapCellInfo::SetRowInfo(PRInt32 aRowX)
+{
+  mCurrentRowFrame = (aRowX == mRowIndex) ? mTopRow : mCurrentRowFrame->GetNextRow();
+}

This function's name implies that you can set it to any arbitrary row index, but the way it's written you can't, you can only set it to the current row index or the next row's index. Therefore you should assert that the given row index is one of the two values that we can compute here, and document this limitation in the function declaration. Also, implement the function so that if it's passed a wrong row index, mCurrentRowFrame doesn't change.

Also I'd remove the "Info" from SetColumnInfo and SetRowInfo. We're not really setting any info here, just changing mCurrent*.

BTW, shouldn't it be aRowY? Since rows progress in the y direction...

+  BCMapCellIterator(nsTableFrame* aTableFrame, const nsRect& aDamageArea);

This should be split into two lines at the comma and the arguments lined up appropriately.

+void 
+BCMapCellInfo::SetTopBorderWidths(nscoord aWidth)
+{
+  SetInnerTopBorderWidths(aWidth);
+  mPropData->mTopBorderWidth = LimitBorderWidth(PR_MAX(mPropData->mTopBorderWidth, (PRUint8) aWidth));
+}

I'm having some trouble understanding what "Inner" means here. If I ignore the names here and just look at what the functions are doing, it seems to me it would make more sense to split these into two independent functions, renaming SetInnerTopBorderWidths() to SetTopBorderWidths() and making the second line of this function into a separate function called SetTableTopBorderWidth(). Likewise for the other sides.

Does that make sense, or am I not understanding this code correctly?

+BCMapCellInfo::SetLeftBorderWidths(PRBool aIsUpperMost, nscoord aWidth)

aIsUpperMost is confusing. The zero check, the comment, and the code should be placed together so that it makes sense; dividing them up with a function call makes the purpose less clear.

Also, I think this function combines too much, and should be broken up. The extra call to SetRowLeftContBCBorder() should be directly inside the main border collapse code, not hidden inside a BCMapCellInfo function. I think you're factoring too much out here, we're losing track of the logic.

+BCMapCellInfo::GetRightInnerBorder()

I suggest renaming "Inner" to "Internal" so as not to confuse it with "inner half" of the border or somesuch.

There's a bunch of places where the name changes lead to misaligned indentation, and you have trailing spaces in various places. I'll point them out if they're still there in the next revision. My editor highlights the trailing spaces at least; so if yours doesn't do that, don't worry about it.

Comment 21

[Bernd]

Attachment: revised patch

>  *  BCMapCellIterator
Comment above the structure changed to reflect the structure that is
following

> Put an empty line between each series of functions, e.g. between the
> Set*BCBorder() functions and the Set*Widths() functions, and between the Set*()
> functions and the Get*() functions.
> 
Done

> A quick summary of why BCMapCellInfo exists and an overview of how it works
> would be useful to have at the top here.
done
> Also, there should be some comments in
> the methods and data members sections. You don't need to comment every function
> and every variable, but you can comment on groups of related functions and
> variables. For example, the Set* functions: they take no arguments, so where do
> they get their data and where do they set it?
done

> The mLeftCol/mRightCol data members, what do they represent and how are 
>they related to mCurrentColFrame?
done

> Ideally, I shouldn't have to look at BCCellMapInfo's users to figure out how to
> use it myself.
There is no other user than CalcBCBorder, the structure helps just to
shield the complexity that the code got when the ContinousBorders have
been introduced. I should have insisted when it got checked in
initially ;-).

> +  void ResetCellReleatedInformation();
> 
> Releated is misspelled. But you should probably just remove it and call the
> method ResetCellInfo(). It's much shorter and still gets the point across. 
done

> But
> do document why it doesn't reset mCurrent*, because that's still not clear.
Brilliant comment, in principle it should reset what setinfo sets, but
setinfo was on the wrong structure, I will spin this of into a seperate patch


> +  void Initialize(nsTableFrame* aTableFrame);
> 
> Initialize() appears to be called only once, from the BCMapCellInfo
> constructor. Paste its contents into BCMapCellInfo's constructor and delete
> this method.
done
> +  BCPropertyData*       mPropData;
> 
> This should probably be mTableBCData, because we don't particularly care how
> it's stored on the table here, but we do care that it's a BC-related data
> structure that's global to the table.
done

> +  nsIFrame*             mCurrentColGroupFrame;
> 
> Why is this an nsIFrame* if we know for sure it's a ColGroupFrame?
changed

> +BCMapCellInfo::SetTableTopLeftContBCBorder()
> 
> Why is the implementation of BCMapCellInfo split with tons of unrelated code in
> between?

I did not want to pollute nsTableFrame.h, so I wrote functions that used other structures when they have been declared. 

> +BCMapCellInfo::SetBotttomBorderWidths(nscoord aWidth)
> 
> Botttom is spelled wrong.
fixed

> +void 
> +BCMapCellInfo::SetRowInfo(PRInt32 aRowX)
> +{
> +  mCurrentRowFrame = (aRowX == mRowIndex) ? mTopRow :
> mCurrentRowFrame->GetNextRow();
> +}
> 
> This function's name implies that you can set it to any arbitrary row index,
> but the way it's written you can't, you can only set it to the current row
> index or the next row's index. Therefore you should assert that the given row
> index is one of the two values that we can compute here, and document this
> limitation in the function declaration. 
Assertion added
> Also, implement the function so that if
> it's passed a wrong row index, mCurrentRowFrame doesn't change.

I don't see the point of this, what ever we do is wrong

> Also I'd remove the "Info" from SetColumnInfo and SetRowInfo. We're not really
> setting any info here, just changing mCurrent*.
done

 
> BTW, shouldn't it be aRowY? Since rows progress in the y direction...
done 
> +  BCMapCellIterator(nsTableFrame* aTableFrame, const nsRect& aDamageArea);
> 
> This should be split into two lines at the comma and the arguments lined up
> appropriately.
done
> +void 
> +BCMapCellInfo::SetTopBorderWidths(nscoord aWidth)
> +{
> +  SetInnerTopBorderWidths(aWidth);
> +  mPropData->mTopBorderWidth =
> LimitBorderWidth(PR_MAX(mPropData->mTopBorderWidth, (PRUint8) aWidth));
> +}
> 
> I'm having some trouble understanding what "Inner" means here. If I ignore the
> names here and just look at what the functions are doing, it seems to me it
> would make more sense to split these into two independent functions, renaming
> SetInnerTopBorderWidths() to SetTopBorderWidths() and making the second line of
> this function into a separate function called SetTableTopBorderWidth().
> Likewise for the other sides.
> 
renamed
> Does that make sense, or am I not understanding this code correctly?
yes

> +BCMapCellInfo::SetLeftBorderWidths(PRBool aIsUpperMost, nscoord aWidth)
> 
> aIsUpperMost is confusing. The zero check, the comment, and the code should be
> placed together so that it makes sense; dividing them up with a function call
> makes the purpose less clear.
fixed 
> Also, I think this function combines too much, and should be broken up. The
> extra call to SetRowLeftContBCBorder() should be directly inside the main
> border collapse code, not hidden inside a BCMapCellInfo function. I think
> you're factoring too much out here, we're losing track of the logic.
spit up
> +BCMapCellInfo::GetRightInnerBorder()
> 
> I suggest renaming "Inner" to "Internal" so as not to confuse it with "inner
> half" of the border or somesuch.
renamed
> There's a bunch of places where the name changes lead to misaligned
> indentation, and you have trailing spaces in various places. I'll point them
> out if they're still there in the next revision. My editor highlights the
> trailing spaces at least; so if yours doesn't do that, don't worry about it.
mangled it trough http://beaufour.dk/jst-review/

Comment 22

[Bernd]

Attachment: promised patch

this patch applies on top of the previous one and moves the setinfo function from the map iterator into the mapinfo structure.

Comment 23

[fantasai]

+  // borders concept). The widths are computed inside this functions based on

s/this/these/

+  // The BCMapCellInfo has functions to set the continous
+  // functions to set the border widths on the table related frames, where the

Tell us where this information is set. Inside the BCMapCellInfo struct?
On the frames? On the proptable?

+  void SetRow(PRInt32 aRowY);

You need to document the limitations on aRowY here; this function seems
like it can accept any aRowY, but it's not implemented that way.

+  // Helper functions to get extend of the cells

s/extend/extent/
s/cells/cell/

+  // storage of table ltr information, the border collapse code

The second half of this comment doesn't make sense. The whole struct
is about border collapse code, what exactly did you mean to point
out here?

+  // a cell with a rowspan has a top and a bottom row, and rows inbetween
+  // a cell with a colspan has a left and right column and columns inbetween

in between

+  // flags to describe the position of the row- and colgroups

I don't understand. What does this mean?

+    mStartSide  = NS_SIDE_LEFT;
+    mStartSide  = NS_SIDE_RIGHT;

Extra space before =

+
+
+
+void BCMapCellInfo::ResetCellInfo()

Doesn't need to be triple-spaced here.

+    mRightCol->SetRightBorderWidth(PR_MAX(half,
+                                           mRightCol->GetRightBorderWidth()));

Pull in this indentation until it lines up with the outer parens. Then
these function calls can have some consistency in their indentation instead
of running up against the 80 character limit in different ways for each one.

+  PRBool tableIsLTR =
+                     GetStyleVisibility()->mDirection == NS_STYLE_DIRECTION_LTR;
+

This should just be removed, it's not used anywhere anymore.
(Also, as a general point, awkwardly breaking lines like this for the sake of
one or two characters over 80 is not worth it. IMHO.)

Merging the whitespace-only changes into this patch was maybe not the best
idea, but since it's done I won't complain. rs=fantasai on the whitespace
changes, I didn't analyze them in detail. r=fantasai on your code if you
remove the tableIsLTR line. Address my comments on your comments and it's
good to go.

Comment 24

[Bernd]

Attachment: patch rev2.

> +  // borders concept). The widths are computed inside this functions based on
> 
> s/this/these/

fixed

> +  // The BCMapCellInfo has functions to set the continous
> +  // functions to set the border widths on the table related frames, where the
> 
> Tell us where this information is set. Inside the BCMapCellInfo struct?
> On the frames? On the proptable?
commented
> +  void SetRow(PRInt32 aRowY);
> 
> You need to document the limitations on aRowY here; this function seems
> like it can accept any aRowY, but it's not implemented that way.

done

> +  // Helper functions to get extend of the cells
> 
> s/extend/extent/
> s/cells/cell/
fixed
> +  // storage of table ltr information, the border collapse code
> 
> The second half of this comment doesn't make sense. The whole struct
> is about border collapse code, what exactly did you mean to point
> out here?
added the missing part of the sentence

> +  // a cell with a rowspan has a top and a bottom row, and rows inbetween
> +  // a cell with a colspan has a left and right column and columns inbetween
> 
> in between
fixed

> +  // flags to describe the position of the row- and colgroups
> 
> I don't understand. What does this mean?

extended the comment

> +    mStartSide  = NS_SIDE_LEFT;
> +    mStartSide  = NS_SIDE_RIGHT;
> 
> Extra space before =
> 
> +
> +
> +
> +void BCMapCellInfo::ResetCellInfo()
> 
> Doesn't need to be triple-spaced here.

fixed 
> +    mRightCol->SetRightBorderWidth(PR_MAX(half,
> +                                           mRightCol->GetRightBorderWidth()));
> 
> Pull in this indentation until it lines up with the outer parens. Then
> these function calls can have some consistency in their indentation instead
> of running up against the 80 character limit in different ways for each one.

made this identical on the corresponding functions
> +  PRBool tableIsLTR =
> +                     GetStyleVisibility()->mDirection ==
> NS_STYLE_DIRECTION_LTR;
> +
> 
> This should just be removed, it's not used anywhere anymore.
yes, done

> (Also, as a general point, awkwardly breaking lines like this for the sake of
> one or two characters over 80 is not worth it. IMHO.)
>
 
> Merging the whitespace-only changes into this patch was maybe not the best
> idea, but since it's done I won't complain. rs=fantasai on the whitespace
> changes, I didn't analyze them in detail. r=fantasai on your code if you
> remove the tableIsLTR line. Address my comments on your comments and it's
> good to go.

Boris this is part I of BC refactoring, no real improvement just a step to get it under control.  Attachment 320802 [details] applies on top of that and moves a function from one stucture to its more natural place.

Part II is attachment 366395 [details] [diff] [review] which focuses more on the paint side, this fixes real issues on the dependent bugs.

So what ever additional comments  you have on those other patches have, they are very wellcome (there are currently 248k of patches in this bug and bug 43178, its like hitting a wall)

Comment 25

[fantasai]

+  // SetRow is used to advance the computation trough the rows of a rowspanned
+  // cell, in order to avoid to seek the rows the function expects either a row
+  // index that corresponds to the first row in a rowspan or to the adjacent row

I don't quite understand this comment. It seems to say that I can pick the first or second row in a rowspan, but not any other row, which doesn't make sense for cells that span more than two rows.

Also s/trough/through/ and, probably s/in order to avoid to seek the rows// because at least to me it just seems to confuse matters...

+  // flags to describe the position of the cell with respect to the row- and
+  // colgroups, for instance mRgIsTop documents that the top cell border hits
+  // a rowgroup border
+  PRPackedBool          mRgIsTop;
+  PRPackedBool          mRgIsBottom;
+  PRPackedBool          mCgIsLeft;
+  PRPackedBool          mCgIsRight;

Ok, this makes a lot more sense now. But, wouldn't the names be better as e.g. mTopIsRg?

Comment 26

[fantasai]

Comment on attachment 370802 [details] [diff] [review]
promised patch

"promised patch" comments:

+
 inline PRInt32 BCMapCellInfo::GetCellEndRowIndex() const

Stray extra newline

+   mCellData = aCellData;
+   mColIndex = aColIndex;
+   // row frame info

3-space indent should be 2 spaces.

+      for (PRInt32 spanX = 2; mBottomRow && (spanX < mRowSpan); spanX++) {

probably should be spanY

+  // try to reuse the rgStart and rgEnd as the later is expensive to compute

s/later/latter/ and I don't understand this comment. Where are we reusing either of them? rgStart and rgEnd are calculated and only used once afaict.

Comment 27

[Bernd]

Attachment: next rev of the promised patch

it addresses the questions from comment 26

Comment 28

[Bernd]

re comment 25
would the following comment be OK with you?

// SetRow is used to advance the computation trough the rows of a rowspanned
// cell. Seeking rows is expensive, so this should be called with
// the first row of row spanned cell  or for all other calls rowY should be
// incremented only by one so that the required row can be found as a sibling
// of the previous row. 


>Ok, this makes a lot more sense now. But, wouldn't the names be better as e.g.
>mTopIsRg?

If there is a change necessary I would prefer mRgAtTop, Please indicate if this would fit.

Comment 29

[fantasai]

Hmm... The index isn't really used at all is it? I think maybe it makes more sense to have the function just take a boolean that says whether to reset to the start or to increment. This way it's clear what the function can and can't do.

  IncrementRow(PRBool aResetToStart = PR_FALSE);

Then you can call it with a check against mRowIndex. Does that make sense?

Yeah, mRgAtTop sounds fine.

Comment 30

[fantasai]

Comment on attachment 377439 [details] [diff] [review]
next rev of the promised patch

Seems you attached the wrong patch, this is the same as the old one.

Comment 31

[Bernd]

Attachment: the updated patch

Comment 32

[Bernd]

Attachment: rev. 3

> IncrementRow(PRBool aResetToStart = PR_FALSE);
Implemented
>Yeah, mRgAtTop sounds fine.
Name changed.

Comment 33

[fantasai]

Comment on attachment 379403 [details] [diff] [review]
rev. 3

Thanks for your patience, Bernd. :) r=fantasai

Comment 34

[fantasai]

Comment on attachment 379361 [details] [diff] [review]
the updated patch

// GetRowCount() are computational expensive it should be avoided if

s/computational expensive it/computationally expensive and/

Other than that, r=fantasai

Comment 35

[Boris Zbarsky [:bzbarsky]]

So just to make sure I understand... "the updated patch" is as separate patch from "rev. 3" and applies on top of it (at least in theory)?

Comment 36

[Bernd]

exactly, although I am afraid that comment 31 has been implemented after comment 31 and does a little bit bitrot the updated patch. If you want I can combine both into a single patch.

Comment 37

[Bernd]

Attachment: single patch

which combines 379361: the updated patch and 379403: rev. 3

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 383148 [details] [diff] [review]
single patch

>+struct BCCellBorder
>+  nscoord       width;    // border segment width in pixel coordinates !!

I know you just moved this, but if it's in px it should really be PRInt32 or PRUint32, not nscoord...  Maybe a followup, unless we plan to revamp this stuff even more?

>+  PRInt32       rowIndex; // rowIndex of temporary stored horizontal border
>+                          // segments

rowIndex wrt what?  The table?  Or the rowgroup?

>+ *  BCMapCellInfo
>+ * This structure stores information about the cellmap and all involved
>+ * table related frames that are used during the computation of winning borders
>+ * in CalcBCBorders so that the do need to be looked up again and again when

s/the do not/they do not/

So the key is that any point in time (or at least any point in time when it's usable for something) the BCMapCellInfo is pointing to some cell, right?  And possibly to some part of the cell if the cell has a rowspan? What about if it has a colspan?

> struct BCMapCellInfo 
>+  void SetInfo(nsTableRowFrame* aNewRow,
>+               PRInt32          aColIndex,
>+               CellData*        aCellData,
>+               BCMapCellIterator* aIter,
>+               nsCellMap*       aCellMap = nsnull);

This function seems underdocumented.

>+  // functions to set the border widths on the table related frames, where the
>+  // knowledge about the current position in the table is used.

I assume the nscoords here are actually app units?  If not, please call that out very explicitly!

>+  // functions to compute the borders, they depend on the

s/,/;/

>+  BCCellBorder GetTopEdgeBorder();
>+  BCCellBorder GetBottomEdgeBorder();
>+  BCCellBorder GetLeftEdgeBorder();
>+  BCCellBorder GetRightEdgeBorder();
>+  BCCellBorder GetRightInternalBorder();
>+  BCCellBorder GetLeftInternalBorder();
>+  BCCellBorder GetTopInternalBorder();
>+  BCCellBorder GetBottomInternalBorder();

It's really not clear to me when I'd use which of these... how can I tell whether I should GetLeftEdgeBorder() or GetLeftInternalBorder(), for example?

>+  // Increment the row as we loop over the rows of a rowspan
>+  void IncrementRow(PRBool aResetToStart = PR_FALSE);

What does that optional arg mean, exactly?

>+  PRInt32               mNumRows;
>+  PRInt32               mNumCols;

For which?  Our current cell?  The table?  Looks like the latter; document that?

>+  // cell information
>+  CellData*             mCellData;

Is that ever not BCCellData?  If not, I'd store a BCCellData* here.

>+BCMapCellInfo::BCMapCellInfo(nsTableFrame* aTableFrame)
>+  mTableBCData = (BCPropertyData*)nsTableFrame::GetProperty(mTableFrame,
>+                 nsGkAtoms::tableBCProperty, PR_FALSE);

static_cast please, assuming it's appropriate here.

>+BCMapCellIterator::BCMapCellIterator(nsTableFrame* aTableFrame,
>   mRowGroupIndex = -1;
>+  

No need for that extra blank line.

>+BCMapCellInfo::SetInfo(nsTableRowFrame* aNewRow,
>+    mCell = (nsBCTableCellFrame*)aCellData->GetCellFrame();

static_cast please.

>+  // GetRowCount() are computational expensive it should be avoided if

"computationally expensive and should be".

>+BCMapCellInfo::SetInnerRowGroupBottomContBCBorder(const nsIFrame* aNextRowGroup,
>+  if (aNextRow) {
>+          aNextRow->SetContinuousBCBorderWidth(NS_SIDE_TOP,

Weird indent here.

I tried to verify that the new code really does the same as the old, but it was just taking too long to do that (e.g. checking that we're making all the same CompareBorders calls in all the same cases with all the same arguments). I'm trusting you and fantasai on that part

sr=bzbarsky with the nits.

Comment 39

[Bernd]

Attachment: revised patch

[e-mail] schrieb:
> Do not reply to this email.  You can add comments to this bug at
> https://bugzilla.mozilla.org/show_bug.cgi?id=452319
> 
> 
> 
> 
> 
> --- Comment #38 from Boris Zbarsky (:bz) (todo: 175+ items) <[e-mail]>  2009-06-15 23:23:27 PDT ---
> (From update of attachment 383148 [details] [diff] [review])
>> +struct BCCellBorder
>> +  nscoord       width;    // border segment width in pixel coordinates !!
> 
> I know you just moved this, but if it's in px it should really be PRInt32 or
> PRUint32, not nscoord...  Maybe a followup, unless we plan to revamp this stuff
> even more?
>
changed
 
>> +  PRInt32       rowIndex; // rowIndex of temporary stored horizontal border
>> +                          // segments
> 
> rowIndex wrt what?  The table?  Or the rowgroup?
the table comment added

>> + *  BCMapCellInfo
>> + * This structure stores information about the cellmap and all involved
>> + * table related frames that are used during the computation of winning borders
>> + * in CalcBCBorders so that the do need to be looked up again and again when
> 
> s/the do not/they do not/
fixed

> So the key is that any point in time (or at least any point in time when it's
> usable for something) the BCMapCellInfo is pointing to some cell, right?  And
> possibly to some part of the cell if the cell has a rowspan? What about if it
> has a colspan?

The same, why do you assume that there is a asymmetry?

>> struct BCMapCellInfo 
>> +  void SetInfo(nsTableRowFrame* aNewRow,
>> +               PRInt32          aColIndex,
>> +               CellData*        aCellData,
>> +               BCMapCellIterator* aIter,
>> +               nsCellMap*       aCellMap = nsnull);
> 
> This function seems underdocumented.
I added some comments, but this function just fills the structure with the required information.
 
>> +  // functions to set the border widths on the table related frames, where the
>> +  // knowledge about the current position in the table is used.
> 
> I assume the nscoords here are actually app units?  If not, please call that
> out very explicitly!

changed the args to BCPixelSize, is that loud enough?

>> +  // functions to compute the borders, they depend on the
> 
> s/,/;/
fixed 

>> +  BCCellBorder GetTopEdgeBorder();
>> +  BCCellBorder GetBottomEdgeBorder();
>> +  BCCellBorder GetLeftEdgeBorder();
>> +  BCCellBorder GetRightEdgeBorder();
>> +  BCCellBorder GetRightInternalBorder();
>> +  BCCellBorder GetLeftInternalBorder();
>> +  BCCellBorder GetTopInternalBorder();
>> +  BCCellBorder GetBottomInternalBorder();
> 
> It's really not clear to me when I'd use which of these... how can I tell
> whether I should GetLeftEdgeBorder() or GetLeftInternalBorder(), for example?
added a comment

>> +  // Increment the row as we loop over the rows of a rowspan
>> +  void IncrementRow(PRBool aResetToStart = PR_FALSE);
> What does that optional arg mean, exactly?
change the variable name to aResetToTopRowOfCell
> 
>> +  PRInt32               mNumRows;
>> +  PRInt32               mNumCols;
> 
> For which?  Our current cell?  The table?  Looks like the latter; document
> that?
renamed the variables to nNumTableRows and mNumTableCols instead of documenting
> 
>> +  // cell information
>> +  CellData*             mCellData;
> 
> Is that ever not BCCellData?  If not, I'd store a BCCellData* here.
changed

>> +BCMapCellInfo::BCMapCellInfo(nsTableFrame* aTableFrame)
>> +  mTableBCData = (BCPropertyData*)nsTableFrame::GetProperty(mTableFrame,
>> +                 nsGkAtoms::tableBCProperty, PR_FALSE);
> 
> static_cast please, assuming it's appropriate here.
done 
>> +BCMapCellIterator::BCMapCellIterator(nsTableFrame* aTableFrame,
>>   mRowGroupIndex = -1;
>> +  
fixed
> No need for that extra blank line.
> 
>> +BCMapCellInfo::SetInfo(nsTableRowFrame* aNewRow,
>> +    mCell = (nsBCTableCellFrame*)aCellData->GetCellFrame();
> 
> static_cast please.
fixed
> 
>> +  // GetRowCount() are computational expensive it should be avoided if
> 
> "computationally expensive and should be".
fixed
>> +BCMapCellInfo::SetInnerRowGroupBottomContBCBorder(const nsIFrame* aNextRowGroup,
>> +  if (aNextRow) {
>> +          aNextRow->SetContinuousBCBorderWidth(NS_SIDE_TOP,
> 
> Weird indent here.
fixed
> I tried to verify that the new code really does the same as the old, but it was
> just taking too long to do that (e.g. checking that we're making all the same
> CompareBorders calls in all the same cases with all the same arguments). I'm
> trusting you and fantasai on that part
> 
> sr=bzbarsky with the nits.
>

Comment 40

[Bernd]

Attachment: changes in response to sr

Comment 41

[Bernd]

I pushed http://hg.mozilla.org/mozilla-central/rev/1d1379babaae

Comment 42

[Bernd]

bz and fantasai: thanks for your review, this should have happened in 2004 when I reviewed the changes for the table background painter, it is like the Russian saying that work is not like a wolf, is doesn't vanish in the wood.
fantasai: there is still the painting open

Comment 43

[Boris Zbarsky [:bzbarsky]]

> The same, why do you assume that there is a asymmetry?

I don't recall at this point; something about the comments made it sound that way...

> changed the args to BCPixelSize, is that loud enough?

Yep.

I assume you can push this yourself, right?  If you do want me to push it for some reason, let me know!

Comment 44

[Bernd]

>I assume you can push this yourself, right?

yes, see comment 41

Comment 45

[Mardeg]

Was the push you just did meant to fix bug 449183 listed as being blocked by this one?
That's incidentally a dupe of bug 332740 so I changed that in the blocks list.

Comment 46

[Bernd]

no

Comment 47

[fantasai]

First round of comments on border iterator refactoring:

+  PRBool                IsTopMostTable()    { return (mCurrentY == 0) && !mTable->GetPrevInFlow(); }
+  PRBool                IsRightMostTable()  { return (mCurrentX >= mNumCols); }
+  PRBool                IsBottomMostTable() { return (mCurrentY >= mNumRows) && !mTable->GetNextInFlow(); }
+  PRBool                IsLeftMostTable()   { return (mCurrentX == 0); }
+  PRBool                IsDamageAreaTopMost()    { return (mCurrentY == mStartY); }
+  PRBool                IsDamageAreaRightMost()  { return (mCurrentX >= mEndX); }
+  PRBool                IsDamageAreaBottomMost() { return (mCurrentY >= mEndY); }
+  PRBool                IsDamageAreaLeftMost()   { return (mCurrentX == mStartX); }


Renaming these to say "DamageArea" I think is a great idea. I'd also switch up the names for the other functions, from IsTopMostTable to IsTableTopMost().

+  mStartX   = mDamageArea.x;
+  mStartY   = mDamageArea.y;
+  mEndY     = mDamageArea.y + mDamageArea.height;
+  mEndX     = mDamageArea.x + mDamageArea.width;

These variables should not exist. Just use mDamageArea.x and mDamageArea.XMost() instead of mStartX and mEndX. This avoids the need to keep mDamageArea and the extra variables in sync, and it also makes it clearer that we are using the bounds of a damage area.

+    mAfterRepeatedHeader = !mIsRepeatedHeader && (mCurrentY == (mRepeatedHeaderY + 1));
+    mStartRepeatedFooter = mIsRepeatedFooter && (mCurrentY == mRowGroupStart) && (mCurrentY != mStartY);

These should be member functions rather than member variables. They can be easily calculated on the fly.

+    mRowRectHeight = mRow->GetRect().height;

This variable is unnecessary, just use mRow->GetRect().height directly, it's only needed once anyway.

   for (PRUint32 rgX = 0; rgX < numRowGroups; rgX++) {

This should probably be rgY.

+struct BCVerticalSeg
+{
+  BCVerticalSeg();

Please don't move this so far away from its implementation.
Just declare BCVerticalSeg to exist further up, leave the
definition where it is.

+  void GetBottomCorner(BCMapBorderIterator* aIter,
+                       nscoord aHorSegHeight);

Fix indentation.

+  nscoord               mOffsetX;

Add a comment explaining what the offset represents and what zero means.
The rest of the member variables should also have comments explaining
what they represent and what their values mean.

+  PRInt16               mSegWidth;   // width in pixels

Shouldn't this be BCPixelSize or whatever it was called?

+  BCVerticalSeg*        mVerInfo;

This should have some comments explaining what it represents and
what the bounds of the array are.

+  void ResetVerinfo();

-> ResetVerInfo()

+BCMapBorderIterator::Initialize(nsRect aDirtyRect)

Since this is only ever called from the BCMapBorderIterator
constructor, and the constructor is only two lines, it should
be folded into the constructor and not be a separate method.
Certainly not a public method.

-  if (IsRightMost() && IsBottomMost()) {
-    cell = nsnull;
-    bcData = &tableCellMap->mBCInfo->mLowerRightCorner;
-  }
-  else if (IsRightMost()) {
-    cellData = nsnull;
-    bcData = (BCData*)tableCellMap->mBCInfo->mRightBorders.ElementAt(aY);
-  }
-  else if (IsBottomMost()) {
-    cellData = nsnull;
-    bcData = (BCData*)tableCellMap->mBCInfo->mBottomBorders.ElementAt(aX);
-  }
+  if (IsRightMostTable() && IsBottomMostTable()) {
+    mCell = nsnull;
+    mBCData = &mTableCellMap->mBCInfo->mLowerRightCorner;
+  }
+  else if (IsRightMostTable()) {
+    mCellData = nsnull;
+    mBCData = (BCData*)mTableCellMap->mBCInfo->mRightBorders.ElementAt(aY);
+  }
+  else if (IsBottomMostTable()) {
+    mCellData = nsnull;
+    mBCData = (BCData*)mTableCellMap->mBCInfo->mBottomBorders.ElementAt(aX);
+  }

Wrt IsRightMost() -> IsRightMostTable(), etc...
AFAICT this is a change in semantics, not just naming. Is that intended?

Comment 48

[Bernd]

Attachment: border iterator refactoring rev. 1

> First round of comments on border iterator refactoring:
> 
> +  PRBool                IsTopMostTable()    { return (mCurrentY == 0) &&
> !mTable->GetPrevInFlow(); }
> +  PRBool                IsRightMostTable()  { return (mCurrentX >= mNumCols);
> }
> +  PRBool                IsBottomMostTable() { return (mCurrentY >= mNumRows)
> && !mTable->GetNextInFlow(); }
> +  PRBool                IsLeftMostTable()   { return (mCurrentX == 0); }
> +  PRBool                IsDamageAreaTopMost()    { return (mCurrentY ==
> mStartY); }
> +  PRBool                IsDamageAreaRightMost()  { return (mCurrentX >=
> mEndX); }
> +  PRBool                IsDamageAreaBottomMost() { return (mCurrentY >=
> mEndY); }
> +  PRBool                IsDamageAreaLeftMost()   { return (mCurrentX ==
> mStartX); }
> 
> 
> Renaming these to say "DamageArea" I think is a great idea. I'd also switch up
> the names for the other functions, from IsTopMostTable to IsTableTopMost().
fixed 

> +  mStartX   = mDamageArea.x;
> +  mStartY   = mDamageArea.y;
> +  mEndY     = mDamageArea.y + mDamageArea.height;
> +  mEndX     = mDamageArea.x + mDamageArea.width;
> 
> These variables should not exist. Just use mDamageArea.x and
> mDamageArea.XMost() instead of mStartX and mEndX. This avoids the need to keep
> mDamageArea and the extra variables in sync, and it also makes it clearer that
> we are using the bounds of a damage area.
fixed 
> +    mAfterRepeatedHeader = !mIsRepeatedHeader && (mCurrentY ==
> (mRepeatedHeaderY + 1));
> +    mStartRepeatedFooter = mIsRepeatedFooter && (mCurrentY == mRowGroupStart)
> && (mCurrentY != mStartY);
> 
> These should be member functions rather than member variables. They can be
> easily calculated on the fly.
fixed
> +    mRowRectHeight = mRow->GetRect().height;
>
> This variable is unnecessary, just use mRow->GetRect().height directly, it's
> only needed once anyway.
fixed
 
>    for (PRUint32 rgX = 0; rgX < numRowGroups; rgX++) {
> 
> This should probably be rgY.
> 
fixed
> +struct BCVerticalSeg
> +{
> +  BCVerticalSeg();
> 
> Please don't move this so far away from its implementation.
> Just declare BCVerticalSeg to exist further up, leave the
> definition where it is.

I very much prefer the current position which groups the horizontal segments, vertical segments and the iterator. So I did not change it as I do not see the benefit of the required action.

> +  void GetBottomCorner(BCMapBorderIterator* aIter,
> +                       nscoord aHorSegHeight);
> 
> Fix indentation.
fixed 
> +  nscoord               mOffsetX;
> 
> Add a comment explaining what the offset represents and what zero means.
> The rest of the member variables should also have comments explaining
> what they represent and what their values mean.
fixed
> +  PRInt16               mSegWidth;   // width in pixels
> 
> Shouldn't this be BCPixelSize or whatever it was called?
*all* pixel variables are now marked so,  
> +  BCVerticalSeg*        mVerInfo;
> 
> This should have some comments explaining what it represents and
> what the bounds of the array are.
fixed
> +  void ResetVerinfo();
> 
> -> ResetVerInfo()
fixed
> +BCMapBorderIterator::Initialize(nsRect aDirtyRect)
> 
> Since this is only ever called from the BCMapBorderIterator
> constructor, and the constructor is only two lines, it should
> be folded into the constructor and not be a separate method.
> Certainly not a public method.
fixed
> -  if (IsRightMost() && IsBottomMost()) {
> -    cell = nsnull;
> -    bcData = &tableCellMap->mBCInfo->mLowerRightCorner;
> -  }
> -  else if (IsRightMost()) {
> -    cellData = nsnull;
> -    bcData = (BCData*)tableCellMap->mBCInfo->mRightBorders.ElementAt(aY);
> -  }
> -  else if (IsBottomMost()) {
> -    cellData = nsnull;
> -    bcData = (BCData*)tableCellMap->mBCInfo->mBottomBorders.ElementAt(aX);
> -  }
> +  if (IsRightMostTable() && IsBottomMostTable()) {
> +    mCell = nsnull;
> +    mBCData = &mTableCellMap->mBCInfo->mLowerRightCorner;
> +  }
> +  else if (IsRightMostTable()) {
> +    mCellData = nsnull;
> +    mBCData = (BCData*)mTableCellMap->mBCInfo->mRightBorders.ElementAt(aY);
> +  }
> +  else if (IsBottomMostTable()) {
> +    mCellData = nsnull;
> +    mBCData = (BCData*)mTableCellMap->mBCInfo->mBottomBorders.ElementAt(aX);
> +  }
> 
> Wrt IsRightMost() -> IsRightMostTable(), etc...
> AFAICT this is a change in semantics, not just naming. Is that intended?
YES, this is a bug fix. This fixes the dependent bugs. The fix becomes  obvious if you try to write the lines
with the damage area functions.

Comment 49

[fantasai]

Comment on attachment 387191 [details] [diff] [review]
border iterator refactoring rev. 1

The comments and the separate of the variables into
groups really helps to make this understandable. Still
a few issues, though.

> +  nscoord               mOffsetX;
>
> Add a comment explaining what the offset represents and what zero means.
> The rest of the member variables should also have comments explaining
> what they represent and what their values mean.

+  nscoord               mOffsetX;       // x-offset with respect to the table

So what does zero represent? The table edge?

+  nsTableCellFrame*     mAjaCell;       // neighboring cell to the first cell
+                                        // where the segment starts, it can be
+                                        // the owner of a segment
+  nsTableCellFrame*     mFirstCell;     // cell at the start of the segment

Is there a consistent relation between mAjaCell and
mFirstCell? E.g. mAjaCell is always to the left, or
mAjaCell is always later in the sibling chain? If so,
please call this out.

+  PRUint8               mBevelSide;     // direction to bevel at the top
+  nscoord               mBevelOffset;   // how much to bevel at the top
+  BCPixelSize           mBottomHorSegHeight; // height of the crossing hor
+                                        // border
+  nscoord               mEndOffset;     // how much longer is the segment due
+                                        // to the horizontal border, by this
+                                        // amount the next segment needs to be
+                                        // shifted.
+  PRBool                mEndBevel;      // should we bevel at the bottom

s/hor/horizontal/
How do we know which side of the end to bevel?
Does mSegHeight include the bevels?
(How come this set of variables is so different from
the equivalent set for horizontal borders?)
Rename to mTopBevelSide etc. and mEndBevel etc. to be explicit and to be
consistent with the corresponding variables for horizontal segments.

+  nscoord               mSegHeight;     // vertical length
+  BCPixelSize           mSegWidth;      // width in pixels
+  nscoord            mWidth;             // horizontal length
+  BCPixelSize        mHeight;            // vertical length in pixels

Can we use some consistent naming here? I'd go for
either mWidth+mHeight, or mWidth+mLength where mWidth
is always the border-width dimension. (I have a slight
preference for mWidth+mLength, but you're more familiar
with how it's used, so whichever you prefer.)

+  PRBool                mEndBevel;      // should we bevel at the bottom
+  PRBool             mLeftBevel;         // should we bevel at the left end
+  PRBool             mRightBevel;        // should we bevel at the right end

Since these are booleans their names should imply that,
e.g. mEndIsBevel/mLeftIsBevel/mRightIsBevel.

> +  BCVerticalSeg*        mVerInfo;
>
> This should have some comments explaining what it represents and
> what the bounds of the array are.

+  BCVerticalSeg*        mVerInfo;           // array storing vertical borders
+                                            // while we move over the colums
+                                            // in the damage area

So what are the bounds of the array? Which vertical borders does it store?
The first row? The row above us? The row below?

+  mNumTableRows       = mTable->GetRowCount();
+  mRowIndex      = 0;
+  mNumTableCols       = mTable->GetColCount();
+  mColIndex      = 0;
+BCPaintBorderIterator::SetNewData(PRInt32 aY,
                                 PRInt32 aX)
+  BCPaintBorderIterator(nsTableFrame*         aTableFrame,
+                      nsTableRowGroupFrame* aRowGroupFrame,
+                      nsTableRowFrame*      aRowFrame,
+                      nsRect                aDamageArea);
+  mOffsetY         += offset;
+  mSegHeight     = -offset;
Fix indentation.

+  nsTableFrame*         mTableFirstInFlow;

You cached this to improve performance on GetColFrame etc?

+  PRInt32               mNumTableRows;  // number of rows in the table and all
+                                        // continuations
+  PRInt32               mNumTableCols;  // number of columns in the table
+  PRBool                mTableIsLTR;
+  PRInt32               mColInc;            // +1 for ltr -1 for rtl
+  const nsStyleBackground* mTableBgColor;

Since these are global to the table, they should be
grouped up with mTable etc.

+  nsTableRowGroupFrame* mPrevRowGroup;
+  nsTableRowGroupFrame* mRowGroup;
+  nsTableRowGroupFrame* mStartRowGroup; // first row group in the damagearea
+  PRInt32               mRowGroupIndex; // current row group index in the
+                                        // mRowgroups array
+  PRInt32               mFifRowGroupStart; // start row index of the first in
+                                           // flow of the row group
+  PRInt32               mRepeatedHeaderRowIndex; // row index in a repeated
+                                                 //header
+  PRInt32               mRowGroupStart; // row index of the first row in the row
+                                        // group
+  PRInt32               mRowGroupEnd;   // row index of the last row in the row
+                                        // group

I think switching back to the RG abbreviation, which is pretty unambiguous,
would help us make these variable names a little clearer without being
overlong. Some suggestions:

mPrevRg
mRg
mStartRg
mRgIndex
mFifRgFirstRowIndex
mRgFirstRowIndex
mRgLastRowIndex
mRepeatedHeaderRowIndex

It's not really clear what the last variable means when we're not in a
repeated header, so you should comment saying that it's equivalent to
mRowIndex when we're in a repeated header, and set to the last row index
of a repeated header when we're not. I'd also group it with mRowIndex,
since those are more related that the row group properties here.

+  PRInt32               mColIndex;      // with respect to the table
+  PRInt32               mRowIndex;      // with respect to the table
+  PRBool                mIsNewRow;
+  PRBool                mAtEnd;             // the iterator cycled over all
+                                            // borders

Since these all describe our current location, I would group them together.

+  PRBool                mIsRepeatedHeader;
+  PRBool                mIsRepeatedFooter;

Since these are properties of the row group, I would shift them up to
be with the row group variables.

+                                            // move over the colums

s/colums/columns/

IsAfterRepeatedHeader() returns true on the first row of a table without
a repeated header. Is that intended? If so, it should be documented why,
and perhaps the function name should be changed to reflect that.
IsFirstNonRepeatedRow or something... Would it also trigger on the first
repeated footer row if there are are no regular row groups?
If the intention is to only trigger after a repeated header row, then
you may need to keep a mPrevRowIsRepeatedHeader boolean instead of the
mRepeatedHeaderRowIndex.

+  nscoord               mHorizontalOffset;  // offsetX of the first border with
+                                            // respect to the table
+  nscoord               mVerticalOffset;    // offsetY of the first border with
+                                            // respect to the table

I would change these to mInitialOffsetX and mInitialOffsetY to be consistent
with the mOffsetX and mOffsetY naming scheme.

+  BCPaintBorderIterator(nsTableFrame*         aTableFrame,
+                      nsTableRowGroupFrame* aRowGroupFrame,
+                      nsTableRowFrame*      aRowFrame,
+                      nsRect                aDamageArea);

Is this constructor ever used? Because I'm not seeing it.
If it's never used, we should remove it.

+  mTableFirstInFlow    = (nsTableFrame*) mTable->GetFirstInFlow();
+  mTableCellMap        = mTable->GetCellMap();
+  mNumTableRows       = mTable->GetRowCount();
+  mNumTableCols       = mTable->GetColCount();
+  mTable->OrderRowGroups(mRowGroups);

These don't change between Reset() calls, right? They
should be moved into the constructor.

+  BCPaintBorderIterator iter(this, aDirtyRect);
+  if (!iter.mNeedsPainting)
+    return;

Given the way this is used, I would split the constructor
into two pieces, a constructor that accepts (this) and
a SetDamageArea(aDirtyRect) call that returns a boolean
saying whether or not we need to paint. You'll want to
make the split right after
  +   mTableBgColor = bgContext->GetStyleBackground();
shifting the needed PRBool declarations down into the
function and shifting the mTableIsLTR and mColInc setup
up into the constructor.

+        while ((mRowIndex < mDamageArea.y) && !mAtEnd) {
           SetNewRow();
         }

Since SetNewRow() does a fair amount of work for each new
row setting, perhaps it would be better to shift the while
loop inside. That way it's only one function call, and we
can skip the extra work until the loop gets us to where we
need to be.

+  if (!verSeg.mCol) { // on the first damaged row and the first segment in the col
+    verSeg.InitialSetting(this, borderOwner, verSegWidth, horSegHeight);
+  }

I don't really understand what's happening here. Can we get some
comments on the method declarations for BCHorizontalSeg, BCVerticalSeg,
and BCPaintBorderIterator?

+void
+BCPaintBorderIterator::Initialize(nsRect aDirtyRect)
+{
+
+}

This function can be removed.


In layout/, we tend to pass by reference rather than pointer when
we have convenience structures that group a lot of data members
together. nsHTMLReflowState, for example, and nsBlockReflowState,
etc. The BCPaintBorderIterator is an iterator, but it also fills
the data-passing package role. So I think it should be passed by
reference into the functions where it fills that role (which is
most, if not all, of the functions where it is handed as an argument).

Perhaps one of the superreviewers here can weigh in on that...

Comment 50

[fantasai]

+void
+BCPaintBorderIterator::PaintVerticalSegment(nsIRenderingContext& aRenderingContext)
+{
...
+    // paint the previous seg or the current one if IsDamageAreaBottomMost()
+    if (verSeg.mSegHeight > 0) {
+      verSeg.GetBottomCorner(this, horSegHeight);
+      if (verSeg.mSegWidth > 0) {
+        verSeg.Paint(this, aRenderingContext, horSegHeight);
+      } // if (verSeg.segWidth > 0)
+      verSeg.AdvanceOffsetY();
+    } // if (verSeg.segHeight > 0)
+    verSeg.Start(this, borderOwner, verSegWidth, horSegHeight);
+  } // if (!IsDamageAreaTopMost() && (isSegStart || IsDamageAreaBottomMost()))
+  verSeg.IncludeCurrentBorder(this);

The primary purpose of this function is to paint the segment, right?
The fact that we're doing stuff after painting is very confusing.
Isn't the stuff after the Paint call setting up for the next segment?
It shouldn't be inside this function.

(In any case, you can remove the // if comments.)

Same for PaintHorizontalSegment.

+void
+BCPaintBorderIterator::ResetVerInfo()
+{
+  if (mVerInfo) {
+    memset(mVerInfo, 0, mDamageArea.width * sizeof(BCVerticalSeg));
+    // XXX reinitialize properly
+    for (PRInt32 xIndex = 0; xIndex < mDamageArea.width; xIndex++) {
+      mVerInfo[xIndex].mColWidth = -1;
+    }
+  }

+void
+BCPaintBorderIterator::StoreColumnWidth(PRInt32 aIndex)
+{
+  if (IsTableRightMost()) {
+      mVerInfo[aIndex].mColWidth = mVerInfo[aIndex - 1].mColWidth;
+  }
+  else {
+    nsTableColFrame* col = mTableFirstInFlow->GetColFrame(mColIndex);
+    if (!col) ABORT0();
+    mVerInfo[aIndex].mColWidth = col->GetSize().width;
+  }
+}

This whole -1 thing is weird. Let's shift the StoreColumnWidth
calls into ResetVerInfo(). Does that make sense?

+   nsTableFrame* firstInFlow = (nsTableFrame*) mTable->GetFirstInFlow();

Don't we have an mTableFirstInFlow member for this?

+  PRUint8 ownerSide;
+   nscoord cornerSubWidth;
+   PRPackedBool bevel;
+   if (aIter->mBCData) {
+     cornerSubWidth = aIter->mBCData->GetCorner(ownerSide, bevel);
+   } else {
+     cornerSubWidth = 0;
+     ownerSide = 0;
+     bevel = PR_FALSE;
+   }

I'd rewrite as
  PRUint8 ownerSide = 0;
  nscoord cornerSubWidth = 0;
  PRBool bevel = PR_FALSE;
  if (aIter->mBCData) {
    cornerSubWidth = aIter->mBCData->GetCorner(ownerSide, bevel);
  }

+      case eColOwner:
+      NS_ASSERTION(aIter->IsTableTopMost() || aIter->IsTableBottomMost(),

Indentation is off.

I think we need a method to compute xAdj, because we do it often.

Ok, that's it for this round. :)

Comment 51

[Bernd]

Attachment: iterator refactor rev2

> --- Comment #49 from fantasai <[e-mail]>  2009-07-16 16:53:31 PDT ---
> (From update of attachment 387191 [details] [diff] [review])
> The comments and the separate of the variables into
> groups really helps to make this understandable. Still
> a few issues, though.
> 
>> +  nscoord               mOffsetX;
>>
>> Add a comment explaining what the offset represents and what zero means.
>> The rest of the member variables should also have comments explaining
>> what they represent and what their values mean.
> 
> +  nscoord               mOffsetX;       // x-offset with respect to the table
> 
> So what does zero represent? The table edge?
yes, comment added
> +  nsTableCellFrame*     mAjaCell;       // neighboring cell to the first cell
> +                                        // where the segment starts, it can be
> +                                        // the owner of a segment
> +  nsTableCellFrame*     mFirstCell;     // cell at the start of the segment
> 
> Is there a consistent relation between mAjaCell and
> mFirstCell? E.g. mAjaCell is always to the left, or
> mAjaCell is always later in the sibling chain? If so,
> please call this out.
its the previous sibling
comment added
> 
> +  PRUint8               mBevelSide;     // direction to bevel at the top
> +  nscoord               mBevelOffset;   // how much to bevel at the top
> +  BCPixelSize           mBottomHorSegHeight; // height of the crossing hor
> +                                        // border
> +  nscoord               mEndOffset;     // how much longer is the segment due
> +                                        // to the horizontal border, by this
> +                                        // amount the next segment needs to be
> +                                        // shifted.
> +  PRBool                mEndBevel;      // should we bevel at the bottom
> 
> s/hor/horizontal/
changed
> How do we know which side of the end to bevel?
The code currently assumes that only corners where two non null width
segments can ever be beveled, so the cellmap store gives the information
that this corner should be beveled and by checking that segment that
comes from top has a nonzero width we know that we should bevel towards
 top (for a horizontal segment).
> Does mSegHeight include the bevels?
No, but it includes the corners, comment adapted
> (How come this set of variables is so different from
> the equivalent set for horizontal borders?)
We always move in horizontal direction, which is the natural direction
for the cell sibling chain. So for the horizontal borders there is just
one border to draw, for the vertical borders there are multiple borders
coming from top and we decide to prolong them or to draw the previous
segment and start a new. Just imagine this as a comb with its tooth
pointing down. I added a comment to the code with this picture
> Rename to mTopBevelSide etc. and mEndBevel etc. to be explicit and to be
> consistent with the corresponding variables for horizontal segments.
done
> +  nscoord               mSegHeight;     // vertical length
> +  BCPixelSize           mSegWidth;      // width in pixels
> +  nscoord            mWidth;             // horizontal length
> +  BCPixelSize        mHeight;            // vertical length in pixels
> 
> Can we use some consistent naming here? I'd go for
> either mWidth+mHeight, or mWidth+mLength where mWidth
> is always the border-width dimension. (I have a slight
> preference for mWidth+mLength, but you're more familiar
> with how it's used, so whichever you prefer.)
I went for mWidth+mLength,
> +  PRBool                mEndBevel;      // should we bevel at the bottom
> +  PRBool             mLeftBevel;         // should we bevel at the left end
> +  PRBool             mRightBevel;        // should we bevel at the right end
> 
> Since these are booleans their names should imply that,
> e.g. mEndIsBevel/mLeftIsBevel/mRightIsBevel.
fixed

>> +  BCVerticalSeg*        mVerInfo;
>>
>> This should have some comments explaining what it represents and
>> what the bounds of the array are.
> 
> +  BCVerticalSeg*        mVerInfo;           // array storing vertical borders
> +                                            // while we move over the colums
> +                                            // in the damage area
> 
> So what are the bounds of the array? Which vertical borders does it store?
> The first row? The row above us? The row below?

comment added

> +  mNumTableRows       = mTable->GetRowCount();
> +  mRowIndex      = 0;
> +  mNumTableCols       = mTable->GetColCount();
> +  mColIndex      = 0;
> +BCPaintBorderIterator::SetNewData(PRInt32 aY,
>                                  PRInt32 aX)
> +  BCPaintBorderIterator(nsTableFrame*         aTableFrame,
> +                      nsTableRowGroupFrame* aRowGroupFrame,
> +                      nsTableRowFrame*      aRowFrame,
> +                      nsRect                aDamageArea);
> +  mOffsetY         += offset;
> +  mSegHeight     = -offset;
> Fix indentation.
done
> +  nsTableFrame*         mTableFirstInFlow;
> 
> You cached this to improve performance on GetColFrame etc?
yes

> +  PRInt32               mNumTableRows;  // number of rows in the table and all
> +                                        // continuations
> +  PRInt32               mNumTableCols;  // number of columns in the table
> +  PRBool                mTableIsLTR;
> +  PRInt32               mColInc;            // +1 for ltr -1 for rtl
> +  const nsStyleBackground* mTableBgColor;
> 
> Since these are global to the table, they should be
> grouped up with mTable etc.
moved

> +  nsTableRowGroupFrame* mPrevRowGroup;
> +  nsTableRowGroupFrame* mRowGroup;
> +  nsTableRowGroupFrame* mStartRowGroup; // first row group in the damagearea
> +  PRInt32               mRowGroupIndex; // current row group index in the
> +                                        // mRowgroups array
> +  PRInt32               mFifRowGroupStart; // start row index of the first in
> +                                           // flow of the row group
> +  PRInt32               mRepeatedHeaderRowIndex; // row index in a repeated
> +                                                 //header
> +  PRInt32               mRowGroupStart; // row index of the first row in the
> row
> +                                        // group
> +  PRInt32               mRowGroupEnd;   // row index of the last row in the
> row
> +                                        // group
> 
> I think switching back to the RG abbreviation, which is pretty unambiguous,
> would help us make these variable names a little clearer without being
> overlong. Some suggestions:
> 
> mPrevRg
> mRg
> mStartRg
> mRgIndex
> mFifRgFirstRowIndex
> mRgFirstRowIndex
> mRgLastRowIndex
> mRepeatedHeaderRowIndex
renamed

> It's not really clear what the last variable means when we're not in a
> repeated header, so you should comment saying that it's equivalent to
> mRowIndex when we're in a repeated header, and set to the last row index
> of a repeated header when we're not. I'd also group it with mRowIndex,
> since those are more related that the row group properties here.

comment added and moved
> +  PRInt32               mColIndex;      // with respect to the table
> +  PRInt32               mRowIndex;      // with respect to the table
> +  PRBool                mIsNewRow;
> +  PRBool                mAtEnd;             // the iterator cycled over all
> +                                            // borders
> 
> Since these all describe our current location, I would group them together.

moved

> +  PRBool                mIsRepeatedHeader;
> +  PRBool                mIsRepeatedFooter;
> 
> Since these are properties of the row group, I would shift them up to
> be with the row group variables.
moved
> +                                            // move over the colums
> 
> s/colums/columns/
fixed

> IsAfterRepeatedHeader() returns true on the first row of a table without
> a repeated header. Is that intended? If so, it should be documented why,
> and perhaps the function name should be changed to reflect that.
It doesn't matter since this is the top of the table and the only place
where we call this function is guarded by

if (!IsDamageAreaTopMost() && (isSegStart || IsDamageAreaBottomMost() ||
                                 IsAfterRepeatedHeader() ||
                                 StartRepeatedFooter())) {

Which will return true for IsDamageAreaTopMost()
> IsFirstNonRepeatedRow or something... Would it also trigger on the first
> repeated footer row if there are are no regular row groups?
> If the intention is to only trigger after a repeated header row, then
> you may need to keep a mPrevRowIsRepeatedHeader boolean instead of the
> mRepeatedHeaderRowIndex.

I dont see the point see above
> 
> +  nscoord               mHorizontalOffset;  // offsetX of the first border
> with
> +                                            // respect to the table
> +  nscoord               mVerticalOffset;    // offsetY of the first border
> with
> +                                            // respect to the table
> 
> I would change these to mInitialOffsetX and mInitialOffsetY to be consistent
> with the mOffsetX and mOffsetY naming scheme.
> 
> +  BCPaintBorderIterator(nsTableFrame*         aTableFrame,
> +                      nsTableRowGroupFrame* aRowGroupFrame,
> +                      nsTableRowFrame*      aRowFrame,
> +                      nsRect                aDamageArea);
> 
> Is this constructor ever used? Because I'm not seeing it.
> If it's never used, we should remove it.
removed
> +  mTableFirstInFlow    = (nsTableFrame*) mTable->GetFirstInFlow();
> +  mTableCellMap        = mTable->GetCellMap();
> +  mNumTableRows       = mTable->GetRowCount();
> +  mNumTableCols       = mTable->GetColCount();
> +  mTable->OrderRowGroups(mRowGroups);
> 
> These don't change between Reset() calls, right? They
> should be moved into the constructor.
moved
> +  BCPaintBorderIterator iter(this, aDirtyRect);
> +  if (!iter.mNeedsPainting)
> +    return;
> 
> Given the way this is used, I would split the constructor
> into two pieces, a constructor that accepts (this) and
> a SetDamageArea(aDirtyRect) call that returns a boolean
> saying whether or not we need to paint. You'll want to
> make the split right after
>   +   mTableBgColor = bgContext->GetStyleBackground();
> shifting the needed PRBool declarations down into the
> function and shifting the mTableIsLTR and mColInc setup
> up into the constructor.

splitted up

> +        while ((mRowIndex < mDamageArea.y) && !mAtEnd) {
>            SetNewRow();
>          }
> 
> Since SetNewRow() does a fair amount of work for each new
> row setting, perhaps it would be better to shift the while
> loop inside. That way it's only one function call, and we
> can skip the extra work until the loop gets us to where we
> need to be.

This will pretty much obfuscate what is going on, as usualy this
function is called when mRowIndex > mDamageArea.y.  I don't think there
is a real need for this.

> +  if (!verSeg.mCol) { // on the first damaged row and the first segment in the
> col
> +    verSeg.InitialSetting(this, borderOwner, verSegWidth, horSegHeight);
> +  }

I splitted this up so thats more obvious whats going on.
> I don't really understand what's happening here. Can we get some
> comments on the method declarations for BCHorizontalSeg, BCVerticalSeg,
> and BCPaintBorderIterator?
Done, although thats a cruel requirement

> +void
> +BCPaintBorderIterator::Initialize(nsRect aDirtyRect)
> +{
> +
> +}
> 
> This function can be removed.
> 
done

> In layout/, we tend to pass by reference rather than pointer when
> we have convenience structures that group a lot of data members
> together. nsHTMLReflowState, for example, and nsBlockReflowState,
> etc. The BCPaintBorderIterator is an iterator, but it also fills
> the data-passing package role. So I think it should be passed by
> reference into the functions where it fills that role (which is
> most, if not all, of the functions where it is handed as an argument).

I changed this
> Perhaps one of the superreviewers here can weigh in on that...
> 




> --- Comment #50 from fantasai <[e-mail]>  2009-07-24 16:56:16 PDT ---
> 
> +void
> +BCPaintBorderIterator::PaintVerticalSegment(nsIRenderingContext&
> aRenderingContext)
> +{
> ...
> +    // paint the previous seg or the current one if IsDamageAreaBottomMost()
> +    if (verSeg.mSegHeight > 0) {
> +      verSeg.GetBottomCorner(this, horSegHeight);
> +      if (verSeg.mSegWidth > 0) {
> +        verSeg.Paint(this, aRenderingContext, horSegHeight);
> +      } // if (verSeg.segWidth > 0)
> +      verSeg.AdvanceOffsetY();
> +    } // if (verSeg.segHeight > 0)
> +    verSeg.Start(this, borderOwner, verSegWidth, horSegHeight);
> +  } // if (!IsDamageAreaTopMost() && (isSegStart || IsDamageAreaBottomMost()))
> +  verSeg.IncludeCurrentBorder(this);
> 
> The primary purpose of this function is to paint the segment, right?
No the primary purpose is to avoid painting and to paint only as last resort.
> The fact that we're doing stuff after painting is very confusing.
> Isn't the stuff after the Paint call setting up for the next segment?
> It shouldn't be inside this function.
But thats the core of the current algorithm condensed into 10 lines. This algorithm is confusing. I am not touching the algorithm at all. What I did is to structure the code so that the algorithm becomes visible, and for me its  more obvious than http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#6772
I am not a fan of the algorithm at all, however due to refactoring the requirements what edge cases the code needs to handle became pretty evident.

> (In any case, you can remove the // if comments.)
done
> Same for PaintHorizontalSegment.
> 
> +void
> +BCPaintBorderIterator::ResetVerInfo()
> +{
> +  if (mVerInfo) {
> +    memset(mVerInfo, 0, mDamageArea.width * sizeof(BCVerticalSeg));
> +    // XXX reinitialize properly
> +    for (PRInt32 xIndex = 0; xIndex < mDamageArea.width; xIndex++) {
> +      mVerInfo[xIndex].mColWidth = -1;
> +    }
> +  }
> 
> +void
> +BCPaintBorderIterator::StoreColumnWidth(PRInt32 aIndex)
> +{
> +  if (IsTableRightMost()) {
> +      mVerInfo[aIndex].mColWidth = mVerInfo[aIndex - 1].mColWidth;
> +  }
> +  else {
> +    nsTableColFrame* col = mTableFirstInFlow->GetColFrame(mColIndex);
> +    if (!col) ABORT0();
> +    mVerInfo[aIndex].mColWidth = col->GetSize().width;
> +  }
> +}
> 
> This whole -1 thing is weird. Let's shift the StoreColumnWidth
> calls into ResetVerInfo(). Does that make sense?
no thats wrong this happens only for horizontal elements not vertical ones see the comment at verinfo

> 
> +   nsTableFrame* firstInFlow = (nsTableFrame*) mTable->GetFirstInFlow();
> 
> Don't we have an mTableFirstInFlow member for this?
fixed 
> +  PRUint8 ownerSide;
> +   nscoord cornerSubWidth;
> +   PRPackedBool bevel;
> +   if (aIter->mBCData) {
> +     cornerSubWidth = aIter->mBCData->GetCorner(ownerSide, bevel);
> +   } else {
> +     cornerSubWidth = 0;
> +     ownerSide = 0;
> +     bevel = PR_FALSE;
> +   }
> 
> I'd rewrite as
>   PRUint8 ownerSide = 0;
>   nscoord cornerSubWidth = 0;
>   PRBool bevel = PR_FALSE;
>   if (aIter->mBCData) {
>     cornerSubWidth = aIter->mBCData->GetCorner(ownerSide, bevel);
>   }
fixed 
> +      case eColOwner:
> +      NS_ASSERTION(aIter->IsTableTopMost() || aIter->IsTableBottomMost(),
> 
> Indentation is off.
fixed 
> I think we need a method to compute xAdj, because we do it often.

I don't see the point its just one subtraction I don't think the code will become better with that, just the opposite. 
> Ok, that's it for this round. :)
>

Comment 52

[fantasai]

Comment on attachment 390742 [details] [diff] [review]
iterator refactor rev2

+   if (mTableIsLTR) {
+     mColInc = 1;
+   }
+   else {
+     mColInc = -1;
+   }

Rewrite as mColInc = (mTableIsLTR) ? 1 : -1;

+  PRUint32 numCols = mTable->GetColCount();

Use mNumTableCols instead.

+ *  Move the iterator to the first position in te damageArea

s/te/the/

+  mColIndex      = 0;
+  mRgIndex = -1;

+    mRgFirstRowIndex    = mRg->GetStartRowIndex();
+    mRgLastRowIndex      = mRgFirstRowIndex + mRg->GetRowCount() - 1;

+    mHorSeg.mOffsetY = mNextOffsetY;
+    mNextOffsetY           = mNextOffsetY + mRow->GetSize().height;

+      rg = (aIter.IsTableBottomMost()) ? aIter.mRg :
+                                          aIter.mPrevRg;

+  mLastCell   = aIter.mCell;
+  mLength += aIter.mRow->GetRect().height;


Fix indentation.

+  //               This looks like a comb with tooth pointing down where
+  // while advancing over the rows and then the cells inside the rows the
+  // segments are either prolonged or painted and new segment is started.

I can't parse this sentence. :/

> I dont see the point see above

The point is that the function doesn't appear to do what it says it does.
This makes for confusing code. Either it should do what it says it does,
or it should say what it actually does. I believe initializing to a
number < -2 should give us the "does what it says" behavior.

> But thats the core of the current algorithm condensed into 10 lines. This
> algorithm is confusing. I am not touching the algorithm at all ...

OK.

> > I think we need a method to compute xAdj, because we do it often.
>
> I don't see the point its just one subtraction I don't think the code will
> become better with that, just the opposite.

It's not about the computation being complicated, but about it being
the same everywhere. Every time I see this computation, I have to think
"is it the same concept as before, or does this represent something
different?". I don't mind if you assign the result of the function into
a temporary variable called xAdj because it's shorter and easier to
use within the function. But I think this calculation should get its
own method, with a meaningful name (I can't guess what xAdj stands for)
and a comment.

r=fantasai with those changes. Please post your updated patch after checking in.

Comment 53

[Bernd]

Attachment: patch to address review comments

Comment 54

[Bernd]

Comment on attachment 392174 [details] [diff] [review]
patch to address review comments

Boris, I am not sure that this requires sr, just to be on the save side. This refactors the BC painting, it fixes a couple of bugs that arose from the fact that the code was so unreadable that even Chris could not read it. Its not the real fix but I needed to figure what this code does to make sure that the rewrite will not regress things.

Comment 55

[Boris Zbarsky [:bzbarsky]]

OK, I'm trying to understand the changes here.  What does the BCVerticalSeg struct represent?  Can we document its methods in the struct decl?  Same for BCPaintBorderIterator (e.g., the ProlongAndPaint methods).  For these last, it might be good to describe what it means to prolong or paint; it's not immediately obvious from the name or the code to me....  Having this stuff documented would make the review a lot easier.

Comment 56

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 392174 [details] [diff] [review]
patch to address review comments

Please rename ProlongAndPaint* to AccumulateOrPaint*, and s/keep tract/keep track/ in the one place it occurs, and sr=bzbarsky.

Comment 57

[Bernd]

Attachment: update to tip (fails on linux, OSX)

I updated to tip and incorporated the sr requirements however it only passed on winXX the try server all other platforms (gcc) failed.

Comment 58

[Bernd]

Attachment: patch which passed the tryserver successfully

Comment 59

[Dão Gottwald [:dao]]

Ugh, you were kind enough to include the commit message in your patch, but your name was missing, so mine was used:

http://hg.mozilla.org/mozilla-central/rev/524233171fa4

I don't know if there's a way to correct that, other than backing this out and relanding it.

Comment 60

[Dão Gottwald [:dao]]

Relanded:

http://hg.mozilla.org/mozilla-central/rev/91e00d39570f

Comment 61

[Bernd]

I pushed testcases fro dependent bugs as http://hg.mozilla.org/mozilla-central/rev/3e4ca31ff5fb

Comment 62

[Mardeg]

(In reply to comment #61)
> I pushed testcases fro dependent bugs as
> http://hg.mozilla.org/mozilla-central/rev/3e4ca31ff5fb

https://bugzilla.mozilla.org/attachment.cgi?id=288066 still shows the same problem, so I'm guessing dependent bug 332740 needs a separate patch on top of this bug's landed patch, which is why you didn't include testcases from it?

Comment 63

[Bernd]

because it is not fixed?

Comment 64

[Daniel Holbert [:dholbert]]

Attachment: followup: fixed signed/unsigned build warning

This bug's checkin introduced this build warning:
> layout/tables/nsTableFrame.cpp:6420: warning: comparison between signed and unsigned integer expressions
... which refers to this line:
> 6420   if (mRgIndex < mRowGroups.Length()) {

|mRgIndex| is signed (and initialized to -1), while |mRowGroups.Length()| is unsigned.

A few lines above, we've incremented mRgIndex (pushing any -1 values up to 0), and a few lines lower, we use it as an index into an array.  So it looks like we're assuming it's non-negative (and hence that it's safe to treat it as an unsigned value there).

This followup makes that assumption explicit with an assertion, and it also casts mRgIndex to an unsigned value, to fix the compiler warning. (Note that this is the same cast the compiler is already implicitly performing for us.)

Comment 65

[Daniel Holbert [:dholbert]]

Landed followup from comment 64: http://hg.mozilla.org/mozilla-central/rev/5cb8340929b8