Move painting code for tables into *Paint classes.

Move painting code for tables into *Paint classes.

BUG=412088
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182356

[chrishtr, 2014-09-18 23:57:05]

chrishtr@chromium.org changed reviewers:
+ pdr@chromium.org, schenney@chromium.org

[mstensho (USE GERRIT), 2014-09-19 10:50:11]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2014-09-19 10:50:12]

https://codereview.chromium.org/576823004/diff/80001/Source/core/paint/TableP...
File Source/core/paint/TablePainter.cpp (right):

https://codereview.chromium.org/576823004/diff/80001/Source/core/paint/TableP...
Source/core/paint/TablePainter.cpp:112: 
Double blank line.

https://codereview.chromium.org/576823004/diff/80001/Source/core/paint/TableP...
File Source/core/paint/TablePainter.h (right):

https://codereview.chromium.org/576823004/diff/80001/Source/core/paint/TableP...
Source/core/paint/TablePainter.h:8: #include "core/rendering/RenderTable.h"
Forward-declaring RenderTable should do.

https://codereview.chromium.org/576823004/diff/80001/Source/core/paint/TableR...
File Source/core/paint/TableRowPainter.h (right):

https://codereview.chromium.org/576823004/diff/80001/Source/core/paint/TableR...
Source/core/paint/TableRowPainter.h:8: #include
"core/rendering/RenderTableRow.h"
Forward-declaring RenderTableRow should do.

https://codereview.chromium.org/576823004/diff/80001/Source/core/paint/TableS...
File Source/core/paint/TableSectionPainter.h (right):

https://codereview.chromium.org/576823004/diff/80001/Source/core/paint/TableS...
Source/core/paint/TableSectionPainter.h:8: #include
"core/rendering/RenderTableSection.h"
Forward-declaring RenderTableSection should do.

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

https://codereview.chromium.org/576823004/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTable.h:248: void setCurrentBorderValue(const
CollapsedBorderValue* val) { m_currentBorder = val; }
m_currentBorder is for painting. So am I right in thinking that it doesn't
really belong in a renderer class? I'm wondering if you should add a FIXME for
this.

https://codereview.chromium.org/576823004/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTable.h:273: virtual void
paintBoxDecorationBackground(PaintInfo&, const LayoutPoint&) OVERRIDE;
If the above comment only refers to this method, I think there should be a blank
line after this declaration.

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

https://codereview.chromium.org/576823004/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTableCell.cpp:995: RenderBlock::paint(paintInfo,
paintOffset);
Why this change? While there's currently no implementation of
RenderBlockFlow::paint(), I think it's good practice to call the super class
unless there are explicit reasons to avoid it.

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

https://codereview.chromium.org/576823004/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTableSection.h:228: LayoutRect
logicalRectForWritingModeAndDirection(const LayoutRect&) const;
Shouldn't there be a blank line after this to make it clearer that the comment
above is only for this method?

[chrishtr, 2014-09-19 16:35:24]

https://codereview.chromium.org/576823004/diff/80001/Source/core/paint/TableP...
File Source/core/paint/TablePainter.cpp (right):

https://codereview.chromium.org/576823004/diff/80001/Source/core/paint/TableP...
Source/core/paint/TablePainter.cpp:112: 
On 2014/09/19 10:50:11, mstensho wrote:
> Double blank line.

Done.

https://codereview.chromium.org/576823004/diff/80001/Source/core/paint/TableR...
File Source/core/paint/TableRowPainter.h (right):

https://codereview.chromium.org/576823004/diff/80001/Source/core/paint/TableR...
Source/core/paint/TableRowPainter.h:8: #include
"core/rendering/RenderTableRow.h"
On 2014/09/19 10:50:11, mstensho wrote:
> Forward-declaring RenderTableRow should do.

Done.

https://codereview.chromium.org/576823004/diff/80001/Source/core/paint/TableS...
File Source/core/paint/TableSectionPainter.h (right):

https://codereview.chromium.org/576823004/diff/80001/Source/core/paint/TableS...
Source/core/paint/TableSectionPainter.h:8: #include
"core/rendering/RenderTableSection.h"
On 2014/09/19 10:50:11, mstensho wrote:
> Forward-declaring RenderTableSection should do.

Done.

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

https://codereview.chromium.org/576823004/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTable.h:248: void setCurrentBorderValue(const
CollapsedBorderValue* val) { m_currentBorder = val; }
On 2014/09/19 10:50:11, mstensho wrote:
> m_currentBorder is for painting. So am I right in thinking that it doesn't
> really belong in a renderer class? I'm wondering if you should add a FIXME for
> this.

Done.

https://codereview.chromium.org/576823004/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTable.h:273: virtual void
paintBoxDecorationBackground(PaintInfo&, const LayoutPoint&) OVERRIDE;
On 2014/09/19 10:50:11, mstensho wrote:
> If the above comment only refers to this method, I think there should be a
blank
> line after this declaration.

Done.

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

https://codereview.chromium.org/576823004/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTableCell.cpp:995: RenderBlock::paint(paintInfo,
paintOffset);
On 2014/09/19 10:50:11, mstensho wrote:
> Why this change? While there's currently no implementation of
> RenderBlockFlow::paint(), I think it's good practice to call the super class
> unless there are explicit reasons to avoid it.

I want it to be explicit, so that I can more easily get this whole code out of
here once everything is in the core/paint/ directory.

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

https://codereview.chromium.org/576823004/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTableSection.h:228: LayoutRect
logicalRectForWritingModeAndDirection(const LayoutRect&) const;
On 2014/09/19 10:50:11, mstensho wrote:
> Shouldn't there be a blank line after this to make it clearer that the comment
> above is only for this method?

Done.

[mstensho (USE GERRIT), 2014-09-19 18:08:55]

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

https://codereview.chromium.org/576823004/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTableCell.cpp:995: RenderBlock::paint(paintInfo,
paintOffset);
On 2014/09/19 16:35:24, chrishtr wrote:
> On 2014/09/19 10:50:11, mstensho wrote:
> > Why this change? While there's currently no implementation of
> > RenderBlockFlow::paint(), I think it's good practice to call the super class
> > unless there are explicit reasons to avoid it.
> 
> I want it to be explicit, so that I can more easily get this whole code out of
> here once everything is in the core/paint/ directory.

But a table cell IS a RenderBlockFlow, and it may contain things like floats and
lines.

[chrishtr, 2014-09-19 18:14:36]

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

https://codereview.chromium.org/576823004/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTableCell.cpp:995: RenderBlock::paint(paintInfo,
paintOffset);
On 2014/09/19 18:08:55, mstensho wrote:
> On 2014/09/19 16:35:24, chrishtr wrote:
> > On 2014/09/19 10:50:11, mstensho wrote:
> > > Why this change? While there's currently no implementation of
> > > RenderBlockFlow::paint(), I think it's good practice to call the super
class
> > > unless there are explicit reasons to avoid it.
> > 
> > I want it to be explicit, so that I can more easily get this whole code out
of
> > here once everything is in the core/paint/ directory.
> 
> But a table cell IS a RenderBlockFlow, and it may contain things like floats
and
> lines.

I don't care that much, so reverted this change.

[pdr., 2014-09-19 20:51:37]

LGTM

https://codereview.chromium.org/576823004/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderTableSection.h (right):

https://codereview.chromium.org/576823004/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderTableSection.h:33: extern float
gMaxAllowedOverflowingCellRatioForFastPaintPath;
I think this can be: const float gMaxAllowedOverflowingCellRatioForFastPaintPath
= 0.1f;

[chrishtr, 2014-09-19 20:59:42]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2014-09-19 20:59:43]

https://codereview.chromium.org/576823004/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderTableSection.h (right):

https://codereview.chromium.org/576823004/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderTableSection.h:33: extern float
gMaxAllowedOverflowingCellRatioForFastPaintPath;
On 2014/09/19 20:51:37, pdr wrote:
> I think this can be: const float
gMaxAllowedOverflowingCellRatioForFastPaintPath
> = 0.1f;

Done.

[commit-bot: I haz the power, 2014-09-19 21:00:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/576823004/160001

[commit-bot: I haz the power, 2014-09-19 22:45:08]

Committed patchset #9 (id:160001) as 182356