Bug 71244

Created attachment 113209 [details] Patch

Created attachment 113784 [details] Patch

Created attachment 113924 [details] Patch

Comment on attachment 113924 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113924&action=review > Source/WebCore/rendering/style/CollapsedBorderValue.h:44 > + , m_offsetFromStart(offsetFromStart) > + , m_offsetFromTop(offsetFromTop) Why not call these row and column offsets? Seems weird to use the terms offsetFromStart and offsetFromTop when you just mean row and column position. > Source/WebCore/rendering/style/CollapsedBorderValue.h:79 > + int m_offsetFromStart; > + int m_offsetFromTop; Shouldn't these be unsigned?

Created attachment 115437 [details] Patch

Comment on attachment 115437 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115437&action=review r- mostly for the int <-> unsigned issues. Also some inconsistencies. > Source/WebCore/rendering/RenderTableCell.cpp:456 > colElt = table->colElement(col() -1, &startColEdge, &endColEdge); Nit: space between '-' and '1'. > Source/WebCore/rendering/RenderTableCell.cpp:484 > + RenderTableCell* nextCell = table->cellAfter(this); That doesn't look like a good change as you don't use nextCell outside the if. > Source/WebCore/rendering/RenderTableCell.cpp:572 > + result = chooseBorder(result, CollapsedBorderValue(prevRow->style()->borderAfter(), prevRow->style()->visitedDependentColor(after), BROW, col(), prevCell->row(), BAFTEREDGE)); Shouldn't it be prevCell->col() here for consistency? > Source/WebCore/rendering/RenderTableCell.cpp:582 > + result = chooseBorder(result, CollapsedBorderValue(currSection->style()->borderBefore(), currSection->style()->visitedDependentColor(before), BROWGROUP, col(), row(), BBEFOREEDGE)); Shouldn't you add the rowSpan() here to match section (6)? > Source/WebCore/rendering/RenderTableCell.cpp:589 > + result = chooseBorder(CollapsedBorderValue(currSection->style()->borderAfter(), currSection->style()->visitedDependentColor(after), BROWGROUP, col(), row() - rowSpan(), BAFTEREDGE), result); Nothing ensures that row() > rowSpan() here. This sounds like you could underflow your unsigned. > Source/WebCore/rendering/RenderTableCell.cpp:643 > + result = chooseBorder(result, CollapsedBorderValue(nextCell->parent()->style()->borderBefore(), nextCell->parent()->style()->visitedDependentColor(before), BROW, col(), nextCell->row(), BBEFOREEDGE)); Again shouldn't it be nextCell->col() for consistency? > Source/WebCore/rendering/style/CollapsedBorderValue.h:39 > + CollapsedBorderValue(const BorderValue& b, Color c, EBorderPrecedence p, int rowOffset, int columnOffset, EEdgePrecedence edge) Those should be unsigned too. > Source/WebCore/rendering/style/CollapsedBorderValue.h:66 > + int rowOffset() const { return m_rowOffset; } Those 2 should return an unsigned too. > Source/WebCore/rendering/style/CollapsedBorderValue.h:79 > + unsigned int m_columnOffset; Unsigned (not int) is the preferred usage in WebKit. > Source/WebCore/rendering/style/RenderStyleConstants.h:101 > +enum EEdgePrecedence { BAFTEREDGE, BENDEDGE, BBEFOREEDGE, BSTARTEDGE}; Space before the final }

Comment on attachment 115437 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115437&action=review >> Source/WebCore/rendering/RenderTableCell.cpp:582 >> + result = chooseBorder(result, CollapsedBorderValue(currSection->style()->borderBefore(), currSection->style()->visitedDependentColor(before), BROWGROUP, col(), row(), BBEFOREEDGE)); > > Shouldn't you add the rowSpan() here to match section (6)? I could, since all that matters is that section 6 is given an offset nearer the top than the one here. See next comment. >> Source/WebCore/rendering/RenderTableCell.cpp:589 >> + result = chooseBorder(CollapsedBorderValue(currSection->style()->borderAfter(), currSection->style()->visitedDependentColor(after), BROWGROUP, col(), row() - rowSpan(), BAFTEREDGE), result); > > Nothing ensures that row() > rowSpan() here. This sounds like you could underflow your unsigned. All that matters is that the after border of a row group above the current one is given an offset nearer the top of the table than the offset given the current one. This is so that its after border will always beat the current row group's before border if there's nothing else between them. So I could just use row + rowSpan() for (5) and row() for (6) - it would have the same effect and allow me to keep the ints unsigned. I guess that's the way I'll go.

Created attachment 115860 [details] Patch

(In reply to comment #6) > (From update of attachment 115437 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115437&action=review > > r- mostly for the int <-> unsigned issues. Also some inconsistencies. > Thanks! Hopefully I addressed all of these in the updated patch.

Comment on attachment 115860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115860&action=review > Source/WebCore/rendering/RenderTableCell.cpp:388 > + // the one further to the top wins Nit: comments starts wtih a capital letters and ends with a '.' > Source/WebCore/rendering/RenderTableCell.cpp:392 > + // the one further to the left if 'ltr' and right if 'rtl' and to the top wins Ditto. > Source/WebCore/rendering/RenderTableCell.cpp:523 > + CollapsedBorderValue startBorder = CollapsedBorderValue(colElt->style()->borderStart(), colElt->style()->visitedDependentColor(start), BCOL, col() + colSpan(), row(), BSTARTEDGE); > All that matters is that the after border of a row group above the current one is given an offset nearer the top of the table than the offset given the current one. This is so that its after border will always beat the current row group's before border if there's nothing else between them. > So I could just use row + rowSpan() for (5) and row() for (6) - it would have the same effect and allow me to keep the ints unsigned. I guess that's the way I'll go. There is something wrong here: if you underflow an unsigned, you end up with a huge value that wouldn't match what you were just saying here. Or at least this new code is not totally equivalent and yet you don't mention any test that was failing or needing a rebaseline. If I am right, it looks like we would need a simple test case for the underflow case. > Source/WebCore/rendering/style/CollapsedBorderValue.h:44 > + , m_columnOffset(columnOffset) I am still not a huge fan to call those 'offsets' as they are just the row and column (apart from one case where one represents the columnAfter) but I could live with it.

Comment on attachment 115860 [details] Patch Attachment 115860 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10462160 New failing tests: css2.1/20110323/abspos-containing-block-initial-004b.htm css2.1/20110323/abspos-containing-block-initial-004d.htm

(In reply to comment #10) > > All that matters is that the after border of a row group above the current one is given an offset nearer the top of the table than the offset given the current one. This is so that its after border will always beat the current row group's before border if there's nothing else between them. > > So I could just use row + rowSpan() for (5) and row() for (6) - it would have the same effect and allow me to keep the ints unsigned. I guess that's the way I'll go. > > There is something wrong here: if you underflow an unsigned, you end up with a huge value that wouldn't match what you were just saying here. Or at least this new code is not totally equivalent and yet you don't mention any test that was failing or needing a rebaseline. If I am right, it looks like we would need a simple test case for the underflow case. I agree the sequence of patches has made this confusing: The first one (113924) used a signed int so coped with row()- rowSpan() fine - so no underflow. The second one (115437) incompletely moved it to a signed int. The constructor used a signed int, stored it in an unsigned int, but then returned it as signed when CollapsedBorderValue::rowOffset() was queried. So the underflow was temporary and internal to CollapsedBorderValue only. In the final patch (115860) the values are all unsigned, so it is the first patch in which row() - rowSpan() will actually underflow when CollapsedBorderValue::rowOffset() is used. And indeed if I do row() - rowSpan() border-conflict-element-021.htm starts failing. So you're absolutely correct to be suspicious but I think it's just a confusing sequence of patches.

(In reply to comment #11) > (From update of attachment 115860 [details]) > Attachment 115860 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/10462160 > > New failing tests: > css2.1/20110323/abspos-containing-block-initial-004b.htm > css2.1/20110323/abspos-containing-block-initial-004d.htm I can't recreate these failures locally.

Created attachment 115980 [details] Patch

Comment on attachment 115980 [details] Patch > So you're absolutely correct to be suspicious but I think it's just a confusing sequence of patches. I see your point but it is not only that. The second patch did not seem to make any test go bad but the underflow should have made one case fail as the resolution would have favored the wrong border. I am asking for such a test to be added as it does not seem to be covered in the CSS test suite. The change looks fine but I really would like to see the case covered before r+'ing.

(In reply to comment #15) > (From update of attachment 115980 [details]) > > So you're absolutely correct to be suspicious but I think it's just a confusing sequence of patches. > > I see your point but it is not only that. The second patch did not seem to make any test go bad but the underflow should have made one case fail as the resolution would have favored the wrong border. I am asking for such a test to be added as it does not seem to be covered in the CSS test suite. The reason I gave for no test failing in the second patch was: "The second one (115437) incompletely moved it to a signed int. The constructor used a signed int, stored it in an unsigned int, but then returned it as signed when CollapsedBorderValue::rowOffset() was queried. So the underflow was temporary and internal to CollapsedBorderValue only." If I had correctly implemented the second patch and converted CollapsedBorderValue::rowOffset() to return an unsigned int, border-conflict-element-021.htm would have started failing. My understanding is that casting from signed->unsigned->signed ensured that in the end the underflowed value was *not* used when deciding where in the table the rowgroup's border was. I confirmed this in practice by converting CollapsedBorderValue to use an unsigned in all places for rowOffset, left the row()- rowSpan() in the place you highlighted it, and border-conflict-element-021.htm started failing on me. I also retested patch '2' and change rowOffset to return an unsigned, and it also failed. So border-conflict-element-021.htm tests for the potential regression you spotted and it didn't fail on patch '2' because the logic that checks the position of the border's cell was using a signed value rather than an unsigned.

Comment on attachment 115980 [details] Patch > If I had correctly implemented the second patch and converted CollapsedBorderValue::rowOffset() to return an unsigned int, border-conflict-element-021.htm would have started failing. My understanding is that casting from signed->unsigned->signed ensured that in the end the underflowed value was *not* used when deciding where in the table the rowgroup's border was. My bad, I missed the double signed <-> unsigned cast which would cancel any visible change in the resolution. > So border-conflict-element-021.htm tests for the potential regression you spotted and it didn't fail on patch '2' because the logic that checks the position of the border's cell was using a signed value rather than an unsigned. Thanks for double checking, appreciated!

Committed r101202: <http://trac.webkit.org/changeset/101202>

This appears to have completely hosed performance on some pages. See bug 73238 for details.

Reverted r101202 for reason: Caused performance regressions when painting collapsed borders Committed r101292: <http://trac.webkit.org/changeset/101292>

Created attachment 117792 [details] Patch

(In reply to comment #21) > Created an attachment (id=117792) [details] > Patch The reason the original patch created such a performance hit was because it increased the number of unique collapsed border values for a table from the number of unique borders by size, width, and color to the number of cells in the table x 4. Since RenderTable paints all cells in the table for each unique border value this meant every cell in a large table would get repainted thousands of times per table paint. The revised solution is to conform to the spec by changing the order in which cells are painted and avoid treating collapsed borders that only differ in color as unique. See the log for more details. This new approach does not depend on the enhancement proposed in 73349 in order to keep performance. By itself it actually improves performance in tables where there are many collapsed borders that differ only in color, since it will save a full repaint of cells for each different color.

Comment on attachment 117792 [details] Patch I am pretty sure this will make us paint badly some cases. Think of cases involving a cell with some overflow on the left of it: with your patch, the painting order will be inverted (the overflow will be under the left box's content). You can add outline, box-shadow, negative margins, negative top/left to this example.

(In reply to comment #23) > (From update of attachment 117792 [details]) > I am pretty sure this will make us paint badly some cases. Think of cases involving a cell with some overflow on the left of it: with your patch, the painting order will be inverted (the overflow will be under the left box's content). You can add outline, box-shadow, negative margins, negative top/left to this example. tables/mozilla_expected_failures/marvin/table_overflow_* seem to cover this kind of thing. One of them is rebaselined in this patch with no major pixel differences. If this was an issue wouldn't painting left to right cause issues with content overflowing to the right?

(In reply to comment #24) > (In reply to comment #23) > > (From update of attachment 117792 [details] [details]) > > I am pretty sure this will make us paint badly some cases. Think of cases involving a cell with some overflow on the left of it: with your patch, the painting order will be inverted (the overflow will be under the left box's content). You can add outline, box-shadow, negative margins, negative top/left to this example. > > tables/mozilla_expected_failures/marvin/table_overflow_* seem to cover this kind of thing. One of them is rebaselined in this patch with no major pixel differences. Actually if you pay attention, your rebaselined change is the difference I mention. It is subtle (1px difference) and it took me some time to see it: the difference is at the intersection between the red border and the border of the left cell. The red border used to be on top but your change made the left cell's border to be on top. If you want to make the change more explicit, add a background color to the left cell and the overflow. > If this was an issue wouldn't painting left to right cause issues with content overflowing to the right? No, AFAICT you are expected to paint in tree order in tables ("left-to-right" is dangerous to use with writing modes and that's my mistake to have used "left" in the first place) so the situation is not symmetric.

Created attachment 118262 [details] Patch

(In reply to comment #25) > > Actually if you pay attention, your rebaselined change is the difference I mention. It is subtle (1px difference) and it took me some time to see it: the difference is at the intersection between the red border and the border of the left cell. The red border used to be on top but your change made the left cell's border to be on top. If you want to make the change more explicit, add a background color to the left cell and the overflow. My solution to this is to paint collapsed borders separately from cells in RenderTableSection. This doesn't introduce any performance overhead as that is what is happening effectively anyway. This approach produces no suspicious-looking pixel differences so hopefully is acceptable!

Comment on attachment 118262 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118262&action=review > Source/WebCore/rendering/RenderTableCell.cpp:807 > void RenderTableCell::paint(PaintInfo& paintInfo, const LayoutPoint& paintOffset) Should just remove this rather than having it call through to the base class. > Source/WebCore/rendering/style/CollapsedBorderValue.h:56 > bool operator==(const CollapsedBorderValue& o) const > { > - return m_border == o.m_border && m_borderColor == o.m_borderColor && m_precedence == o.m_precedence; > + return m_border.width() == o.m_border.width() && m_border.style() == o.m_border.style() && m_precedence == o.m_precedence; > } Having this equality operation is fine, but it should not be named "==". It should be a named function instead if it's not a real equality comparison.

Comment on attachment 118262 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118262&action=review I think this approach could work. Some comments on the implementation though. >> Source/WebCore/rendering/RenderTableCell.cpp:807 > > Should just remove this rather than having it call through to the base class. I am ambivalent about that: I would like the following ASSERT added either here or in RenderBlock::paint. ASSERT(!(paintInfo.phase == PaintPhaseCollapsedTableBorders)); In RenderBlock it would catch more abuse as this phase should be limited to the table code. However it also exposing this phase in RenderBlock that should not know about it. > Source/WebCore/rendering/RenderTableCell.cpp:813 > +{ Can you add the following ASSERT: ASSERT(paintInfo.phase == PaintPhaseCollapsedTableBorders); > Source/WebCore/rendering/RenderTableCell.cpp:814 > + if (!paintInfo.shouldPaintWithinRoot(this)) The style()->visibility() == VISIBLE check should be kept here (see comments below). > Source/WebCore/rendering/RenderTableCell.cpp:822 > + return; unneeded return. > Source/WebCore/rendering/RenderTableCell.h:125 > + void paintCellCollapsedBorders(PaintInfo&, const LayoutPoint&); Do we really need a separate function or could we inline this shim function in paintCollapsedBorder? Also paintCollapsedBorders would make a lot more sense here. > Source/WebCore/rendering/RenderTableSection.cpp:1063 > + if (paintInfo.phase == PaintPhaseCollapsedTableBorders && style()->visibility() == VISIBLE) { This check does not replace a check for each cell. It would match: <table><thead style="visibility: hidden"><td></td></thead></table> but would miss: <table><thead><td style="visibility: hidden"></td></thead></table> This is also not the right place to put this check (same comment for the other check). It should be added at the beginning of the function to avoid all the computing we are doing above. This is a separate change, could it be postponed to another bug please? > Source/WebCore/rendering/RenderTableSection.cpp:1080 > + for (unsigned r = startrow; r < endrow; r++) { r -> row for consistency. > Source/WebCore/rendering/RenderTableSection.cpp:1081 > + for (unsigned c = startcol; c < endcol; c++) { c -> col for consistency. > Source/WebCore/rendering/RenderTableSection.cpp:1086 > + paintCell(cell, paintInfo, paintOffset); There is a check for paintInfo.phase == PaintPhaseCollapsedTableBorders in paintCell which should be removed as it will never be hit. > Source/WebCore/rendering/RenderTableSection.cpp:1128 > + LayoutPoint cellPoint = flipForWritingModeForChild(cells[i-1], paintOffset); That's cool that you fix the code for writing mode! Are different writing mode covered in the tests you check in? > Source/WebCore/rendering/RenderTableSection.cpp:1129 > + cells[i-1]->paintCellCollapsedBorders(paintInfo, cellPoint); Style: cells[i - 1] (twice in the 2 previous lines).

Created attachment 118609 [details] Patch

Comment on attachment 118609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118609&action=review > Source/WebCore/rendering/RenderTableCell.cpp:809 > + ASSERT(!(paintInfo.phase == PaintPhaseCollapsedTableBorders)); Probably would read better with !=. > Source/WebCore/rendering/RenderTableCell.cpp:896 > + if ((*a).isSameIgnoringColor(*b)) Should use the -> operator here instead of (*a). > Source/WebCore/rendering/RenderTableSection.cpp:998 > + if ((!cell->hasSelfPaintingLayer() && !row->hasSelfPaintingLayer())) There’s an extra set of parentheses here we should remove.

Comment on attachment 118609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118609&action=review Yay to ref-tests! > Source/WebCore/rendering/RenderTableSection.cpp:1063 > + if (paintInfo.phase == PaintPhaseCollapsedTableBorders && style()->visibility() == VISIBLE) { the style()->visibility() == VISIBLE is a good optimization but it is not related to this bug and I am pretty sure we are still doing too much work in this case. Could this be pushed to another bug? > Source/WebCore/rendering/RenderTableSection.cpp:1126 > + if (paintInfo.phase == PaintPhaseCollapsedTableBorders && style()->visibility() == VISIBLE) { Ditto.

Committed r103251: <http://trac.webkit.org/changeset/103251>

A whole bunch of layout tests have been failing on Apple's Mac port since this patch was landed: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r104531%20(36197)/results.html Could you either rebaseline these tests or fix whatever regressions this patch may have caused?

(In reply to comment #34) > A whole bunch of layout tests have been failing on Apple's Mac port since this patch was landed: > > http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r104531%20(36197)/results.html > > Could you either rebaseline these tests or fix whatever regressions this patch may have caused? The expectations need to be fixed to prevent them displaying on the results page. They're suppressed in the expectations file, see the items listed for BUGWK74888 in: http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/test_expectations.txt Unfortunately, the SL bot doesn't test png results, so the failure is TEXT and not IMAGE+TEXT. That's the reason they show up in the results page. They aren't counted as failures as you can see from: http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Tests%29/builds/36197/steps/layout-test/logs/stdio Those failing canvas tests aren't related to the patch here. All of which is just FYI, I need to fix the expectations to TEXT for Mac until someone in Apple has the time to rebaseline. Though if there is a Mac buildbot that checks png results then *it* will start getting this cruft in its results page.