First version of new merge algorithm (not enabled yet)

First version of new merge algorithm (not enabled yet)

The new paint list contains necessary information to keep the z order
and parent-children relationship, so that it is virtually a complete
paint list.

Additional meta display items are:
- SubtreeCachedDisplayItem: a subtree is skipped during paint (maybe
  because of out-of-viewport);
- BeginSubtreeDisplayItem and EndtreeDisplayItem: when a subtree is
  painted, they mark the begin/end of the subtree's display items in
  the new paint list.

When the merge algorithm meets a SubtreeCachedDisplayItem, it matches
it to the cached BeginSubtreeDisplayItem, and copy the subtree until
the corresponding EndSubtreeDisplayItem is reached.

This algorithm requires display item ids to be unique. We still have
many cases of duplicated display item ids. Only after we fix all of
the duplicated id cases can this algorithm fully work.

For now it's controlled by RuntimeEnabledFeatures::
slimmingPaintDisplayItemCacheEnabled.

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

[Xianzhu, 2015-02-02 20:11:58]

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

[Xianzhu, 2015-02-02 20:11:59]

This is the first version. It still requires unique display item ids which has
not been ensured. Instead of fixing all issues of unique display item id issues
to make the algorithm fully work, I'd like to send this out earlier which is
disabled by RuntimeEnabledFlags::slimmingPaintDisplayItemCached. Will enable it
after it fully works.

[chrishtr, 2015-02-03 00:39:00]

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

https://codereview.chromium.org/892293002/diff/80001/Source/platform/graphics...
Source/platform/graphics/paint/DisplayItemList.cpp:32: // Remove empty pairs.
Why bother to clean these out?

https://codereview.chromium.org/892293002/diff/80001/Source/platform/graphics...
Source/platform/graphics/paint/DisplayItemList.cpp:103: return;
Don't you want to insert the CachedSubtreeDisplayItem in this case?

https://codereview.chromium.org/892293002/diff/80001/Source/platform/graphics...
Source/platform/graphics/paint/DisplayItemList.cpp:111: }
Add an assert that it found the endSubtreeId. Better yet, why not look up the
index of the first and last in the displayItemIndexMap, and do a simple for loop
count between them?

https://codereview.chromium.org/892293002/diff/80001/Source/platform/graphics...
Source/platform/graphics/paint/DisplayItemList.cpp:136: if
(newDisplayItem->isSubtreeCached()) {
Subtree cached can be thought of as a type of cached, right? Then you can use
the same helper method to map from cached display item to non-cached.
Then if you add a fallback to the existing cached item if the non-cached is not
present, you can also avoid the 'continue' jumps.

[Xianzhu, 2015-02-03 01:57:13]

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

https://codereview.chromium.org/892293002/diff/80001/Source/platform/graphics...
Source/platform/graphics/paint/DisplayItemList.cpp:32: // Remove empty pairs.
On 2015/02/03 00:39:00, chrishtr wrote:
> Why bother to clean these out?

Without this, most of the display items in a paint list would be empty
BeginSubtree/EndSubtree pairs.

Tested ViewDisplayListTest.FullDocumentPaintingWithCaret without this, and it
produced 30 display items (originally 2). With this code, we generate 10 display
items (see the patched ViewDisplayListTest.cpp).

https://codereview.chromium.org/892293002/diff/80001/Source/platform/graphics...
Source/platform/graphics/paint/DisplayItemList.cpp:103: return;
On 2015/02/03 00:39:00, chrishtr wrote:
> Don't you want to insert the CachedSubtreeDisplayItem in this case?

No. This is a valid case that the subtree previously produced nothing. We don't
store empty BeginSubtree/EndSubtree pairs otherwise there would be too many of
them.

Added comments about the case.

https://codereview.chromium.org/892293002/diff/80001/Source/platform/graphics...
Source/platform/graphics/paint/DisplayItemList.cpp:111: }
On 2015/02/03 00:39:00, chrishtr wrote:
> Add an assert that it found the endSubtreeId. Better yet, why not look up the
> index of the first and last in the displayItemIndexMap, and do a simple for
loop
> count between them?

Looping between first/last looks clearer. Done.

In this patch only Drawing and BeginSubtree display items are mapped. The
limitation seems not necessary. Removed it.

https://codereview.chromium.org/892293002/diff/80001/Source/platform/graphics...
Source/platform/graphics/paint/DisplayItemList.cpp:136: if
(newDisplayItem->isSubtreeCached()) {
On 2015/02/03 00:39:00, chrishtr wrote:
> Subtree cached can be thought of as a type of cached, right? Then you can use
> the same helper method to map from cached display item to non-cached.
> Then if you add a fallback to the existing cached item if the non-cached is
not
> present, you can also avoid the 'continue' jumps.

There seems little code to share by SubtreeCached and normal Cached (extracted
findDisplayItemById though in the new patch set).

The 'continue' jumps is to skip the display item which previously generated
nothing (empty subtree or empty picture), so seems unavoidable. (I could use a
bool variable to avoid the 'continue's but this seems no better).

The patch in https://codereview.chromium.org/847783003/ used the 'not-found'
result as an indicator of under-invalidation and appended the cached display
items into paint list to draw visual indications. However, the method is far
from complete to detect under-invalidations, and requires us to keep empty
display items (which was also a must when we used linear storage, but not
necessary when we use the map).

[chrishtr, 2015-02-03 22:30:48]

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

https://codereview.chromium.org/892293002/diff/120001/Source/core/paint/Block...
Source/core/paint/BlockPainter.cpp:37: SubtreeInfoRecorder
subtreeInfoRecorder(paintInfo.context, m_renderBlock, localPaintInfo.phase);
Just to be sure, this is just one example right?

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

https://codereview.chromium.org/892293002/diff/120001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:32: // Remove empty pairs.
Why would a begin and end be output rather than cached subtree in this case?

https://codereview.chromium.org/892293002/diff/120001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:54: void
DisplayItemList::invalidateAll()
Also clear out m_displayItemIndexMap?

https://codereview.chromium.org/892293002/diff/120001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:151: m_newPaints.clear();
Maybe factor into a clear() method that shares code with invalidateAll().

https://codereview.chromium.org/892293002/diff/120001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemList.h (right):

https://codereview.chromium.org/892293002/diff/120001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.h:32: const PaintList&
newPaintListForTesting() const { return m_newPaints; }
friend classses would be better for these two. Chromium has FRIEND_TEST to make
this easy..

[Xianzhu, 2015-02-04 00:13:24]

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

https://codereview.chromium.org/892293002/diff/120001/Source/core/paint/Block...
Source/core/paint/BlockPainter.cpp:37: SubtreeInfoRecorder
subtreeInfoRecorder(paintInfo.context, m_renderBlock, localPaintInfo.phase);
On 2015/02/03 22:30:47, chrishtr wrote:
> Just to be sure, this is just one example right?

Yes. This looks the most common case of subtree painting. Adding of this code
can be incremental. For other subtree painting code if any, we'll repaint the
subtree when it appears the next time.

I'll find other places and add this code, perhaps in later changes.

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

https://codereview.chromium.org/892293002/diff/120001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:32: // Remove empty pairs.
On 2015/02/03 22:30:47, chrishtr wrote:
> Why would a begin and end be output rather than cached subtree in this case?

We output both: for subtrees that are painted (subtreeInfoRecorder.begin()
called), we'll output BeginSubtree and EndSubtree. For skipped subtrees we'll
output SubtreeCached.

Subtree invalid/valid flag in this change yet, so we don't output SubtreeCached
for non-invalidated subtree for now. I'll do it in later changes.

The above code also removes many empty Clip/EndClip, Transform/EndTransform,
etc. pairs.

Based on my test, most of the empty pairs are for phases that produces nothing.

https://codereview.chromium.org/892293002/diff/120001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:54: void
DisplayItemList::invalidateAll()
On 2015/02/03 22:30:47, chrishtr wrote:
> Also clear out m_displayItemIndexMap?

Done.

https://codereview.chromium.org/892293002/diff/120001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemList.h (right):

https://codereview.chromium.org/892293002/diff/120001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.h:32: const PaintList&
newPaintListForTesting() const { return m_newPaints; }
On 2015/02/03 22:30:48, chrishtr wrote:
> friend classses would be better for these two. Chromium has FRIEND_TEST to
make
> this easy..

For now we can't include either <gtest/gtest.h> or "base/gtest_prod_util.h" from
non-testing blink code.

Use another way: let the test base classes be friend, and access the private
member in the base classes.

[Xianzhu, 2015-02-04 00:15:17]

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

https://codereview.chromium.org/892293002/diff/120001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:32: // Remove empty pairs.
s/Subtree invalid\/valid flag in this change yet,.../Haven't added subtree
invalid\/valid flag in this change yet,.../

[Xianzhu, 2015-02-04 00:36:54]

Patchset #9 (id:160001) has been deleted

[pdr., 2015-02-04 05:19:58]

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

https://codereview.chromium.org/892293002/diff/180001/Source/core/paint/Block...
Source/core/paint/BlockPainter.cpp:37: SubtreeInfoRecorder
subtreeInfoRecorder(paintInfo.context, m_renderBlock, localPaintInfo.phase);
Can you move this up one line? The localPaintInfo phase will actually be
modified below to be the child's phase.

https://codereview.chromium.org/892293002/diff/180001/Source/core/paint/Subtr...
File Source/core/paint/SubtreeInfoRecorder.h (right):

https://codereview.chromium.org/892293002/diff/180001/Source/core/paint/Subtr...
Source/core/paint/SubtreeInfoRecorder.h:18: class SubtreeInfoRecorder {
Could you add a short comment here about what this is for?


I wonder if we should name this "StackingContextRecorder" since it maps to the
web painting algorithm's concept of a stacking context?

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

https://codereview.chromium.org/892293002/diff/180001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItem.h:222: bool isBegin() const { return
isClip() || isFloatClip() || isScroll() || isBeginSubtree() || type() ==
BeginFilter || type() == BeginCompositing || type() == BeginTransform || type()
== BeginClipPath; }
Can we introduce a subclass of paired display item types to make this cleaner?
Keeping this up to date is error-prone.

https://codereview.chromium.org/892293002/diff/180001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemList.h (right):

https://codereview.chromium.org/892293002/diff/180001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.h:66: DisplayItemClientSet
m_cachedClients;
We're adding a lot of complexity (exposing Id, hashing Id, two hash tables, and
keeping the map and set synced) for what may not be a lot of gain. What's the
maximum set of unique ids any given client can have? I wonder if it would be
both faster and simpler to track a HashSet<client, Vector<DisplayItemClient>>?

https://codereview.chromium.org/892293002/diff/180001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.h:67: DisplayItemIndexMap
m_displayItemIndexMap;
Can you add a comment here saying that this maps from display item to an index
in the paint list?

[chrishtr, 2015-02-04 07:08:14]

https://codereview.chromium.org/892293002/diff/180001/Source/core/paint/Subtr...
File Source/core/paint/SubtreeInfoRecorder.h (right):

https://codereview.chromium.org/892293002/diff/180001/Source/core/paint/Subtr...
Source/core/paint/SubtreeInfoRecorder.h:18: class SubtreeInfoRecorder {
On 2015/02/04 at 05:19:58, pdr wrote:
> Could you add a short comment here about what this is for?
> 
> 
> I wonder if we should name this "StackingContextRecorder" since it maps to the
web painting algorithm's concept of a stacking context?

It's not quite the same as stacking context. It can refer to a subtree only
within a given phase, for example.

But I agree the class needs a comment.

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

https://codereview.chromium.org/892293002/diff/180001/Source/core/paint/ViewD...
Source/core/paint/ViewDisplayListTest.cpp:440:
EXPECT_DISPLAY_LIST(rootDisplayItemList().paintList(), 10,
Why are there so many changes in this test?

[Xianzhu, 2015-02-04 20:46:32]

PTAL. The new patch set is based on
https://codereview.chromium.org/847783003/#ps200001.

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

https://codereview.chromium.org/892293002/diff/180001/Source/core/paint/Block...
Source/core/paint/BlockPainter.cpp:37: SubtreeInfoRecorder
subtreeInfoRecorder(paintInfo.context, m_renderBlock, localPaintInfo.phase);
On 2015/02/04 05:19:58, pdr wrote:
> Can you move this up one line? The localPaintInfo phase will actually be
> modified below to be the child's phase.

Done.

https://codereview.chromium.org/892293002/diff/180001/Source/core/paint/Subtr...
File Source/core/paint/SubtreeInfoRecorder.h (right):

https://codereview.chromium.org/892293002/diff/180001/Source/core/paint/Subtr...
Source/core/paint/SubtreeInfoRecorder.h:18: class SubtreeInfoRecorder {
On 2015/02/04 07:08:14, chrishtr wrote:
> On 2015/02/04 at 05:19:58, pdr wrote:
> > Could you add a short comment here about what this is for?
> > 
> > 
> > I wonder if we should name this "StackingContextRecorder" since it maps to
the
> web painting algorithm's concept of a stacking context?
> 
> It's not quite the same as stacking context. It can refer to a subtree only
> within a given phase, for example.
> 
> But I agree the class needs a comment.

Done.

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

https://codereview.chromium.org/892293002/diff/180001/Source/core/paint/ViewD...
Source/core/paint/ViewDisplayListTest.cpp:440:
EXPECT_DISPLAY_LIST(rootDisplayItemList().paintList(), 10,
On 2015/02/04 07:08:14, chrishtr wrote:
> Why are there so many changes in this test?

- Some test cases containing incremental painting behavior were disabled when we
disable display item caching. In this patch some of them are re-enabled. As
caching feature is tested with new test cases, the original cases are either
re-enabled with incremental painting behavior removed, or removed if test case
is no longer applicable and replaced by new tests.

- To test the caching behavior before updatePaintList(), removed the anonymous
namespace to allow the test class to be friend of DisplayItemList.

- BTW, removed redundant ViewDisplayListTest_ prefix of test case names.

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

https://codereview.chromium.org/892293002/diff/180001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItem.h:222: bool isBegin() const { return
isClip() || isFloatClip() || isScroll() || isBeginSubtree() || type() ==
BeginFilter || type() == BeginCompositing || type() == BeginTransform || type()
== BeginClipPath; }
On 2015/02/04 05:19:58, pdr wrote:
> Can we introduce a subclass of paired display item types to make this cleaner?
> Keeping this up to date is error-prone.

Done and separated into https://codereview.chromium.org/900653003/.

Tried to avoid virtual functions, but it seems not worth it to sacrifice code
maintainability.

https://codereview.chromium.org/892293002/diff/180001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemList.h (right):

https://codereview.chromium.org/892293002/diff/180001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.h:66: DisplayItemClientSet
m_cachedClients;
On 2015/02/04 05:19:58, pdr wrote:
> We're adding a lot of complexity (exposing Id, hashing Id, two hash tables,
and
> keeping the map and set synced) for what may not be a lot of gain. What's the
> maximum set of unique ids any given client can have? I wonder if it would be
> both faster and simpler to track a HashSet<client, Vector<DisplayItemClient>>?

Actually the method was used in an older version of the new merge algorithm
(https://codereview.chromium.org/847783003/#ps200001). It was based on the
implication that the number and order of all new display items of a
non-invalidated client match those of all cached display items of the client.
With this implication, the algorithm can be simpler (and we don't need unique
display item ids). Some drawbacks might be:

- we must keep all empty BeginSubtree/EndSubtree display item pairs (as well as
dummy display items) whose number can be very large. Based on my test more than
50% of the paint list are these empty pairs;

- it prevents us from doing more optimizations (crbug.com/451090), e.g. adding a
child without invalidating the parent if the parent's drawings are not affected.

I think we can use the vector method first at least, and evaluate if it's worth
to avoid the above drawbacks.

[chrishtr, 2015-02-04 21:58:01]

On 2015/02/04 at 07:08:14, chrishtr wrote:
>
https://codereview.chromium.org/892293002/diff/180001/Source/core/paint/Subtr...
> File Source/core/paint/SubtreeInfoRecorder.h (right):
> 
>
https://codereview.chromium.org/892293002/diff/180001/Source/core/paint/Subtr...
> Source/core/paint/SubtreeInfoRecorder.h:18: class SubtreeInfoRecorder {
> On 2015/02/04 at 05:19:58, pdr wrote:
> > Could you add a short comment here about what this is for?
> > 
> > 
> > I wonder if we should name this "StackingContextRecorder" since it maps to
the web painting algorithm's concept of a stacking context?
> 
> It's not quite the same as stacking context. It can refer to a subtree only
within a given phase, for example.
> 
> But I agree the class needs a comment.
> 
>
https://codereview.chromium.org/892293002/diff/180001/Source/core/paint/ViewD...
> File Source/core/paint/ViewDisplayListTest.cpp (right):
> 
>
https://codereview.chromium.org/892293002/diff/180001/Source/core/paint/ViewD...
> Source/core/paint/ViewDisplayListTest.cpp:440:
EXPECT_DISPLAY_LIST(rootDisplayItemList().paintList(), 10,
> Why are there so many changes in this test?

let's discuss here:

https://codereview.chromium.org/892293002/

[pdr., 2015-02-05 00:06:22]

On 2015/02/04 at 20:46:32, wangxianzhu wrote:
>
https://codereview.chromium.org/892293002/diff/180001/Source/platform/graphic...
> Source/platform/graphics/paint/DisplayItemList.h:66: DisplayItemClientSet
m_cachedClients;
> On 2015/02/04 05:19:58, pdr wrote:
> > We're adding a lot of complexity (exposing Id, hashing Id, two hash tables,
and
> > keeping the map and set synced) for what may not be a lot of gain. What's
the
> > maximum set of unique ids any given client can have? I wonder if it would be
> > both faster and simpler to track a HashSet<client,
Vector<DisplayItemClient>>?
> 
> Actually the method was used in an older version of the new merge algorithm
(https://codereview.chromium.org/847783003/#ps200001). It was based on the
implication that the number and order of all new display items of a
non-invalidated client match those of all cached display items of the client.
With this implication, the algorithm can be simpler (and we don't need unique
display item ids). Some drawbacks might be:
> 
> - we must keep all empty BeginSubtree/EndSubtree display item pairs (as well
as dummy display items) whose number can be very large. Based on my test more
than 50% of the paint list are these empty pairs;
> 
> - it prevents us from doing more optimizations (crbug.com/451090), e.g. adding
a child without invalidating the parent if the parent's drawings are not
affected.
> 
> I think we can use the vector method first at least, and evaluate if it's
worth to avoid the above drawbacks.

I did a poor job of explaining what I meant. I was suggesting a hybrid of the
two where we still have unique ids but we track them in a vector instead of as a
separate map.

The previous approach used the following:
HashSet<DisplayItemClient> m_cachedClients
HashMap<DisplayItem::Id, size_t> m_displayItemIndexMap
struct Id { DisplayItemClient, Type }

I meant something like the following:
HashMap<DisplayItemClient, Vector<size_t>> m_cachedClients

To answer a query for whether DisplayItemClient, Type is cached, you would do:
bool isCached(DisplayItemClient client, Type type) {
    Vector<size_t>* indices = m_cachedClients.get(client);
    if (!indices) return false;
    for (size_t index : indices) {
        if (m_displayList[index].type == type)
            return true;
    }
    return false;
}

The idea is just to do linear probing ourselves because we know there will be
relatively few types per client. The PairInt hash function is poor and I suspect
it would be slower with even moderate load factors on the HashMap. This approach
lets us keep Id private in DisplayItem and will be simpler overall.

[chrishtr, 2015-02-05 01:06:33]

On 2015/02/05 at 00:06:22, pdr wrote:
> On 2015/02/04 at 20:46:32, wangxianzhu wrote:
> >
https://codereview.chromium.org/892293002/diff/180001/Source/platform/graphic...
> > Source/platform/graphics/paint/DisplayItemList.h:66: DisplayItemClientSet
m_cachedClients;
> > On 2015/02/04 05:19:58, pdr wrote:
> > > We're adding a lot of complexity (exposing Id, hashing Id, two hash
tables, and
> > > keeping the map and set synced) for what may not be a lot of gain. What's
the
> > > maximum set of unique ids any given client can have? I wonder if it would
be
> > > both faster and simpler to track a HashSet<client,
Vector<DisplayItemClient>>?
> > 
> > Actually the method was used in an older version of the new merge algorithm
(https://codereview.chromium.org/847783003/#ps200001). It was based on the
implication that the number and order of all new display items of a
non-invalidated client match those of all cached display items of the client.
With this implication, the algorithm can be simpler (and we don't need unique
display item ids). Some drawbacks might be:
> > 
> > - we must keep all empty BeginSubtree/EndSubtree display item pairs (as well
as dummy display items) whose number can be very large. Based on my test more
than 50% of the paint list are these empty pairs;
> > 
> > - it prevents us from doing more optimizations (crbug.com/451090), e.g.
adding a child without invalidating the parent if the parent's drawings are not
affected.
> > 
> > I think we can use the vector method first at least, and evaluate if it's
worth to avoid the above drawbacks.
> 
> I did a poor job of explaining what I meant. I was suggesting a hybrid of the
two where we still have unique ids but we track them in a vector instead of as a
separate map.
> 
> The previous approach used the following:
> HashSet<DisplayItemClient> m_cachedClients
> HashMap<DisplayItem::Id, size_t> m_displayItemIndexMap
> struct Id { DisplayItemClient, Type }
> 
> I meant something like the following:
> HashMap<DisplayItemClient, Vector<size_t>> m_cachedClients
> 
> To answer a query for whether DisplayItemClient, Type is cached, you would do:
> bool isCached(DisplayItemClient client, Type type) {
>     Vector<size_t>* indices = m_cachedClients.get(client);
>     if (!indices) return false;
>     for (size_t index : indices) {
>         if (m_displayList[index].type == type)
>             return true;
>     }
>     return false;
> }
> 
> The idea is just to do linear probing ourselves because we know there will be
relatively few types per client. The PairInt hash function is poor and I suspect
it would be slower with even moderate load factors on the HashMap. This approach
lets us keep Id private in DisplayItem and will be simpler overall.

? I think that is what the latest patchset does, no?

[Xianzhu, 2015-02-05 02:12:01]

On 2015/02/05 01:06:33, chrishtr wrote:
> On 2015/02/05 at 00:06:22, pdr wrote:
> > On 2015/02/04 at 20:46:32, wangxianzhu wrote:
> > >
>
https://codereview.chromium.org/892293002/diff/180001/Source/platform/graphic...
> > > Source/platform/graphics/paint/DisplayItemList.h:66: DisplayItemClientSet
> m_cachedClients;
> > > On 2015/02/04 05:19:58, pdr wrote:
> > > > We're adding a lot of complexity (exposing Id, hashing Id, two hash
> tables, and
> > > > keeping the map and set synced) for what may not be a lot of gain.
What's
> the
> > > > maximum set of unique ids any given client can have? I wonder if it
would
> be
> > > > both faster and simpler to track a HashSet<client,
> Vector<DisplayItemClient>>?
> > > 
> > > Actually the method was used in an older version of the new merge
algorithm
> (https://codereview.chromium.org/847783003/#ps200001). It was based on the
> implication that the number and order of all new display items of a
> non-invalidated client match those of all cached display items of the client.
> With this implication, the algorithm can be simpler (and we don't need unique
> display item ids). Some drawbacks might be:
> > > 
> > > - we must keep all empty BeginSubtree/EndSubtree display item pairs (as
well
> as dummy display items) whose number can be very large. Based on my test more
> than 50% of the paint list are these empty pairs;
> > > 
> > > - it prevents us from doing more optimizations (crbug.com/451090), e.g.
> adding a child without invalidating the parent if the parent's drawings are
not
> affected.
> > > 
> > > I think we can use the vector method first at least, and evaluate if it's
> worth to avoid the above drawbacks.
> > 
> > I did a poor job of explaining what I meant. I was suggesting a hybrid of
the
> two where we still have unique ids but we track them in a vector instead of as
a
> separate map.
> > 
> > The previous approach used the following:
> > HashSet<DisplayItemClient> m_cachedClients
> > HashMap<DisplayItem::Id, size_t> m_displayItemIndexMap
> > struct Id { DisplayItemClient, Type }
> > 
> > I meant something like the following:
> > HashMap<DisplayItemClient, Vector<size_t>> m_cachedClients
> > 
> > To answer a query for whether DisplayItemClient, Type is cached, you would
do:
> > bool isCached(DisplayItemClient client, Type type) {
> >     Vector<size_t>* indices = m_cachedClients.get(client);
> >     if (!indices) return false;
> >     for (size_t index : indices) {
> >         if (m_displayList[index].type == type)
> >             return true;
> >     }
> >     return false;
> > }
> > 
> > The idea is just to do linear probing ourselves because we know there will
be
> relatively few types per client. The PairInt hash function is poor and I
suspect
> it would be slower with even moderate load factors on the HashMap. This
approach
> lets us keep Id private in DisplayItem and will be simpler overall.
> 
> ? I think that is what the latest patchset does, no?

The patchset doesn't go over the whole vector so requires the order implication.
Will upload the new patchset soon, which will remove the order requirement so
that we can avoid the many empty beginsubtree/endsubtree pairs as well as the
dummy display items.

[Xianzhu, 2015-02-05 03:21:00]

On 2015/02/05 00:06:22, pdr wrote:
> 
> The idea is just to do linear probing ourselves because we know there will be
> relatively few types per client. The PairInt hash function is poor and I
suspect
> it would be slower with even moderate load factors on the HashMap. This
approach
> lets us keep Id private in DisplayItem and will be simpler overall.

Done. PTAL.

[Xianzhu, 2015-02-05 03:24:28]

Patchset #13 (id:250001) has been deleted

[pdr., 2015-02-05 03:35:58]

I think this is looking great. A few minor comments below.

https://codereview.chromium.org/892293002/diff/270001/Source/core/paint/Subtr...
File Source/core/paint/SubtreeInfoRecorder.h (right):

https://codereview.chromium.org/892293002/diff/270001/Source/core/paint/Subtr...
Source/core/paint/SubtreeInfoRecorder.h:21: class SubtreeInfoRecorder {
(Optional) bikeshed: SubtreeRecorder?

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

https://codereview.chromium.org/892293002/diff/270001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:132: if
(newDisplayItem->isSubtreeCached()) {
It looks like we may be able to combine this with the isCached branch below into
a single function: copyCachedItems(), and then have the subtree logic in that
function.

https://codereview.chromium.org/892293002/diff/270001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemList.h (right):

https://codereview.chromium.org/892293002/diff/270001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.h:56: // Indices into PaintList
of all DrawingDisplayItems of a client.
Do we need to track these separately or can we just use a single vector here?

[Xianzhu, 2015-02-05 17:57:03]

https://codereview.chromium.org/892293002/diff/270001/Source/core/paint/Subtr...
File Source/core/paint/SubtreeInfoRecorder.h (right):

https://codereview.chromium.org/892293002/diff/270001/Source/core/paint/Subtr...
Source/core/paint/SubtreeInfoRecorder.h:21: class SubtreeInfoRecorder {
On 2015/02/05 03:35:57, pdr wrote:
> (Optional) bikeshed: SubtreeRecorder?

SubtreeRecorder looks better. Done.

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

https://codereview.chromium.org/892293002/diff/270001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:132: if
(newDisplayItem->isSubtreeCached()) {
On 2015/02/05 03:35:57, pdr wrote:
> It looks like we may be able to combine this with the isCached branch below
into
> a single function: copyCachedItems(), and then have the subtree logic in that
> function.

Done.

https://codereview.chromium.org/892293002/diff/270001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemList.h (right):

https://codereview.chromium.org/892293002/diff/270001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.h:56: // Indices into PaintList
of all DrawingDisplayItems of a client.
On 2015/02/05 03:35:57, pdr wrote:
> Do we need to track these separately or can we just use a single vector here?

Thought to reduce the number of items of each vector. Seems not necessary
because the total number will not be big, and seems to sacrifice readibility.

Combined them.

[chrishtr, 2015-02-05 18:22:44]

LGTM modulo pdr's comments.

[Xianzhu, 2015-02-05 20:17:58]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-02-05 20:19:11]

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

[commit-bot: I haz the power, 2015-02-05 20:19:28]

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

[commit-bot: I haz the power, 2015-02-05 20:19:30]

Failed to apply patch for Source/core/paint/ViewDisplayListTest.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/paint/ViewDisplayListTest.cpp
  Hunk #1 FAILED at 5.
  1 out of 18 hunks FAILED -- saving rejects to file
Source/core/paint/ViewDisplayListTest.cpp.rej

Patch:       Source/core/paint/ViewDisplayListTest.cpp
Index: Source/core/paint/ViewDisplayListTest.cpp
diff --git a/Source/core/paint/ViewDisplayListTest.cpp
b/Source/core/paint/ViewDisplayListTest.cpp
index
925ca4f4be1a5ee5059317b5bb4c459e2a90e126..6d3fc037a2e19d9efb339463e81c4fa53d3047e7
100644
--- a/Source/core/paint/ViewDisplayListTest.cpp
+++ b/Source/core/paint/ViewDisplayListTest.cpp
@@ -5,9 +5,11 @@
 #include "config.h"
 
 #include "core/layout/compositing/LayerCompositor.h"
+#include "core/page/FocusController.h"
 #include "core/paint/LayerClipRecorder.h"
 #include "core/paint/LayerPainter.h"
 #include "core/paint/RenderDrawingRecorder.h"
+#include "core/paint/SubtreeRecorder.h"
 #include "core/rendering/InlineTextBox.h"
 #include "core/rendering/RenderText.h"
 #include "core/rendering/RenderView.h"
@@ -19,7 +21,6 @@
 #include <gtest/gtest.h>
 
 namespace blink {
-namespace {
 
 class ViewDisplayListTest : public RenderingTest {
 public:
@@ -28,6 +29,7 @@ public:
 protected:
     RenderView* renderView() { return m_renderView; }
     DisplayItemList& rootDisplayItemList() { return
*renderView()->layer()->graphicsLayerBacking()->displayItemList(); }
+    const Vector<OwnPtr<DisplayItem>>& newPaintListBeforeUpdate() { return
rootDisplayItemList().m_newPaints; }
 
 private:
     virtual void SetUp() override
@@ -60,18 +62,18 @@ public:
 };
 
 #ifndef NDEBUG
-#define TRACE_DISPLAY_ITEMS(expected, actual) \
-    String trace = "Expected: " + (expected).asDebugString() + " Actual: " +
(actual).asDebugString(); \
+#define TRACE_DISPLAY_ITEMS(i, expected, actual) \
+    String trace = String::format("%d: ", (int)i) + "Expected: " +
(expected).asDebugString() + " Actual: " + (actual).asDebugString(); \
     SCOPED_TRACE(trace.utf8().data());
 #else
-#define TRACE_DISPLAY_ITEMS(expected, actual)
+#define TRACE_DISPLAY_ITEMS(i, expected, actual)
 #endif
 
 #define EXPECT_DISPLAY_LIST(actual, expectedSize, ...) { \
     EXPECT_EQ((size_t)expectedSize, actual.size()); \
     const TestDisplayItem expected[] = { __VA_ARGS__ }; \
     for (size_t index = 0; index < std::min<size_t>(actual.size(),
expectedSize); index++) { \
-        TRACE_DISPLAY_ITEMS(expected[index], *actual[index]); \
+        TRACE_DISPLAY_ITEMS(index, expected[index], *actual[index]); \
         EXPECT_EQ(expected[index].client(), actual[index]->client()); \
         EXPECT_EQ(expected[index].type(), actual[index]->type()); \
     } \
@@ -94,7 +96,7 @@ void drawClippedRect(GraphicsContext* context,
LayoutLayerModelObject* renderer,
     drawRect(context, renderer, phase, bound);
 }
 
-TEST_F(ViewDisplayListTest, ViewDisplayListTest_NestedRecorders)
+TEST_F(ViewDisplayListTest, NestedRecorders)
 {
     GraphicsContext context(nullptr, &rootDisplayItemList());
     FloatRect bound = renderView()->viewRect();
@@ -108,7 +110,7 @@ TEST_F(ViewDisplayListTest,
ViewDisplayListTest_NestedRecorders)
         TestDisplayItem(renderView(),
DisplayItem::clipTypeToEndClipType(DisplayItem::ClipLayerForeground)));
 }
 
-TEST_F(ViewDisplayListTest, ViewDisplayListTest_UpdateBasic)
+TEST_F(ViewDisplayListTest, UpdateBasic)
 {
     setBodyInnerHTML("<div id='first'><div id='second'></div></div>");
     RenderObject* first = document().body()->firstChild()->renderer();
@@ -135,7 +137,7 @@ TEST_F(ViewDisplayListTest, ViewDisplayListTest_UpdateBasic)
         TestDisplayItem(first,
DisplayItem::paintPhaseToDrawingType(PaintPhaseOutline)));
 }
 
-TEST_F(ViewDisplayListTest, ViewDisplayListTest_UpdateSwapOrder)
+TEST_F(ViewDisplayListTest, UpdateSwapOrder)
 {
     setBodyInnerHTML("<div id='first'><div id='second'></div></div><div
id='unaffected'></div>");
     RenderObject* first = document().body()->firstChild()->renderer();
@@ -165,7 +167,7 @@ TEST_F(ViewDisplayListTest,
ViewDisplayListTest_UpdateSwapOrder)
         TestDisplayItem(unaffected,
DisplayItem::paintPhaseToDrawingType(PaintPhaseBlockBackground)));
 }
 
-TEST_F(ViewDisplayListTest, ViewDisplayListTest_UpdateNewItemInMiddle)
+TEST_F(ViewDisplayListTest, UpdateNewItemInMiddle)
 {
     setBodyInnerHTML("<div id='first'><div id='second'><div
id='third'></div></div></div>");
     RenderObject* first = document().body()->firstChild()->renderer();
@@ -193,7 +195,7 @@ TEST_F(ViewDisplayListTest,
ViewDisplayListTest_UpdateNewItemInMiddle)
         TestDisplayItem(second,
DisplayItem::paintPhaseToDrawingType(PaintPhaseBlockBackground)));
 }
 
-TEST_F(ViewDisplayListTest, ViewDisplayListTest_UpdateInvalidationWithPhases)
+TEST_F(ViewDisplayListTest, UpdateInvalidationWithPhases)
 {
     setBodyInnerHTML("<div id='first'><div id='second'></div></div><div
id='third'></div>");
     RenderObject* first = document().body()->firstChild()->renderer();
@@ -245,55 +247,9 @@ TEST_F(ViewDisplayListTest,
ViewDisplayListTest_UpdateInvalidationWithPhases)
         TestDisplayItem(first,
DisplayItem::paintPhaseToDrawingType(PaintPhaseOutline)),
         TestDisplayItem(second,
DisplayItem::paintPhaseToDrawingType(PaintPhaseOutline)),
         TestDisplayItem(third,
DisplayItem::paintPhaseToDrawingType(PaintPhaseOutline)));
-
-    // The following is only applicable when we support incremental paint.
-#if 0
-    rootDisplayItemList().invalidate(second->displayItemClient());
-
-    EXPECT_DISPLAY_LIST(rootDisplayItemList().paintList(), 6,
-        TestDisplayItem(first,
DisplayItem::paintPhaseToDrawingType(PaintPhaseBlockBackground)),
-        TestDisplayItem(third,
DisplayItem::paintPhaseToDrawingType(PaintPhaseBlockBackground)),
-        TestDisplayItem(first,
DisplayItem::paintPhaseToDrawingType(PaintPhaseForeground)),
-        TestDisplayItem(third,
DisplayItem::paintPhaseToDrawingType(PaintPhaseForeground)),
-        TestDisplayItem(first,
DisplayItem::paintPhaseToDrawingType(PaintPhaseOutline)),
-        TestDisplayItem(third,
DisplayItem::paintPhaseToDrawingType(PaintPhaseOutline)));
-#endif
-}
-
-// This test is only applicable when we support incremental paint.
-TEST_F(ViewDisplayListTest,
DISABLED_ViewDisplayListTest_UpdateAddFirstNoOverlap)
-{
-    setBodyInnerHTML("<div id='first'></div><div id='second'></div>");
-    RenderObject* first = document().body()->firstChild()->renderer();
-    RenderObject* second =
document().body()->firstChild()->nextSibling()->renderer();
-    GraphicsContext context(nullptr, &rootDisplayItemList());
-
-    drawRect(&context, second, PaintPhaseBlockBackground, FloatRect(200, 200,
50, 50));
-    drawRect(&context, second, PaintPhaseOutline, FloatRect(200, 200, 50, 50));
-
-    EXPECT_DISPLAY_LIST(rootDisplayItemList().paintList(), 2,
-        TestDisplayItem(second,
DisplayItem::paintPhaseToDrawingType(PaintPhaseBlockBackground)),
-        TestDisplayItem(second,
DisplayItem::paintPhaseToDrawingType(PaintPhaseOutline)));
-
-    rootDisplayItemList().invalidate(first->displayItemClient());
-    drawRect(&context, first, PaintPhaseBlockBackground, FloatRect(100, 100,
50, 50));
-    drawRect(&context, first, PaintPhaseOutline, FloatRect(100, 100, 50, 50));
-
-    EXPECT_DISPLAY_LIST(rootDisplayItemList().paintList(), 4,
-        TestDisplayItem(first,
DisplayItem::paintPhaseToDrawingType(PaintPhaseBlockBackground)),
-        TestDisplayItem(first,
DisplayItem::paintPhaseToDrawingType(PaintPhaseOutline)),
-        TestDisplayItem(second,
DisplayItem::paintPhaseToDrawingType(PaintPhaseBlockBackground)),
-        TestDisplayItem(second,
DisplayItem::paintPhaseToDrawingType(PaintPhaseOutline)));
-
-    rootDisplayItemList().invalidate(first->displayItemClient());
-
-    EXPECT_DISPLAY_LIST(rootDisplayItemList().paintList(), 2,
-        TestDisplayItem(second,
DisplayItem::paintPhaseToDrawingType(PaintPhaseBlockBackground)),
-        TestDisplayItem(second,
DisplayItem::paintPhaseToDrawingType(PaintPhaseOutline)));
 }
 
-// This test is only applicable when we support incremental paint.
-TEST_F(ViewDisplayListTest, DISABLED_ViewDisplayListTest_UpdateAddFirstOverlap)
+TEST_F(ViewDisplayListTest, UpdateAddFirstOverlap)
 {
     setBodyInnerHTML("<div id='first'></div><div id='second'></div>");
     RenderObject* first = document().body()->firstChild()->renderer();
@@ -302,6 +258,7 @@ TEST_F(ViewDisplayListTest,
DISABLED_ViewDisplayListTest_UpdateAddFirstOverlap)
 
     drawRect(&context, second, PaintPhaseBlockBackground, FloatRect(200, 200,
50, 50));
     drawRect(&context, second, PaintPhaseOutline, FloatRect(200, 200, 50, 50));
+    rootDisplayItemList().endNewPaints();
 
     EXPECT_DISPLAY_LIST(rootDisplayItemList().paintList(), 2,
         TestDisplayItem(second,
DisplayItem::paintPhaseToDrawingType(PaintPhaseBlockBackground)),
@@ -313,6 +270,7 @@ TEST_F(ViewDisplayListTest,
DISABLED_ViewDisplayListTest_UpdateAddFirstOverlap)
     drawRect(&context, first, PaintPhaseOutline, FloatRect(100, 100, 150,
150));
     drawRect(&context, second, PaintPhaseBlockBackground, FloatRect(200, 200,
50, 50));
     drawRect(&context, second, PaintPhaseOutline, FloatRect(200, 200, 50, 50));
+    rootDisplayItemList().endNewPaints();
 
     EXPECT_DISPLAY_LIST(rootDisplayItemList().paintList(), 4,
         TestDisplayItem(first,
DisplayItem::paintPhaseToDrawingType(PaintPhaseBlockBackground)),
@@ -323,46 +281,14 @@ TEST_F(ViewDisplayListTest,
DISABLED_ViewDisplayListTest_UpdateAddFirstOverlap)
     rootDisplayItemList().invalidate(first->displayItemClient());
     drawRect(&context, second, PaintPhaseBlockBackground, FloatRect(200, 200,
50, 50));
     drawRect(&context, second, PaintPhaseOutline, FloatRect(200, 200, 50, 50));
+    rootDisplayItemList().endNewPaints();
 
     EXPECT_DISPLAY_LIST(rootDisplayItemList().paintList(), 2,
         TestDisplayItem(second, DisplayItem::…
(message too large)

[Xianzhu, 2015-02-05 20:31:33]

New patchsets have been uploaded after l-g-t-m from chrishtr@chromium.org

[Xianzhu, 2015-02-05 20:31:47]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-02-05 20:32:38]

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

[commit-bot: I haz the power, 2015-02-05 21:09:21]

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

[commit-bot: I haz the power, 2015-02-05 21:09:22]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[pdr., 2015-02-05 21:16:01]

LGTM2, very nice!

It looks like there's a real compile failure though.

[Xianzhu, 2015-02-05 21:33:35]

On 2015/02/05 21:16:01, pdr wrote:
> LGTM2, very nice!
> 
> It looks like there's a real compile failure though.

I'm confused by the failure. The unresolved externals are inline functions.
Should I declare PLATFORM_EXPORT for the functions, or create an empty .cpp?

[jbroman, 2015-02-05 21:41:28]

On 2015/02/05 21:33:35, Xianzhu wrote:
> On 2015/02/05 21:16:01, pdr wrote:
> > LGTM2, very nice!
> > 
> > It looks like there's a real compile failure though.
> 
> I'm confused by the failure. The unresolved externals are inline functions.
> Should I declare PLATFORM_EXPORT for the functions, or create an empty .cpp?

It's not guaranteed that the compiler will choose to inline it (especially in
debug builds), even though the definition is provided. IIUC the correct solution
is to include the header in at least one translation unit of the platform
component; creating a trivial .cpp file would do the trick.

https://msdn.microsoft.com/en-us/library/xa0d9ste.aspx

[Xianzhu, 2015-02-05 21:57:27]

On 2015/02/05 21:41:28, jbroman wrote:
> On 2015/02/05 21:33:35, Xianzhu wrote:
> > On 2015/02/05 21:16:01, pdr wrote:
> > > LGTM2, very nice!
> > > 
> > > It looks like there's a real compile failure though.
> > 
> > I'm confused by the failure. The unresolved externals are inline functions.
> > Should I declare PLATFORM_EXPORT for the functions, or create an empty .cpp?
> 
> It's not guaranteed that the compiler will choose to inline it (especially in
> debug builds), even though the definition is provided. IIUC the correct
solution
> is to include the header in at least one translation unit of the platform
> component; creating a trivial .cpp file would do the trick.
> 
> https://msdn.microsoft.com/en-us/library/xa0d9ste.aspx

Thanks for the info. It really worked. win_blink_compile_dbg is green now. :)

[Xianzhu, 2015-02-05 21:57:34]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-02-05 21:58:52]

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

[commit-bot: I haz the power, 2015-02-05 23:20:09]

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

[commit-bot: I haz the power, 2015-02-05 23:20:11]

Failed to apply patch for Source/core/paint/DrawingRecorderTest.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: Source/core/paint/DrawingRecorderTest.cpp
  |diff --git a/Source/core/paint/DrawingRecorderTest.cpp
b/Source/core/paint/DrawingRecorderTest.cpp
  |index
5ea1bbb250c3e4263c9e8e5a69ccf1b3c4a752c7..96c371e87fb5ccb2e62d0a116b9746b826629b3d
100644
  |--- a/Source/core/paint/DrawingRecorderTest.cpp
  |+++ b/Source/core/paint/DrawingRecorderTest.cpp
  --------------------------
  No file to patch.  Skipping patch.
  6 out of 6 hunks ignored

Patch:       Source/core/paint/DrawingRecorderTest.cpp
Index: Source/core/paint/DrawingRecorderTest.cpp
diff --git a/Source/core/paint/DrawingRecorderTest.cpp
b/Source/core/paint/DrawingRecorderTest.cpp
index
5ea1bbb250c3e4263c9e8e5a69ccf1b3c4a752c7..96c371e87fb5ccb2e62d0a116b9746b826629b3d
100644
--- a/Source/core/paint/DrawingRecorderTest.cpp
+++ b/Source/core/paint/DrawingRecorderTest.cpp
@@ -14,7 +14,6 @@
 #include <gtest/gtest.h>
 
 namespace blink {
-namespace {
 
 class DrawingRecorderTest : public RenderingTest {
 public:
@@ -23,6 +22,7 @@ public:
 protected:
     RenderView* renderView() { return m_renderView; }
     DisplayItemList& rootDisplayItemList() { return
*renderView()->layer()->graphicsLayerBacking()->displayItemList(); }
+    const Vector<OwnPtr<DisplayItem>>& newPaintListBeforeUpdate() { return
rootDisplayItemList().m_newPaints; }
 
 private:
     virtual void SetUp() override
@@ -36,6 +36,12 @@ private:
         ASSERT_TRUE(m_renderView);
     }
 
+    virtual void TearDown() override
+    {
+        RuntimeEnabledFeatures::setSlimmingPaintEnabled(false);
+        RuntimeEnabledFeatures::setSlimmingPaintDisplayItemCacheEnabled(false);
+    }
+
     RenderView* m_renderView;
 };
 
@@ -54,7 +60,7 @@ void drawRect(GraphicsContext* context, RenderView* renderer,
PaintPhase phase,
 }
 
 
-TEST_F(DrawingRecorderTest, DrawingRecorderTest_Nothing)
+TEST_F(DrawingRecorderTest, Nothing)
 {
     GraphicsContext context(nullptr, &rootDisplayItemList());
     FloatRect bound = renderView()->viewRect();
@@ -62,10 +68,10 @@ TEST_F(DrawingRecorderTest, DrawingRecorderTest_Nothing)
 
     drawNothing(&context, renderView(), PaintPhaseForeground, bound);
     rootDisplayItemList().endNewPaints();
-    EXPECT_EQ((size_t)1, rootDisplayItemList().paintList().size());
+    EXPECT_EQ((size_t)0, rootDisplayItemList().paintList().size());
 }
 
-TEST_F(DrawingRecorderTest, DrawingRecorderTest_Rect)
+TEST_F(DrawingRecorderTest, Rect)
 {
     GraphicsContext context(nullptr, &rootDisplayItemList());
     FloatRect bound = renderView()->viewRect();
@@ -75,22 +81,26 @@ TEST_F(DrawingRecorderTest, DrawingRecorderTest_Rect)
     EXPECT_TRUE(rootDisplayItemList().paintList()[0]->isDrawing());
 }
 
-TEST_F(DrawingRecorderTest, DrawingRecorderTest_Cached)
+TEST_F(DrawingRecorderTest, Cached)
 {
+    RuntimeEnabledFeatures::setSlimmingPaintDisplayItemCacheEnabled(true);
+
     GraphicsContext context(nullptr, &rootDisplayItemList());
     FloatRect bound = renderView()->viewRect();
     drawNothing(&context, renderView(), PaintPhaseBlockBackground, bound);
     drawRect(&context, renderView(), PaintPhaseForeground, bound);
     rootDisplayItemList().endNewPaints();
-    EXPECT_EQ((size_t)2, rootDisplayItemList().paintList().size());
-    EXPECT_TRUE(rootDisplayItemList().paintList()[1]->isDrawing());
+    EXPECT_EQ((size_t)1, rootDisplayItemList().paintList().size());
+    EXPECT_TRUE(rootDisplayItemList().paintList()[0]->isDrawing());
 
     drawNothing(&context, renderView(), PaintPhaseBlockBackground, bound);
     drawRect(&context, renderView(), PaintPhaseForeground, bound);
+    EXPECT_EQ((size_t)2, newPaintListBeforeUpdate().size());
+    EXPECT_TRUE(newPaintListBeforeUpdate()[0]->isCached());
+    EXPECT_TRUE(newPaintListBeforeUpdate()[1]->isCached());
     rootDisplayItemList().endNewPaints();
-    EXPECT_EQ((size_t)2, rootDisplayItemList().paintList().size());
-    EXPECT_TRUE(rootDisplayItemList().paintList()[1]->isDrawing());
+    EXPECT_EQ((size_t)1, rootDisplayItemList().paintList().size());
+    EXPECT_TRUE(rootDisplayItemList().paintList()[0]->isDrawing());
 }
 
 }
-}

[commit-bot: I haz the power, 2015-02-05 23:20:29]

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

[Xianzhu, 2015-02-05 23:49:04]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-02-05 23:50:18]

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

[commit-bot: I haz the power, 2015-02-06 01:03:58]

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

[commit-bot: I haz the power, 2015-02-06 01:03:59]

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

[Xianzhu, 2015-02-06 01:27:47]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-02-06 01:28:13]

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

[commit-bot: I haz the power, 2015-02-06 01:31:03]

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

[commit-bot: I haz the power, 2015-02-06 01:31:05]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/41649)

[Xianzhu, 2015-02-06 01:40:46]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-02-06 01:41:17]

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

[commit-bot: I haz the power, 2015-02-06 04:22:34]

Committed patchset #19 (id:390001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189610