Let table rows and sections paint their backgrounds in proper paint phases

Let table rows and sections paint their backgrounds in proper paint phases

Previously they ignored PaintPhaseSelfBlockBackgroundOnly and painted
their own backgrounds in PaintPhaseDescendantBlockBackgroundsOnly behind
cell. Now paint their own backgrounds in PaintPhaseBlockBackground or
PaintPhaseSelfBlockBackgroundOnly phase to be consistent with other
painters.

BUG=574938
TEST=PaintLayerPainterTest.PaintPhaseBlockBackground

Committed: https://crrev.com/65b7f83a0ccfd3a1329f35198a9f5a1c8efb501b
Cr-Commit-Position: refs/heads/master@{#372171}

[Xianzhu, 2016-01-28 00:13:44]

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

[chrishtr, 2016-01-28 00:35:14]

Description was changed from

==========
Let table rows and sections paint their backgrounds in proper paint phases

Previously they ignored PaintPhaseSelfBlockBackgroundOnly and painted
their own backgrounds in PaintPhaseDescendantBlockBackgroundsOnly behind
cell. Now paint their own backgrounds in PaintPhaseBlockBackground or
PaintPhaseSelfBlockBackgroundOnly phase to keep consistence with other
painters.

BUG=574938
TEST=PaintLayerPainterTest.PaintPhaseBlockBackground
==========

to

==========
Let table rows and sections paint their backgrounds in proper paint phases

Previously they ignored PaintPhaseSelfBlockBackgroundOnly and painted
their own backgrounds in PaintPhaseDescendantBlockBackgroundsOnly behind
cell. Now paint their own backgrounds in PaintPhaseBlockBackground or
PaintPhaseSelfBlockBackgroundOnly phase to be consistent with other
painters.

BUG=574938
TEST=PaintLayerPainterTest.PaintPhaseBlockBackground
==========

[chrishtr, 2016-01-28 18:14:33]

https://codereview.chromium.org/1646743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (left):

https://codereview.chromium.org/1646743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:31: if
(paintInfo.phase == PaintPhaseSelfBlockBackgroundOnly)
How did this work? I don't see calls into table cells painting the background,
in the code before this patch. Is the comment
incorrect?

[Xianzhu, 2016-01-28 18:34:36]

https://codereview.chromium.org/1646743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (left):

https://codereview.chromium.org/1646743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:31: if
(paintInfo.phase == PaintPhaseSelfBlockBackgroundOnly)
On 2016/01/28 18:14:33, chrishtr wrote:
> How did this work? I don't see calls into table cells painting the background,
> in the code before this patch. Is the comment
> incorrect?

This method is called from PaintLayerPainter::paintLayerContents() via
PaintLayerPainter::paintBackgroundForFragments() and LayoutObject::paint()
interface when the table cell is self painting layer. The comment is correct.

[Xianzhu, 2016-01-28 18:51:09]

On 2016/01/28 18:34:36, Xianzhu wrote:
>
https://codereview.chromium.org/1646743002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (left):
> 
>
https://codereview.chromium.org/1646743002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:31: if
> (paintInfo.phase == PaintPhaseSelfBlockBackgroundOnly)
> On 2016/01/28 18:14:33, chrishtr wrote:
> > How did this work? I don't see calls into table cells painting the
background,
> > in the code before this patch. Is the comment
> > incorrect?
> 
> This method is called from PaintLayerPainter::paintLayerContents() via
> PaintLayerPainter::paintBackgroundForFragments() and LayoutObject::paint()
> interface when the table cell is self painting layer. The comment is correct.

P.S.

The above also applies to self-painting table sections and rows.
Backgrounds of non-self-painting table sections and rows are painted by
TablePainter.

[chrishtr, 2016-01-28 18:53:26]

Let me put it another way: why was the code written the other way? Was there
some reason?

https://codereview.chromium.org/1646743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (left):

https://codereview.chromium.org/1646743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:31: if
(paintInfo.phase == PaintPhaseSelfBlockBackgroundOnly)
On 2016/01/28 at 18:34:36, Xianzhu wrote:
> On 2016/01/28 18:14:33, chrishtr wrote:
> > How did this work? I don't see calls into table cells painting the
background,
> > in the code before this patch. Is the comment
> > incorrect?
> 
> This method is called from PaintLayerPainter::paintLayerContents() via
PaintLayerPainter::paintBackgroundForFragments() and LayoutObject::paint()
interface when the table cell is self painting layer. The comment is correct.

TableSectionPainter::paint is called from LayoutTableSection::paint. Did you
mean when the table section is self-painting?

[Xianzhu, 2016-01-28 19:05:06]

On 2016/01/28 18:53:26, chrishtr wrote:
> Let me put it another way: why was the code written the other way? Was there
> some reason?

The old code (painting table row/section background in
PaintPhaseBlockBackground/PaintPhaseDescendantBlockBackgroundOnly instead of
PaintPhaseSelfBlockBackground) got correct results to, because either way
painted the same thing.

With empty descendant background phase optimization, with the old code, we would
miss painting of a self-painting row/section's background if the row/section
doesn't have descendant backgrounds (needsPaintPhaseDescendantBlockBackgrounds
not set).

>
https://codereview.chromium.org/1646743002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (left):
> 
>
https://codereview.chromium.org/1646743002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:31: if
> (paintInfo.phase == PaintPhaseSelfBlockBackgroundOnly)
> On 2016/01/28 at 18:34:36, Xianzhu wrote:
> > On 2016/01/28 18:14:33, chrishtr wrote:
> > > How did this work? I don't see calls into table cells painting the
> background,
> > > in the code before this patch. Is the comment
> > > incorrect?
> > 
> > This method is called from PaintLayerPainter::paintLayerContents() via
> PaintLayerPainter::paintBackgroundForFragments() and LayoutObject::paint()
> interface when the table cell is self painting layer. The comment is correct.
> 
> TableSectionPainter::paint is called from LayoutTableSection::paint. Did you
> mean when the table section is self-painting?

Yes. I noticed you may have this question so I answered it in #6. :)

[chrishtr, 2016-01-28 19:50:08]

lgtm

[Xianzhu, 2016-01-28 19:51:48]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-01-28 19:51:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1646743002/#ps20001
(title: "Rebase")

[commit-bot: I haz the power, 2016-01-28 19:53:26]

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

[commit-bot: I haz the power, 2016-01-28 21:30:44]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-28 21:31:42]

Description was changed from

==========
Let table rows and sections paint their backgrounds in proper paint phases

Previously they ignored PaintPhaseSelfBlockBackgroundOnly and painted
their own backgrounds in PaintPhaseDescendantBlockBackgroundsOnly behind
cell. Now paint their own backgrounds in PaintPhaseBlockBackground or
PaintPhaseSelfBlockBackgroundOnly phase to be consistent with other
painters.

BUG=574938
TEST=PaintLayerPainterTest.PaintPhaseBlockBackground
==========

to

==========
Let table rows and sections paint their backgrounds in proper paint phases

Previously they ignored PaintPhaseSelfBlockBackgroundOnly and painted
their own backgrounds in PaintPhaseDescendantBlockBackgroundsOnly behind
cell. Now paint their own backgrounds in PaintPhaseBlockBackground or
PaintPhaseSelfBlockBackgroundOnly phase to be consistent with other
painters.

BUG=574938
TEST=PaintLayerPainterTest.PaintPhaseBlockBackground

Committed: https://crrev.com/65b7f83a0ccfd3a1329f35198a9f5a1c8efb501b
Cr-Commit-Position: refs/heads/master@{#372171}
==========

[commit-bot: I haz the power, 2016-01-28 21:31:43]

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