Avoid re-iterate out-of-order display items

Source/platform/graphics/paint/DisplayItemList.cpp

Index: Source/platform/graphics/paint/DisplayItemList.cpp
diff --git a/Source/platform/graphics/paint/DisplayItemList.cpp b/Source/platform/graphics/paint/DisplayItemList.cpp
index 8a66d0c723c1491684403b72ab143d992878312c..72054a59f70cfa4b1268a28629a46333fc3ff898 100644
--- a/Source/platform/graphics/paint/DisplayItemList.cpp
+++ b/Source/platform/graphics/paint/DisplayItemList.cpp
@@ -149,8 +149,8 @@ size_t DisplayItemList::findMatchingItemFromIndex(const DisplayItem::Id& id, con
     for (size_t index : indices) {
         // TODO(pdr): elementAt is not cheap so this should be refactored (See crbug.com/505965).
         const DisplayItem& existingItem = list[index];
-        ASSERT(existingItem.ignoreFromDisplayList() || existingItem.client() == id.client);
-        if (!existingItem.ignoreFromDisplayList() && id.matches(existingItem))
+        ASSERT(!existingItem.isValid() || existingItem.client() == id.client);
+        if (existingItem.isValid() && id.matches(existingItem))
             return index;
     }
 
@@ -168,7 +168,7 @@ void DisplayItemList::addItemToIndexIfNeeded(const DisplayItem& displayItem, siz
     indices.append(index);
 }
 
-DisplayItems::iterator DisplayItemList::findOutOfOrderCachedItem(DisplayItems::iterator currentIt, const DisplayItem::Id& id, DisplayItemIndicesByClientMap& displayItemIndicesByClient)
+DisplayItems::iterator DisplayItemList::findOutOfOrderCachedItem(DisplayItems::iterator& nextItemToIndexIt, const DisplayItem::Id& id, DisplayItemIndicesByClientMap& displayItemIndicesByClient)
 {
     ASSERT(clientCacheIsValid(id.client));
 
@@ -176,22 +176,21 @@ DisplayItems::iterator DisplayItemList::findOutOfOrderCachedItem(DisplayItems::i
     if (foundIndex != kNotFound)
         return m_currentDisplayItems.begin() + foundIndex;
 
-    return findOutOfOrderCachedItemForward(currentIt, id, displayItemIndicesByClient);
+    return findOutOfOrderCachedItemForward(nextItemToIndexIt, id, displayItemIndicesByClient);
 }
 
 // Find forward for the item and index all skipped indexable items.
-DisplayItems::iterator DisplayItemList::findOutOfOrderCachedItemForward(DisplayItems::iterator currentIt, const DisplayItem::Id& id, DisplayItemIndicesByClientMap& displayItemIndicesByClient)
+DisplayItems::iterator DisplayItemList::findOutOfOrderCachedItemForward(DisplayItems::iterator& nextItemToIndexIt, const DisplayItem::Id& id, DisplayItemIndicesByClientMap& displayItemIndicesByClient)
 {
     DisplayItems::iterator currentEnd = m_currentDisplayItems.end();
-    for (; currentIt != currentEnd; ++currentIt) {
-        const DisplayItem& item = *currentIt;
-        if (!item.ignoreFromDisplayList()
-            && item.isCacheable()
-            && m_validlyCachedClients.contains(item.client())) {
+    for (; nextItemToIndexIt != currentEnd; ++nextItemToIndexIt) {
+        const DisplayItem& item = *nextItemToIndexIt;
+        ASSERT(item.isValid());
+        if (item.isCacheable() && clientCacheIsValid(item.client())) {
             if (id.matches(item))
-                return currentIt;
+                return nextItemToIndexIt++;
 
-            addItemToIndexIfNeeded(item, currentIt - m_currentDisplayItems.begin(), displayItemIndicesByClient);
+            addItemToIndexIfNeeded(item, nextItemToIndexIt - m_currentDisplayItems.begin(), displayItemIndicesByClient);
         }
     }
     return currentEnd;
@@ -203,14 +202,12 @@ void DisplayItemList::copyCachedSubtree(DisplayItems::iterator& currentIt, Displ
     ASSERT(currentIt->isBeginSubtree());
     ASSERT(!currentIt->scope());
     DisplayItem::Id endSubtreeId(currentIt->client(), DisplayItem::beginSubtreeTypeToEndSubtreeType(currentIt->type()), 0);
-    while (true) {
-        updatedList.appendByMoving(*currentIt, currentIt->derivedSize());
-        if (endSubtreeId.matches(updatedList.last()))
-            break;
-        ++currentIt;
+    do {
         // We should always find the EndSubtree display item.
         ASSERT(currentIt != m_currentDisplayItems.end());
-    }
+        updatedList.appendByMoving(*currentIt, currentIt->derivedSize());
+        ++currentIt;
+    } while (!endSubtreeId.matches(updatedList.last()));
 }
 
 // Update the existing display items by removing invalidated entries, updating
@@ -276,22 +273,23 @@ void DisplayItemList::commitNewDisplayItems(DisplayListDiff*)
         std::max(m_currentDisplayItems.usedCapacityInBytes(), m_newDisplayItems.usedCapacityInBytes()));
     DisplayItems::iterator currentIt = m_currentDisplayItems.begin();
     DisplayItems::iterator currentEnd = m_currentDisplayItems.end();
+    DisplayItems::iterator nextItemToIndexIt = currentIt;
     for (DisplayItems::iterator newIt = m_newDisplayItems.begin(); newIt != m_newDisplayItems.end(); ++newIt) {
         const DisplayItem& newDisplayItem = *newIt;
         const DisplayItem::Id newDisplayItemId = newDisplayItem.nonCachedId();
         bool newDisplayItemHasCachedType = newDisplayItem.type() != newDisplayItemId.type;
 
-        bool isSynchronized = currentIt != currentEnd
-            && !currentIt->ignoreFromDisplayList()
-            && newDisplayItemId.matches(*currentIt);
+        bool isSynchronized = currentIt != currentEnd && newDisplayItemId.matches(*currentIt);
 
         if (newDisplayItemHasCachedType) {
             ASSERT(!RuntimeEnabledFeatures::slimmingPaintUnderInvalidationCheckingEnabled());
             ASSERT(newDisplayItem.isCached());
             ASSERT(clientCacheIsValid(newDisplayItem.client()));
             if (!isSynchronized) {
-                DisplayItems::iterator foundIt = findOutOfOrderCachedItem(currentIt, newDisplayItemId, displayItemIndicesByClient);
-                ASSERT(foundIt != currentIt);
+                // Skip indexing of copied items.
+                if (currentIt - nextItemToIndexIt > 0)
+                    nextItemToIndexIt = currentIt;
+                DisplayItems::iterator foundIt = findOutOfOrderCachedItem(nextItemToIndexIt, newDisplayItemId, displayItemIndicesByClient);
 
                 if (foundIt == currentEnd) {
 #ifndef NDEBUG
@@ -305,11 +303,14 @@ void DisplayItemList::commitNewDisplayItems(DisplayListDiff*)
                     // attempt to recover rather than crashing or bailing on display of the rest of the display list.
                     continue;
                 }
+
+                ASSERT(foundIt != currentIt); // otherwise isSynchronized should be true.

[pdr., 2015/08/17 23:28:50]

ASSERT(isSynchronized)?

[Xianzhu, 2015/08/17 23:46:59]

The comment above seems confusing.

Changed to:
    ASSERT(foundIt != currentIt); // because we are in 'if (!isSynchronized)'

                 currentIt = foundIt;
             }
 
             if (newDisplayItem.isCachedDrawing()) {
                 updatedList.appendByMoving(*currentIt, currentIt->derivedSize());
+                ++currentIt;
             } else {
                 ASSERT(newDisplayItem.isCachedSubtree());
                 copyCachedSubtree(currentIt, updatedList);
@@ -323,10 +324,10 @@ void DisplayItemList::commitNewDisplayItems(DisplayListDiff*)
                 ASSERT(!newDisplayItem.isDrawing() || newDisplayItem.skippedCache() || !clientCacheIsValid(newDisplayItem.client()));
 #endif // ENABLE(ASSERT)
             updatedList.appendByMoving(*newIt, newIt->derivedSize());
-        }
 
-        if (isSynchronized)
-            ++currentIt;
+            if (isSynchronized)
+                ++currentIt;
+        }
     }
 
 #if ENABLE(ASSERT)
@@ -440,8 +441,8 @@ void DisplayItemList::checkCachedDisplayItemIsUnchanged(const DisplayItem& displ
     DisplayItems::iterator foundItem = m_currentDisplayItems.begin() + index;
     RefPtr<const SkPicture> newPicture = static_cast<const DrawingDisplayItem&>(displayItem).picture();
     RefPtr<const SkPicture> oldPicture = static_cast<const DrawingDisplayItem&>(*foundItem).picture();
-    // Mark the display item as ignored so that we can check if there are any remaining cached display items after merging.
-    foundItem->setIgnoredFromDisplayList();
+    // Invalidate the display item so that we can check if there are any remaining cached display items after merging.
+    foundItem->invalidate();
 
     if (!newPicture && !oldPicture)
         return;
@@ -497,7 +498,7 @@ void DisplayItemList::checkNoRemainingCachedDisplayItems()
     ASSERT(RuntimeEnabledFeatures::slimmingPaintUnderInvalidationCheckingEnabled());
 
     for (const auto& displayItem : m_currentDisplayItems) {
-        if (displayItem.ignoreFromDisplayList() || !displayItem.isDrawing() || !clientCacheIsValid(displayItem.client()))
+        if (!displayItem.isValid() || !displayItem.isDrawing() || !clientCacheIsValid(displayItem.client()))
             continue;
         showUnderInvalidationError("May be under-invalidation: no new display item", displayItem);
     }
@@ -515,7 +516,7 @@ WTF::String DisplayItemList::displayItemsAsDebugString(const DisplayItems& list)
         const DisplayItem& displayItem = *it;
         if (i)
             stringBuilder.append(",\n");
-        if (displayItem.ignoreFromDisplayList()) {
+        if (!displayItem.isValid()) {
             stringBuilder.append("null");
             continue;
         }