Issue 744493002: Let RenderTable reach table cells needing overflow recalc

Issue 744493002: Let RenderTable reach table cells needing overflow recalc (Closed)

Created:
6 years, 1 month ago by Xianzhu

Modified:
5 years, 9 months ago

Reviewers:
leviw_travelin_and_unemployed, trchen, Julien - ping for review

CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, zoltan1

Base URL:
svn://svn.chromium.org/blink/trunk

Project:
blink

Visibility:
Public.

More Reviews

Description

Let RenderTable reach table cells needing overflow recalc When a RenderObject in a table cell is marked needsRecalcOverflowAfterStyleChange, the object won't be reached by the original RenderBlock::recalcChildOverflowAfterStyleChange(). Add RenderTable::recalcChildOverflowAfterStyleChange to ensure the RenderObject's needsRecalcOverflowAfterStyleChange is handled. (Basically this is like other layout flags like selfNeedsLayout, needsSimplifiedLayout, etc. that RenderTable needs special handling to reach the child needing layout). BUG=434700 TEST=fast/table/outline-change-in-table-cell.html TEST=fast/table/table-cell-outline-change.html

Patch Set 1 #

Total comments: 2

Patch Set 2 : Fix layout test crash #

Total comments: 7

Patch Set 3 : Avoid O(rows*cols) #

Total comments: 11

Patch Set 4 : address comments #

Total comments: 9

Patch Set 5 : #

Total comments: 9

Patch Set 6 : RenderTableRow::recomputeOverflow #

Total comments: 8

Patch Set 7 : #

Created: 6 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+175 lines, -21 lines)			Patch
A	LayoutTests/fast/table/outline-change-in-table-cell.html	View	1 2 3 4 5 6	1 chunk	+21 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/table/outline-change-in-table-cell-expected.txt	View	1 2 3 4 5 6	1 chunk	+21 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/table/table-cell-outline-change.html	View	1 2 3 4	1 chunk	+22 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/table/table-cell-outline-change-expected.txt	View	1 2 3 4	1 chunk	+20 lines, -0 lines	0 comments	Download
M	Source/core/rendering/RenderBlock.h	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/RenderBlock.cpp	View	1 2 3 4 5 6	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/core/rendering/RenderObject.h	View	1 2 3 4 5 6	3 chunks	+5 lines, -3 lines	0 comments	Download
M	Source/core/rendering/RenderObject.cpp	View	1 2 3 4 5 6	2 chunks	+13 lines, -6 lines	0 comments	Download
M	Source/core/rendering/RenderTable.h	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/rendering/RenderTable.cpp	View	1 2 3 4 5 6	1 chunk	+14 lines, -0 lines	0 comments	Download
M	Source/core/rendering/RenderTableRow.h	View	1 2 3 4 5	1 chunk	+3 lines, -1 line	0 comments	Download
M	Source/core/rendering/RenderTableRow.cpp	View	1 2 3 4 5	1 chunk	+9 lines, -0 lines	0 comments	Download
M	Source/core/rendering/RenderTableSection.h	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/rendering/RenderTableSection.cpp	View	1 2 3 4 5 6	6 chunks	+42 lines, -8 lines	0 comments	Download

Messages

Total messages: 18 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Julien - ping for review

https://codereview.chromium.org/744493002/diff/40001/LayoutTests/fast/table/outline-change-in-table-cell-expected.txt File LayoutTests/fast/table/outline-change-in-table-cell-expected.txt (right): https://codereview.chromium.org/744493002/diff/40001/LayoutTests/fast/table/outline-change-in-table-cell-expected.txt#newcode16 LayoutTests/fast/table/outline-change-in-table-cell-expected.txt:16: [21, 129, 140, 140], That's a lot of invalidations ...

6 years, 1 month ago (2014-11-22 02:35:04 UTC) #6

Xianzhu

6 years ago (2014-11-24 17:23:00 UTC) #7

https://codereview.chromium.org/744493002/diff/40001/LayoutTests/fast/table/o...
File LayoutTests/fast/table/outline-change-in-table-cell-expected.txt (right):

https://codereview.chromium.org/744493002/diff/40001/LayoutTests/fast/table/o...
LayoutTests/fast/table/outline-change-in-table-cell-expected.txt:16: [21, 129,
140, 140],
On 2014/11/22 02:35:04, Julien Chaffraix - PST wrote:
> That's a lot of invalidations when you *know* that only a single cell has
> changed.

These are expected based on current table layout code because cell's overflow
also contributes to overflow of its containing row, section, and table.

https://codereview.chromium.org/744493002/diff/40001/LayoutTests/fast/table/o...
LayoutTests/fast/table/outline-change-in-table-cell-expected.txt:17: [8, 252,
784, 17]
On 2014/11/22 02:35:04, Julien Chaffraix - PST wrote:
> That's a lot of invalidations when we *know* that a single cell has changed in
> the test case.

The overflow change also propagates to <BODY>.

https://codereview.chromium.org/744493002/diff/40001/LayoutTests/fast/table/o...
LayoutTests/fast/table/outline-change-in-table-cell-expected.txt:17: [8, 252,
784, 17]
On 2014/11/22 02:35:04, Julien Chaffraix - PST wrote:
> That's a lot of invalidations when we *know* that a single cell has changed in
> the test case.

This is the overflow change of <BODY>. I can make the table position:absolute to
avoid this.

https://codereview.chromium.org/744493002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/744493002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderTableSection.cpp:1148: }
On 2014/11/22 02:35:04, Julien Chaffraix - PST wrote:
> Recomputing the overflow is now O(columns*rows) :(
> 
> Overflow can only happen from table cells so they should just notify the
section
> that it needs to recompute the overflow.

I think it's
  [O(columns*rows) of cost_of_testing_flag] + [O(N) of
cost_of_recomputing_overflow]
(N = number of cells needing recalc overflow).

If this looks not good, I can reduce the cost to
  [O(rows) + O(columns) of cost_of_testing_flag] + [O(N) of
cost_of_recomputing_overflow]
by also setting childNeedsRecalcOverflow flag on RenderTableRow and
RenderTableSection in markContainingBlocksForOverflowRecalc().

mstensho (USE GERRIT)

https://codereview.chromium.org/744493002/diff/1/Source/core/rendering/RenderTableSection.cpp File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/744493002/diff/1/Source/core/rendering/RenderTableSection.cpp#newcode1138 Source/core/rendering/RenderTableSection.cpp:1138: // Now that our height has been determined, add ...

6 years ago (2014-11-24 18:01:06 UTC) #8

Xianzhu

PTAL. The new Patch Set avoids O(rows*cols) checking of cells' needsOverflowRecalc flags. https://codereview.chromium.org/744493002/diff/1/Source/core/rendering/RenderTableSection.cpp File Source/core/rendering/RenderTableSection.cpp ...

6 years ago (2014-11-24 23:03:28 UTC) #9

Julien - ping for review

https://codereview.chromium.org/744493002/diff/60001/LayoutTests/fast/table/outline-change-in-table-cell.html File LayoutTests/fast/table/outline-change-in-table-cell.html (right): https://codereview.chromium.org/744493002/diff/60001/LayoutTests/fast/table/outline-change-in-table-cell.html#newcode16 LayoutTests/fast/table/outline-change-in-table-cell.html:16: Passes if there is a thick green outline and ...

6 years ago (2014-11-25 18:33:10 UTC) #10

Xianzhu

ptal. https://codereview.chromium.org/744493002/diff/60001/LayoutTests/fast/table/outline-change-in-table-cell.html File LayoutTests/fast/table/outline-change-in-table-cell.html (right): https://codereview.chromium.org/744493002/diff/60001/LayoutTests/fast/table/outline-change-in-table-cell.html#newcode16 LayoutTests/fast/table/outline-change-in-table-cell.html:16: Passes if there is a thick green outline ...

6 years ago (2014-11-25 20:17:13 UTC) #11

ptal.

https://codereview.chromium.org/744493002/diff/60001/LayoutTests/fast/table/o...
File LayoutTests/fast/table/outline-change-in-table-cell.html (right):

https://codereview.chromium.org/744493002/diff/60001/LayoutTests/fast/table/o...
LayoutTests/fast/table/outline-change-in-table-cell.html:16: Passes if there is
a thick green outline and no red.
On 2014/11/25 18:33:10, Julien Chaffraix - PST wrote:
> The outline should be 20px for the test to pass as you're doing 2 outline
> changes.
> 
> Maybe it would be better to just do a thick one, following by a small one
(2px)
> so that you can say that the test passes with a thin outline.

Good catch. I changed the intermediate color to yellow to distinguish. 2px makes
just 1px difference between the good and bad results which seems easy to be
ignored by future rebaseliners when regression occurs.

https://codereview.chromium.org/744493002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/744493002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1623: }
On 2014/11/25 18:33:10, Julien Chaffraix - PST wrote:
> :(
> 
> Wouldn't container() work without needing this logic?
> 

This seems not the best because we'll set the flag on unnecessary objects which
won't handle the flag, and leave the flag uncleared.

How about renaming the function to markContainersForOverflowRecalcIfNeeded()
which reflect the fact that we are not marking all containers.

> Also what you really need to do is propagate the needs-overflow-computation
flag
> from RenderTableCell::styleDidChange. That would require a lot less
> modifications.

styleDidChange doesn't work for the test case because
RenderTableCell::styleDidChange won't be called when its descendant needs
overflow recalc.

https://codereview.chromium.org/744493002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderTable.cpp (right):

https://codereview.chromium.org/744493002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderTable.cpp:404: childrenOverflowChanged = true;
On 2014/11/25 18:33:10, Julien Chaffraix - PST wrote:
> This is better written:
> 
> childrenOverflowChanged |= section->childNeedsOverflowRecalcAfterStyleChange()
> && section->recalcChildOverflowAfterStyleChange();
> 
> However here, you don't need to continue if it's true:
> 
> if (section->childNeedsOverflowRecalcAfterStyleChange() &&
> section->recalcChildOverflowAfterStyleChange())
>     return true;

We still need to continue the loop to call recalcChildOverflowAfterStyleChange
for other sections.

It seems that embedding section->recalcChildOverflowAfterStyleChange() in a
complex boolean expression let us think we are just calculating the boolean
result and overlook the real action.

Would this be better:

        if (!section->childNeedsOverflowRecalcAfterStyleChange())
            continue;
        if (section->recalcChildOverflowAfterStyleChange())
            childrenOverflowChanged = true;

https://codereview.chromium.org/744493002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/744493002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderTableSection.cpp:1094: m_overflowingCells.clear();
On 2014/11/25 18:33:10, Julien Chaffraix - PST wrote:
> We were clearing the overflowing cells early to mitigate any stale pointer. By
> moving it here, you're losing this defensive pattern.

Restored the original one.

Julien - ping for review

https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/RenderObject.cpp File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/RenderObject.cpp#newcode1622 Source/core/rendering/RenderObject.cpp:1622: return object->isTablePart() ? object->parent() : object->containingBlock(); You don't care ...

6 years ago (2014-11-26 19:23:34 UTC) #12

https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1622: return object->isTablePart() ?
object->parent() : object->containingBlock();
You don't care about table caption so that check is not entirely true. Also as
the only renderer that's a block (apart from RenderTableCaption) is
RenderTableCell so using this function hides the real intent :(

I really think we should just inline the check for table part:

for (RenderObject* container = containerForOverflowRecalc(this); container &&
!container->childNeedsOverflowRecalcAfterStyleChange(); container =
containerForOverflowRecalc(container)) {
    // Cell needs to propagate the flag to their containing section and row as
their containing block is the table wrapper.
    // This enables us to only recompute overflow the modified sections / rows.
    if (container->isTableCell()) {
        RenderTableCell* containerCell = toRenderTableCell(container);
        row()->setChildNeedsOverflowRecalcAfterStyleChange();
        section()->setChildNeedsOverflowRecalcAfterStyleChange();
    }
    container->setChildNeedsOverflowRecalcAfterStyleChange();
}

https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderTable.cpp (right):

https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTable.cpp:406: childrenOverflowChanged = true;
The new code is better! I would still keep this on a single line:

childrenOverflowChanged = section->recalcChildOverflowAfterStyleChange();

https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTableSection.cpp:1168: if (childrenOverflowChanged)
If we changed only overflow for the second (think change an outline / filter /
box-shadow on the section itself), then we won't recompute the overflow? It
seems to be an existing bug in the code that we don't call
addVisualEffectOverflow() so it's fine to put a FIXME about it.

https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTableSection.cpp:1169:
computeOverflowFromCells(totalRows, numEffCols);

While the performance is better, you will still blow away the computed overflow
for rows / cells that haven't changed. That's caused by keeping only a single
bit of information about what changed. As child needs overflow recalc is set, I
would assume we should *always* call computeOverflowFromCell and we can just
lazily recompute their overflow based on their booleans.

Xianzhu

6 years ago (2014-11-26 20:53:31 UTC) #13

https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1622: return object->isTablePart() ?
object->parent() : object->containingBlock();
On 2014/11/26 19:23:33, Julien Chaffraix - PST wrote:
> You don't care about table caption so that check is not entirely true. Also as
> the only renderer that's a block (apart from RenderTableCaption) is
> RenderTableCell so using this function hides the real intent :(
> 
> I really think we should just inline the check for table part:
> 
> for (RenderObject* container = containerForOverflowRecalc(this); container &&
> !container->childNeedsOverflowRecalcAfterStyleChange(); container =
> containerForOverflowRecalc(container)) {
>     // Cell needs to propagate the flag to their containing section and row as
> their containing block is the table wrapper.
>     // This enables us to only recompute overflow the modified sections /
rows.
>     if (container->isTableCell()) {
>         RenderTableCell* containerCell = toRenderTableCell(container);
>         row()->setChildNeedsOverflowRecalcAfterStyleChange();
>         section()->setChildNeedsOverflowRecalcAfterStyleChange();
>     }
>     container->setChildNeedsOverflowRecalcAfterStyleChange();
> }

Avoided the containerForOverflowRecalc() with a slightly different method of the
above.
The above misses 2 corner cases:
- when this->isTableCell()
- when a table row changes overflow (though still buggy now. Will fix the
remaining issue for crbug.com/437012).

Noticed that we should actually mark all containing boxes (not just containg
blocks) childNeedsOverflowRecalc because the boxes should recompute overflow
when any child's overflow changed. crbug.com/437012.

https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTableSection.cpp:1168: if (childrenOverflowChanged)
On 2014/11/26 19:23:34, Julien Chaffraix - PST wrote:
> If we changed only overflow for the second (think change an outline / filter /
> box-shadow on the section itself), then we won't recompute the overflow? It
> seems to be an existing bug in the code that we don't call
> addVisualEffectOverflow() so it's fine to put a FIXME about it.

Done.

Actually we never call addVisualEffectOverflow() for RenderTableSection (as well
as RenderTableRow), because RenderTableSection is a RenderBox, but
addVisualEffectOverflow() is only called from RenderBlock.

I'm working on a change to move RenderBlock::recomputeOverflow and
RenderBlock::recalcOverflowAfterStyleChange etc. into RenderBox so that
RenderBoxes can also recompute their overflows when needed. With that change,
computeOverflowFromCells will be part of recomputeOverflow which will be called
from from recalcOverflowAfterStyleChange() when
recalcChildOverflowAfterStyleChange() returns true.

https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTableSection.cpp:1169:
computeOverflowFromCells(totalRows, numEffCols);
On 2014/11/26 19:23:34, Julien Chaffraix - PST wrote:
> 
> While the performance is better, you will still blow away the computed
overflow
> for rows / cells that haven't changed. That's caused by keeping only a single
> bit of information about what changed. As child needs overflow recalc is set,
I
> would assume we should *always* call computeOverflowFromCell and we can just
> lazily recompute their overflow based on their booleans.

Thought of a similar method, but considering the following complexities, this
seems not feasible because we keep only one accumulated visual overflow rect
(for each of the rows and sections):
- when a cell's overflow shrinks;
- when a cell's overflow changes by expanding at one side and shrinking at
another side.

In the above cases, we don't know how to incrementally update the accumulated
overflow rect of the section. It seems the only way to fully recompute the
overflow.

Julien - ping for review

https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/RenderTableSection.cpp File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/RenderTableSection.cpp#newcode1169 Source/core/rendering/RenderTableSection.cpp:1169: computeOverflowFromCells(totalRows, numEffCols); On 2014/11/26 20:53:30, Xianzhu wrote: > On ...

6 years ago (2014-11-26 23:21:25 UTC) #14

https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTableSection.cpp:1169:
computeOverflowFromCells(totalRows, numEffCols);
On 2014/11/26 20:53:30, Xianzhu wrote:
> On 2014/11/26 19:23:34, Julien Chaffraix - PST wrote:
> > 
> > While the performance is better, you will still blow away the computed
> overflow
> > for rows / cells that haven't changed. That's caused by keeping only a
single
> > bit of information about what changed. As child needs overflow recalc is
set,
> I
> > would assume we should *always* call computeOverflowFromCell and we can just
> > lazily recompute their overflow based on their booleans.
> 
> Thought of a similar method, but considering the following complexities, this
> seems not feasible because we keep only one accumulated visual overflow rect
> (for each of the rows and sections):
> - when a cell's overflow shrinks;
> - when a cell's overflow changes by expanding at one side and shrinking at
> another side.
> 
> In the above cases, we don't know how to incrementally update the accumulated
> overflow rect of the section. It seems the only way to fully recompute the
> overflow.

By incremental, I mean we have to recompute the full rect but we don't have to
recompute the rows / cells that haven't been marked for overflow computation.

https://codereview.chromium.org/744493002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/744493002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderTableSection.cpp:949: m_overflowingCells.clear();
I missed that layoutRows calls computeOverflowFromCells so it makes sense just
to move all the overflow-related logic there.

https://codereview.chromium.org/744493002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderTableSection.cpp:1124: // FIXME: this misses visual
effect overflow of of the rowRenderer. crbug.com/437012.
Note that RenderTableRow::layout() actually calls addVisualEffectOverflow() so
we should centralize the re-computation of overflow as part of crbug.com/437012.

https://codereview.chromium.org/744493002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderTableSection.cpp:1157:
rowRenderer->clearChildNeedsOverflowRecalcAfterStyleChange();
Do we check somewhere that this is cleared for every renderer in the tree?

https://codereview.chromium.org/744493002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderTableSection.cpp:1169: // crbug.com/437012.
I don't think we should do it this way: we should fix the computeOverflow
situation and then this change would land without nefarious FIXMEs and
performance degradations.

Xianzhu

6 years ago (2014-11-27 00:28:48 UTC) #15

https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/744493002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTableSection.cpp:1169:
computeOverflowFromCells(totalRows, numEffCols);
On 2014/11/26 23:21:25, Julien Chaffraix - PST wrote:
> On 2014/11/26 20:53:30, Xianzhu wrote:
> > On 2014/11/26 19:23:34, Julien Chaffraix - PST wrote:
> > > 
> > > While the performance is better, you will still blow away the computed
> > overflow
> > > for rows / cells that haven't changed. That's caused by keeping only a
> single
> > > bit of information about what changed. As child needs overflow recalc is
> set,
> > I
> > > would assume we should *always* call computeOverflowFromCell and we can
just
> > > lazily recompute their overflow based on their booleans.
> > 
> > Thought of a similar method, but considering the following complexities,
this
> > seems not feasible because we keep only one accumulated visual overflow rect
> > (for each of the rows and sections):
> > - when a cell's overflow shrinks;
> > - when a cell's overflow changes by expanding at one side and shrinking at
> > another side.
> > 
> > In the above cases, we don't know how to incrementally update the
accumulated
> > overflow rect of the section. It seems the only way to fully recompute the
> > overflow.
> 
> By incremental, I mean we have to recompute the full rect but we don't have to
> recompute the rows / cells that haven't been marked for overflow computation.

computeOverflowFromCells() doesn't recompute overflow for cells. It does
recompute overflow for all rows. Extracted RenderTableRow::recomputeOverflow()
from the original place and call it if rowChildrenOverflowChanged. Then
computeOverflowFromCells() just recomputes overflow of section itself (will be
RenderTableSection::recomputeOverflow() for crbug.com/437012).

https://codereview.chromium.org/744493002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/744493002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderTableSection.cpp:949: m_overflowingCells.clear();
On 2014/11/26 23:21:25, Julien Chaffraix - PST wrote:
> I missed that layoutRows calls computeOverflowFromCells so it makes sense just
> to move all the overflow-related logic there.

Acknowledged.

https://codereview.chromium.org/744493002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderTableSection.cpp:1124: // FIXME: this misses visual
effect overflow of of the rowRenderer. crbug.com/437012.
On 2014/11/26 23:21:25, Julien Chaffraix - PST wrote:
> Note that RenderTableRow::layout() actually calls addVisualEffectOverflow() so
> we should centralize the re-computation of overflow as part of
crbug.com/437012.

Acknowledged.

https://codereview.chromium.org/744493002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderTableSection.cpp:1124: // FIXME: this misses visual
effect overflow of of the rowRenderer. crbug.com/437012.
On 2014/11/26 23:21:25, Julien Chaffraix - PST wrote:
> Note that RenderTableRow::layout() actually calls addVisualEffectOverflow() so
> we should centralize the re-computation of overflow as part of
crbug.com/437012.

Acknowledged.

https://codereview.chromium.org/744493002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderTableSection.cpp:1157:
rowRenderer->clearChildNeedsOverflowRecalcAfterStyleChange();
On 2014/11/26 23:21:25, Julien Chaffraix - PST wrote:
> Do we check somewhere that this is cleared for every renderer in the tree?

No for now. Will consider adding it.

https://codereview.chromium.org/744493002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderTableSection.cpp:1169: // crbug.com/437012.
On 2014/11/26 23:21:25, Julien Chaffraix - PST wrote:
> I don't think we should do it this way: we should fix the computeOverflow
> situation and then this change would land without nefarious FIXMEs and
> performance degradations.

Removed FIXME. With extracted RenderTableRow::recomputeOverflow(), there is also
no performance degradations.

Julien - ping for review

lgtm (though the 2 pass architecture makes us do a lot of extra computation) https://codereview.chromium.org/744493002/diff/120001/LayoutTests/fast/table/outline-change-in-table-cell.html ...

6 years ago (2014-12-03 19:44:36 UTC) #16

Xianzhu

https://codereview.chromium.org/744493002/diff/120001/LayoutTests/fast/table/outline-change-in-table-cell.html File LayoutTests/fast/table/outline-change-in-table-cell.html (right): https://codereview.chromium.org/744493002/diff/120001/LayoutTests/fast/table/outline-change-in-table-cell.html#newcode15 LayoutTests/fast/table/outline-change-in-table-cell.html:15: Passes if there is a thick green outline and ...

6 years ago (2014-12-03 22:56:35 UTC) #17

Xianzhu

6 years ago (2014-12-04 16:55:54 UTC) #18

This patch needs submitting only after we reapply the
outline-change-optimization change (reverted by
https://codereview.chromium.org/747493002). Tried with it reapplied and with
this change, I still found some bugs about overflows caused by outline-change,
so I will postpone reapplying that change and this until all of the bugs
resolved.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages