Let synchronized painting generate correct paint invalidation rects

Let synchronized painting generate correct paint invalidation rects

- Still use the original paint invalidation path (LayoutObject ->
  CompositedLayerMapping -> GraphicsLayer -> cc) for rect-based
  invalidations;
- Pass correct paint invalidation rect (bounds) of clients to
  PaintController for bug 529938;
  Several TODOs for corner cases to deal with and/or check;
- Other changes to make synchronized painting work.

Also enable virtual/syncpaint for fast/repaint/, compositing/repaint/
and paint/.

BUG=536999, 529938
Committed: https://crrev.com/6b0fe6c757aa2b884be424c7a90c567e8ca39701
Cr-Commit-Position: refs/heads/master@{#355977}

[Xianzhu, 2015-10-21 23:07:58]

Description was changed from

==========
Let synchronized painting generate correct paint invalidation rects

- Fully support the original full and incremental paint invalidation;
- Collect invalidation rects generated by original paint invalidation
  code;
- Apply subpixel accumulation and layoutObjectOffset of layers.

Also enable virtual/syncpaint for fast/repaint/, compositing/repaint/
and paint/.

BUG=536999
==========

to

==========
Let synchronized painting generate correct paint invalidation rects

- Fully support the original full and incremental paint invalidation;
- Collect invalidation rects generated by original paint invalidation
  code;
- Apply subpixel accumulation and layoutObjectOffset of layers.

Also enable virtual/syncpaint for fast/repaint/, compositing/repaint/
and paint/.

BUG=536999
==========

[Xianzhu, 2015-10-21 23:08:35]

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

[Xianzhu, 2015-10-21 23:21:41]

Description was changed from

==========
Let synchronized painting generate correct paint invalidation rects

- Fully support the original full and incremental paint invalidation;
- Collect invalidation rects generated by original paint invalidation
  code;
- Apply subpixel accumulation and layoutObjectOffset of layers.

Also enable virtual/syncpaint for fast/repaint/, compositing/repaint/
and paint/.

BUG=536999
==========

to

==========
Let synchronized painting generate correct paint invalidation rects

- Fully support the original full and incremental paint invalidation;
- Collect invalidation rects generated by original paint invalidation
  code;
- Apply subpixel accumulation and layoutObjectOffset of layers;
- Other changes to make synchronized painting work.

Also enable virtual/syncpaint for fast/repaint/, compositing/repaint/
and paint/.

BUG=536999
==========

[chrishtr, 2015-10-22 00:02:41]

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/LeakExpectations (right):

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/LeakExpectations:60: crbug.com/364417
virtual/syncpaint/fast/repaint/japanese-rl-selection-clear.html [ Leak ]
Why the changes in this file? What leaks?

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/virtual/syncpaint/fast/repaint/README.txt
(right):

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/virtual/syncpaint/fast/repaint/README.txt:1: #
This suite runs tests with SlimmingPaintSynchronizedPainting enabled.
How many tests are you enabling in virtual suites?

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1439: if (Scrollbar*
verticalScrollbar = area->verticalScrollbar())
Why is this conditional needed now but not before?

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2696: if (compositingState()
== PaintsIntoOwnBacking)
Not squashing?

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (left):

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:237:
box().invalidatePaintRectangleNotInvalidatingDisplayItemClients(LayoutRect(intRect));
Why did the old code avoid invalidating display item clients?

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:229:
box().invalidateDisplayItemClient(*scrollbar);
Why this change?

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:46: void
invalidate(const DisplayItemClientWrapper&, PaintInvalidationReason, const
Vector<IntRect>& paintInvalidationRects);
If it's a vector, how is Walter's code supposed to know what the new bounds of
the display item are?

[Xianzhu, 2015-10-22 01:17:04]

TL;DR: I have a much simpler method to make rect invalidation work for spv1 sync
painting. Most of the comments below won't apply to the simple method.

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/LeakExpectations (right):

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/LeakExpectations:60: crbug.com/364417
virtual/syncpaint/fast/repaint/japanese-rl-selection-clear.html [ Leak ]
On 2015/10/22 00:02:41, chrishtr wrote:
> Why the changes in this file? What leaks?

These are due to leaks of the base test, causing the same leaks of
virtual/syncpaint tests, not related to syncpaint.

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/virtual/syncpaint/fast/repaint/README.txt
(right):

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/virtual/syncpaint/fast/repaint/README.txt:1: #
This suite runs tests with SlimmingPaintSynchronizedPainting enabled.
On 2015/10/22 00:02:41, chrishtr wrote:
> How many tests are you enabling in virtual suites?

virtual/syncpaint/compositing/repaint: 36
virtual/syncpaint/fast/repaint: 425
virtual/syncpaint/paint: 69
total: 530

Using 1 process, it takes about 2min to run the additional tests. I believe they
will have little effect on total running time when run in parallel with other
tests.

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1439: if (Scrollbar*
verticalScrollbar = area->verticalScrollbar())
On 2015/10/22 00:02:41, chrishtr wrote:
> Why is this conditional needed now but not before?

Previously we invalidate the rect directly to GraphicsLayer, but with sync
painting, the rect is lost. Here we just need to pass the rect along with 'some
display item client' through PaintController::invalidate(), but it requires us
to also pass a display item client. The following code tries to pass the actual
client (scrollbar) if it's still there, or pass the box as the client. Perhaps
this logic is unneeded because we actually want to associate the rects to
PaintChunk not individual display item client, but that needs a change of the
PaintController interface. Would like to address this during spv2 paint
invalidation.

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2696: if (compositingState()
== PaintsIntoOwnBacking)
On 2015/10/22 00:02:41, chrishtr wrote:
> Not squashing?

We need to mark needsRepaint until real GraphicsLayers because we issue
CachedDisplayItemList according to the needsRepaint flag of the root layer of a
real GraphicsLayer.

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (left):

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:237:
box().invalidatePaintRectangleNotInvalidatingDisplayItemClients(LayoutRect(intRect));
On 2015/10/22 00:02:41, chrishtr wrote:
> Why did the old code avoid invalidating display item clients?

Because we had invalidated display item clients above. Now we output
invalidation rects along with invalidations of display item clients, so must
change this otherwise we'll lose this invalidation rect.

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:229:
box().invalidateDisplayItemClient(*scrollbar);
On 2015/10/22 00:02:41, chrishtr wrote:
> Why this change?

See the comment for old code line 237.

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:46: void
invalidate(const DisplayItemClientWrapper&, PaintInvalidationReason, const
Vector<IntRect>& paintInvalidationRects);
On 2015/10/22 00:02:41, chrishtr wrote:
> If it's a vector, how is Walter's code supposed to know what the new bounds of
> the display item are?

This is a good question which actually answers my question about how
invalidation rects will be used.

Before this change, we also can't ensure newPaintInvalidationRect to be the
bounding box (see examples below).

We should separate paint invalidation rects from new bounds because not every
invalidation is a full invalidation. For example
- incremental invalidation: I had been thinking of removing it for spv2, but
based on today's discussion about raster invalidation, I think it's still useful
to keep raster invalidation small. (We'll need to keep also the original logics
to reduce rect-based invalidations e.g. [1].)
- rect based invalidations: LayoutObject::invalidatePaintRectangle();
- empty rect (or vector) passed when we just want to invalidate the display item
client.

We may have two methods for paint invalidation:
void invalidateDisplayItemClient(const DisplayItemClientWrapper&, const IntRect&
boundingBox);
void invalidateRect(PaintInvalidationReason, const IntRect& rect);

However, the above seem to only work with invalidation-in-paint because we must
know which PaintChunk the invalidated rect belongs to.

I have a much simpler method to make rect-based invalidation work for spv1
synchronized painting: use the whole original GraphicsLayer-based paint
invalidation. I'd like to leave PaintController rect-based invalidation to spv2.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....

[chrishtr, 2015-10-22 15:11:59]

On 2015/10/22 at 01:17:04, wangxianzhu wrote:
> TL;DR: I have a much simpler method to make rect invalidation work for spv1
sync painting. Most of the comments below won't apply to the simple method.
> 
>
https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/LeakExpectations (right):
> 
>
https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/LeakExpectations:60: crbug.com/364417
virtual/syncpaint/fast/repaint/japanese-rl-selection-clear.html [ Leak ]
> On 2015/10/22 00:02:41, chrishtr wrote:
> > Why the changes in this file? What leaks?
> 
> These are due to leaks of the base test, causing the same leaks of
virtual/syncpaint tests, not related to syncpaint.
> 
>
https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/virtual/syncpaint/fast/repaint/README.txt
(right):
> 
>
https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/virtual/syncpaint/fast/repaint/README.txt:1: #
This suite runs tests with SlimmingPaintSynchronizedPainting enabled.
> On 2015/10/22 00:02:41, chrishtr wrote:
> > How many tests are you enabling in virtual suites?
> 
> virtual/syncpaint/compositing/repaint: 36
> virtual/syncpaint/fast/repaint: 425
> virtual/syncpaint/paint: 69
> total: 530
> 
> Using 1 process, it takes about 2min to run the additional tests. I believe
they will have little effect on total running time when run in parallel with
other tests.
> 
>
https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):
> 
>
https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBox.cpp:1439: if (Scrollbar*
verticalScrollbar = area->verticalScrollbar())
> On 2015/10/22 00:02:41, chrishtr wrote:
> > Why is this conditional needed now but not before?
> 
> Previously we invalidate the rect directly to GraphicsLayer, but with sync
painting, the rect is lost. Here we just need to pass the rect along with 'some
display item client' through PaintController::invalidate(), but it requires us
to also pass a display item client. The following code tries to pass the actual
client (scrollbar) if it's still there, or pass the box as the client. Perhaps
this logic is unneeded because we actually want to associate the rects to
PaintChunk not individual display item client, but that needs a change of the
PaintController interface. Would like to address this during spv2 paint
invalidation.
> 
>
https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):
> 
>
https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintLayer.cpp:2696: if
(compositingState() == PaintsIntoOwnBacking)
> On 2015/10/22 00:02:41, chrishtr wrote:
> > Not squashing?
> 
> We need to mark needsRepaint until real GraphicsLayers because we issue
CachedDisplayItemList according to the needsRepaint flag of the root layer of a
real GraphicsLayer.
> 
>
https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (left):
> 
>
https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:237:
box().invalidatePaintRectangleNotInvalidatingDisplayItemClients(LayoutRect(intRect));
> On 2015/10/22 00:02:41, chrishtr wrote:
> > Why did the old code avoid invalidating display item clients?
> 
> Because we had invalidated display item clients above. Now we output
invalidation rects along with invalidations of display item clients, so must
change this otherwise we'll lose this invalidation rect.
> 
>
https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
(right):
> 
>
https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:229:
box().invalidateDisplayItemClient(*scrollbar);
> On 2015/10/22 00:02:41, chrishtr wrote:
> > Why this change?
> 
> See the comment for old code line 237.
> 
>
https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):
> 
>
https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/paint/PaintController.h:46: void
invalidate(const DisplayItemClientWrapper&, PaintInvalidationReason, const
Vector<IntRect>& paintInvalidationRects);
> On 2015/10/22 00:02:41, chrishtr wrote:
> > If it's a vector, how is Walter's code supposed to know what the new bounds
of
> > the display item are?
> 
> This is a good question which actually answers my question about how
invalidation rects will be used.
> 
> Before this change, we also can't ensure newPaintInvalidationRect to be the
bounding box (see examples below).
> 
> We should separate paint invalidation rects from new bounds because not every
invalidation is a full invalidation. For example
> - incremental invalidation: I had been thinking of removing it for spv2, but
based on today's discussion about raster invalidation, I think it's still useful
to keep raster invalidation small. (We'll need to keep also the original logics
to reduce rect-based invalidations e.g. [1].)

Is [1] referring to not invalidating an entire pagination container?
Can you enumerate what use cases of incremental invalidation you think we should
keep?

The only use case I know of so far where sub-DisplayItemClient invalidation made
sense for performance reasons was invalidation of 
only the visible area of a very large sprite that has a clipping region on in,
in cases when it is expensive to paint the sprite (e.g. SVG).

However, the current code handles this, because the "bounds" of a
DisplayItemClient is defined as the bounds of the pixels that it draws to, in
the space of the GraphicsLayer. In other words, clipping is taken into account.

> - rect based invalidations: LayoutObject::invalidatePaintRectangle();
> - empty rect (or vector) passed when we just want to invalidate the display
item client.
> 
> We may have two methods for paint invalidation:
> void invalidateDisplayItemClient(const DisplayItemClientWrapper&, const
IntRect& boundingBox);
> void invalidateRect(PaintInvalidationReason, const IntRect& rect);
> 
> However, the above seem to only work with invalidation-in-paint because we
must know which PaintChunk the invalidated rect belongs to.

I agree that we need to separate rect vs old/new bounds. That was a flaw in the
existing code. In the semantics of spv1, the "PaintChunk" that a
DisplayItemClient belongs to is the GraphicsLayer. In spv2 it will be real
PaintChunks.

> 
> I have a much simpler method to make rect-based invalidation work for spv1
synchronized painting: use the whole original GraphicsLayer-based paint
invalidation. I'd like to leave PaintController rect-based invalidation to spv2.

The problem is that this blocks Walter's current work to compute display item
bounds, right? He needs this data to be plumbed.


> 
> [1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....

[Xianzhu, 2015-10-22 16:15:11]

On 2015/10/22 15:11:59, chrishtr wrote:
> On 2015/10/22 at 01:17:04, wangxianzhu wrote:
>
https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/graphics/paint/PaintController.h:46: void
> invalidate(const DisplayItemClientWrapper&, PaintInvalidationReason, const
> Vector<IntRect>& paintInvalidationRects);
> > On 2015/10/22 00:02:41, chrishtr wrote:
> > > If it's a vector, how is Walter's code supposed to know what the new
bounds
> of
> > > the display item are?
> > 
> > This is a good question which actually answers my question about how
> invalidation rects will be used.
> > 
> > Before this change, we also can't ensure newPaintInvalidationRect to be the
> bounding box (see examples below).
> > 
> > We should separate paint invalidation rects from new bounds because not
every
> invalidation is a full invalidation. For example
> > - incremental invalidation: I had been thinking of removing it for spv2, but
> based on today's discussion about raster invalidation, I think it's still
useful
> to keep raster invalidation small. (We'll need to keep also the original
logics
> to reduce rect-based invalidations e.g. [1].)
> 
> Is [1] referring to not invalidating an entire pagination container?
> Can you enumerate what use cases of incremental invalidation you think we
should
> keep?
> 
> The only use case I know of so far where sub-DisplayItemClient invalidation
made
> sense for performance reasons was invalidation of 
> only the visible area of a very large sprite that has a clipping region on in,
> in cases when it is expensive to paint the sprite (e.g. SVG).
> 
> However, the current code handles this, because the "bounds" of a
> DisplayItemClient is defined as the bounds of the pixels that it draws to, in
> the space of the GraphicsLayer. In other words, clipping is taken into
account.
> 

[1] is to avoid invalidation of a 'blank' containers. e.g.
   <body style="margin: 0">
     <div style="position: relative; left: 500px; width: 100px; height: 100px;
background-color: blue"></div>
   </body>
   and div's height changes to '120px'.

with [1], we won't invalidate <body> because it paints nothing. Otherwise we
would invalidate 0,0 600x120 (without incremental invalidation) or 0,100 600x20
(with incremental invalidation)

This is also an example of incremental invalidation. With it, we invalidate only
500,100 100,20 not the whole div 500,0 100,120.

I thought incremental invalidation and [1] would be removed for spv2 because my
previous thought was cc would generate the rects based on diffs of display item
list.

> The problem is that this blocks Walter's current work to compute display item
> bounds, right? He needs this data to be plumbed.
>

Is Walter's work also for spv1?

[Xianzhu, 2015-10-22 16:22:05]

Note the [1] link is not pointing to the expected code because the file changed.
I wanted to point to LayoutObject::skipInvalidationWhenLaidOutChildren().

[chrishtr, 2015-10-22 16:30:01]

On 2015/10/22 at 16:15:11, wangxianzhu wrote:
> On 2015/10/22 15:11:59, chrishtr wrote:
> > On 2015/10/22 at 01:17:04, wangxianzhu wrote:
> >
https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/platform/graphics/paint/PaintController.h:46:
void
> > invalidate(const DisplayItemClientWrapper&, PaintInvalidationReason, const
> > Vector<IntRect>& paintInvalidationRects);
> > > On 2015/10/22 00:02:41, chrishtr wrote:
> > > > If it's a vector, how is Walter's code supposed to know what the new
bounds
> > of
> > > > the display item are?
> > > 
> > > This is a good question which actually answers my question about how
> > invalidation rects will be used.
> > > 
> > > Before this change, we also can't ensure newPaintInvalidationRect to be
the
> > bounding box (see examples below).
> > > 
> > > We should separate paint invalidation rects from new bounds because not
every
> > invalidation is a full invalidation. For example
> > > - incremental invalidation: I had been thinking of removing it for spv2,
but
> > based on today's discussion about raster invalidation, I think it's still
useful
> > to keep raster invalidation small. (We'll need to keep also the original
logics
> > to reduce rect-based invalidations e.g. [1].)
> > 
> > Is [1] referring to not invalidating an entire pagination container?
> > Can you enumerate what use cases of incremental invalidation you think we
should
> > keep?
> > 
> > The only use case I know of so far where sub-DisplayItemClient invalidation
made
> > sense for performance reasons was invalidation of 
> > only the visible area of a very large sprite that has a clipping region on
in,
> > in cases when it is expensive to paint the sprite (e.g. SVG).
> > 
> > However, the current code handles this, because the "bounds" of a
> > DisplayItemClient is defined as the bounds of the pixels that it draws to,
in
> > the space of the GraphicsLayer. In other words, clipping is taken into
account.
> > 
> 
> [1] is to avoid invalidation of a 'blank' containers. e.g.
>    <body style="margin: 0">
>      <div style="position: relative; left: 500px; width: 100px; height: 100px;
background-color: blue"></div>
>    </body>
>    and div's height changes to '120px'.
> 
> with [1], we won't invalidate <body> because it paints nothing. Otherwise we
would invalidate 0,0 600x120 (without incremental invalidation) or 0,100 600x20
(with incremental invalidation)

It paints something, but that something is the same before and after for many of
the raster tiles. Also, if this optimization is only important for solid-color
backgrounds of the root, we already have optimizations in cc that have extremely
efficient raster of such solid color tiles.

> 
> This is also an example of incremental invalidation. With it, we invalidate
only 500,100 100,20 not the whole div 500,0 100,120.
> 
> I thought incremental invalidation and [1] would be removed for spv2 because
my previous thought was cc would generate the rects based on diffs of display
item list.
> 
> > The problem is that this blocks Walter's current work to compute display
item
> > bounds, right? He needs this data to be plumbed.
> >
> 
> Is Walter's work also for spv1?

Yes. He is trying to unblock using an RTree, which would get us a 10-15% savings
in paint time.

[Xianzhu, 2015-10-22 16:47:54]

On 2015/10/22 16:30:01, chrishtr wrote: 
> Yes. He is trying to unblock using an RTree, which would get us a 10-15%
savings
> in paint time.

I'm clear now :)

Will combine this CL and the simple CL:
- use the original paint invalidation path for paint invalidation rects for
raster invalidation;
- pass paint invalidation bounds of display item clients to PaintController;
- for clients (e.g. scrollbars, line boxes) that we don't calculate paint
invalidation bounds, pass the bounds of their containers.

Will add a new path of rater invalidation for spv2 later.

Do we need previous paint invalidation bounds of display item clients? If not,
we'll save code and/or avoid passing incorrect previous invalidation bounds in
some cases (e.g. when invalidating a display item client not during paint
invalidation).

[chrishtr, 2015-10-22 16:50:43]

On 2015/10/22 at 16:47:54, wangxianzhu wrote:
> On 2015/10/22 16:30:01, chrishtr wrote: 
> > Yes. He is trying to unblock using an RTree, which would get us a 10-15%
savings
> > in paint time.
> 
> I'm clear now :)
> 
> Will combine this CL and the simple CL:
> - use the original paint invalidation path for paint invalidation rects for
raster invalidation;
> - pass paint invalidation bounds of display item clients to PaintController;
> - for clients (e.g. scrollbars, line boxes) that we don't calculate paint
invalidation bounds, pass the bounds of their containers.
> 
> Will add a new path of rater invalidation for spv2 later.

Cool. I'll follow up separately about incremental invalidation. Maybe we can
experiment with getting rid of it now, to reduce complexity earlier rather than
later.

> 
> Do we need previous paint invalidation bounds of display item clients? If not,
we'll save code and/or avoid passing incorrect previous invalidation bounds in
some cases (e.g. when invalidating a display item client not during paint
invalidation).

If GraphicsLayer is going to be passing along the rects to cc, then I think we
don't need the previous rects to be passed to the PaintController, because
Walter's code will be maintaining a map from DisplayItemClient to bounds, for
this purpose.

[chrishtr, 2015-10-22 16:51:18]

On 2015/10/22 at 16:50:43, chrishtr wrote:
> On 2015/10/22 at 16:47:54, wangxianzhu wrote:
> > On 2015/10/22 16:30:01, chrishtr wrote: 
> > > Yes. He is trying to unblock using an RTree, which would get us a 10-15%
savings
> > > in paint time.
> > 
> > I'm clear now :)
> > 
> > Will combine this CL and the simple CL:
> > - use the original paint invalidation path for paint invalidation rects for
raster invalidation;
> > - pass paint invalidation bounds of display item clients to PaintController;
> > - for clients (e.g. scrollbars, line boxes) that we don't calculate paint
invalidation bounds, pass the bounds of their containers.
> > 
> > Will add a new path of rater invalidation for spv2 later.
> 
> Cool. I'll follow up separately about incremental invalidation. Maybe we can
experiment with getting rid of it now, to reduce complexity earlier rather than
later.
> 
> > 
> > Do we need previous paint invalidation bounds of display item clients? If
not, we'll save code and/or avoid passing incorrect previous invalidation bounds
in some cases (e.g. when invalidating a display item client not during paint
invalidation).
> 
> If GraphicsLayer is going to be passing along the rects to cc, then I think we
don't need the previous rects to be passed to the PaintController, because
Walter's code will be maintaining a map from DisplayItemClient to bounds, for
this purpose.

Strike "for this purpose" from the sentence, it makes no sense. :)

[Xianzhu, 2015-10-22 18:19:31]

Description was changed from

==========
Let synchronized painting generate correct paint invalidation rects

- Fully support the original full and incremental paint invalidation;
- Collect invalidation rects generated by original paint invalidation
  code;
- Apply subpixel accumulation and layoutObjectOffset of layers;
- Other changes to make synchronized painting work.

Also enable virtual/syncpaint for fast/repaint/, compositing/repaint/
and paint/.

BUG=536999
==========

to

==========
Let synchronized painting generate correct paint invalidation rects

- Still use the original paint invalidation path (LayoutObject ->
  CompositedLayerMapping -> GraphicsLayer -> cc) for rect-based
  invalidations;
- Pass correct paint invalidation rect (bounds) of clients to
  PaintController for bug 529938;
  Several TODOs for corner cases to deal with and/or check;
- Other changes to make synchronized painting work.

Also enable virtual/syncpaint for fast/repaint/, compositing/repaint/
and paint/.

BUG=536999
==========

[Xianzhu, 2015-10-22 22:51:48]

Ready for review. Ptal.

https://codereview.chromium.org/1416053003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/repaint/scrollbar-damage-and-full-viewport-repaint-expected.txt
(left):

https://codereview.chromium.org/1416053003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/repaint/scrollbar-damage-and-full-viewport-repaint-expected.txt:10:
[1, 236, 185, 15]
The original file was not used because every platform has their own expectation
overriding this one. Removed redundant platform expectations.

The actual change is that the sequence of the two objects are reversed.

https://codereview.chromium.org/1416053003/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/repaint/scrollbar-parts-expected.txt
(left):

https://codereview.chromium.org/1416053003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/repaint/scrollbar-parts-expected.txt:11: ]
The original file was not used because every platform has their own expectation
overriding this one. Removed redundant platform expectations.

The actual change is that the sequence of the two objects are reversed.

https://codereview.chromium.org/1416053003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/overflow/background-should-be-recorded-full.html
(right):

https://codereview.chromium.org/1416053003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/overflow/background-should-be-recorded-full.html:22:
console.log('x');
This will be removed in the next patch set after try results of this patch set
is available.

[chrishtr, 2015-10-23 17:15:07]

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2696: if (compositingState()
== PaintsIntoOwnBacking)
On 2015/10/22 at 01:17:04, Xianzhu wrote:
> On 2015/10/22 00:02:41, chrishtr wrote:
> > Not squashing?
> 
> We need to mark needsRepaint until real GraphicsLayers because we issue
CachedDisplayItemList according to the needsRepaint flag of the root layer of a
real GraphicsLayer.

Then you need to stop at squashing layers and mark the owningLayer of the CLM
which has the squashing layer as needsRepaint.

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1398: //
TODO(wangxianzhu): Pass current bounds of lineboxes to PaintController.
Please file a bug to not lose track of this.

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1271: if (PaintLayer*
enclosingLayer = this->enclosingLayer())
Why this change? Don't remember if I already asked it, sorry..

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3345:
enclosingLayer->setNeedsRepaint();
Same question here.

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1023: void
invalidatePaintRectangleNotInvalidatingDisplayItemClients(const LayoutRect& r)
const { invalidatePaintRectangleInternal(r); }
Don't inline this.

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1420: virtual void
invalidateDisplayItemClients(const LayoutBoxModelObject&
paintInvalidationContainer, PaintInvalidationReason, const LayoutRect*
paintInvalidationRect) const;
rename to visibleContentRect? Can skip for this CL if you want.

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutText.cpp:1880: // TODO(wangxianzhu):
Pass current bounds of text boxes to PaintController.
File bug.

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
(right):

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:126:
void invalidateDisplayItemClient(const DisplayItemClientWrapper&,
PaintInvalidationReason, const LayoutRect* paintInvalidatioRect);
Typo

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:48: void
invalidate(const DisplayItemClientWrapper&, PaintInvalidationReason, const
IntRect* bounds);
visibleContentRect

[Xianzhu, 2015-10-23 17:36:34]

Description was changed from

==========
Let synchronized painting generate correct paint invalidation rects

- Still use the original paint invalidation path (LayoutObject ->
  CompositedLayerMapping -> GraphicsLayer -> cc) for rect-based
  invalidations;
- Pass correct paint invalidation rect (bounds) of clients to
  PaintController for bug 529938;
  Several TODOs for corner cases to deal with and/or check;
- Other changes to make synchronized painting work.

Also enable virtual/syncpaint for fast/repaint/, compositing/repaint/
and paint/.

BUG=536999
==========

to

==========
Let synchronized painting generate correct paint invalidation rects

- Still use the original paint invalidation path (LayoutObject ->
  CompositedLayerMapping -> GraphicsLayer -> cc) for rect-based
  invalidations;
- Pass correct paint invalidation rect (bounds) of clients to
  PaintController for bug 529938;
  Several TODOs for corner cases to deal with and/or check;
- Other changes to make synchronized painting work.

Also enable virtual/syncpaint for fast/repaint/, compositing/repaint/
and paint/.

BUG=536999,529938
==========

[Xianzhu, 2015-10-23 18:43:27]

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/1416053003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2696: if (compositingState()
== PaintsIntoOwnBacking)
On 2015/10/23 17:15:06, chrishtr wrote:
> On 2015/10/22 at 01:17:04, Xianzhu wrote:
> > On 2015/10/22 00:02:41, chrishtr wrote:
> > > Not squashing?
> > 
> > We need to mark needsRepaint until real GraphicsLayers because we issue
> CachedDisplayItemList according to the needsRepaint flag of the root layer of
a
> real GraphicsLayer.
> 
> Then you need to stop at squashing layers and mark the owningLayer of the CLM
> which has the squashing layer as needsRepaint.

I realized this too :) Fixed in the patch set 7.

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1398: //
TODO(wangxianzhu): Pass current bounds of lineboxes to PaintController.
On 2015/10/23 17:15:07, chrishtr wrote:
> Please file a bug to not lose track of this.

Done.

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1271: if (PaintLayer*
enclosingLayer = this->enclosingLayer())
On 2015/10/23 17:15:07, chrishtr wrote:
> Why this change? Don't remember if I already asked it, sorry..

This should not be there. This should be done by invalidateDisplayItemClients()
in LayoutObject::invalidatePaintRectangle(). Perhaps I added this when I changed
other code and this was necessary at that time, but not now.

Removed.

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3345:
enclosingLayer->setNeedsRepaint();
On 2015/10/23 17:15:07, chrishtr wrote:
> Same question here.

This is needed for now for custom scrollbar parts which can't find their own
enclosing layers. Added a TODO referencing crbug.com/547119.

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1023: void
invalidatePaintRectangleNotInvalidatingDisplayItemClients(const LayoutRect& r)
const { invalidatePaintRectangleInternal(r); }
On 2015/10/23 17:15:07, chrishtr wrote:
> Don't inline this.

Done.

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1420: virtual void
invalidateDisplayItemClients(const LayoutBoxModelObject&
paintInvalidationContainer, PaintInvalidationReason, const LayoutRect*
paintInvalidationRect) const;
On 2015/10/23 17:15:07, chrishtr wrote:
> rename to visibleContentRect? Can skip for this CL if you want.

I think we can keep paintInvalidationRect in LayoutObject classes for a while
because the name states that it's in the coordinates of
paintInvalidationContainer.

Renamed paintInvalidationRect and bounds to visibleContentRect in
CompositedLayerMapping, GraphicsLayer and PaintController because
paintInvalidationRect doesn't have clear meaning in these context.

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutText.cpp:1880: // TODO(wangxianzhu):
Pass current bounds of text boxes to PaintController.
On 2015/10/23 17:15:07, chrishtr wrote:
> File bug.

Done.

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
(right):

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:126:
void invalidateDisplayItemClient(const DisplayItemClientWrapper&,
PaintInvalidationReason, const LayoutRect* paintInvalidatioRect);
On 2015/10/23 17:15:07, chrishtr wrote:
> Typo

Done. Renamed to visibleContentRect and added comments.

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/1416053003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:48: void
invalidate(const DisplayItemClientWrapper&, PaintInvalidationReason, const
IntRect* bounds);
On 2015/10/23 17:15:07, chrishtr wrote:
> visibleContentRect

Done.

[chrishtr, 2015-10-23 19:57:49]

https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3364:
enclosingLayer->setNeedsRepaint();
If they can't find their layers, how can they call a function which finds the
enclosing layer? What's the call stack?

https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:46: //
Otherwise we must pass a correct value (bounds of the display item client in
coordinates of the
"bounds" needs to be defined here. I suggest just saying this is a rect in the
space of the GraphicsLayer into which the DisplayItem paints which contains all
pixels within the GraphicsLayer which might be painted into by the DisplayItem.

Also, danakj explained to me that this should be visualLayerRect, not
visualContentRect. "content" in cc means the space that takes into account pinch
zoom and such; "layer" is in the space of the cc::Layer (GraphicsLayer).

[Xianzhu, 2015-10-23 20:56:53]

https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3364:
enclosingLayer->setNeedsRepaint();
On 2015/10/23 19:57:49, chrishtr wrote:
> If they can't find their layers, how can they call a function which finds the
> enclosing layer? What's the call stack?

Here the |this| object is the LayoutBox containing the custom scrollbars. The
LayoutBox can find its containingLayer. Normally we should expect the
descendants call enclosingLayer->setNeedsRepaint() when they are traversed and
call their invalidateDisplayItemClients(), but a custom scrollbar part can't
find its enclosingLayer.

This is also why we had
invalidateDisplayItemClientForNonCompositingDescendants*Of*() which lets an
object in the layout tree (|this|) to invalidate some object not in the layout
tree (|object|) but painted onto the same backing.

https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:46: //
Otherwise we must pass a correct value (bounds of the display item client in
coordinates of the
On 2015/10/23 19:57:49, chrishtr wrote:
> "bounds" needs to be defined here. I suggest just saying this is a rect in the
> space of the GraphicsLayer into which the DisplayItem paints which contains
all
> pixels within the GraphicsLayer which might be painted into by the
DisplayItem.
> 

Done.

> Also, danakj explained to me that this should be visualLayerRect, not
> visualContentRect. "content" in cc means the space that takes into account
pinch
> zoom and such; "layer" is in the space of the cc::Layer (GraphicsLayer).

I think visualLayerRect looks confusing to blink people and needs more
explanation and reference to cc to be understood. How about avoiding 'content',
just 'visualRect', or 'visualBoundingRect'?

[chrishtr, 2015-10-23 20:59:45]

https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3364:
enclosingLayer->setNeedsRepaint();
On 2015/10/23 at 20:56:53, Xianzhu wrote:
> On 2015/10/23 19:57:49, chrishtr wrote:
> > If they can't find their layers, how can they call a function which finds
the
> > enclosing layer? What's the call stack?
> 
> Here the |this| object is the LayoutBox containing the custom scrollbars. The
LayoutBox can find its containingLayer. Normally we should expect the
descendants call enclosingLayer->setNeedsRepaint() when they are traversed and
call their invalidateDisplayItemClients(), but a custom scrollbar part can't
find its enclosingLayer.

How is the containing LayoutBox getting called into if the scrollbar can't find
its inclosing layer?

> 
> This is also why we had
invalidateDisplayItemClientForNonCompositingDescendants*Of*() which lets an
object in the layout tree (|this|) to invalidate some object not in the layout
tree (|object|) but painted onto the same backing.

https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:46: //
Otherwise we must pass a correct value (bounds of the display item client in
coordinates of the
On 2015/10/23 at 20:56:53, Xianzhu wrote:
> On 2015/10/23 19:57:49, chrishtr wrote:
> > "bounds" needs to be defined here. I suggest just saying this is a rect in
the
> > space of the GraphicsLayer into which the DisplayItem paints which contains
all
> > pixels within the GraphicsLayer which might be painted into by the
DisplayItem.
> > 
> 
> Done.
> 
> > Also, danakj explained to me that this should be visualLayerRect, not
> > visualContentRect. "content" in cc means the space that takes into account
pinch
> > zoom and such; "layer" is in the space of the cc::Layer (GraphicsLayer).
> 
> I think visualLayerRect looks confusing to blink people and needs more
explanation and reference to cc to be understood. How about avoiding 'content',
just 'visualRect', or 'visualBoundingRect'?

visualRect is ok then, with comment.

[Xianzhu, 2015-10-23 21:28:22]

https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3364:
enclosingLayer->setNeedsRepaint();
On 2015/10/23 20:59:45, chrishtr wrote:
> On 2015/10/23 at 20:56:53, Xianzhu wrote:
> > On 2015/10/23 19:57:49, chrishtr wrote:
> > > If they can't find their layers, how can they call a function which finds
> the
> > > enclosing layer? What's the call stack?
> > 
> > Here the |this| object is the LayoutBox containing the custom scrollbars.
The
> LayoutBox can find its containingLayer. Normally we should expect the
> descendants call enclosingLayer->setNeedsRepaint() when they are traversed and
> call their invalidateDisplayItemClients(), but a custom scrollbar part can't
> find its enclosingLayer.
> 
> How is the containing LayoutBox getting called into if the scrollbar can't
find
> its inclosing layer?
>

I should have included the following in the previous reply:

The function is called from:
- PaintLayerScrollableArea::invalidateScrollCornerRect():
  
box().invalidateDisplayItemClientForNonCompositingDescendantsOf(*m_scrollCorner);
   box().invalidateDisplayItemClientForNonCompositingDescendantsOf(*m_resizer);
- LayoutScrollbar::invalidateRect()
  
owningLayoutObject()->invalidateDisplayItemClientForNonCompositingDescendantsOf(*part.value)

LayoutObject::enclosingLayer() doesn't work for scrollbar parts which are not in
the layout tree, but they have other ways to find their containing box. I ever
filed crbug.com/484263 about whether we can hookup scrollbar parts into the
layout tree to avoid such complications.
 
> > 
> > This is also why we had
> invalidateDisplayItemClientForNonCompositingDescendants*Of*() which lets an
> object in the layout tree (|this|) to invalidate some object not in the layout
> tree (|object|) but painted onto the same backing.
>

https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:46: //
Otherwise we must pass a correct value (bounds of the display item client in
coordinates of the
On 2015/10/23 20:59:45, chrishtr wrote:
> On 2015/10/23 at 20:56:53, Xianzhu wrote:
> > On 2015/10/23 19:57:49, chrishtr wrote:
> > > "bounds" needs to be defined here. I suggest just saying this is a rect in
> the
> > > space of the GraphicsLayer into which the DisplayItem paints which
contains
> all
> > > pixels within the GraphicsLayer which might be painted into by the
> DisplayItem.
> > > 
> > 
> > Done.
> > 
> > > Also, danakj explained to me that this should be visualLayerRect, not
> > > visualContentRect. "content" in cc means the space that takes into account
> pinch
> > > zoom and such; "layer" is in the space of the cc::Layer (GraphicsLayer).
> > 
> > I think visualLayerRect looks confusing to blink people and needs more
> explanation and reference to cc to be understood. How about avoiding
'content',
> just 'visualRect', or 'visualBoundingRect'?
> 
> visualRect is ok then, with comment.

Done.

[chrishtr, 2015-10-23 21:44:54]

On 2015/10/23 at 21:28:22, wangxianzhu wrote:
>
https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):
> 
>
https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutObject.cpp:3364:
enclosingLayer->setNeedsRepaint();
> On 2015/10/23 20:59:45, chrishtr wrote:
> > On 2015/10/23 at 20:56:53, Xianzhu wrote:
> > > On 2015/10/23 19:57:49, chrishtr wrote:
> > > > If they can't find their layers, how can they call a function which
finds
> > the
> > > > enclosing layer? What's the call stack?
> > > 
> > > Here the |this| object is the LayoutBox containing the custom scrollbars.
The
> > LayoutBox can find its containingLayer. Normally we should expect the
> > descendants call enclosingLayer->setNeedsRepaint() when they are traversed
and
> > call their invalidateDisplayItemClients(), but a custom scrollbar part can't
> > find its enclosingLayer.
> > 
> > How is the containing LayoutBox getting called into if the scrollbar can't
find
> > its inclosing layer?
> >
> 
> I should have included the following in the previous reply:
> 
> The function is called from:
> - PaintLayerScrollableArea::invalidateScrollCornerRect():
>   
box().invalidateDisplayItemClientForNonCompositingDescendantsOf(*m_scrollCorner);
>   
box().invalidateDisplayItemClientForNonCompositingDescendantsOf(*m_resizer);
> - LayoutScrollbar::invalidateRect()
>   
owningLayoutObject()->invalidateDisplayItemClientForNonCompositingDescendantsOf(*part.value)
> 
> LayoutObject::enclosingLayer() doesn't work for scrollbar parts which are not
in the layout tree, but they have other ways to find their containing box. I
ever filed crbug.com/484263 about whether we can hookup scrollbar parts into the
layout tree to avoid such complications.


Ok then can this special code go in
PaintLayerScrollableArea::invalidateScrollCornerRect?
>  
> > > 
> > > This is also why we had
> > invalidateDisplayItemClientForNonCompositingDescendants*Of*() which lets an
> > object in the layout tree (|this|) to invalidate some object not in the
layout
> > tree (|object|) but painted onto the same backing.
> >
> 
>
https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):
> 
>
https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/graphics/paint/PaintController.h:46: //
Otherwise we must pass a correct value (bounds of the display item client in
coordinates of the
> On 2015/10/23 20:59:45, chrishtr wrote:
> > On 2015/10/23 at 20:56:53, Xianzhu wrote:
> > > On 2015/10/23 19:57:49, chrishtr wrote:
> > > > "bounds" needs to be defined here. I suggest just saying this is a rect
in
> > the
> > > > space of the GraphicsLayer into which the DisplayItem paints which
contains
> > all
> > > > pixels within the GraphicsLayer which might be painted into by the
> > DisplayItem.
> > > > 
> > > 
> > > Done.
> > > 
> > > > Also, danakj explained to me that this should be visualLayerRect, not
> > > > visualContentRect. "content" in cc means the space that takes into
account
> > pinch
> > > > zoom and such; "layer" is in the space of the cc::Layer (GraphicsLayer).
> > > 
> > > I think visualLayerRect looks confusing to blink people and needs more
> > explanation and reference to cc to be understood. How about avoiding
'content',
> > just 'visualRect', or 'visualBoundingRect'?
> > 
> > visualRect is ok then, with comment.
> 
> Done.

[Xianzhu, 2015-10-23 21:48:37]

On 2015/10/23 21:44:54, chrishtr wrote:
> On 2015/10/23 at 21:28:22, wangxianzhu wrote:
> >
>
https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1416053003/diff/160001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/layout/LayoutObject.cpp:3364:
> enclosingLayer->setNeedsRepaint();
> > On 2015/10/23 20:59:45, chrishtr wrote:
> > > On 2015/10/23 at 20:56:53, Xianzhu wrote:
> > > > On 2015/10/23 19:57:49, chrishtr wrote:
> > > > > If they can't find their layers, how can they call a function which
> finds
> > > the
> > > > > enclosing layer? What's the call stack?
> > > > 
> > > > Here the |this| object is the LayoutBox containing the custom
scrollbars.
> The
> > > LayoutBox can find its containingLayer. Normally we should expect the
> > > descendants call enclosingLayer->setNeedsRepaint() when they are traversed
> and
> > > call their invalidateDisplayItemClients(), but a custom scrollbar part
can't
> > > find its enclosingLayer.
> > > 
> > > How is the containing LayoutBox getting called into if the scrollbar can't
> find
> > > its inclosing layer?
> > >
> > 
> > I should have included the following in the previous reply:
> > 
> > The function is called from:
> > - PaintLayerScrollableArea::invalidateScrollCornerRect():
> >   
>
box().invalidateDisplayItemClientForNonCompositingDescendantsOf(*m_scrollCorner);
> >   
> box().invalidateDisplayItemClientForNonCompositingDescendantsOf(*m_resizer);
> > - LayoutScrollbar::invalidateRect()
> >   
>
owningLayoutObject()->invalidateDisplayItemClientForNonCompositingDescendantsOf(*part.value)
> > 
> > LayoutObject::enclosingLayer() doesn't work for scrollbar parts which are
not
> in the layout tree, but they have other ways to find their containing box. I
> ever filed crbug.com/484263 about whether we can hookup scrollbar parts into
the
> layout tree to avoid such complications.
> 
> 
> Ok then can this special code go in
> PaintLayerScrollableArea::invalidateScrollCornerRect?

This will be part of the patch for crbug.com/547119. It involves moving all rect
and object invalidation before paint invalidation state into paint invalidation
so that we can calculate the current paint invalidation rects for scrollbar and
scrollbar parts. I have started working on it.

[chrishtr, 2015-10-23 21:49:10]

lgtm

Ok.

[Xianzhu, 2015-10-23 23:41:00]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-10-23 23:41:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1416053003/#ps220001
(title: "For landing")

[commit-bot: I haz the power, 2015-10-23 23:41:40]

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

[commit-bot: I haz the power, 2015-10-24 01:17:47]

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

[commit-bot: I haz the power, 2015-10-24 01:17:48]

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

[Xianzhu, 2015-10-24 15:47:51]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-10-24 15:48:07]

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

[commit-bot: I haz the power, 2015-10-24 16:34:06]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-10-24 16:35:11]

Patchset 12 (id:??) landed as
https://crrev.com/6b0fe6c757aa2b884be424c7a90c567e8ca39701
Cr-Commit-Position: refs/heads/master@{#355977}