Refactor background painting behind table cells

Refactor background painting behind table cells

This is a preparation for adding box-shadow support for table rows
and sections.

Also improve performance by moving some common conditions out of
inner loops.

BUG=94871
Committed: https://crrev.com/9491e3bf40943622ff32cf0eda416dd6232b563f
Cr-Commit-Position: refs/heads/master@{#394280}

[Xianzhu, 2016-05-16 21:47:52]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org

[Xianzhu, 2016-05-16 21:47:53]

This is separated from https://codereview.chromium.org/1963313002/. Ptal.

https://codereview.chromium.org/1983853003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (left):

https://codereview.chromium.org/1983853003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:56: // Use the visual
overflow rect here, because it will include overflow introduced by the theme.
On 2016/05/14 08:56:14, chrishtr wrote:
> This comment is incorrect somehow?

The comment doesn't look to make sense to me. We definitely should use
visualOverflowRect, but it's not (just) because it includes theme overflow.

https://codereview.chromium.org/1983853003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TableCellPainter.cpp (right):

https://codereview.chromium.org/1983853003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableCellPainter.cpp:143:
paintBackground(paintInfo, paintRect, backgroundObject);
On 2016/05/14 18:03:46, chrishtr wrote:
> Why not paintContainerBackgroundBehindCell? The old code
> called paintBackgroundsBehindCell.

Previously paintBackgroundsBehindCell was like:
{
   if (isPaintingContainerBackgroundsBehindCell) {
       ...
   }

   code to paint background;
}

Now the latter part is extracted into paintBackground() so here we can call it
directly.

https://codereview.chromium.org/1983853003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TableRowPainter.h (right):

https://codereview.chromium.org/1983853003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableRowPainter.h:21: void
paintBackgroundBehindCell(const LayoutTableCell&, const PaintInfo&, const
LayoutPoint&);
On 2016/05/14 18:03:46, chrishtr wrote:
> Doesn't a cell know its row? Why is this method not on
> TableCellPainter?

It calls TableCellPainter::paintContainerBackgroundBehindCell() after setting up
the paint offset in LayoutTableRow's specific way.

I could move this into TableCellPainter
(TableCellPainter::paintRowBackgroundBehindCell()) but it seems better to put it
here because it contains LayoutTableRow-specific code.

https://codereview.chromium.org/1983853003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (left):

https://codereview.chromium.org/1983853003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:182: return;
On 2016/05/14 18:03:46, chrishtr wrote:
> Why is this code gone?

The caller of paintCell() has ensured this. Removing this reduces computations
in inner loop.

I would not add DCHECK() here because there might be small differences between
the conditions of callers and here about spanned or overlapping cells, and the
difference is not important because it doesn't matter if we paint a few more
cells out of the paint rect in the corner cases.

https://codereview.chromium.org/1983853003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TableSectionPainter.h (right):

https://codereview.chromium.org/1983853003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.h:39: const
LayoutTableCell* primaryCellToPaint(unsigned row, unsigned column, const
CellSpan& dirtiedRows, const CellSpan& dirtiedColumns) const;
On 2016/05/14 18:03:46, chrishtr wrote:
> This needs a comment.

Done.

[chrishtr, 2016-05-17 21:22:19]

https://codereview.chromium.org/1983853003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (left):

https://codereview.chromium.org/1983853003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:182: return;
On 2016/05/16 at 21:47:53, Xianzhu wrote:
> On 2016/05/14 18:03:46, chrishtr wrote:
> > Why is this code gone?
> 
> The caller of paintCell() has ensured this. Removing this reduces computations
> in inner loop.

Can you show me where? I can't see it yet.

> 
> I would not add DCHECK() here because there might be small differences between
> the conditions of callers and here about spanned or overlapping cells, and the
> difference is not important because it doesn't matter if we paint a few more
> cells out of the paint rect in the corner cases.

[Xianzhu, 2016-05-17 21:49:57]

https://codereview.chromium.org/1983853003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (left):

https://codereview.chromium.org/1983853003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:182: return;
On 2016/05/17 21:22:19, chrishtr wrote:
> On 2016/05/16 at 21:47:53, Xianzhu wrote:
> > On 2016/05/14 18:03:46, chrishtr wrote:
> > > Why is this code gone?
> > 
> > The caller of paintCell() has ensured this. Removing this reduces
computations
> > in inner loop.
> 
> Can you show me where? I can't see it yet.
> 
> > 
> > I would not add DCHECK() here because there might be small differences
between
> > the conditions of callers and here about spanned or overlapping cells, and
the
> > difference is not important because it doesn't matter if we paint a few more
> > cells out of the paint rect in the corner cases.
> 

See new line 121-127, which calculates dirtiedRows and dirtiedColumns based on
the paint rect. Then only the cells intersecting dirtiedRows and dirtiedColumns
are painted in the following loops.

[chrishtr, 2016-05-17 21:54:04]

lgtm

[Xianzhu, 2016-05-17 21:55:54]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-05-17 21:56:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983853003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1983853003/20001

[commit-bot: I haz the power, 2016-05-18 00:17:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-18 00:19:25]

Description was changed from

==========
Refactor background painting behind table cells

This is a preparation for adding box-shadow support for table rows
and sections.

Also improve performance by moving some common conditions out of
inner loops.

BUG=94871
==========

to

==========
Refactor background painting behind table cells

This is a preparation for adding box-shadow support for table rows
and sections.

Also improve performance by moving some common conditions out of
inner loops.

BUG=94871

Committed: https://crrev.com/9491e3bf40943622ff32cf0eda416dd6232b563f
Cr-Commit-Position: refs/heads/master@{#394280}
==========

[commit-bot: I haz the power, 2016-05-18 00:19:28]

Patchset 2 (id:??) landed as
https://crrev.com/9491e3bf40943622ff32cf0eda416dd6232b563f
Cr-Commit-Position: refs/heads/master@{#394280}