Implement clip and scroll DisplayItems for PartPainter

Implement clip and scroll DisplayItems for PartPainter

This CL implements clip and scroll DisplayItems for PartPainter for
slimming paint. A new recorder RoundedInnerRectClipper is added as the
slimming-paint version of BoxPainter::clipRoundedInnerRect.

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

[trchen, 2015-01-22 23:47:56]

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

[trchen, 2015-01-22 23:47:57]

Hello reviewers, this CL enables non-composited iframe scrolling for slimming
paint. PTAL. Thanks.

https://codereview.chromium.org/865353002/diff/1/Source/core/frame/FrameView.cpp
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/865353002/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:1395: }
FIXME: Currently we always use the cached DisplayItem even if we have a new one.
This will change once https://codereview.chromium.org/847783003/ lands and we
won't need this block anymore.

[chrishtr, 2015-01-23 05:07:12]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-01-23 05:07:12]

https://codereview.chromium.org/865353002/diff/1/Source/core/paint/ContentBox...
File Source/core/paint/ContentBoxRadiusClipper.cpp (right):

https://codereview.chromium.org/865353002/diff/1/Source/core/paint/ContentBox...
Source/core/paint/ContentBoxRadiusClipper.cpp:43: if
(roundedContentBox.isRenderable()) {
I don't get it. Where is all the logic in this class in the old code? If it is
present, why duplicate code?

[chrishtr, 2015-01-23 05:07:17]

The CQ bit was unchecked by chrishtr@chromium.org

[chrishtr, 2015-01-23 05:07:30]

On 2015/01/23 at 05:07:17, chrishtr wrote:
> The CQ bit was unchecked by chrishtr@chromium.org

Accidental click.

[trchen, 2015-01-23 05:16:08]

On 2015/01/23 05:07:12, chrishtr wrote:
>
https://codereview.chromium.org/865353002/diff/1/Source/core/paint/ContentBox...
> File Source/core/paint/ContentBoxRadiusClipper.cpp (right):
> 
>
https://codereview.chromium.org/865353002/diff/1/Source/core/paint/ContentBox...
> Source/core/paint/ContentBoxRadiusClipper.cpp:43: if
> (roundedContentBox.isRenderable()) {
> I don't get it. Where is all the logic in this class in the old code? If it is
> present, why duplicate code?

It was copied from BoxPainter::clipRoundedInnerRect(). RenderReplaced is still
using it so we can't remove the non-DisplayItem version. I didn't migrate
RenderReplaced because its content only generate one single DrawDisplayItem, and
we can simply include the radius clip in it.

[chrishtr, 2015-01-23 18:18:44]

On 2015/01/23 at 05:16:08, trchen wrote:
> On 2015/01/23 05:07:12, chrishtr wrote:
> >
https://codereview.chromium.org/865353002/diff/1/Source/core/paint/ContentBox...
> > File Source/core/paint/ContentBoxRadiusClipper.cpp (right):
> > 
> >
https://codereview.chromium.org/865353002/diff/1/Source/core/paint/ContentBox...
> > Source/core/paint/ContentBoxRadiusClipper.cpp:43: if
> > (roundedContentBox.isRenderable()) {
> > I don't get it. Where is all the logic in this class in the old code? If it
is
> > present, why duplicate code?
> 
> It was copied from BoxPainter::clipRoundedInnerRect(). RenderReplaced is still
using it so we can't remove the non-DisplayItem version. I didn't migrate
RenderReplaced because its content only generate one single DrawDisplayItem, and
we can simply include the radius clip in it.

I see. Instead, I think you should do one of two alternatives:
a) Refactor BoxPainter::clipRoundedInnerRect to take a ClipDisplayItem and not
call GraphicsContext methods directly
b) Refactor other call sites of BoxPainter::clipRoundedInnerRect to use your new
class.

Without looking deeper, not sure which of the above is better. Probably b?

The recorder classes are designed to work in slimming paint and non-slimming
paint versions, so one of the above should be straightforward.

It's definitely not good to duplicate this code.

[chrishtr, 2015-01-23 23:35:34]

https://codereview.chromium.org/865353002/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/865353002/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1391: if
(RuntimeEnabledFeatures::slimmingPaintEnabled()) {
Remove this now right?

https://codereview.chromium.org/865353002/diff/20001/Source/core/paint/BoxPai...
File Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/865353002/diff/20001/Source/core/paint/BoxPai...
Source/core/paint/BoxPainter.cpp:340: OwnPtr<RoundedInnerRectClipper>
clipToBorder;
Is there a save() you should be deleting?

https://codereview.chromium.org/865353002/diff/20001/Source/core/paint/BoxPai...
Source/core/paint/BoxPainter.cpp:340: OwnPtr<RoundedInnerRectClipper>
clipToBorder;
Is there a save() you should be deleting?

https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/BoxPai...
File Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/BoxPai...
Source/core/paint/BoxPainter.cpp:340: OwnPtr<RoundedInnerRectClipper>
clipToBorder;
Where are the save() and restore() for these?

https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/PartPa...
File Source/core/paint/PartPainter.cpp (right):

https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/PartPa...
Source/core/paint/PartPainter.cpp:56: clipper.clear();
Instead of manual clear, put it in a sub-block with { }

https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/Replac...
File Source/core/paint/ReplacedPainter.cpp (right):

https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/Replac...
Source/core/paint/ReplacedPainter.cpp:88: clipper.clear();
Same comment here.

[trchen, 2015-01-23 23:40:43]

https://codereview.chromium.org/865353002/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/865353002/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1391: if
(RuntimeEnabledFeatures::slimmingPaintEnabled()) {
On 2015/01/23 23:35:33, chrishtr wrote:
> Remove this now right?

Is https://codereview.chromium.org/847783003/ landing anytime soon?

https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/BoxPai...
File Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/BoxPai...
Source/core/paint/BoxPainter.cpp:340: OwnPtr<RoundedInnerRectClipper>
clipToBorder;
On 2015/01/23 23:35:33, chrishtr wrote:
> Where are the save() and restore() for these?

GraphicsContextStateSaver did the save and store. The OwnPtr here has exactly
the save scope and the clipper ctor/dtor will do the save and store.

https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/PartPa...
File Source/core/paint/PartPainter.cpp (right):

https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/PartPa...
Source/core/paint/PartPainter.cpp:56: clipper.clear();
On 2015/01/23 23:35:33, chrishtr wrote:
> Instead of manual clear, put it in a sub-block with { }

Acknowledged.

https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/Replac...
File Source/core/paint/ReplacedPainter.cpp (right):

https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/Replac...
Source/core/paint/ReplacedPainter.cpp:88: clipper.clear();
On 2015/01/23 23:35:33, chrishtr wrote:
> Same comment here.

Acknowledged.

[trchen, 2015-01-23 23:41:22]

https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/BoxPai...
File Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/BoxPai...
Source/core/paint/BoxPainter.cpp:340: OwnPtr<RoundedInnerRectClipper>
clipToBorder;
s/the save scope/the same scope/

[chrishtr, 2015-01-23 23:43:56]

I assume you're working on those comments now?

On 2015/01/23 at 23:40:43, trchen wrote:
>
https://codereview.chromium.org/865353002/diff/20001/Source/core/frame/FrameV...
> File Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/865353002/diff/20001/Source/core/frame/FrameV...
> Source/core/frame/FrameView.cpp:1391: if
(RuntimeEnabledFeatures::slimmingPaintEnabled()) {
> On 2015/01/23 23:35:33, chrishtr wrote:
> > Remove this now right?
> 
> Is https://codereview.chromium.org/847783003/ landing anytime soon?

I hoped today. Haven't heard back from Xianzhu though. Ok to commit this with a
big FIXME on it and remove it early next week.

> 
>
https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/BoxPai...
> File Source/core/paint/BoxPainter.cpp (right):
> 
>
https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/BoxPai...
> Source/core/paint/BoxPainter.cpp:340: OwnPtr<RoundedInnerRectClipper>
clipToBorder;
> On 2015/01/23 23:35:33, chrishtr wrote:
> > Where are the save() and restore() for these?
> 
> GraphicsContextStateSaver did the save and store. The OwnPtr here has exactly
the save scope and the clipper ctor/dtor will do the save and store.
> 
>
https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/PartPa...
> File Source/core/paint/PartPainter.cpp (right):
> 
>
https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/PartPa...
> Source/core/paint/PartPainter.cpp:56: clipper.clear();
> On 2015/01/23 23:35:33, chrishtr wrote:
> > Instead of manual clear, put it in a sub-block with { }
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/Replac...
> File Source/core/paint/ReplacedPainter.cpp (right):
> 
>
https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/Replac...
> Source/core/paint/ReplacedPainter.cpp:88: clipper.clear();
> On 2015/01/23 23:35:33, chrishtr wrote:
> > Same comment here.
> 
> Acknowledged.

[trchen, 2015-01-23 23:48:00]

https://codereview.chromium.org/865353002/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/865353002/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1391: if
(RuntimeEnabledFeatures::slimmingPaintEnabled()) {
On 2015/01/23 23:40:42, trchen wrote:
> On 2015/01/23 23:35:33, chrishtr wrote:
> > Remove this now right?
> 
> Is https://codereview.chromium.org/847783003/ landing anytime soon?

Done.

https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/PartPa...
File Source/core/paint/PartPainter.cpp (right):

https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/PartPa...
Source/core/paint/PartPainter.cpp:56: clipper.clear();
On 2015/01/23 23:40:43, trchen wrote:
> On 2015/01/23 23:35:33, chrishtr wrote:
> > Instead of manual clear, put it in a sub-block with { }
> 
> Acknowledged.

Done.

https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/Replac...
File Source/core/paint/ReplacedPainter.cpp (right):

https://codereview.chromium.org/865353002/diff/40001/Source/core/paint/Replac...
Source/core/paint/ReplacedPainter.cpp:88: clipper.clear();
On 2015/01/23 23:40:43, trchen wrote:
> On 2015/01/23 23:35:33, chrishtr wrote:
> > Same comment here.
> 
> Acknowledged.

Done.

[chrishtr, 2015-01-23 23:53:04]

Looks good modulu my comments about where the save/restore was for the second
call to clipping in BoxPainter.

[trchen, 2015-01-23 23:55:52]

On 2015/01/23 23:53:04, chrishtr wrote:
> Looks good modulu my comments about where the save/restore was for the second
> call to clipping in BoxPainter.

GraphicsContextStateSaver did the save and store before the change. The new
OwnPtr here has exactly
the same scope and the clipper ctor/dtor will do the save and store.

Sorry my previous comment was lost in chaos.

[chrishtr, 2015-01-24 00:06:56]

On 2015/01/23 at 23:55:52, trchen wrote:
> On 2015/01/23 23:53:04, chrishtr wrote:
> > Looks good modulu my comments about where the save/restore was for the
second
> > call to clipping in BoxPainter.
> 
> GraphicsContextStateSaver did the save and store before the change. The new
OwnPtr here has exactly
> the same scope and the clipper ctor/dtor will do the save and store.
> 
> Sorry my previous comment was lost in chaos.

Ah. I don't think you need the GraphicsContextStateSaver any more, since a
save/restore is needed iff inner clipping occurs.

[chrishtr, 2015-01-24 00:57:20]

lgtm

[trchen, 2015-01-24 01:02:12]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2015-01-24 01:02:55]

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

[trchen, 2015-01-24 01:31:13]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2015-01-24 01:31:19]

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

[commit-bot: I haz the power, 2015-01-24 02:46:19]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188919