Move a bunch more painting code out of RenderBoxModelObject and into BoxPainter.

Move a bunch more painting code out of RenderBoxModelObject and into BoxPainter.

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

[chrishtr, 2014-09-11 20:35:58]

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

[pdr., 2014-09-11 21:00:39]

Another great code move. LGTM with two questions.

https://codereview.chromium.org/564973002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderFieldset.cpp (right):

https://codereview.chromium.org/564973002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderFieldset.cpp:192:
BoxPainter(*this).paintBorder(paintInfo, paintRect, style());
Can you use the boxPainter local from above?

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

https://codereview.chromium.org/564973002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderTableCell.cpp:1213:
BoxPainter(*this).paintBorder(paintInfo, paintRect, style());
Similarly here.

[chrishtr, 2014-09-11 21:26:35]

Good catches.

https://codereview.chromium.org/564973002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderFieldset.cpp (right):

https://codereview.chromium.org/564973002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderFieldset.cpp:192:
BoxPainter(*this).paintBorder(paintInfo, paintRect, style());
On 2014/09/11 21:00:39, pdr wrote:
> Can you use the boxPainter local from above?

Done.

[jbroman, 2014-09-11 21:31:36]

drive-by: caught one trivial thing while skimming this

https://codereview.chromium.org/564973002/diff/40001/Source/core/rendering/In...
File Source/core/rendering/InlineFlowBox.cpp (right):

https://codereview.chromium.org/564973002/diff/40001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1231: ASSERT(!renderer()->isText);
ASSERT(!renderer()->isText());

[jbroman, 2014-09-11 21:32:43]

https://codereview.chromium.org/564973002/diff/40001/Source/core/rendering/In...
File Source/core/rendering/InlineFlowBox.cpp (right):

https://codereview.chromium.org/564973002/diff/40001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1231: ASSERT(!renderer()->isText);
On 2014/09/11 21:31:36, jbroman wrote:
> ASSERT(!renderer()->isText());

Actually, this returns RenderObject&.

You actually want, here and elsewhere:

ASSERT(!renderer().isText());

[pdr., 2014-09-11 22:01:37]

https://codereview.chromium.org/564973002/diff/40001/Source/core/rendering/In...
File Source/core/rendering/InlineFlowBox.cpp (right):

https://codereview.chromium.org/564973002/diff/40001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1231: ASSERT(!renderer()->isText);
On 2014/09/11 21:32:42, jbroman wrote:
> On 2014/09/11 21:31:36, jbroman wrote:
> > ASSERT(!renderer()->isText());
> 
> Actually, this returns RenderObject&.
> 
> You actually want, here and elsewhere:
> 
> ASSERT(!renderer().isText());

toRenderBox already contains this assert.

[chrishtr, 2014-09-12 17:01:10]

On 2014/09/11 at 22:01:37, pdr wrote:
>
https://codereview.chromium.org/564973002/diff/40001/Source/core/rendering/In...
> File Source/core/rendering/InlineFlowBox.cpp (right):
> 
>
https://codereview.chromium.org/564973002/diff/40001/Source/core/rendering/In...
> Source/core/rendering/InlineFlowBox.cpp:1231: ASSERT(!renderer()->isText);
> On 2014/09/11 21:32:42, jbroman wrote:
> > On 2014/09/11 21:31:36, jbroman wrote:
> > > ASSERT(!renderer()->isText());
> > 
> > Actually, this returns RenderObject&.
> > 
> > You actually want, here and elsewhere:
> > 
> > ASSERT(!renderer().isText());
> 
> toRenderBox already contains this assert.

Fixed.

[chrishtr, 2014-09-12 18:00:15]

I made several more methods static, similar to what I did in the previous CL.
Shortly I will get to cleaning up the resulting static/non-static semi-craziness
in BoxPainter, but not in this patch.

https://codereview.chromium.org/564973002/diff/40001/Source/core/rendering/In...
File Source/core/rendering/InlineFlowBox.cpp (right):

https://codereview.chromium.org/564973002/diff/40001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1231: ASSERT(!renderer()->isText);
On 2014/09/11 21:31:36, jbroman wrote:
> ASSERT(!renderer()->isText());

Done.

https://codereview.chromium.org/564973002/diff/40001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1231: ASSERT(!renderer()->isText);
On 2014/09/11 21:32:42, jbroman wrote:
> On 2014/09/11 21:31:36, jbroman wrote:
> > ASSERT(!renderer()->isText());
> 
> Actually, this returns RenderObject&.
> 
> You actually want, here and elsewhere:
> 
> ASSERT(!renderer().isText());

Done.

[pdr., 2014-09-12 18:23:33]

LGTM2 (with or without my friendly suggestion)

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

https://codereview.chromium.org/564973002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1084: // FIXME: make this not public.
We could leave this as protected and use a friend class.

[chrishtr, 2014-09-12 18:30:41]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-09-12 18:31:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/564973002/80001

[commit-bot: I haz the power, 2014-09-12 20:54:18]

Committed patchset #5 (id:80001) as 181930