Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1676933004:
Table cell background painting (Closed)
|
Created:
4 years, 10 months ago by atotic1 Modified:
4 years, 9 months ago Reviewers:
Xianzhu CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, slimming-paint-reviews_chromium.org, dshwang, jchaffraix+rendering, blink-reviews-paint_chromium.org, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncol/colgroup clippedOverflowRectForPaintInvalidation fix
this routine was returning an incorrectly positioned rect.
The fix positions it correctly
fast/repaint/table-col-background.html test has been modified to demonstrate the bug
fast/repaint/table-col-background-offset.html tests has been added to test
offset columns
BUG=125336
Committed: https://crrev.com/e0d6d28effb0c20aebff303355e5d81321878ddb
Cr-Commit-Position: refs/heads/master@{#381290}
Patch Set 1 : colgroup paint bug125336 fix #
Total comments: 22
Patch Set 2 : fix for LayoutTableCol::clippedOverflowRectForPaintInvalidation #
Total comments: 41
Patch Set 3 : LayoutTableCol::clippedOverflowRectForPaintInvalidation fix #
Total comments: 5
Patch Set 4 : use visualOverflowRect, add test #
Total comments: 4
Patch Set 5 : tests: add 2nd col, rebaseline old. fix setLocation() #
Messages
Total messages: 38 (15 generated)
Description was changed from ========== Table cell background painting This fix fixes painting of <colgroup>, <col>, <tbody>, and <tr> backgrounds. Original code painted the background using cell as the background destination rect. New code uses position of background element as destination rect. Fix consists of several parts: - TableCellPainter::paintCellBackground uses position of backgroundElement for background destination. - Big part of the fix is determining position of background destination. I've added many new functions to make this possible: LayoutTableSection::positionOfIdealCell is the foundation. It returns the position of cell with rowspan & colspan 1 From this, we can compute position of <tr> From <tr>, we can compute position of <tbody> From <tbody>, we can compute position of <colgroup> and <col> Anther interesting part of the fix was LayoutTableCol::imageChanged LayoutTableCol::setShouldDoFullPaintInvalidation() did not cause cells to repaint. The fix was to traverse all cells in the column and invalidate them individually. As a result of these fixes, the border in test crbug.com/585208 fast/table/border-collapsing/composited-row-collapsed-border.html looks different. I've filed a separate bug about this crbug.com/585208, and will fix it separately I've had to rebase many of the marvin tests. I've compared new images to FF, and they look a lot better. There are still some border painting issues ( #2902 ). BUG=125336 ========== to ========== Table cell background painting This fix fixes painting of <colgroup>, <col>, <tbody>, and <tr> backgrounds. Original code painted the background using cell as the background destination rect. New code uses position of background element as destination rect. Fix consists of several parts: - TableCellPainter::paintCellBackground uses position of backgroundElement for background destination. - Big part of the fix is determining position of background destination. I've added many new functions to make this possible: LayoutTableSection::positionOfIdealCell is the foundation. It returns the position of cell with rowspan & colspan 1 From this, we can compute position of <tr> From <tr>, we can compute position of <tbody> From <tbody>, we can compute position of <colgroup> and <col> Anther interesting part of the fix was LayoutTableCol::imageChanged LayoutTableCol::setShouldDoFullPaintInvalidation() did not cause cells to repaint. The fix was to traverse all cells in the column and invalidate them individually. As a result of these fixes, the border in test crbug.com/585208 fast/table/border-collapsing/composited-row-collapsed-border.html looks different. I've filed a separate bug about this crbug.com/585208, and will fix it separately I've had to rebase many of the marvin tests. I've compared new images to FF, and they look a lot better. There are still some border painting issues ( #2902 ). BUG=125336 ==========
atotic@chromium.org changed reviewers: + wangxianzhu@chromium.org
Description was changed from ========== Table cell background painting This fix fixes painting of <colgroup>, <col>, <tbody>, and <tr> backgrounds. Original code painted the background using cell as the background destination rect. New code uses position of background element as destination rect. Fix consists of several parts: - TableCellPainter::paintCellBackground uses position of backgroundElement for background destination. - Big part of the fix is determining position of background destination. I've added many new functions to make this possible: LayoutTableSection::positionOfIdealCell is the foundation. It returns the position of cell with rowspan & colspan 1 From this, we can compute position of <tr> From <tr>, we can compute position of <tbody> From <tbody>, we can compute position of <colgroup> and <col> Anther interesting part of the fix was LayoutTableCol::imageChanged LayoutTableCol::setShouldDoFullPaintInvalidation() did not cause cells to repaint. The fix was to traverse all cells in the column and invalidate them individually. As a result of these fixes, the border in test crbug.com/585208 fast/table/border-collapsing/composited-row-collapsed-border.html looks different. I've filed a separate bug about this crbug.com/585208, and will fix it separately I've had to rebase many of the marvin tests. I've compared new images to FF, and they look a lot better. There are still some border painting issues ( #2902 ). BUG=125336 ========== to ========== Table cell background painting This fix fixes painting of <colgroup>, <col>, <tbody>, and <tr> backgrounds. Original code painted the background using cell as the background destination rect. New code uses position of background element as destination rect. Fix consists of several parts: - TableCellPainter::paintCellBackground uses position of backgroundElement for background destination. - Big part of the fix is determining position of background destination. I've added many new functions to make this possible: LayoutTableSection::positionOfIdealCell is the foundation. It returns the position of cell with rowspan & colspan 1 From this, we can compute position of <tr> From <tr>, we can compute position of <tbody> From <tbody>, we can compute position of <colgroup> and <col> Anther interesting part of the fix was LayoutTableCol::imageChanged LayoutTableCol::setShouldDoFullPaintInvalidation() did not cause cells to repaint. The fix was to traverse all cells in the column and invalidate them individually. As a result of these fixes, the top left border in test crbug.com/585208 fast/table/border-collapsing/composited-row-collapsed-border.html is not painting. This is because TableRowPainter in will-change paints its background on top of already painted borders. I am not sure what the right fix is here. I've had to rebase many of the marvin tests. I've compared new images to FF, and they look a lot better. There are still some border painting issues ( #2902 ). BUG=125336 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Table cell background painting This fix fixes painting of <colgroup>, <col>, <tbody>, and <tr> backgrounds. Original code painted the background using cell as the background destination rect. New code uses position of background element as destination rect. Fix consists of several parts: - TableCellPainter::paintCellBackground uses position of backgroundElement for background destination. - Big part of the fix is determining position of background destination. I've added many new functions to make this possible: LayoutTableSection::positionOfIdealCell is the foundation. It returns the position of cell with rowspan & colspan 1 From this, we can compute position of <tr> From <tr>, we can compute position of <tbody> From <tbody>, we can compute position of <colgroup> and <col> Anther interesting part of the fix was LayoutTableCol::imageChanged LayoutTableCol::setShouldDoFullPaintInvalidation() did not cause cells to repaint. The fix was to traverse all cells in the column and invalidate them individually. As a result of these fixes, the top left border in test crbug.com/585208 fast/table/border-collapsing/composited-row-collapsed-border.html is not painting. This is because TableRowPainter in will-change paints its background on top of already painted borders. I am not sure what the right fix is here. I've had to rebase many of the marvin tests. I've compared new images to FF, and they look a lot better. There are still some border painting issues ( #2902 ). BUG=125336 ========== to ========== Table cell background painting This fix fixes painting of <colgroup>, <col>, <tbody>, and <tr> backgrounds. Original code painted the background using cell as the background destination rect. New code uses position of background element as destination rect. Fix consists of several parts: - TableCellPainter::paintCellBackground uses position of backgroundElement for background destination. - Big part of the fix is determining position of background destination. I've added many new functions to make this possible: LayoutTableSection::positionOfIdealCell is the foundation. It returns the position of cell with rowspan & colspan 1 From this, we can compute position of <tr> From <tr>, we can compute position of <tbody> From <tbody>, we can compute position of <colgroup> and <col> Anther interesting part of the fix was LayoutTableCol::imageChanged LayoutTableCol::setShouldDoFullPaintInvalidation() did not cause cells to repaint. The fix was to traverse all cells in the column and invalidate them individually. LayoutTableCol::clippedOverflowRectForPaintInvalidation had a similar problem. Wrong rect was being invalidated, which caused lack of repaint in some cases. As a result of these fixes, the top left border in test crbug.com/585208 fast/table/border-collapsing/composited-row-collapsed-border.html is not painting. This is because TableRowPainter in will-change paints its background on top of already painted borders. I am not sure what the right fix is here, there is no clean way to paint the border inside the TableRowPainter::paint I've had to rebase many of the marvin tests. I've compared new images to FF, and they look a lot better. There are still some border painting issues ( #2902 ). Fixing clippedOverflowRectForPaintInvalidation also caused a rebase of 5 tests that were comparing paint rects. BUG=125336 ==========
Patchset #1 (id:60001) has been deleted
Hi, the background painting patch is done, happy to talk you through it.
Thanks for working on this. Please separate the following into other CLs to ease code review and revision management: - Fix LayoutTableCol::clippedOverflowRectForPaintInvalidation; - Fix LayoutTableCol::imageChanged() and LayoutTableCol::setShouldDoFullPaintInvalidation() (or further split to 2 if possible). You can manage multiple local branches having dependencies using 'git branch --set-upstream-to <depended-branch>'. Rietveld (this codereview tool) can handle such dependencies. Please don't include layout test rebaselines in the first patch set. Run 'git cl try' for the first patch set not including rebaselines. In the try result I can easily find the diffs (which is easier than looking at the patch). Haven't finished all review. Publishing comments so far. Please fix all style issues before uploading a patch. https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:1390: Nit: Unnecessary change https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right): https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTable.cpp:851: unsigned LayoutTable::colElementToCol(const LayoutTableCol * colElement) const Nit: extra space before '*'. https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTable.cpp:855: for (i = 0; i < m_columnLayoutObjects.size() && col < m_columns.size(); i++) { The following style is preferred: for (const LayoutTableCol* column : m_columnLayoutObjects) { ... } https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTable.cpp:855: for (i = 0; i < m_columnLayoutObjects.size() && col < m_columns.size(); i++) { The col < m_columns.size() seems incorrect. m_columns.size() is the number of effective columns, not the actual column index that this function is to return. https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTable.cpp:859: if (!m_columnLayoutObjects[i]->isTableColumnGroup()) Is this condition correct? It seems different from slowColElement(). https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableCol.cpp (right): https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:126: // setShouldDoFullPaintInvalidation(); Why doesn't the logic in LayoutTable::invalidatePaintOfSubtreesIfNeeded() work for the case? https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:194: if (!next && p && p->isLayoutTableCol()) Can parent() be null? https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableCol.h (right): https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableCol.h:98: Vector<LayoutTableCell*> getColumnCells() const; Nit: omit 'get'. https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/TableCellPainter.cpp (left): https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:166: ASSERT(paintRect.location() == paintOffset); Why is this removed? https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/TableCellPainter.cpp (right): https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:145: Nit: unnecessary change. https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:160: // Background objects have to enclose the entire cell Nit: '.' as the end of the comment. https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:161: if (backgroundObject != &m_layoutTableCell) { Nit: extra space after != Can you combine this block into the block at line 171 (new)? https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:173: } Nit: Unnecessary change.
@Xianzhu I've broken this into smaller patches. Here is the first one, it fixes the LayoutTableCol::clippedOverflowRectForPaintInvalidation You can test it by watching the paint rects in dev tools when colgroup background changes.
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1676933004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1676933004/100001
Thanks for creating the patch. Please also update the CL description. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right): https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTable.cpp:855: // Inverse of slowColElement: There is no more slowColElement. It's renamed to slowColElementAtAbsoluteColumn(). Please update. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTable.cpp:856: // maps LayoutTableCol to effectiveColumnIndex useful for columnPositions Start a sentence with an uppercase letter. Add a ';' or '.' at the end. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTable.cpp:857: // returns npos if out of range Add a '.' at the end. We should always find the column of the element (should ASSERT the situation of not-found), so we should never return error result. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTable.cpp:866: col += m_columnLayoutObjects[i]->span(); m_columnLayoutObjects[i].span is in number of absolute columns. If you sum up it here, you'll get the index of absolute column, not effective column. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableCol.cpp (right): https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:116: // FIXME: check for paintInvalidationContainer each time here? Remove the above FIXMEs. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:155: LayoutTableCol * LayoutTableCol::lastColumnInGroup() const Remove extra space before '*'. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:160: LayoutTableCol * currentCol = this->nextColumn(); Remove extra spaces. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:161: // if column has children, traverse the children to find last Start a sentence with uppercase letter. End a sentence with a '.'. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:219: // computes col/colgroup position that only spans cells Move the comment into header file. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:223: LayoutTable * myTable = table(); Suggestion: LayoutTable* table = this->table(); https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:227: return position; We don't need to check null of table(). https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:251: // could return vector with size 0 if out of range Ditto (comment style). https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableCol.h (right): https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.h:96: LayoutRect positionByCellSpan() const; Add document for this function. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1654: // Position is relative to section Move comments into header file. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1665: LayoutRect position(LayoutPoint(left, top), LayoutSize(right - left, bottom - top)); Change to: LayoutRect position(left, top, right - left, bottom - top); https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1669: WritingMode writingMode = rowLayout ? rowLayout->style()->getWritingMode() : TopToBottomWritingMode; Why fallback to TopToBottomWritingMode if there is no row? https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1670: bool isLTR = style()->isLeftToRightDirection(); The code below can be simplified by reusing existing writing mode and direction handling code. You can check LayoutTableSection::logicalRectForWritingModeAndDirection() (which seems the inversion of the code below) to see how and what can be reused. BTW I guess you would be interested in fixing the FIXME in LayoutTableSection::logicalRectForWritingModeAndDirection(). You can look at the patch that Julien created in https://bugs.webkit.org/show_bug.cgi?id=96691 if you are interested. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1723: // position.width().toInt() << " x " << position.height().toInt(); Please remove temporary debug code before sending out a CL for review. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right): https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableSection.h:316: LayoutRect positionOfIdealCell(unsigned row, unsigned col) const; s/col/effectiveCol/ so that any one reading this header file will know a effective column is accepted. Also move document from .cc to here. I think we can just use 'Cell' instead of 'IdealCell', because LayoutTableSection is already using the term to indicate the grid cells (may correspond to a part of real cell). See cellAt(), primaryCellAt(), etc. The implementation seems to return physical position of the 'ideal cell', so this function could be named getCellPhysicalPosition(). (Our current style suggests a verb at the beginning of a function.) Perhaps you can split this function into two: 1. getCellPosition() which returns logical position; 2. a function to convert logical position to physical position; Then getCellPhysicalPosition() can be implemented by calling the above two functions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableCol.cpp (right): https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:247: return position; What if the column contains some cell having a span bigger than the column's span?
https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableCol.cpp (right): https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:247: return position; On 2016/03/09 20:15:57, Xianzhu wrote: > What if the column contains some cell having a span bigger than the column's > span? The test case for my question: <!DOCTYPE html> <table> <col style="background-color: green"> <col style="background-color: blue"> <tr> <td>Cell11</td> <td>Cell12</td> <tr> <td colspan="2">Cell2</td> </tr> </table> The first column actually covers Cell11 and Cell2, not just the 'ideal cells' 1,1 and 2,1. You might need to unite overflow rects of all actual cells in the column instead of just uniting the top and bottom 'ideal cells'. An alternative way to fix the column paint invalidation issue might be to keep the original non-optimal method (that uses table's invalidation rect for columns), but just fix the offset issue.
Description was changed from ========== Table cell background painting This fix fixes painting of <colgroup>, <col>, <tbody>, and <tr> backgrounds. Original code painted the background using cell as the background destination rect. New code uses position of background element as destination rect. Fix consists of several parts: - TableCellPainter::paintCellBackground uses position of backgroundElement for background destination. - Big part of the fix is determining position of background destination. I've added many new functions to make this possible: LayoutTableSection::positionOfIdealCell is the foundation. It returns the position of cell with rowspan & colspan 1 From this, we can compute position of <tr> From <tr>, we can compute position of <tbody> From <tbody>, we can compute position of <colgroup> and <col> Anther interesting part of the fix was LayoutTableCol::imageChanged LayoutTableCol::setShouldDoFullPaintInvalidation() did not cause cells to repaint. The fix was to traverse all cells in the column and invalidate them individually. LayoutTableCol::clippedOverflowRectForPaintInvalidation had a similar problem. Wrong rect was being invalidated, which caused lack of repaint in some cases. As a result of these fixes, the top left border in test crbug.com/585208 fast/table/border-collapsing/composited-row-collapsed-border.html is not painting. This is because TableRowPainter in will-change paints its background on top of already painted borders. I am not sure what the right fix is here, there is no clean way to paint the border inside the TableRowPainter::paint I've had to rebase many of the marvin tests. I've compared new images to FF, and they look a lot better. There are still some border painting issues ( #2902 ). Fixing clippedOverflowRectForPaintInvalidation also caused a rebase of 5 tests that were comparing paint rects. BUG=125336 ========== to ========== col/colgroup clippedOverflowRectForPaintInvalidation fix this routine was returning an incorrectly positioned rect. The fix calculates the exact position of col/colgroup. fast/repaint/table-col-background.html test has been modified to demonstrate the bug BUG=125336 ==========
I've addressed all of the issues you've raised. I have one question remaining
before my next patch:
> An alternative way to fix the column paint invalidation issue might be to keep
> the original non-optimal method (that uses table's invalidation rect for
> columns), but just fix the offset issue.
You are correct: invalidating just the column rect will not cover the entire
invalidated area.
There are two different ways to fix this:
1) invalidate the entire table
- simple
- we will repaint more than necessary
2) invalidate all the cells that intersect the column
- code is more complex
- repaint is optimal
1) is a minor change, clippedOverflowRectForPaintInvalidation becomes
LayoutRect r = table()->frameRect();
r.setLocation(LayoutPoint(0,0));
mapToVisibleRectInAncestorSpace(paintInvalidationContainer, r,
paintInvalidationState);
If you pick 1)
- the rest of my code that deals with getCellPhysicalPosition is unnecessary to
fix clippedOverflowRectForPaintInvalidation
- should the rest of the code be removed from this patch?
- if it is removed, I need to figure out how to land the rest of the changes in
parts.
On 2016/03/14 18:22:54, atotic1 wrote: > I've addressed all of the issues you've raised. I have one question remaining > before my next patch: > > > An alternative way to fix the column paint invalidation issue might be to keep > > the original non-optimal method (that uses table's invalidation rect for > > columns), but just fix the offset issue. > > You are correct: invalidating just the column rect will not cover the entire > invalidated area. > > There are two different ways to fix this: > 1) invalidate the entire table > - simple > - we will repaint more than necessary > 2) invalidate all the cells that intersect the column > - code is more complex > - repaint is optimal > > 1) is a minor change, clippedOverflowRectForPaintInvalidation becomes > > LayoutRect r = table()->frameRect(); > r.setLocation(LayoutPoint(0,0)); > mapToVisibleRectInAncestorSpace(paintInvalidationContainer, r, > paintInvalidationState); > > If you pick 1) > - the rest of my code that deals with getCellPhysicalPosition is unnecessary to > fix clippedOverflowRectForPaintInvalidation > - should the rest of the code be removed from this patch? > - if it is removed, I need to figure out how to land the rest of the changes in > parts. I would like to pick 1. Table cols are very rare, so I think it is not worth the complexity for its paint invalidation. Yes, please delete any unrelated code from this CL. Fwiw, you can manage multiple CLs depending each other and move changes among them in the following methods: 1. Create dependencies between to branches: git checkout <branch-that-should-depend-on-another-branch> git branch --set-uptream-to <the-depended-branch> git rebase 2. Split one CL into two: git checkout -t -b <new-branch> origin/master git cherry-pick <the-CL-to-be-split> git checkout origin/master <files-to-be-excluded-from-this-CL> ... manually remove unnecessary changes in this CL. You can use a diff tool (like Eclipse's diff tool) to do this. Then you can create a dependency between the two CLs.
Description was changed from ========== col/colgroup clippedOverflowRectForPaintInvalidation fix this routine was returning an incorrectly positioned rect. The fix calculates the exact position of col/colgroup. fast/repaint/table-col-background.html test has been modified to demonstrate the bug BUG=125336 ========== to ========== col/colgroup clippedOverflowRectForPaintInvalidation fix this routine was returning an incorrectly positioned rect. The fix positions it correctly fast/repaint/table-col-background.html test has been modified to demonstrate the bug BUG=125336 ==========
The latest patch contains a minimal fix for LayoutTableCol::clippedOverflowRectForPaintInvalidation I've also modified one of the existing tests to demonstrate the bug. How should I proceed with the rest of the patch for fixing painting of table cell's backgrounds? The rest of the patch consists of: Foundation: 1) routines for finding LayoutTableCol physicalColumn 2) LayoutTableSection::getCellPhysicalPosition Positioning: 3) routines for obtaining physical rectangle for colgroup/col/tbody/tr using foundation routines Painting: 4) TableCellPainter::paintBackgroundsBehindCell modifications
On 2016/03/14 19:33:45, atotic1 wrote:
> The latest patch contains a minimal fix for
> LayoutTableCol::clippedOverflowRectForPaintInvalidation
>
> I've also modified one of the existing tests to demonstrate the bug.
>
> How should I proceed with the rest of the patch for fixing painting of table
> cell's backgrounds?
>
> The rest of the patch consists of:
>
> Foundation:
> 1) routines for finding LayoutTableCol physicalColumn
> 2) LayoutTableSection::getCellPhysicalPosition
>
> Positioning:
> 3) routines for obtaining physical rectangle for colgroup/col/tbody/tr using
> foundation routines
>
> Painting:
> 4) TableCellPainter::paintBackgroundsBehindCell modifications
I think it's fine to either include them in one patch or separate them because
the above are for the same functionality that would not work without any of the
parts, and they seem to not include any refactory of existing code. They also
simply add code into existing files, so reverting would be clean when it caused
any problems.
However, because our layout tests might not fully cover all of the changed
behaviors, we may find bugs after the patch is landed. So please add a
RuntimeEnabledFeatures flag and choose from old and new paths according to the
value of the flag, in the following steps:
- Add a line in platform/RuntimeEnabledFeatures.in:
NewTableCellBackgroundPainting status=stable [1]
- Use code like:
if (RuntimeEnabledFeatures::newTableCellBackgroundPaintingEnabled())
new code;
else
old code;
- As we use status=stable, the feature will be enabled by default after the
patch is landed. If we find any bug of the new code, we have 2 choices:
a) fix the bug if the fix is simple;
b) disable the feature by default first (by removing 'status=stable'), until
the bug is fixed.
[1] For more complex features, normally we don't enable the feature in the first
patch, because a) the feature implementation has not been completed yet, and/or
b) we are not sure if the feature would be stable. Instead we add virtual test
suites to run a subset of layout tests with the new feature. After many rounds
of CLs to complete the feature and make it stable, we can try to enable the
feature by adding 'status=stable' or ('status=experimental' for even more
complex ones).
https://codereview.chromium.org/1676933004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/repaint/table-col-background.html (right): https://codereview.chromium.org/1676933004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/repaint/table-col-background.html:13: <div style="height:100px"></div> Normally we don't change existing tests, but add new tests. However, this change looks OK to me. Will the expectation change? If yes, please do the following to ease review: 1. Schedule a try jobs git cl try and ask for review after the job finishes without unexpected errors. (Normally you might always need this for any CL you asking for review. This is normally better than your local run of all layout tests because: - you can continue to work on other things when the try job is running; - both you and reviewer can see the results; - it will run on all platforms and more environments, not just your own development environment.) 2. Optionally, rebase the expectation before uploading the first patch. I said this is not good for CLs that changes a lot of expectations, but it's good for such CL that might change only a few expectations (especially without pixel changes). https://codereview.chromium.org/1676933004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableCol.cpp (right): https://codereview.chromium.org/1676933004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:113: // FIXME: Issue paint invalidation of only the rect the image paints in. "FIXME:" style has been obsoleted by "TODO(name)". If you do feel we should fix it, please add "TODO(atotic)", otherwise just explain why we are doing in the non-optimal way, like // Just invalidate paint of the whole table to avoid the complexity to .... https://codereview.chromium.org/1676933004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:117: LayoutRect r = table->frameRect(); This is incorrect because frameRect doesn't include overflows and consider clippings. You might need to still call table->clippedOverflowRectForPaintInvalidation() and adjust the wrong offset.
> You might need to still call > table->clippedOverflowRectForPaintInvalidation() and adjust the wrong offset. This does not work because clippedOverflowRectForPaintInvalidation ends up calling mapToVisibleRectInAncestorSpace, which can transform table's rect (ex: inside a transform). Calling table->visualOverflowRect() would return the rect I need, that is what LayoutBox::clippedOverflowRectForPaintInvalidation does. Are you ok with that? > table-col-background.html test change > Normally we don't change existing tests, but add new tests. However, this change > looks OK to me. Thanks. This test would start failing anyway with my change. The addition just demonstrates the bug.
On 2016/03/14 23:03:11, atotic1 wrote: (Please reply inline in the review diff page, and then click "Publish+Mail Comments" link in this page, so that we can track the contexts of the comments.) > > You might need to still call > > table->clippedOverflowRectForPaintInvalidation() and adjust the wrong offset. > > This does not work because clippedOverflowRectForPaintInvalidation ends up > calling > mapToVisibleRectInAncestorSpace, which can transform table's rect (ex: inside a > transform). Calling table->visualOverflowRect() would return the rect I need, > that is > what LayoutBox::clippedOverflowRectForPaintInvalidation does. Are you ok with > that? Yes. > > table-col-background.html test change > > > Normally we don't change existing tests, but add new tests. However, this > change > > looks OK to me. > > Thanks. This test would start failing anyway with my change. The addition just > demonstrates the bug.
BTW, can you add another test testing paint invalidation of the second col (which has an offset from the table)?
Description was changed from ========== col/colgroup clippedOverflowRectForPaintInvalidation fix this routine was returning an incorrectly positioned rect. The fix positions it correctly fast/repaint/table-col-background.html test has been modified to demonstrate the bug BUG=125336 ========== to ========== col/colgroup clippedOverflowRectForPaintInvalidation fix this routine was returning an incorrectly positioned rect. The fix positions it correctly fast/repaint/table-col-background.html test has been modified to demonstrate the bug fast/repaint/table-col-background-offset.html tests has been added to test offset columns BUG=125336 ==========
Apologies for this lengthy reply. It is my first time using Rietveld, and I was unaware that comments were held in queue. At the bottom, there is a comment about a try job. I've scheduled it, it ran, and failed as expected. What should I do next? https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:1390: On 2016/02/23 22:58:27, Xianzhu wrote: > Nit: Unnecessary change Done. https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right): https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTable.cpp:851: unsigned LayoutTable::colElementToCol(const LayoutTableCol * colElement) const On 2016/02/23 22:58:27, Xianzhu wrote: > Nit: extra space before '*'. Done. https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableCol.cpp (right): https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:126: // setShouldDoFullPaintInvalidation(); On 2016/02/23 22:58:27, Xianzhu wrote: > Why doesn't the logic in LayoutTable::invalidatePaintOfSubtreesIfNeeded() work > for the case? We do not call invalidatePaintOfSubtreesIfNeeded from here, I did not know about that call. Looking at it, it does something similar, invalidating every cell in the table instead of every cell in the column. We could use that instead, slightly less efficient. What would you like to do? https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:194: if (!next && p && p->isLayoutTableCol()) On 2016/02/23 22:58:27, Xianzhu wrote: > Can parent() be null? Yes, seen it live when restoring from history. https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableCol.h (right): https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableCol.h:98: Vector<LayoutTableCell*> getColumnCells() const; On 2016/02/23 22:58:27, Xianzhu wrote: > Nit: omit 'get'. Done. https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/TableCellPainter.cpp (right): https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:145: On 2016/02/23 22:58:27, Xianzhu wrote: > Nit: unnecessary change. I need this because paintBounds now takes a LayoutBox. https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:160: // Background objects have to enclose the entire cell On 2016/02/23 22:58:27, Xianzhu wrote: > Nit: '.' as the end of the comment. Done. https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:161: if (backgroundObject != &m_layoutTableCell) { On 2016/02/23 22:58:27, Xianzhu wrote: > Nit: extra space after != > > Can you combine this block into the block at line 171 (new)? Done. https://codereview.chromium.org/1676933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/TableCellPainter.cpp:173: } On 2016/02/23 22:58:27, Xianzhu wrote: > Nit: Unnecessary change. Done. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right): https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTable.cpp:855: // Inverse of slowColElement: On 2016/03/09 18:40:55, Xianzhu wrote: > There is no more slowColElement. It's renamed to > slowColElementAtAbsoluteColumn(). Please update. Done. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTable.cpp:856: // maps LayoutTableCol to effectiveColumnIndex useful for columnPositions On 2016/03/09 18:40:55, Xianzhu wrote: > Start a sentence with an uppercase letter. > Add a ';' or '.' at the end. Done. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTable.cpp:857: // returns npos if out of range On 2016/03/09 18:40:55, Xianzhu wrote: > Add a '.' at the end. > We should always find the column of the element (should ASSERT the situation of > not-found), so we should never return error result. You are correct. I was finding absolute columns, not effective. Fixe. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTable.cpp:866: col += m_columnLayoutObjects[i]->span(); On 2016/03/09 18:40:55, Xianzhu wrote: > m_columnLayoutObjects[i].span is in number of absolute columns. If you sum up it > here, you'll get the index of absolute column, not effective column. Done. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableCol.cpp (right): https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:116: // FIXME: check for paintInvalidationContainer each time here? On 2016/03/09 18:40:55, Xianzhu wrote: > Remove the above FIXMEs. Done. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:155: LayoutTableCol * LayoutTableCol::lastColumnInGroup() const On 2016/03/09 18:40:55, Xianzhu wrote: > Remove extra space before '*'. Done. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:160: LayoutTableCol * currentCol = this->nextColumn(); On 2016/03/09 18:40:56, Xianzhu wrote: > Remove extra spaces. Done. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:161: // if column has children, traverse the children to find last On 2016/03/09 18:40:55, Xianzhu wrote: > Start a sentence with uppercase letter. End a sentence with a '.'. Done. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:219: // computes col/colgroup position that only spans cells On 2016/03/09 18:40:55, Xianzhu wrote: > Move the comment into header file. Done. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:223: LayoutTable * myTable = table(); On 2016/03/09 18:40:56, Xianzhu wrote: > Suggestion: LayoutTable* table = this->table(); Done. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:227: return position; On 2016/03/09 18:40:56, Xianzhu wrote: > We don't need to check null of table(). table() can return nullptr. and I have to guard against it. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:247: return position; On 2016/03/09 20:15:57, Xianzhu wrote: > What if the column contains some cell having a span bigger than the column's > span? That's ok. We are not looking for cell position, but position of the column. If the caller needs a region spanned by cells that intersect this region, that's a different call. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:251: // could return vector with size 0 if out of range On 2016/03/09 18:40:55, Xianzhu wrote: > Ditto (comment style). Done. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableCol.h (right): https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.h:96: LayoutRect positionByCellSpan() const; On 2016/03/09 18:40:56, Xianzhu wrote: > Add document for this function. Done. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1654: // Position is relative to section On 2016/03/09 18:40:56, Xianzhu wrote: > Move comments into header file. Done. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1665: LayoutRect position(LayoutPoint(left, top), LayoutSize(right - left, bottom - top)); On 2016/03/09 18:40:56, Xianzhu wrote: > Change to: LayoutRect position(left, top, right - left, bottom - top); Done. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1665: LayoutRect position(LayoutPoint(left, top), LayoutSize(right - left, bottom - top)); On 2016/03/09 18:40:56, Xianzhu wrote: > Change to: LayoutRect position(left, top, right - left, bottom - top); Done. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1669: WritingMode writingMode = rowLayout ? rowLayout->style()->getWritingMode() : TopToBottomWritingMode; On 2016/03/09 18:40:56, Xianzhu wrote: > Why fallback to TopToBottomWritingMode if there is no row? Just defensive coding because I have no guarantees when any pointer returning routine will return null. I can just ASSERT(rowLayout) https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1723: // position.width().toInt() << " x " << position.height().toInt(); On 2016/03/09 18:40:56, Xianzhu wrote: > Please remove temporary debug code before sending out a CL for review. Done. Rewrote as a much simpler version. https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right): https://codereview.chromium.org/1676933004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableSection.h:316: LayoutRect positionOfIdealCell(unsigned row, unsigned col) const; Split the function into: getCellPosition getCellPhysicalPosition transformLogicalToPhysicalPosition https://codereview.chromium.org/1676933004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/repaint/table-col-background.html (right): https://codereview.chromium.org/1676933004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/repaint/table-col-background.html:13: <div style="height:100px"></div> On 2016/03/14 20:40:18, Xianzhu wrote: > Normally we don't change existing tests, but add new tests. However, this change > looks OK to me. > > Will the expectation change? If yes, please do the following to ease review: > > 1. Schedule a try jobs > git cl try > and ask for review after the job finishes without unexpected errors. > (Normally you might always need this for any CL you asking for review. > This is normally better than your local run of all layout tests because: > - you can continue to work on other things when the try job is running; > - both you and reviewer can see the results; > - it will run on all platforms and more environments, not just your own > development environment.) > > 2. Optionally, rebase the expectation before uploading the first patch. I said > this is not good for CLs that changes a lot of expectations, but it's good for > such CL that might change only a few expectations (especially without pixel > changes). I've scheduled a try job, and it failed as expected. What's next?
https://codereview.chromium.org/1676933004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/repaint/table-col-background.html (right): https://codereview.chromium.org/1676933004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/repaint/table-col-background.html:13: <div style="height:100px"></div> On 2016/03/15 16:50:48, atotic1 wrote: > On 2016/03/14 20:40:18, Xianzhu wrote: > > Normally we don't change existing tests, but add new tests. However, this > change > > looks OK to me. > > > > Will the expectation change? If yes, please do the following to ease review: > > > > 1. Schedule a try jobs > > git cl try > > and ask for review after the job finishes without unexpected errors. > > (Normally you might always need this for any CL you asking for review. > > This is normally better than your local run of all layout tests because: > > - you can continue to work on other things when the try job is running; > > - both you and reviewer can see the results; > > - it will run on all platforms and more environments, not just your own > > development environment.) > > > > 2. Optionally, rebase the expectation before uploading the first patch. I said > > this is not good for CLs that changes a lot of expectations, but it's good for > > such CL that might change only a few expectations (especially without pixel > > changes). > > I've scheduled a try job, and it failed as expected. What's next? Please rebaseline expectations and upload again and optionally try (which can save time for landing the CL because commit-queue will skip green bots). What I suggested are mainly for ease of review and CL development. For reviewer, it's easier to see expectation diffs from try results. For developer, it's easier to know what tests will fail with the change without running all layout tests on all platforms by himself/herself. For a small number of tests, you can either try first then rebaseline, or upload rebaselined expectation directly. I believe most reviewers would be happen with either way. For the latter way, if there are pixel rebaselines, you'd better tell reviewers what changed. For a large number of layout tests needing rebaselined, it's always better to upload the change without rebaselined expectations first, run try bots, analyze the result, rebaseline tests if the failures are expected [1], upload again and try. Reviewers can see the expectation diffs from the previous try result (normally you need to tell reviewer where to see the results). [1] You have two ways to rebaseline tests 1. run-webkit-tests --reset-results <tests>. Suits: small number of tests; the tests produce the same results on all platforms (or expectation changes on other platforms are know) 2. Mark tests 'NeedsRebaseline' in TestExpectations. Suits: otherwise. A rebaseline bot will rebaseline the tests on all platforms for you after the CL is landed. For this method, you should run try job first and tell reviewer, or tell the reviewer what will change. https://codereview.chromium.org/1676933004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableCol.cpp (right): https://codereview.chromium.org/1676933004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:121: r.setLocation(LayoutPoint(0, 0)); The above line should be removed. visualOverflowRect is in space of the object. Normally its location is zero, but if the object has visual overflow, the location will be negative and should be counted in the mapped result. https://codereview.chromium.org/1676933004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:121: r.setLocation(LayoutPoint(0, 0)); The correctness of this method depends on the fact that LayoutTableCol is not laid out to have an offset from the table (this is why I asked you to add the test). Please add // The correctness of this method depends on the fact that LayoutTableCol's // location is always zero. ASSERT(this->location() == LayoutPoint());
Code review changes done. Rebaselined table-col-background.html test Added table-col-background-offset.html test Started the trybot https://codereview.chromium.org/1676933004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableCol.cpp (right): https://codereview.chromium.org/1676933004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:121: r.setLocation(LayoutPoint(0, 0)); On 2016/03/15 17:24:39, Xianzhu wrote: > The correctness of this method depends on the fact that LayoutTableCol is not > laid out to have an offset from the table (this is why I asked you to add the > test). Please add > // The correctness of this method depends on the fact that LayoutTableCol's > // location is always zero. > ASSERT(this->location() == LayoutPoint()); Done. https://codereview.chromium.org/1676933004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableCol.cpp:121: r.setLocation(LayoutPoint(0, 0)); On 2016/03/15 17:24:39, Xianzhu wrote: > The correctness of this method depends on the fact that LayoutTableCol is not > laid out to have an offset from the table (this is why I asked you to add the > test). Please add > // The correctness of this method depends on the fact that LayoutTableCol's > // location is always zero. > ASSERT(this->location() == LayoutPoint()); Done.
The CQ bit was checked by wangxianzhu@chromium.org
lgtm lgtm!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1676933004/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1676933004/150001
Message was sent while issue was closed.
Committed patchset #5 (id:150001)
Message was sent while issue was closed.
Description was changed from ========== col/colgroup clippedOverflowRectForPaintInvalidation fix this routine was returning an incorrectly positioned rect. The fix positions it correctly fast/repaint/table-col-background.html test has been modified to demonstrate the bug fast/repaint/table-col-background-offset.html tests has been added to test offset columns BUG=125336 ========== to ========== col/colgroup clippedOverflowRectForPaintInvalidation fix this routine was returning an incorrectly positioned rect. The fix positions it correctly fast/repaint/table-col-background.html test has been modified to demonstrate the bug fast/repaint/table-col-background-offset.html tests has been added to test offset columns BUG=125336 Committed: https://crrev.com/e0d6d28effb0c20aebff303355e5d81321878ddb Cr-Commit-Position: refs/heads/master@{#381290} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e0d6d28effb0c20aebff303355e5d81321878ddb Cr-Commit-Position: refs/heads/master@{#381290} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
