New display item caching

New display item caching

The algorithm becomes much simpler after we forced full layer painting
because all objects needing paint will appear in the new paint list.

To handler reordering, we need to lookup the matching display item of
a CachedDisplayItem by client.

If we can't find a matching display item for a CachedDisplayItem, there
must be under-invalidations (crbug.com/450725). We output an error
message, and in debug mode, paint the object red.

BUG=444163
TEST=ViewDisplayListTest.ComplexUpdateSwapOrder

[Xianzhu, 2015-01-18 03:35:11]

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

[Xianzhu, 2015-01-20 17:16:25]

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

[Xianzhu, 2015-01-20 17:16:26]

This seems still not correct when the new paint list is a partial paint list
(e.g. painted within a small rect).

[Xianzhu, 2015-01-22 01:46:48]

PTAL.

New display item caching

The algorithm becomes much simpler after we forced full layer painting
because all objects needing paint will appear in the new paint list.

To handler reordering, we need to lookup the matching display item of
a CachedDisplayItem by client.

If we can't find a matching display item for a CachedDisplayItem, there
must be under-invalidations (crbug.com/450725). We output an error
message, and in debug mode, paint the object red.

BUG=444163
TEST=ViewDisplayListTest.ComplexUpdateSwapOrder

[trchen, 2015-01-22 01:52:50]

On 2015/01/22 01:46:48, Xianzhu wrote:
> The algorithm becomes much simpler after we forced full layer painting
> because all objects needing paint will appear in the new paint list.

Are you going to work on ignoring damage rect optimization (i.e. forced full
layer painting) as a separate CL?

[Xianzhu, 2015-01-22 17:24:50]

On 2015/01/22 01:52:50, trchen wrote:
> On 2015/01/22 01:46:48, Xianzhu wrote:
> > The algorithm becomes much simpler after we forced full layer painting
> > because all objects needing paint will appear in the new paint list.
> 
> Are you going to work on ignoring damage rect optimization (i.e. forced full
> layer painting) as a separate CL?

The CLs have been submitted to disable display item caching to let telemetry
tests work for slimming paint:
- https://codereview.chromium.org/862773002/
- https://codereview.chromium.org/860563003/

This CL enables display item caching again, with full layer painting.

[chrishtr, 2015-01-22 20:13:07]

https://codereview.chromium.org/847783003/diff/20001/Source/platform/graphics...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/847783003/diff/20001/Source/platform/graphics...
Source/platform/graphics/paint/DisplayItemList.cpp:55: while (offset <
info.displayItemIndexes.size() &&
!m_paintList[info.displayItemIndexes[offset]]->idsEqual(displayItem))
The optimization using offset implies that cached display items within an
uninvalidated DisplayItemClient never change order...not obvious anymore that
that holds.

Why not just use a nested hash map for now? Or a sorted array.

https://codereview.chromium.org/847783003/diff/20001/Source/platform/graphics...
File Source/platform/graphics/paint/DisplayItemList.h (right):

https://codereview.chromium.org/847783003/diff/20001/Source/platform/graphics...
Source/platform/graphics/paint/DisplayItemList.h:58: // from O(m*n) to O(m*n).
From O(m*n) to O(m*n)?

[Xianzhu, 2015-01-22 21:48:12]

https://codereview.chromium.org/847783003/diff/20001/Source/platform/graphics...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/847783003/diff/20001/Source/platform/graphics...
Source/platform/graphics/paint/DisplayItemList.cpp:55: while (offset <
info.displayItemIndexes.size() &&
!m_paintList[info.displayItemIndexes[offset]]->idsEqual(displayItem))
On 2015/01/22 20:13:07, chrishtr wrote:
> The optimization using offset implies that cached display items within an
> uninvalidated DisplayItemClient never change order...not obvious anymore that
> that holds.
> 
> Why not just use a nested hash map for now? Or a sorted array.

There might be two reasons of using a vector containing items in the original
order:

- I'm afraid that removing the implication of display item order of an
uninvalidated DisplayItemClient will hide more under-invalidation bugs
(crbug.com/450725). However, I think allowing reordering without invalidation
could be a useful optimization in the future and added the case in
crbug.com/451090).

- We still have duplicate ids (mostly for EndClip, but maybe also others) which
prevent us from using a map or ignoring the original order. Duplicate ids seem
not to harm so far. Do we actually want to fix it?

https://codereview.chromium.org/847783003/diff/20001/Source/platform/graphics...
File Source/platform/graphics/paint/DisplayItemList.h (right):

https://codereview.chromium.org/847783003/diff/20001/Source/platform/graphics...
Source/platform/graphics/paint/DisplayItemList.h:58: // from O(m*n) to O(m*n).
On 2015/01/22 20:13:07, chrishtr wrote:
> From O(m*n) to O(m*n)?

Sorry, this should be "From O(m*n) to O(m+n)". The whole statement is also
inaccurate.

Updated.

[chrishtr, 2015-01-22 21:57:52]

On 2015/01/22 at 21:48:12, wangxianzhu wrote:
>
https://codereview.chromium.org/847783003/diff/20001/Source/platform/graphics...
> File Source/platform/graphics/paint/DisplayItemList.cpp (right):
> 
>
https://codereview.chromium.org/847783003/diff/20001/Source/platform/graphics...
> Source/platform/graphics/paint/DisplayItemList.cpp:55: while (offset <
info.displayItemIndexes.size() &&
!m_paintList[info.displayItemIndexes[offset]]->idsEqual(displayItem))
> On 2015/01/22 20:13:07, chrishtr wrote:
> > The optimization using offset implies that cached display items within an
> > uninvalidated DisplayItemClient never change order...not obvious anymore
that
> > that holds.
> > 
> > Why not just use a nested hash map for now? Or a sorted array.
> 
> There might be two reasons of using a vector containing items in the original
order:
> 
> - I'm afraid that removing the implication of display item order of an
uninvalidated DisplayItemClient will hide more under-invalidation bugs
(crbug.com/450725). However, I think allowing reordering without invalidation
could be a useful optimization in the future and added the case in
crbug.com/451090).
> 
> - We still have duplicate ids (mostly for EndClip, but maybe also others)
which prevent us from using a map or ignoring the original order. Duplicate ids
seem not to harm so far. Do we actually want to fix it?

Right, knew about these duplicates. I think we do need to fix it, but not
required right away.

> 
>
https://codereview.chromium.org/847783003/diff/20001/Source/platform/graphics...
> File Source/platform/graphics/paint/DisplayItemList.h (right):
> 
>
https://codereview.chromium.org/847783003/diff/20001/Source/platform/graphics...
> Source/platform/graphics/paint/DisplayItemList.h:58: // from O(m*n) to O(m*n).
> On 2015/01/22 20:13:07, chrishtr wrote:
> > From O(m*n) to O(m*n)?
> 
> Sorry, this should be "From O(m*n) to O(m+n)". The whole statement is also
inaccurate.
> 
> Updated.

[chrishtr, 2015-01-22 22:17:21]

https://codereview.chromium.org/847783003/diff/100001/Source/core/paint/ViewD...
File Source/core/paint/ViewDisplayListTest.cpp (right):

https://codereview.chromium.org/847783003/diff/100001/Source/core/paint/ViewD...
Source/core/paint/ViewDisplayListTest.cpp:549:
rootDisplayItemList().invalidate(container1->displayItemClient());
Add a comment that this is simulating the situation when container1 gets e.g. a
z-index that is now greater than container2.

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

https://codereview.chromium.org/847783003/diff/100001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:76: info.updateOffset = 0;
Put this in the constructor instead.

https://codereview.chromium.org/847783003/diff/100001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:113:
appendDisplayItem(updatedList, newCachedDisplayItemsInfoByClient,
m_paintList[index].release());
How about replacing 94-113 with:

if (newDisplayItem->isCached() {
  size_t index = findNextMatchingCachedItem(*newDisplayItem);
  if (index == kNotFound) {
    log errors
  } else {
    ASSERT(!m_paintList[index]->isCached()); // check for bugs
    newDisplayItem.swap(m_paintList[index]);
  }
}
appendDisplayItem(updatedList, newCachedDisplayItemsInfoByClient,
newDisplayItem.release());

https://codereview.chromium.org/847783003/diff/100001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:153:
stringBuilder.append(String::format("{client:%p, mergeIndex:%d,
displayItemIndexes:[", item.key, (int)item.value.updateOffset));
s/mergeIndex/updateOffset/. Or better yet, just ASSERT(item.value.updateOffset
== 0) since printing during update is not a common debug need.

[Xianzhu, 2015-01-22 23:50:59]

https://codereview.chromium.org/847783003/diff/100001/Source/core/paint/ViewD...
File Source/core/paint/ViewDisplayListTest.cpp (right):

https://codereview.chromium.org/847783003/diff/100001/Source/core/paint/ViewD...
Source/core/paint/ViewDisplayListTest.cpp:549:
rootDisplayItemList().invalidate(container1->displayItemClient());
On 2015/01/22 22:17:21, chrishtr wrote:
> Add a comment that this is simulating the situation when container1 gets e.g.
a
> z-index that is now greater than container2.

Done.

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

https://codereview.chromium.org/847783003/diff/100001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:76: info.updateOffset = 0;
On 2015/01/22 22:17:21, chrishtr wrote:
> Put this in the constructor instead.

Done.

https://codereview.chromium.org/847783003/diff/100001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:113:
appendDisplayItem(updatedList, newCachedDisplayItemsInfoByClient,
m_paintList[index].release());
On 2015/01/22 22:17:21, chrishtr wrote:
> How about replacing 94-113 with:
> 
> if (newDisplayItem->isCached() {
>   size_t index = findNextMatchingCachedItem(*newDisplayItem);
>   if (index == kNotFound) {
>     log errors
>   } else {
>     ASSERT(!m_paintList[index]->isCached()); // check for bugs
>     newDisplayItem.swap(m_paintList[index]);
>   }
> }
> appendDisplayItem(updatedList, newCachedDisplayItemsInfoByClient,
> newDisplayItem.release());

Just discussed a related issue with trchen@. We noticed that sometimes some meta
display items (items other than DrawingDisplayItem/CachedDisplayItem) change
without invalidation. For example, when an iframe is scrolled, the transform
display items change, but we don't invalidate the iframe. With the old code,
we'll reuse the old transform display items because we find matches, causing
scrolling not working. Your suggested code basically does the same as the old
code.

In the new code, we use old display items only for CachedDisplayItems. For other
display items, we don't try to match, but use the new display item directly.
This avoids findNextMachingCachedItem() calls for non-CachedDisplayItems.

https://codereview.chromium.org/847783003/diff/100001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:153:
stringBuilder.append(String::format("{client:%p, mergeIndex:%d,
displayItemIndexes:[", item.key, (int)item.value.updateOffset));
On 2015/01/22 22:17:21, chrishtr wrote:
> s/mergeIndex/updateOffset/.

Done.

> Or better yet, just ASSERT(item.value.updateOffset
> == 0) since printing during update is not a common debug need.

I found this is useful when debugging updatePaintList() method. In gdb, I can
manually call showDebugData() anytime.

The change in paintListAsDebugString() is also for debugging updatePaintList()
(when we may encounter null entries in the paint list).

[chrishtr, 2015-01-23 05:03:06]

On 2015/01/22 at 23:50:59, wangxianzhu wrote:
>
https://codereview.chromium.org/847783003/diff/100001/Source/core/paint/ViewD...
> File Source/core/paint/ViewDisplayListTest.cpp (right):
> 
>
https://codereview.chromium.org/847783003/diff/100001/Source/core/paint/ViewD...
> Source/core/paint/ViewDisplayListTest.cpp:549:
rootDisplayItemList().invalidate(container1->displayItemClient());
> On 2015/01/22 22:17:21, chrishtr wrote:
> > Add a comment that this is simulating the situation when container1 gets
e.g. a
> > z-index that is now greater than container2.
> 
> Done.
> 
>
https://codereview.chromium.org/847783003/diff/100001/Source/platform/graphic...
> File Source/platform/graphics/paint/DisplayItemList.cpp (right):
> 
>
https://codereview.chromium.org/847783003/diff/100001/Source/platform/graphic...
> Source/platform/graphics/paint/DisplayItemList.cpp:76: info.updateOffset = 0;
> On 2015/01/22 22:17:21, chrishtr wrote:
> > Put this in the constructor instead.
> 
> Done.
> 
>
https://codereview.chromium.org/847783003/diff/100001/Source/platform/graphic...
> Source/platform/graphics/paint/DisplayItemList.cpp:113:
appendDisplayItem(updatedList, newCachedDisplayItemsInfoByClient,
m_paintList[index].release());
> On 2015/01/22 22:17:21, chrishtr wrote:
> > How about replacing 94-113 with:
> > 
> > if (newDisplayItem->isCached() {
> >   size_t index = findNextMatchingCachedItem(*newDisplayItem);
> >   if (index == kNotFound) {
> >     log errors
> >   } else {
> >     ASSERT(!m_paintList[index]->isCached()); // check for bugs
> >     newDisplayItem.swap(m_paintList[index]);
> >   }
> > }
> > appendDisplayItem(updatedList, newCachedDisplayItemsInfoByClient,
> > newDisplayItem.release());
> 
> Just discussed a related issue with trchen@. We noticed that sometimes some
meta display items (items other than DrawingDisplayItem/CachedDisplayItem)
change without invalidation. For example, when an iframe is scrolled, the
transform display items change, but we don't invalidate the iframe. With the old
code, we'll reuse the old transform display items because we find matches,
causing scrolling not working. Your suggested code basically does the same as
the old code.

I don't think so... My suggestion was just for a simpler way to write the code
you already had, without resorting to the continue operator or calling
appendDisplayItem in multiple lines. Did I make a mistake?

> 
> In the new code, we use old display items only for CachedDisplayItems. For
other display items, we don't try to match, but use the new display item
directly. This avoids findNextMachingCachedItem() calls for
non-CachedDisplayItems.
> 
>
https://codereview.chromium.org/847783003/diff/100001/Source/platform/graphic...
> Source/platform/graphics/paint/DisplayItemList.cpp:153:
stringBuilder.append(String::format("{client:%p, mergeIndex:%d,
displayItemIndexes:[", item.key, (int)item.value.updateOffset));
> On 2015/01/22 22:17:21, chrishtr wrote:
> > s/mergeIndex/updateOffset/.
> 
> Done.
> 
> > Or better yet, just ASSERT(item.value.updateOffset
> > == 0) since printing during update is not a common debug need.
> 
> I found this is useful when debugging updatePaintList() method. In gdb, I can
manually call showDebugData() anytime.
> 
> The change in paintListAsDebugString() is also for debugging updatePaintList()
(when we may encounter null entries in the paint list).

Ok.

[Xianzhu, 2015-01-23 23:52:45]

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

https://codereview.chromium.org/847783003/diff/100001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:113:
appendDisplayItem(updatedList, newCachedDisplayItemsInfoByClient,
m_paintList[index].release());
Sorry I was wrong about your suggested code which is actually much better :)

Done.

Didn't add ASSERT(!m_paintList[index]->isCached()) because in debug mode
CachedDisplayItems are added into paint list to visually indicate the errors.

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

The CQ bit was checked by chrishtr@chromium.org

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

lgtm

[commit-bot: I haz the power, 2015-01-24 00:06:30]

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

[commit-bot: I haz the power, 2015-01-24 00:26:22]

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

[commit-bot: I haz the power, 2015-01-24 00:26:23]

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

[Xianzhu, 2015-01-24 00:37:11]

Patchset #8 (id:140001) has been deleted

[Xianzhu, 2015-01-24 00:37:15]

The CQ bit was checked by wangxianzhu@chromium.org

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

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

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

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

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

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

[Xianzhu, 2015-01-26 06:55:23]

Because of the layout test failure, and the potential issues to do full layer
painting, I'd like to hold off on this CL until we find a way to merge or decide
to go on no-merge.

The following merge method seems to work with incremental paint: 
- Issue a special type display item SubtreeCachedDisplayItem (which seems the
best name) before the early returns on no-dirty-rect-intersection;
- (In the future, we can also issue SubtreeCachedDisplayItem when we are sure
that a whole subtree is not invalidated, to avoid unnecessary tree walk and many
CachedDisplayItems for each descendant. Might need to add needsPaint and
childNeedsPaint flags for RenderObject)
- If the paint method doesn't early return, it always issues a pair of display
items (e.g. Clip/EndClip, even if no clip; or other special display item pair);
- In updatePaintList(), if it meets SubtreeCachedDisplayItem, copy the whole
subtree display items.

[Xianzhu, 2015-01-27 18:01:09]

On 2015/01/26 06:55:23, Xianzhu wrote:
> Because of the layout test failure, and the potential issues to do full layer
> painting, I'd like to hold off on this CL until we find a way to merge or
decide
> to go on no-merge.
> 
> The following merge method seems to work with incremental paint: 
> - Issue a special type display item SubtreeCachedDisplayItem (which seems the
> best name) before the early returns on no-dirty-rect-intersection;
> - (In the future, we can also issue SubtreeCachedDisplayItem when we are sure
> that a whole subtree is not invalidated, to avoid unnecessary tree walk and
many
> CachedDisplayItems for each descendant. Might need to add needsPaint and
> childNeedsPaint flags for RenderObject)
> - If the paint method doesn't early return, it always issues a pair of display
> items (e.g. Clip/EndClip, even if no clip; or other special display item
pair);
> - In updatePaintList(), if it meets SubtreeCachedDisplayItem, copy the whole
> subtree display items.

Patch Set 8 is an initial implementation of the above method. PTAL (for
discussion).

Some principles of the method:
- The new paint list is virtually a complete paint list (less some uninterested,
invalidated objects), with
  * SubtreeCachedDisplayItem placeholders for subtrees which are validly cached
but weren't painted in this frame;
  * BeginSubtreeDisplayItems and EndSubtreeDisplayItems to allow the merge
algorithm to know the tree structure when needed;

- The new paint list passes the following additional information to the merge
algorithm:
  * z-order information (by the places of SubtreeCachedDisplayItems in the list)
  * tree structure information

- The merge algorithm replaces SubtreeCachedDisplayItem with cached subtree
display items.

The biggest problem of the initial implementation may be too many BeginSubtree
and EndSubtree display items. For a simple test in ViewDisplayItemListTest, the
total number of display items increased from 6 to 30. However, the empty pairs
are needed because of our linear matching algorithm for a client's display
items, otherwise we need to search the whole list for a mismatched item. To
avoid the empty pairs, we need
- Unique display item ids;
- Secondary map (or ordered list) for each client's display items.
With the above needs satisfied, we can also eliminate the need of dummy display
items which are to help to match CachedDisplayItems for an object which produces
nothing drawable.

With the above, we'll lose some ability to detect under-invalidations. However,
because the detection is incomplete (e.g. it can't detect drawing changes within
a display item), definitely we need a better way to discover
under-invalidations.

[Xianzhu, 2015-01-27 19:09:29]

Sorry, just noticed that uploading of the patch failed. Patch Set 9 is the
initial implementation of the method described in #25 and #26.

[Xianzhu, 2015-01-27 19:13:47]

Patchset #9 (id:180001) has been deleted

[Stephen Chennney, 2015-01-27 19:45:45]

This seems to reflect one of the options we discussed yesterday, which is all I
feel qualified to comment on.

https://codereview.chromium.org/847783003/diff/200001/Source/platform/graphic...
File Source/platform/graphics/paint/CachedDisplayItem.cpp (right):

https://codereview.chromium.org/847783003/diff/200001/Source/platform/graphic...
Source/platform/graphics/paint/CachedDisplayItem.cpp:19:
paint.setColor(SK_ColorRED);
bikeshed: Red is probably OK, but better is something even more obnoxious and
not likely to be seen on a page. Purple might be better because it is very rare
in web content and stands out visually.

[trchen, 2015-01-27 19:52:01]

https://codereview.chromium.org/847783003/diff/200001/Source/platform/graphic...
File Source/platform/graphics/paint/CachedDisplayItem.cpp (right):

https://codereview.chromium.org/847783003/diff/200001/Source/platform/graphic...
Source/platform/graphics/paint/CachedDisplayItem.cpp:19:
paint.setColor(SK_ColorRED);
On 2015/01/27 19:45:45, Stephen Chenney wrote:
> bikeshed: Red is probably OK, but better is something even more obnoxious and
> not likely to be seen on a page. Purple might be better because it is very
rare
> in web content and stands out visually.

I would vote for red for the sake of consistency, since it is already used in
GraphicsLayer and FramePainter for debugging.

[chrishtr, 2015-01-29 19:04:38]

https://codereview.chromium.org/847783003/diff/220001/Source/core/paint/Block...
File Source/core/paint/BlockPainter.cpp (right):

https://codereview.chromium.org/847783003/diff/220001/Source/core/paint/Block...
Source/core/paint/BlockPainter.cpp:36: SubtreeInfoRecorder
subtreeInfoRecorder(paintInfo.context, m_renderBlock, localPaintInfo.phase);
Is the change in this file just an example?

https://codereview.chromium.org/847783003/diff/220001/Source/platform/graphic...
File Source/platform/graphics/paint/CachedDisplayItem.h (right):

https://codereview.chromium.org/847783003/diff/220001/Source/platform/graphic...
Source/platform/graphics/paint/CachedDisplayItem.h:5: #ifndef
CachedDisplayItem_h
Should this class just go away?

[Xianzhu, 2015-01-29 19:55:03]

I'm still working on unique ids which is needed by the final patch of this CL.
There will be multiple CLs. The first one refactors DisplayItem::Type. The
others will fix improper display item clients (e.g. RenderText/RenderInline vs
inline boxes, ScrollbarTheme vs Scrollbar) and some special types of drawings
(e.g. BoxDecorationBackground).

https://codereview.chromium.org/847783003/diff/220001/Source/core/paint/Block...
File Source/core/paint/BlockPainter.cpp (right):

https://codereview.chromium.org/847783003/diff/220001/Source/core/paint/Block...
Source/core/paint/BlockPainter.cpp:36: SubtreeInfoRecorder
subtreeInfoRecorder(paintInfo.context, m_renderBlock, localPaintInfo.phase);
On 2015/01/29 19:04:38, chrishtr wrote:
> Is the change in this file just an example?

Actually I wanted to insert this code at all early returns like here, but I was
not sure how to find all of them. Would be great if you already had list of them
(partial list would be also helpful) :)

https://codereview.chromium.org/847783003/diff/220001/Source/platform/graphic...
File Source/platform/graphics/paint/CachedDisplayItem.h (right):

https://codereview.chromium.org/847783003/diff/220001/Source/platform/graphic...
Source/platform/graphics/paint/CachedDisplayItem.h:5: #ifndef
CachedDisplayItem_h
On 2015/01/29 19:04:38, chrishtr wrote:
> Should this class just go away?

This class is still needed for placeholders of individual cached display items.
We may have an uninvalidated ancestor with invalidated descendants. In this
case, CachedDisplayItems are needed for the display items of the ancestor.

[chrishtr, 2015-01-29 21:41:08]

On 2015/01/29 at 19:55:03, wangxianzhu wrote:
> I'm still working on unique ids which is needed by the final patch of this CL.
There will be multiple CLs. The first one refactors DisplayItem::Type. The
others will fix improper display item clients (e.g. RenderText/RenderInline vs
inline boxes, ScrollbarTheme vs Scrollbar) and some special types of drawings
(e.g. BoxDecorationBackground).

Ok.

> 
>
https://codereview.chromium.org/847783003/diff/220001/Source/core/paint/Block...
> File Source/core/paint/BlockPainter.cpp (right):
> 
>
https://codereview.chromium.org/847783003/diff/220001/Source/core/paint/Block...
> Source/core/paint/BlockPainter.cpp:36: SubtreeInfoRecorder
subtreeInfoRecorder(paintInfo.context, m_renderBlock, localPaintInfo.phase);
> On 2015/01/29 19:04:38, chrishtr wrote:
> > Is the change in this file just an example?
> 
> Actually I wanted to insert this code at all early returns like here, but I
was not sure how to find all of them. Would be great if you already had list of
them (partial list would be also helpful) :)
> 
>
https://codereview.chromium.org/847783003/diff/220001/Source/platform/graphic...
> File Source/platform/graphics/paint/CachedDisplayItem.h (right):
> 
>
https://codereview.chromium.org/847783003/diff/220001/Source/platform/graphic...
> Source/platform/graphics/paint/CachedDisplayItem.h:5: #ifndef
CachedDisplayItem_h
> On 2015/01/29 19:04:38, chrishtr wrote:
> > Should this class just go away?
> 
> This class is still needed for placeholders of individual cached display
items. We may have an uninvalidated ancestor with invalidated descendants. In
this case, CachedDisplayItems are needed for the display items of the ancestor.

[chrishtr, 2015-01-31 01:18:11]

https://codereview.chromium.org/847783003/diff/220001/Source/core/paint/Block...
File Source/core/paint/BlockPainter.cpp (right):

https://codereview.chromium.org/847783003/diff/220001/Source/core/paint/Block...
Source/core/paint/BlockPainter.cpp:36: SubtreeInfoRecorder
subtreeInfoRecorder(paintInfo.context, m_renderBlock, localPaintInfo.phase);
On 2015/01/29 19:55:02, Xianzhu wrote:
> On 2015/01/29 19:04:38, chrishtr wrote:
> > Is the change in this file just an example?
> 
> Actually I wanted to insert this code at all early returns like here, but I
was
> not sure how to find all of them. Would be great if you already had list of
them
> (partial list would be also helpful) :)

Sorry, I don't have that info either..

https://codereview.chromium.org/847783003/diff/220001/Source/platform/graphic...
File Source/platform/graphics/paint/CachedDisplayItem.h (right):

https://codereview.chromium.org/847783003/diff/220001/Source/platform/graphic...
Source/platform/graphics/paint/CachedDisplayItem.h:5: #ifndef
CachedDisplayItem_h
On 2015/01/29 at 19:55:03, Xianzhu wrote:
> On 2015/01/29 19:04:38, chrishtr wrote:
> > Should this class just go away?
> 
> This class is still needed for placeholders of individual cached display
items. We may have an uninvalidated ancestor with invalidated descendants. In
this case, CachedDisplayItems are needed for the display items of the ancestor.

Ah, good point.

[Xianzhu, 2015-02-02 20:07:46]

Switched to https://codereview.chromium.org/892293002 for new merging algorithm.
This one closed.

[Xianzhu, 2015-02-04 19:16:38]

Patchset #10 (id:220001) has been deleted