Don't cache subsequence whose layer is not fully contained by repaint rect

Don't cache subsequence whose layer is not fully contained by repaint rect

When the interest rect changes, a layer paints newly uncovered contents
and doesn't paint contents out of the rect. We should not cache the
layer's subsequence in the case.

BUG=410097
TEST=DisplayItemListPaintTestForSlimmingPaintV2.CachedSubsequenceOnInterestRectChange

Committed: https://crrev.com/b1739f766d8d1825abc5a2fe8c9d4354e1053537
git-svn-id: svn://svn.chromium.org/blink/trunk@202078 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[Xianzhu, 2015-09-01 01:30:58]

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

[chrishtr, 2015-09-01 17:42:11]

How about we just do this:

1. Make sure not to clip the DisplayItemList at the edge of the interest rect
(which I think is already the case)

2. Don't clip elements that span the interest rect (except for known special
cases such as themed content which can cause OOM issues, and for which we can
accept special code to skip the cache)

#2 will require plumbing an extra bool to children during paint indicating that
the parent spans the interest rect.

Then we can treat the cache as separate from the interest rect, and use an
eviction policy to reduce its size as desired. e.g. if you scroll down and then
back up by more than theinterest rect, we don't evict things from the cache
unless it gets too big, so the cost of re-recording will be minimized during
large scrolls.

[Xianzhu, 2015-09-02 05:41:33]

On 2015/09/01 17:42:11, chrishtr wrote:
> How about we just do this:
> 
> 1. Make sure not to clip the DisplayItemList at the edge of the interest rect
> (which I think is already the case)
>

I think you meant not to clip DisplayItem, right?

> 2. Don't clip elements that span the interest rect (except for known special
> cases such as themed content which can cause OOM issues, and for which we can
> accept special code to skip the cache)
> 
> #2 will require plumbing an extra bool to children during paint indicating
that
> the parent spans the interest rect.
> 
> Then we can treat the cache as separate from the interest rect, and use an
> eviction policy to reduce its size as desired. e.g. if you scroll down and
then
> back up by more than the interest rect, we don't evict things from the cache
> unless it gets too big, so the cost of re-recording will be minimized during
> large scrolls.


I haven't thought out a way to achieve this, or I didn't quite understand your
idea.

DPLs/LayoutObjects are in a tree structure, so it seems that we either don't
clip any DPL/LayoutObject [subtree] (including the root DPL/LayoutObject), or
clip at some level.

If we can evict a part of the drawings of a DPL subtree, then it seems we still
need the logic in this CL to make sure we repaint the evicted part when the
interest rect covers them again.

There are possible optimizations for this CL:
- this CL has a potential performance issue when calling
DPL::boundingBoxForCompositing() which may traverse the DPL subtree. The issue
can be avoided if we cache the bounding box during the traversal.
- if a layer's boundingBoxForCompositing() doesn't intersect the interest rect,
we can directly skip painting of the whole DPL subtree.

[chrishtr, 2015-09-02 17:33:48]

On 2015/09/02 at 05:41:33, wangxianzhu wrote:
> On 2015/09/01 17:42:11, chrishtr wrote:
> > How about we just do this:
> > 
> > 1. Make sure not to clip the DisplayItemList at the edge of the interest
rect
> > (which I think is already the case)
> >
> 
> I think you meant not to clip DisplayItem, right?

Yes.

> 
> > 2. Don't clip elements that span the interest rect (except for known special
> > cases such as themed content which can cause OOM issues, and for which we
can
> > accept special code to skip the cache)
> > 
> > #2 will require plumbing an extra bool to children during paint indicating
that
> > the parent spans the interest rect.
> > 
> > Then we can treat the cache as separate from the interest rect, and use an
> > eviction policy to reduce its size as desired. e.g. if you scroll down and
then
> > back up by more than the interest rect, we don't evict things from the cache
> > unless it gets too big, so the cost of re-recording will be minimized during
> > large scrolls.
> 
> 
> I haven't thought out a way to achieve this, or I didn't quite understand your
idea.
> 
> DPLs/LayoutObjects are in a tree structure, so it seems that we either don't
clip any DPL/LayoutObject [subtree] (including the root DPL/LayoutObject), or
clip at some level.

You are right. I had overlooked that aspect. Thinking more...

> 
> If we can evict a part of the drawings of a DPL subtree, then it seems we
still need the logic in this CL to make sure we repaint the evicted part when
the interest rect covers them again.
> 
> There are possible optimizations for this CL:
> - this CL has a potential performance issue when calling
DPL::boundingBoxForCompositing() which may traverse the DPL subtree. The issue
can be avoided if we cache the bounding box during the traversal.
> - if a layer's boundingBoxForCompositing() doesn't intersect the interest
rect, we can directly skip painting of the whole DPL subtree.

[chrishtr, 2015-09-03 20:06:06]

How about this:

Whenever painting {Begin,End}SubsequenceDisplayItem, record cache-ability bits
on them. A
{Begin,End}SubsequenceDisplayItem pair is cacheable iff all of its contents
painted, i.e. all of it was inside the interest rect.

So if a subsequence is present in the previous frame, cacheable, and not
invalidated, then we can always reuse it in the new frame, regardless of whether
it intersects the interest rect. In this case we would end up with a
DisplayItemlist that extends beyond the interest rect, but that doesn't really
matter since it's efficient enough.

If a subsequence does not meet the conditions above, then we have to paint it.
If when painting it all of the subsequence was painted, then we cache the
subsequence for future frames. If not, we don't.

We can detect if all of a subsequence has painted by passing a bit up the paint
recursion stack (maybe as part of paintInfo) that is true if some child DPL or
LayoutObject decided it didn't want to paint since it was outside the interest
rect. This bit can then be set on the BeginSubsequenceDisplayItem after the
fact, and then used by the DisplayItemList to skip caching.

[Xianzhu, 2015-09-04 18:29:51]

On 2015/09/03 20:06:06, chrishtr wrote:
> How about this:
> 
> Whenever painting {Begin,End}SubsequenceDisplayItem, record cache-ability bits
> on them. A
> {Begin,End}SubsequenceDisplayItem pair is cacheable iff all of its contents
> painted, i.e. all of it was inside the interest rect.
> 
> So if a subsequence is present in the previous frame, cacheable, and not
> invalidated, then we can always reuse it in the new frame, regardless of
whether
> it intersects the interest rect. In this case we would end up with a
> DisplayItemlist that extends beyond the interest rect, but that doesn't really
> matter since it's efficient enough.
> 
> If a subsequence does not meet the conditions above, then we have to paint it.
> If when painting it all of the subsequence was painted, then we cache the
> subsequence for future frames. If not, we don't.
> 
> We can detect if all of a subsequence has painted by passing a bit up the
paint
> recursion stack (maybe as part of paintInfo) that is true if some child DPL or
> LayoutObject decided it didn't want to paint since it was outside the interest
> rect. This bit can then be set on the BeginSubsequenceDisplayItem after the
> fact, and then used by the DisplayItemList to skip caching.

I think your method is good to avoid the cost of boundingBoxForCompositing(),
but we'll need to set the flag at every early return on
no-paint-rect-intersection in painters. I'm afraid if we could make this
complete, test this and avoid future ignorance of this.

I'd like to compare the pros and cons of all possible methods. What do you think
the cons of the patch, cost of boundingBoxForCompositing() and/or the extra
space for DPL::m_previousPaintRect? I think caching of
boundingBoxForCompositing() can reduce the cost of it, and also benefit all
callers of it and
DPL::physicalBoundingBoxIncludingReflectionAndStackingChildren().

[chrishtr, 2015-09-04 19:59:56]

On 2015/09/04 at 18:29:51, wangxianzhu wrote:
> On 2015/09/03 20:06:06, chrishtr wrote:
> > How about this:
> > 
> > Whenever painting {Begin,End}SubsequenceDisplayItem, record cache-ability
bits
> > on them. A
> > {Begin,End}SubsequenceDisplayItem pair is cacheable iff all of its contents
> > painted, i.e. all of it was inside the interest rect.
> > 
> > So if a subsequence is present in the previous frame, cacheable, and not
> > invalidated, then we can always reuse it in the new frame, regardless of
whether
> > it intersects the interest rect. In this case we would end up with a
> > DisplayItemlist that extends beyond the interest rect, but that doesn't
really
> > matter since it's efficient enough.
> > 
> > If a subsequence does not meet the conditions above, then we have to paint
it.
> > If when painting it all of the subsequence was painted, then we cache the
> > subsequence for future frames. If not, we don't.
> > 
> > We can detect if all of a subsequence has painted by passing a bit up the
paint
> > recursion stack (maybe as part of paintInfo) that is true if some child DPL
or
> > LayoutObject decided it didn't want to paint since it was outside the
interest
> > rect. This bit can then be set on the BeginSubsequenceDisplayItem after the
> > fact, and then used by the DisplayItemList to skip caching.
> 
> I think your method is good to avoid the cost of boundingBoxForCompositing(),
but we'll need to set the flag at every early return on
no-paint-rect-intersection in painters. I'm afraid if we could make this
complete, test this and avoid future ignorance of this.
> 
> I'd like to compare the pros and cons of all possible methods. What do you
think the cons of the patch, cost of boundingBoxForCompositing() and/or the
extra space for DPL::m_previousPaintRect? I think caching of
boundingBoxForCompositing() can reduce the cost of it, and also benefit all
callers of it and
DPL::physicalBoundingBoxIncludingReflectionAndStackingChildren().

Makes sense to list pro and con.

--> Checking interset rect approach:

Pro: Centralizes logic for DPL caching in one place
Con: Expensive to compute boundingBoxForCompositing. Caching it requires extra
bits on DPLs that are dirtied when any descendant changes anything.
Con: We might be able to get rid of this code otherwise, and replace with a
computation based on property trees (*)

(*) I looked through the callers of boundingBoxForCompositing and
physicalBoundingBoxIncludingReflectionAndStackingChildren, and I think they are
all ones that will be gone with slimming paint v2, except for clip path and
filter painting (I may be wrong, let me know). Those use cases are not super
common  so as to be worth being careful to optimize.

--> Instrumenting code when early-outing approach:

Pro: No performance overhead
Pro: Allows us to preserve cached output for content outside the interest rect.
Con: Requires instrumenting every early-out case and setting bits for use by the
ancestor. Harder to enforce correctness for the future.

[chrishtr, 2015-09-04 20:00:43]

On 2015/09/04 at 19:59:56, chrishtr wrote:
> On 2015/09/04 at 18:29:51, wangxianzhu wrote:
> > On 2015/09/03 20:06:06, chrishtr wrote:
> > > How about this:
> > > 
> > > Whenever painting {Begin,End}SubsequenceDisplayItem, record cache-ability
bits
> > > on them. A
> > > {Begin,End}SubsequenceDisplayItem pair is cacheable iff all of its
contents
> > > painted, i.e. all of it was inside the interest rect.
> > > 
> > > So if a subsequence is present in the previous frame, cacheable, and not
> > > invalidated, then we can always reuse it in the new frame, regardless of
whether
> > > it intersects the interest rect. In this case we would end up with a
> > > DisplayItemlist that extends beyond the interest rect, but that doesn't
really
> > > matter since it's efficient enough.
> > > 
> > > If a subsequence does not meet the conditions above, then we have to paint
it.
> > > If when painting it all of the subsequence was painted, then we cache the
> > > subsequence for future frames. If not, we don't.
> > > 
> > > We can detect if all of a subsequence has painted by passing a bit up the
paint
> > > recursion stack (maybe as part of paintInfo) that is true if some child
DPL or
> > > LayoutObject decided it didn't want to paint since it was outside the
interest
> > > rect. This bit can then be set on the BeginSubsequenceDisplayItem after
the
> > > fact, and then used by the DisplayItemList to skip caching.
> > 
> > I think your method is good to avoid the cost of
boundingBoxForCompositing(), but we'll need to set the flag at every early
return on no-paint-rect-intersection in painters. I'm afraid if we could make
this complete, test this and avoid future ignorance of this.
> > 
> > I'd like to compare the pros and cons of all possible methods. What do you
think the cons of the patch, cost of boundingBoxForCompositing() and/or the
extra space for DPL::m_previousPaintRect? I think caching of
boundingBoxForCompositing() can reduce the cost of it, and also benefit all
callers of it and
DPL::physicalBoundingBoxIncludingReflectionAndStackingChildren().
> 
> Makes sense to list pro and con.
> 
> --> Checking interset rect approach:
> 
> Pro: Centralizes logic for DPL caching in one place
> Con: Expensive to compute boundingBoxForCompositing. Caching it requires extra
bits on DPLs that are dirtied when any descendant changes anything.
> Con: We might be able to get rid of this code otherwise, and replace with a
computation based on property trees (*)

"this code" == boundingBoxForCompositing and friends.

> 
> (*) I looked through the callers of boundingBoxForCompositing and
physicalBoundingBoxIncludingReflectionAndStackingChildren, and I think they are
all ones that will be gone with slimming paint v2, except for clip path and
filter painting (I may be wrong, let me know). Those use cases are not super
common  so as to be worth being careful to optimize.
> 
> --> Instrumenting code when early-outing approach:
> 
> Pro: No performance overhead
> Pro: Allows us to preserve cached output for content outside the interest
rect.
> Con: Requires instrumenting every early-out case and setting bits for use by
the ancestor. Harder to enforce correctness for the future.

[Xianzhu, 2015-09-04 21:09:35]

On 2015/09/04 20:00:43, chrishtr wrote:
> On 2015/09/04 at 19:59:56, chrishtr wrote:
> > On 2015/09/04 at 18:29:51, wangxianzhu wrote:
> > > On 2015/09/03 20:06:06, chrishtr wrote:
> > > > How about this:
> > > > 
> > > > Whenever painting {Begin,End}SubsequenceDisplayItem, record
cache-ability
> bits
> > > > on them. A
> > > > {Begin,End}SubsequenceDisplayItem pair is cacheable iff all of its
> contents
> > > > painted, i.e. all of it was inside the interest rect.
> > > > 
> > > > So if a subsequence is present in the previous frame, cacheable, and not
> > > > invalidated, then we can always reuse it in the new frame, regardless of
> whether
> > > > it intersects the interest rect. In this case we would end up with a
> > > > DisplayItemlist that extends beyond the interest rect, but that doesn't
> really
> > > > matter since it's efficient enough.
> > > > 
> > > > If a subsequence does not meet the conditions above, then we have to
paint
> it.
> > > > If when painting it all of the subsequence was painted, then we cache
the
> > > > subsequence for future frames. If not, we don't.
> > > > 
> > > > We can detect if all of a subsequence has painted by passing a bit up
the
> paint
> > > > recursion stack (maybe as part of paintInfo) that is true if some child
> DPL or
> > > > LayoutObject decided it didn't want to paint since it was outside the
> interest
> > > > rect. This bit can then be set on the BeginSubsequenceDisplayItem after
> the
> > > > fact, and then used by the DisplayItemList to skip caching.
> > > 
> > > I think your method is good to avoid the cost of
> boundingBoxForCompositing(), but we'll need to set the flag at every early
> return on no-paint-rect-intersection in painters. I'm afraid if we could make
> this complete, test this and avoid future ignorance of this.
> > > 
> > > I'd like to compare the pros and cons of all possible methods. What do you
> think the cons of the patch, cost of boundingBoxForCompositing() and/or the
> extra space for DPL::m_previousPaintRect? I think caching of
> boundingBoxForCompositing() can reduce the cost of it, and also benefit all
> callers of it and
> DPL::physicalBoundingBoxIncludingReflectionAndStackingChildren().
> > 
> > Makes sense to list pro and con.
> > 
> > --> Checking interset rect approach:
> > 
> > Pro: Centralizes logic for DPL caching in one place
> > Con: Expensive to compute boundingBoxForCompositing. Caching it requires
extra
> bits on DPLs that are dirtied when any descendant changes anything.
> > Con: We might be able to get rid of this code otherwise, and replace with a
> computation based on property trees (*)
> 
> "this code" == boundingBoxForCompositing and friends.
> 
> > 
> > (*) I looked through the callers of boundingBoxForCompositing and
> physicalBoundingBoxIncludingReflectionAndStackingChildren, and I think they
are
> all ones that will be gone with slimming paint v2, except for clip path and
> filter painting (I may be wrong, let me know). Those use cases are not super
> common  so as to be worth being careful to optimize.
> > 
> > --> Instrumenting code when early-outing approach:
> > 
> > Pro: No performance overhead
> > Pro: Allows us to preserve cached output for content outside the interest
> rect.
> > Con: Requires instrumenting every early-out case and setting bits for use by
> the ancestor. Harder to enforce correctness for the future.

Thanks for the analysis.

Just thought of a way to avoid the cons of the cacheable flag method: instead of
instrumenting early-outs, we can check
intersection(physicalBoundingBox,paintRect) in DPL and pass the flag up to
ancestor DPLs. Trying it.

[chrishtr, 2015-09-04 21:14:14]

On 2015/09/04 at 21:09:35, wangxianzhu wrote:
> On 2015/09/04 20:00:43, chrishtr wrote:
> > On 2015/09/04 at 19:59:56, chrishtr wrote:
> > > On 2015/09/04 at 18:29:51, wangxianzhu wrote:
> > > > On 2015/09/03 20:06:06, chrishtr wrote:
> > > > > How about this:
> > > > > 
> > > > > Whenever painting {Begin,End}SubsequenceDisplayItem, record
cache-ability
> > bits
> > > > > on them. A
> > > > > {Begin,End}SubsequenceDisplayItem pair is cacheable iff all of its
> > contents
> > > > > painted, i.e. all of it was inside the interest rect.
> > > > > 
> > > > > So if a subsequence is present in the previous frame, cacheable, and
not
> > > > > invalidated, then we can always reuse it in the new frame, regardless
of
> > whether
> > > > > it intersects the interest rect. In this case we would end up with a
> > > > > DisplayItemlist that extends beyond the interest rect, but that
doesn't
> > really
> > > > > matter since it's efficient enough.
> > > > > 
> > > > > If a subsequence does not meet the conditions above, then we have to
paint
> > it.
> > > > > If when painting it all of the subsequence was painted, then we cache
the
> > > > > subsequence for future frames. If not, we don't.
> > > > > 
> > > > > We can detect if all of a subsequence has painted by passing a bit up
the
> > paint
> > > > > recursion stack (maybe as part of paintInfo) that is true if some
child
> > DPL or
> > > > > LayoutObject decided it didn't want to paint since it was outside the
> > interest
> > > > > rect. This bit can then be set on the BeginSubsequenceDisplayItem
after
> > the
> > > > > fact, and then used by the DisplayItemList to skip caching.
> > > > 
> > > > I think your method is good to avoid the cost of
> > boundingBoxForCompositing(), but we'll need to set the flag at every early
> > return on no-paint-rect-intersection in painters. I'm afraid if we could
make
> > this complete, test this and avoid future ignorance of this.
> > > > 
> > > > I'd like to compare the pros and cons of all possible methods. What do
you
> > think the cons of the patch, cost of boundingBoxForCompositing() and/or the
> > extra space for DPL::m_previousPaintRect? I think caching of
> > boundingBoxForCompositing() can reduce the cost of it, and also benefit all
> > callers of it and
> > DPL::physicalBoundingBoxIncludingReflectionAndStackingChildren().
> > > 
> > > Makes sense to list pro and con.
> > > 
> > > --> Checking interset rect approach:
> > > 
> > > Pro: Centralizes logic for DPL caching in one place
> > > Con: Expensive to compute boundingBoxForCompositing. Caching it requires
extra
> > bits on DPLs that are dirtied when any descendant changes anything.
> > > Con: We might be able to get rid of this code otherwise, and replace with
a
> > computation based on property trees (*)
> > 
> > "this code" == boundingBoxForCompositing and friends.
> > 
> > > 
> > > (*) I looked through the callers of boundingBoxForCompositing and
> > physicalBoundingBoxIncludingReflectionAndStackingChildren, and I think they
are
> > all ones that will be gone with slimming paint v2, except for clip path and
> > filter painting (I may be wrong, let me know). Those use cases are not super
> > common  so as to be worth being careful to optimize.
> > > 
> > > --> Instrumenting code when early-outing approach:
> > > 
> > > Pro: No performance overhead
> > > Pro: Allows us to preserve cached output for content outside the interest
> > rect.
> > > Con: Requires instrumenting every early-out case and setting bits for use
by
> > the ancestor. Harder to enforce correctness for the future.
> 
> Thanks for the analysis.
> 
> Just thought of a way to avoid the cons of the cacheable flag method: instead
of instrumenting early-outs, we can check
intersection(physicalBoundingBox,paintRect) in DPL and pass the flag up to
ancestor DPLs. Trying it.

You mean physicalBoundingBox of LayoutObjects? Otherwise we'll have to pay the
cost of computing right?

[Xianzhu, 2015-09-04 21:20:37]

On 2015/09/04 21:14:14, chrishtr wrote:

> You mean physicalBoundingBox of LayoutObjects? Otherwise we'll have to pay the
> cost of computing right?

I mean DPL::physicalBoundingBox which is fast to get. It's better than
boundingBoxForCompositing because it doesn't traverse the descendants. We'll
accumulate the cacheable flag of descendants during painting (instead of
accumulating descendant bounding boxes in boundingBoxForCompositing).

[Xianzhu, 2015-09-05 01:32:19]

Patch Set 3 is the implementation of the new method. Still WIP.

[Xianzhu, 2015-09-08 16:33:35]

It's ready for review. Ptal.

[chrishtr, 2015-09-08 22:48:00]

https://codereview.chromium.org/1327563003/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/1327563003/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItem.h:262: bool isUncacheable() const {
return m_isUncacheable; }
This can be private.

https://codereview.chromium.org/1327563003/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/1327563003/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:98: // TODO(wangxianzhu):
Distinguish skippingCache and uncacheablility.
What's the difference exactly?

https://codereview.chromium.org/1327563003/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/SubsequenceRecorder.cpp (right):

https://codereview.chromium.org/1327563003/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/SubsequenceRecorder.cpp:42:
m_beginSubsequenceIndex = m_displayItemList->newDisplayItems().size();
You only need access to the size, not the whole vector. Just allowing access to
the former will be safer for the future.

[Xianzhu, 2015-09-09 16:50:33]

https://codereview.chromium.org/1327563003/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/1327563003/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:98: // TODO(wangxianzhu):
Distinguish skippingCache and uncacheablility.
On 2015/09/08 22:48:00, chrishtr wrote:
> What's the difference exactly?

Wanted to distinguish "skip-cache but cacheable" and "uncachable". However, the
former is not used for now so I removed uncacheable flag in the new patch. (I
planned to use the former to optimize full-subtree invalidation to avoid mark
every object for invalidation.)

https://codereview.chromium.org/1327563003/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/SubsequenceRecorder.cpp (right):

https://codereview.chromium.org/1327563003/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/SubsequenceRecorder.cpp:42:
m_beginSubsequenceIndex = m_displayItemList->newDisplayItems().size();
On 2015/09/08 22:48:00, chrishtr wrote:
> You only need access to the size, not the whole vector. Just allowing access
to
> the former will be safer for the future.

It's also needed to set uncacheable flag or a previous begin subsequence display
item (line 63 below).

[chrishtr, 2015-09-09 17:23:45]

https://codereview.chromium.org/1327563003/diff/80001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerPainter.h (right):

https://codereview.chromium.org/1327563003/diff/80001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerPainter.h:25: enum PaintResult {
FullyPainted, NotFullyPainted };
s/NotFullyPainted/MaybeNotFullyPainted/

Also add a comment that if MaybeNotFullyPainted, then the results cannot be
cached.

https://codereview.chromium.org/1327563003/diff/80001/Source/core/paint/Displ...
File Source/core/paint/DisplayItemListPaintTest.cpp (right):

https://codereview.chromium.org/1327563003/diff/80001/Source/core/paint/Displ...
Source/core/paint/DisplayItemListPaintTest.cpp:251:
TEST_F(DisplayItemListPaintTestForSlimmingPaintV2,
CachedSubsequenceOnInterestRectChange)
Move to a new DeprecatedPaintLayerPainterTest.cpp file?

[Xianzhu, 2015-09-09 21:30:44]

https://codereview.chromium.org/1327563003/diff/80001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerPainter.h (right):

https://codereview.chromium.org/1327563003/diff/80001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerPainter.h:25: enum PaintResult {
FullyPainted, NotFullyPainted };
On 2015/09/09 17:23:45, chrishtr wrote:
> s/NotFullyPainted/MaybeNotFullyPainted/
> 
> Also add a comment that if MaybeNotFullyPainted, then the results cannot be
> cached.

Done.

https://codereview.chromium.org/1327563003/diff/80001/Source/core/paint/Displ...
File Source/core/paint/DisplayItemListPaintTest.cpp (right):

https://codereview.chromium.org/1327563003/diff/80001/Source/core/paint/Displ...
Source/core/paint/DisplayItemListPaintTest.cpp:251:
TEST_F(DisplayItemListPaintTestForSlimmingPaintV2,
CachedSubsequenceOnInterestRectChange)
On 2015/09/09 17:23:45, chrishtr wrote:
> Move to a new DeprecatedPaintLayerPainterTest.cpp file?

Done.

[chrishtr, 2015-09-09 21:35:27]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-09-09 21:35:27]

lgtm

[commit-bot: I haz the power, 2015-09-09 21:35:41]

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

[commit-bot: I haz the power, 2015-09-09 22:39:39]

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

[commit-bot: I haz the power, 2015-09-09 22:39:39]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Xianzhu, 2015-09-09 22:47:59]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-09-09 22:48:07]

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

[commit-bot: I haz the power, 2015-09-09 23:29:45]

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

[commit-bot: I haz the power, 2015-09-09 23:29:45]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xianzhu, 2015-09-10 16:41:10]

Ptal the following diff in the latest patch:
https://codereview.chromium.org/1327563003/diff2/100001:120001/Source/core/pa...

It fixes two issue with the following changes:
- pass up MaybeNotFullyPainted for reflections;
- deal with old slow scrolling path that clips the layer contents with the
scrolling container which is an additional MaybeNotFullyPainted case.

[chrishtr, 2015-09-10 16:48:26]

https://codereview.chromium.org/1327563003/diff/120001/Source/core/paint/Depr...
File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right):

https://codereview.chromium.org/1327563003/diff/120001/Source/core/paint/Depr...
Source/core/paint/DeprecatedPaintLayerPainter.cpp:115: result =
MaybeNotFullyPainted;
Why always MaybeNotFullyPainted?

[Xianzhu, 2015-09-10 17:04:31]

https://codereview.chromium.org/1327563003/diff/120001/Source/core/paint/Depr...
File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right):

https://codereview.chromium.org/1327563003/diff/120001/Source/core/paint/Depr...
Source/core/paint/DeprecatedPaintLayerPainter.cpp:115: result =
MaybeNotFullyPainted;
On 2015/09/10 16:48:26, chrishtr wrote:
> Why always MaybeNotFullyPainted?

- We don't cache reflection drawings;
- If we did, it would need a lot of plumbing to pass the result through the
following invoke path:
  DeprecatedPaintLayerPainter::paintLayerContentsAndReflection()
  DeprecatedPaintLayerReflectionInfo::paint()
  DeprecatedPaintLayerPainter::paintLayer(the reflection layer)
  ReplicaPainter::paint()
  DeprecatedPaintLayerPainter::paintLayer(the base layer)

[Xianzhu, 2015-09-10 22:20:09]

Chris, do you have more comments?

[chrishtr, 2015-09-10 22:23:43]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-09-10 22:23:44]

lgtm

[commit-bot: I haz the power, 2015-09-10 22:24:16]

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

[commit-bot: I haz the power, 2015-09-10 23:11:56]

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

[commit-bot: I haz the power, 2015-09-23 12:16:03]

Patchset 7 (id:??) landed as
https://crrev.com/b1739f766d8d1825abc5a2fe8c9d4354e1053537