The recent patch for bug #86605 introduced new functions that handles writing-mode, text direction and calculates rows and columns intersected by a rectangle. This is somewhat similar to what the functions dirtiedRows() and dirtiedColumns() already does, and the code should be combined. This will also solve a problem of dirtiedColumns() not handling right-to-left text.
Created attachment 145248 [details] Patch
Created attachment 145969 [details] Patch
Comment on attachment 145969 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145969&action=review > Source/WebCore/rendering/RenderTableSection.cpp:1053 > CellSpan RenderTableSection::dirtiedRows(const LayoutRect& damageRect) const I would really have loved for dirtiedRows / dirtiedColumns to just die but it seems that we need to patch the CellSpan returned from spannedRows. Having different behaviors for hit-testing and painting is unfortunate and should make us wonder if we don't prefer a unified code path (less bug but also any improvement would improve both code path). > Source/WebCore/rendering/RenderTableSection.cpp:1058 > + CellSpan coveredRows = spannedRows(damageRect); I don't think this is right: spannedRows takes a rect that is flipped to account for the section's direction. However the damage rect is in device coordinates which doesn't account for direction. > Source/WebCore/rendering/RenderTableSection.cpp:1062 > + // To repaint the border we might need to repaint first or last row even if they are not spanned themselves. > + if (coveredRows.start() >= m_rowPos.size() - 1 && m_rowPos[m_rowPos.size() - 1] + table()->outerBorderAfter() >= damageRect.y()) > + --coveredRows.start(); Is your change working with vertical writing modes as m_rowPos is actually in the x direction? (which was why |before| needed to take the writing mode into account) > Source/WebCore/rendering/RenderTableSection.cpp:1064 > + if (!coveredRows.end() && m_rowPos[0] - table()->outerBorderBefore() <= damageRect.maxY()) This is not mixed direction aware AFAICT (e.g if table and table-section have opposite directions - see bug 87900). Could you add a FIXME about that?
Comment on attachment 145969 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145969&action=review >> Source/WebCore/rendering/RenderTableSection.cpp:1064 >> + if (!coveredRows.end() && m_rowPos[0] - table()->outerBorderBefore() <= damageRect.maxY()) > > This is not mixed direction aware AFAICT (e.g if table and table-section have opposite directions - see bug 87900). Could you add a FIXME about that? Scratch that actually, I misread the direction (inline base direction is start / end) so no FIXME needed.
(In reply to comment #3) > (From update of attachment 145969 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145969&action=review > > > Source/WebCore/rendering/RenderTableSection.cpp:1058 > > + CellSpan coveredRows = spannedRows(damageRect); > > I don't think this is right: spannedRows takes a rect that is flipped to account for the section's direction. However the damage rect is in device coordinates which doesn't account for direction. > The variable name should probably have been updated, because the damage-rect is now pre-flipped. > > Source/WebCore/rendering/RenderTableSection.cpp:1062 > > + // To repaint the border we might need to repaint first or last row even if they are not spanned themselves. > > + if (coveredRows.start() >= m_rowPos.size() - 1 && m_rowPos[m_rowPos.size() - 1] + table()->outerBorderAfter() >= damageRect.y()) > > + --coveredRows.start(); > > Is your change working with vertical writing modes as m_rowPos is actually in the x direction? (which was why |before| needed to take the writing mode into account) > As far as I can tell this is the correct use. Look at outBorderLeft for instance, which selects the correct border-value based on writing-mode and direction.
Created attachment 147270 [details] Patch
Comment on attachment 147270 [details] Patch Remove dirtiedRows and dirtiedColumns functions. Otherwise unchanged logic.
Comment on attachment 147270 [details] Patch Attachment 147270 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12943964 New failing tests: css1/box_properties/padding_top_inline.html css1/formatting_model/inline_elements.html css2.1/20110323/replaced-intrinsic-ratio-001.htm fast/table/table-display-types-strict.html fast/loader/loadInProgress.html fast/loader/unload-form-post-about-blank.html fast/repaint/table-section-overflow.html fast/css-generated-content/015.html fullscreen/full-screen-zIndex.html fast/table/border-collapsing/border-collapsing-head-foot-vertical.html fast/css/box-shadow-and-border-radius.html fast/repaint/float-overflow.html http/tests/security/script-crossorigin-loads-correctly.html fast/repaint/table-cell-vertical-overflow.html fast/repaint/float-overflow-right.html fast/css/min-width-with-spanned-cell-fixed.html fast/repaint/overflow-flipped-writing-mode-table.html css1/box_properties/border_style_inline.html css1/box_properties/border_inline.html fullscreen/full-screen-iframe-zIndex.html fast/table/009.html fast/borders/bidi-009a.html fast/block/basic/quirk-percent-height-table-cell.html css1/box_properties/border_top_inline.html fast/text/whitespace/025.html
Created attachment 147366 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
(In reply to comment #9) > Created an attachment (id=147366) [details] > Archive of layout-test-results from ec2-cr-linux-02 > > The attached test failures were seen while running run-webkit-tests on the chromium-ews. > Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick The regression is due to the change in fullTableColumnSpan. I missed the fact it calls size of table->columns() and not size of table->columnPositions(). Only columnPositions() is one larger than the number of columns.
Created attachment 148304 [details] Patch
I'm excited about this patch, but I have another 600 bugs to go through from vacation-back-up. If you don't have a review in the next 48 hours, feel encouraged to pick on me.
Comment on attachment 148304 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148304&action=review Sorry for the back and forth. > Source/WebCore/rendering/RenderTableSection.cpp:1107 > + if (m_forceSlowPaintPathWithOverflowingCell) { Note that this should apply also to hit-testing. We just handle it separately for the moment. > Source/WebCore/rendering/RenderTableSection.cpp:1129 > + --dirtiedColumns.start(); If the alternative is to inline dirtiedRows / dirtiedColumns in paintObject which is already massive, I prefer it if we don't remove the 2 functions. It was short sighted from me to ask that as I missed the patching needed to properly paint the first / last row and column.
(In reply to comment #13) > (From update of attachment 148304 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148304&action=review > > Sorry for the back and forth. > > > Source/WebCore/rendering/RenderTableSection.cpp:1107 > > + if (m_forceSlowPaintPathWithOverflowingCell) { > > Note that this should apply also to hit-testing. We just handle it separately for the moment. > I just checked. This should already be handled by hasOwerflowingCell() check in the top of nodeAtPoint. > > Source/WebCore/rendering/RenderTableSection.cpp:1129 > > + --dirtiedColumns.start(); > > If the alternative is to inline dirtiedRows / dirtiedColumns in paintObject which is already massive, I prefer it if we don't remove the 2 functions. It was short sighted from me to ask that as I missed the patching needed to properly paint the first / last row and column. No problem. I think I already have a patch like that :)
Created attachment 148328 [details] Patch
> > > Source/WebCore/rendering/RenderTableSection.cpp:1107 > > > + if (m_forceSlowPaintPathWithOverflowingCell) { > > > > Note that this should apply also to hit-testing. We just handle it separately for the moment. > > > I just checked. This should already be handled by hasOwerflowingCell() check in the top of nodeAtPoint. You are totally right, however the hit-testing code doesn't benefit from the overflowing cell optimization. This was the meaning of this comment (as in: we need to move this to the common spanning logic at some point) but it's non-trivial to fix. Sorry for being unclear.
Comment on attachment 148328 [details] Patch Clearing flags on attachment: 148328 Committed r120721: <http://trac.webkit.org/changeset/120721>
All reviewed patches have been landed. Closing bug.