Generalize paint chunks to clips, and implement clips in LayerPainter.

Generalize paint chunks to clips, and implement clips in LayerPainter.

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

[chrishtr, 2014-10-15 18:21:49]

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

[leviw_travelin_and_unemployed, 2014-10-15 18:51:39]

https://codereview.chromium.org/653303003/diff/20001/Source/core/paint/LayerP...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/653303003/diff/20001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:402: if (paintClipRecorder) {
Putting this if everywhere that calls clip seems ugly... Does it not make sense
to hang this on GraphicsContext and have it behave more like how we begin and
end recordings?

https://codereview.chromium.org/653303003/diff/20001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:436: if (paintClipRecorder)
Why is it okay to unbalance this with the calls to restore in the
GraphicsContext?

[chrishtr, 2014-10-16 04:18:19]

Now with cleaner code.

[Stephen Chennney, 2014-10-16 14:23:21]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org

[Stephen Chennney, 2014-10-16 14:23:22]

DsiplayList includes a clip to apply when drawing the display list. Should we
remove that and roll it into a clip chunk? 

The downside is that it would add significantly to the total number of clip
chunks. The upside is that we might be able to optimize better if all the clips
appear in one kind of chunk.

[chrishtr, 2014-10-16 16:27:08]

On 2014/10/16 at 14:23:22, schenney wrote:
> DsiplayList includes a clip to apply when drawing the display list. Should we
remove that and roll it into a clip chunk? 
> 
> The downside is that it would add significantly to the total number of clip
chunks. The upside is that we might be able to optimize better if all the clips
appear in one kind of chunk.

We might/probably need to do that. The clips in this CL definitely need to be
externalized from the paint chunks they clip, since there is more than one paint
chunk. Making clips into a single uniform display item subtype will probably
make the compositor code's job (to maintain clip trees, etc) easier.

[pdr., 2014-10-16 17:38:12]

I think this looks good.

https://codereview.chromium.org/653303003/diff/80001/Source/core/paint/LayerP...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/653303003/diff/80001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:390: StartClipRecorder
startClipRecorder(&m_renderLayer, context, clipType, clipRect);
This may be cleaner if it is just a static function (similarly
addRoundedRectClip and EndClipRecorder, below).

https://codereview.chromium.org/653303003/diff/80001/Source/core/paint/ViewDi...
File Source/core/paint/ViewDisplayList.h (right):

https://codereview.chromium.org/653303003/diff/80001/Source/core/paint/ViewDi...
Source/core/paint/ViewDisplayList.h:27: virtual ~DisplayItem() { }
Why dtor?

[chrishtr, 2014-10-16 17:43:53]

https://codereview.chromium.org/653303003/diff/80001/Source/core/paint/ViewDi...
File Source/core/paint/ViewDisplayList.h (right):

https://codereview.chromium.org/653303003/diff/80001/Source/core/paint/ViewDi...
Source/core/paint/ViewDisplayList.h:27: virtual ~DisplayItem() { }
On 2014/10/16 17:38:12, pdr wrote:
> Why dtor?

You need a virtual destructor if you have a virtual method.

[Stephen Chennney, 2014-10-16 17:56:53]

On 2014/10/16 17:38:12, pdr wrote:
> I think this looks good.
> 
>
https://codereview.chromium.org/653303003/diff/80001/Source/core/paint/LayerP...
> File Source/core/paint/LayerPainter.cpp (right):
> 
>
https://codereview.chromium.org/653303003/diff/80001/Source/core/paint/LayerP...
> Source/core/paint/LayerPainter.cpp:390: StartClipRecorder
> startClipRecorder(&m_renderLayer, context, clipType, clipRect);
> This may be cleaner if it is just a static function (similarly
> addRoundedRectClip and EndClipRecorder, below).

The method creates internal per-instance state (the clip object). If these are
ever nested you'd run into problems.

This pattern matches that for DisplayLists, I believe.

[Stephen Chennney, 2014-10-16 17:57:27]

.. you'd run into problemms if the object were static ...

[pdr., 2014-10-16 17:57:31]

On 2014/10/16 at 17:56:53, schenney wrote:
> On 2014/10/16 17:38:12, pdr wrote:
> > I think this looks good.
> > 
> >
https://codereview.chromium.org/653303003/diff/80001/Source/core/paint/LayerP...
> > File Source/core/paint/LayerPainter.cpp (right):
> > 
> >
https://codereview.chromium.org/653303003/diff/80001/Source/core/paint/LayerP...
> > Source/core/paint/LayerPainter.cpp:390: StartClipRecorder
> > startClipRecorder(&m_renderLayer, context, clipType, clipRect);
> > This may be cleaner if it is just a static function (similarly
> > addRoundedRectClip and EndClipRecorder, below).
> 
> The method creates internal per-instance state (the clip object). If these are
ever nested you'd run into problems.
> 
> This pattern matches that for DisplayLists, I believe.

Talked offline, I think we're going to go for a RAII object.

[Stephen Chennney, 2014-10-16 17:57:35]

.. you'd run into problems if the object were static ...

[leviw_travelin_and_unemployed, 2014-10-16 17:59:11]

https://codereview.chromium.org/653303003/diff/80001/Source/core/paint/ViewDi...
File Source/core/paint/ViewDisplayList.h (right):

https://codereview.chromium.org/653303003/diff/80001/Source/core/paint/ViewDi...
Source/core/paint/ViewDisplayList.h:71: typedef Vector<OwnPtr<DisplayItem> >
PaintList;
I believe we support the magic C++11 ">> in templates" parser "upgrade" now,
though I haven't tried it myself.

[Stephen Chennney, 2014-10-16 17:59:33]

On 2014/10/16 17:57:31, pdr wrote:
> On 2014/10/16 at 17:56:53, schenney wrote:
> > On 2014/10/16 17:38:12, pdr wrote:
> > > I think this looks good.
> > > 
> > >
>
https://codereview.chromium.org/653303003/diff/80001/Source/core/paint/LayerP...
> > > File Source/core/paint/LayerPainter.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/653303003/diff/80001/Source/core/paint/LayerP...
> > > Source/core/paint/LayerPainter.cpp:390: StartClipRecorder
> > > startClipRecorder(&m_renderLayer, context, clipType, clipRect);
> > > This may be cleaner if it is just a static function (similarly
> > > addRoundedRectClip and EndClipRecorder, below).
> > 
> > The method creates internal per-instance state (the clip object). If these
are
> ever nested you'd run into problems.
> > 
> > This pattern matches that for DisplayLists, I believe.
> 
> Talked offline, I think we're going to go for a RAII object.

Yes, that makes sense. Damn code review is super slow when there are multiple
commenters.

[pdr., 2014-10-16 19:29:09]

LGTM

[chrishtr, 2014-10-16 19:49:34]

Simplified yet more, please take another look.

[pdr., 2014-10-16 19:53:05]

https://codereview.chromium.org/653303003/diff/180001/Source/core/paint/Layer...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/653303003/diff/180001/Source/core/paint/Layer...
Source/core/paint/LayerPainter.cpp:746: ClipDisplayItem::ClipType clipType =
(ClipDisplayItem::ClipType)0;
Can you create an unknown clip type for this?

[pdr., 2014-10-16 19:55:14]

On 2014/10/16 at 19:53:05, pdr wrote:
>
https://codereview.chromium.org/653303003/diff/180001/Source/core/paint/Layer...
> File Source/core/paint/LayerPainter.cpp (right):
> 
>
https://codereview.chromium.org/653303003/diff/180001/Source/core/paint/Layer...
> Source/core/paint/LayerPainter.cpp:746: ClipDisplayItem::ClipType clipType =
(ClipDisplayItem::ClipType)0;
> Can you create an unknown clip type for this?

Talked offline, assigning to the float one works as well.

LGTM.

[chrishtr, 2014-10-16 20:48:36]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-10-16 20:49:59]

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

[chrishtr, 2014-10-16 21:44:12]

https://codereview.chromium.org/653303003/diff/180001/Source/core/paint/Layer...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/653303003/diff/180001/Source/core/paint/Layer...
Source/core/paint/LayerPainter.cpp:746: ClipDisplayItem::ClipType clipType =
(ClipDisplayItem::ClipType)0;
On 2014/10/16 19:53:05, pdr wrote:
> Can you create an unknown clip type for this?

Changed to float.

[chrishtr, 2014-10-16 22:14:14]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-10-16 22:15:06]

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

[commit-bot: I haz the power, 2014-10-17 00:30:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-17 00:30:38]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/32171)

[pdr., 2014-10-17 00:44:12]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-10-17 00:45:17]

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

[commit-bot: I haz the power, 2014-10-17 01:48:40]

Committed patchset #13 (id:240001) as 183848