[SPv2] Defer decision of raster invalidation after paint for changes z-index, transform, etc.

third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp

Index: third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp b/third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
index f6be5d8b2d67e0f45739d75a43e883a218ff13ba..4b024b6e799bfc2d99aaa3ddd65dd9ea47b07fbd 100644
--- a/third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
+++ b/third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
@@ -8,6 +8,7 @@
 #include "platform/graphics/GraphicsLayer.h"
 #include "platform/graphics/paint/DrawingDisplayItem.h"
 #include "third_party/skia/include/core/SkPictureAnalyzer.h"
+#include "wtf/AutoReset.h"
 #include "wtf/text/StringBuilder.h"
 
 #ifndef NDEBUG
@@ -195,15 +196,11 @@ void PaintController::processNewItem(DisplayItem& displayItem, NewItemSource new
     }
 
     if (RuntimeEnabledFeatures::slimmingPaintV2Enabled()) {
-        if (newItemSource != FromCachedSubsequence)
-            m_currentChunkIsFromCachedSubsequence = false;
-
         size_t lastChunkIndex = m_newPaintChunks.lastChunkIndex();
         if (m_newPaintChunks.incrementDisplayItemIndex(displayItem)) {
             DCHECK(lastChunkIndex != m_newPaintChunks.lastChunkIndex());
             if (lastChunkIndex != kNotFound)
                 generateChunkRasterInvalidationRects(m_newPaintChunks.paintChunkAt(lastChunkIndex));
-            m_currentChunkIsFromCachedSubsequence = true;
         }
     }
 
@@ -219,8 +216,8 @@ void PaintController::processNewItem(DisplayItem& displayItem, NewItemSource new
     if (index != kNotFound) {
 #ifndef NDEBUG
         showDebugData();
-        WTFLogAlways("DisplayItem %s has duplicated id with previous %s (index=%d)\n",
-            displayItem.asDebugString().utf8().data(), m_newDisplayItemList[index].asDebugString().utf8().data(), static_cast<int>(index));
+        WTFLogAlways("DisplayItem %s has duplicated id with previous %s (index=%zu)\n",
+            displayItem.asDebugString().utf8().data(), m_newDisplayItemList[index].asDebugString().utf8().data(), index);
 #endif
         NOTREACHED();
     }
@@ -268,7 +265,9 @@ size_t PaintController::findMatchingItemFromIndex(const DisplayItem::Id& id, con
     const Vector<size_t>& indices = it->value;
     for (size_t index : indices) {
         const DisplayItem& existingItem = list[index];
-        DCHECK(!existingItem.hasValidClient() || existingItem.client() == id.client);
+        if (existingItem.hasBeenMoved())
+            continue;
+        DCHECK(existingItem.client() == id.client);
         if (id == existingItem.getId())
             return index;
     }
@@ -296,7 +295,7 @@ size_t PaintController::findCachedItem(const DisplayItem::Id& id)
     for (size_t i = m_nextItemToMatch; i < m_currentPaintArtifact.getDisplayItemList().size(); ++i) {
         // We encounter an item that has already been copied which indicates we can't do sequential matching.
         const DisplayItem& item = m_currentPaintArtifact.getDisplayItemList()[i];
-        if (!item.hasValidClient())
+        if (item.hasBeenMoved())
             break;
         if (id == item.getId()) {
 #if DCHECK_IS_ON()
@@ -325,7 +324,7 @@ size_t PaintController::findOutOfOrderCachedItemForward(const DisplayItem::Id& i
 {
     for (size_t i = m_nextItemToIndex; i < m_currentPaintArtifact.getDisplayItemList().size(); ++i) {
         const DisplayItem& item = m_currentPaintArtifact.getDisplayItemList()[i];
-        DCHECK(item.hasValidClient());
+        DCHECK(!item.hasBeenMoved());
         if (id == item.getId()) {
 #if DCHECK_IS_ON()
             ++m_numSequentialMatches;
@@ -357,6 +356,7 @@ size_t PaintController::findOutOfOrderCachedItemForward(const DisplayItem::Id& i
 // but mark the begin and end of the subsequence for under-invalidation checking.
 void PaintController::copyCachedSubsequence(size_t& cachedItemIndex)
 {
+    AutoReset<size_t> subsequenceBeginIndex(&m_currentCachedSubsequenceBeginIndexInNewList, m_newDisplayItemList.size());

[chrishtr, 2016/09/06 22:14:12]

What does changing to m_currentCachedSubsequenceBeginIndexInNewList achieve? Not
sure I see it yet.

[Xianzhu, 2016/09/06 22:42:38]

This replaces the original m_currentChunkIsFromCachedSubsequence which had a bug
when a chunk is not from cached subsequence but contains no invalidated
cacheable display items. Now use newChunk.beginIndex >=
m_currentCachedSubsequenceBeginIndexInNewList to check if a chunk is fully from
a cached subsequence.

     DisplayItem* cachedItem = &m_currentPaintArtifact.getDisplayItemList()[cachedItemIndex];
 #if DCHECK_IS_ON()
     DCHECK(cachedItem->getType() == DisplayItem::kSubsequence);
@@ -371,6 +371,7 @@ void PaintController::copyCachedSubsequence(size_t& cachedItemIndex)
     Vector<PaintChunk>::const_iterator cachedChunk;
     if (RuntimeEnabledFeatures::slimmingPaintV2Enabled()) {
         cachedChunk = m_currentPaintArtifact.findChunkByDisplayItemIndex(cachedItemIndex);
+        DCHECK(cachedChunk != m_currentPaintArtifact.paintChunks().end());
         updateCurrentPaintChunkProperties(cachedChunk->id ? &*cachedChunk->id : nullptr, cachedChunk->properties);
     } else {
         // This is to avoid compilation error about uninitialized variable on Windows.
@@ -378,7 +379,7 @@ void PaintController::copyCachedSubsequence(size_t& cachedItemIndex)
     }
 
     while (true) {
-        DCHECK(cachedItem->hasValidClient());
+        DCHECK(!cachedItem->hasBeenMoved());
 #if CHECK_DISPLAY_ITEM_CLIENT_ALIVENESS
         CHECK(cachedItem->client().isAlive());
 #endif
@@ -387,6 +388,7 @@ void PaintController::copyCachedSubsequence(size_t& cachedItemIndex)
         if (!DCHECK_IS_ON() || !RuntimeEnabledFeatures::slimmingPaintUnderInvalidationCheckingEnabled()) {
             if (RuntimeEnabledFeatures::slimmingPaintV2Enabled() && cachedItemIndex == cachedChunk->endIndex) {
                 ++cachedChunk;
+                DCHECK(cachedChunk != m_currentPaintArtifact.paintChunks().end());
                 updateCurrentPaintChunkProperties(cachedChunk->id ? &*cachedChunk->id : nullptr, cachedChunk->properties);
             }
             processNewItem(m_newDisplayItemList.appendByMoving(*cachedItem), FromCachedSubsequence);
@@ -537,7 +539,7 @@ void PaintController::appendDebugDrawingAfterCommit(const DisplayItemClient& dis
 void PaintController::generateChunkRasterInvalidationRects(PaintChunk& newChunk)
 {
     DCHECK(RuntimeEnabledFeatures::slimmingPaintV2Enabled());
-    if (m_currentChunkIsFromCachedSubsequence)
+    if (newChunk.beginIndex >= m_currentCachedSubsequenceBeginIndexInNewList)
         return;
 
     if (!newChunk.id) {
@@ -584,33 +586,44 @@ void PaintController::generateChunkRasterInvalidationRectsComparingOldChunk(Pain
 {
     DCHECK(RuntimeEnabledFeatures::slimmingPaintV2Enabled());
 
-    // TODO(wangxianzhu): Support raster invalidation for reordered display items without invalidating
-    // display item clients. Currently we invalidate display item clients ensuring raster invalidation.
     // TODO(wangxianzhu): Handle PaintInvalidationIncremental.
     // TODO(wangxianzhu): Optimize paint offset change.
 
-    // Maps from each client to the index of the first drawing-content display item of the client.
-    HashMap<const DisplayItemClient*, size_t> oldChunkClients;
-    for (size_t i = oldChunk.beginIndex; i < oldChunk.endIndex; ++i) {
-        const DisplayItem& oldItem = m_currentPaintArtifact.getDisplayItemList()[i];
-        // oldItem.hasValidClient() indicates that the item has not been copied as a cached item into
-        // m_newDislayItemList, so the item either disappeared or changed, and needs raster invalidation.
-        if (oldItem.hasValidClient() && oldItem.drawsContent() && oldChunkClients.add(&oldItem.client(), i).isNewEntry)
-            newChunk.rasterInvalidationRects.append(m_currentPaintArtifact.getDisplayItemList().visualRect(i));
-    }
-
-    HashSet<const DisplayItemClient*> newChunkClients;
-    for (size_t i = newChunk.beginIndex; i < newChunk.endIndex; ++i) {
-        const DisplayItem& newItem = m_newDisplayItemList[i];
-        if (newItem.drawsContent()) {
-            if (!clientCacheIsValid(newItem.client())) {
-                if (newChunkClients.add(&newItem.client()).isNewEntry)
-                    newChunk.rasterInvalidationRects.append(newItem.client().visualRect());
-            } else {
-                // The cached item was moved from the old chunk which should not contain any item of the client now.
-                DCHECK(!oldChunkClients.contains(&newItem.client()));
+    HashSet<const DisplayItemClient*> invalidatedClientsInOldChunk;
+    size_t highestMovedToIndex = 0;
+    for (size_t oldIndex = oldChunk.beginIndex; oldIndex < oldChunk.endIndex; ++oldIndex) {
+        const DisplayItem& oldItem = m_currentPaintArtifact.getDisplayItemList()[oldIndex];
+        const DisplayItemClient* clientToInvalidate = nullptr;
+        if (oldItem.hasBeenMoved()) {
+            size_t movedToIndex = oldItem.movedToIndex();
+            if (m_newDisplayItemList[movedToIndex].drawsContent()) {
+                if (movedToIndex < newChunk.beginIndex || movedToIndex >= newChunk.endIndex) {
+                    // The item has been moved into another chunk, so need to invalidate it in the old chunk.
+                    clientToInvalidate = &m_newDisplayItemList[movedToIndex].client();
+                    // And invalidate in the new chunk into which the item was moved.
+                    PaintChunk& movedToChunk = m_newPaintChunks.findChunkByDisplayItemIndex(movedToIndex);
+                    movedToChunk.rasterInvalidationRects.append(clientToInvalidate->visualRect());
+                } else if (movedToIndex < highestMovedToIndex) {
+                    // The item has been moved behind other cached items, so need to invalidate the area
+                    // that is probably exposed by the item.

[chrishtr, 2016/09/06 22:14:12]

 "exposed by the item moving earlier"

[Xianzhu, 2016/09/06 22:42:38]

Done.

+                    clientToInvalidate = &m_newDisplayItemList[movedToIndex].client();
+                } else {
+                    highestMovedToIndex = movedToIndex;
+                }
             }
+        } else if (oldItem.drawsContent()) {
+            clientToInvalidate = &oldItem.client();
         }
+        if (clientToInvalidate && invalidatedClientsInOldChunk.add(clientToInvalidate).isNewEntry) {
+            newChunk.rasterInvalidationRects.append(m_currentPaintArtifact.getDisplayItemList().visualRect(oldIndex));
+        }
+    }
+
+    HashSet<const DisplayItemClient*> invalidatedClientsInNewChunk;
+    for (size_t newIndex = newChunk.beginIndex; newIndex < newChunk.endIndex; ++newIndex) {
+        const DisplayItem& newItem = m_newDisplayItemList[newIndex];
+        if (newItem.drawsContent() && !clientCacheIsValid(newItem.client()) && invalidatedClientsInNewChunk.add(&newItem.client()).isNewEntry)
+            newChunk.rasterInvalidationRects.append(newItem.client().visualRect());
     }
 }
 
@@ -693,13 +706,13 @@ String PaintController::displayItemListAsDebugString(const DisplayItemList& list
         const DisplayItem& displayItem = *it;
         if (i)
             stringBuilder.append(",\n");
-        stringBuilder.append(String::format("{index: %d, ", (int)i));
+        stringBuilder.append(String::format("{index: %zu, ", i));
 #ifndef NDEBUG
         displayItem.dumpPropertiesAsDebugString(stringBuilder);
 #else
         stringBuilder.append(String::format("clientDebugName: %s", displayItem.client().debugName().ascii().data()));
 #endif
-        if (displayItem.hasValidClient()) {
+        if (!displayItem.hasBeenMoved()) {
             do {
 #if CHECK_DISPLAY_ITEM_CLIENT_ALIVENESS
                 if (!displayItem.client().isAlive()) {