Reland "[PE] Change CHECK to LOG(WARNING) in PaintController::FindOutOfOrderCachedItemForward"

This is a reland of 8bc8a796

Updated unit tests, and add comments to
DisplayItemClient::SetDisplayItemsUncached()
and PaintController::ClientCacheIsValid() (also made private).

Original change's description:
> [PE] Change CHECK to LOG(WARNING) in PaintController::FindOutOfOrderCachedItemForward
>
> The situation doesn't cause corrupted rendering but just slightly
> affects performance. It's fine not to DCHECK given that the situation
> is rare.
>
> Bug: 805024
> Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
> Change-Id: Iac8cd617d5df51da2516fc6a9df1308a0daaedd0
> Reviewed-on: https://chromium-review.googlesource.com/990074
> Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
> Reviewed-by: Philip Rogers <pdr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#547923}

Bug: 805024
Change-Id: Ibcd443972c41b03cf03ea946da3c499987ad3b59
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Reviewed-on: https://chromium-review.googlesource.com/994339Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548177}

third_party/WebKit/Source/core/paint/PaintControllerPaintTest.cpp

@@ -121,9 +121,9 @@ TEST_P(PaintControllerPaintTestForNonSPv1, ChunkIdClientCacheFlag) {
   EXPECT_FALSE(div.Layer()->IsJustCreated());
   // Client used by only paint chunks and non-cachaeable display items but not
   // by any cacheable display items won't be marked as validly cached.
-  EXPECT_TRUE(RootPaintController().ClientCacheIsValid(*div.Layer()));
-  EXPECT_FALSE(RootPaintController().ClientCacheIsValid(div));
-  EXPECT_TRUE(RootPaintController().ClientCacheIsValid(sub_div));
+  EXPECT_TRUE(ClientCacheIsValid(*div.Layer()));
+  EXPECT_FALSE(ClientCacheIsValid(div));
+  EXPECT_TRUE(ClientCacheIsValid(sub_div));
 }
 
 TEST_P(PaintControllerPaintTestForNonSPv1, CompositingNoFold) {

third_party/WebKit/Source/core/paint/PaintControllerPaintTest.h

@@ -108,6 +108,10 @@ class PaintControllerPaintTestBase : public RenderingTest {
   void InvalidateAll(PaintController& paint_controller) {
     paint_controller.InvalidateAllForTesting();
   }
+
+  bool ClientCacheIsValid(const DisplayItemClient& client) {
+    return RootPaintController().ClientCacheIsValid(client);
+  }
 };
 
 class PaintControllerPaintTest : public PaintTestConfigurations,

third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h

@@ -68,6 +68,11 @@ class PLATFORM_EXPORT DisplayItemClient {
     return false;
   }
 
+  // Indicates that the client will paint display items different from the ones
+  // cached by PaintController. However, PaintController allows a client to
+  // paint new display items that are not cached or to no longer paint some
+  // cached display items without calling this method.
+  // See PaintController::ClientCacheIsValid() for more details.
   void SetDisplayItemsUncached(
       PaintInvalidationReason reason = PaintInvalidationReason::kFull) const {
     cache_generation_or_invalidation_reason_.Invalidate(reason);

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

@@ -480,7 +480,7 @@ size_t PaintController::FindOutOfOrderCachedItemForward(
 #endif
     // Ensure our paint invalidation tests don't trigger the less performant
     // situation which should be rare.
-    CHECK(false) << "Can't find cached display item: " << id.client.DebugName()
+    LOG(WARNING) << "Can't find cached display item: " << id.client.DebugName()
                  << " " << id.ToString();
   }
   return kNotFound;

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

@@ -173,8 +173,6 @@ class PLATFORM_EXPORT PaintController {
     return GetPaintArtifact().PaintChunks();
   }
 
-  bool ClientCacheIsValid(const DisplayItemClient&) const;
-
   // For micro benchmarking of record time.
   bool DisplayItemConstructionIsDisabled() const {
     return construction_disabled_;
@@ -249,6 +247,18 @@ class PLATFORM_EXPORT PaintController {
   friend class PaintControllerTestBase;
   friend class PaintControllerPaintTestBase;
 
+  // True if all display items associated with the client are validly cached.
+  // However, the current algorithm allows the following situations even if
+  // ClientCacheIsValid() is true for a client during painting:
+  // 1. The client paints a new display item that is not cached:
+  //    UseCachedDrawingIfPossible() returns false for the display item and the
+  //    newly painted display item will be added into the cache. This situation
+  //    has slight performance hit (see FindOutOfOrderCachedItemForward()) so we
+  //    print a warning in the situation and should keep it rare.
+  // 2. the client no longer paints a display item that is cached: the cached
+  //    display item will be removed. This doesn't affect performance.
+  bool ClientCacheIsValid(const DisplayItemClient&) const;
+
   void InvalidateAllForTesting() { InvalidateAllInternal(); }
   void InvalidateAllInternal();
 

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

@@ -645,8 +645,8 @@ TEST_P(PaintControllerTest, CachedDisplayItems) {
   EXPECT_DISPLAY_LIST(GetPaintController().GetDisplayItemList(), 2,
                       TestDisplayItem(first, kBackgroundType),
                       TestDisplayItem(second, kBackgroundType));
-  EXPECT_TRUE(GetPaintController().ClientCacheIsValid(first));
-  EXPECT_TRUE(GetPaintController().ClientCacheIsValid(second));
+  EXPECT_TRUE(ClientCacheIsValid(first));
+  EXPECT_TRUE(ClientCacheIsValid(second));
   sk_sp<const PaintRecord> first_paint_record =
       static_cast<const DrawingDisplayItem&>(
           GetPaintController().GetDisplayItemList()[0])
@@ -657,8 +657,8 @@ TEST_P(PaintControllerTest, CachedDisplayItems) {
           .GetPaintRecord();
 
   first.SetDisplayItemsUncached();
-  EXPECT_FALSE(GetPaintController().ClientCacheIsValid(first));
-  EXPECT_TRUE(GetPaintController().ClientCacheIsValid(second));
+  EXPECT_FALSE(ClientCacheIsValid(first));
+  EXPECT_TRUE(ClientCacheIsValid(second));
 
   if (RuntimeEnabledFeatures::SlimmingPaintV175Enabled()) {
     InitRootChunk();
@@ -682,12 +682,12 @@ TEST_P(PaintControllerTest, CachedDisplayItems) {
                   GetPaintController().GetDisplayItemList()[1])
                   .GetPaintRecord());
   }
-  EXPECT_TRUE(GetPaintController().ClientCacheIsValid(first));
-  EXPECT_TRUE(GetPaintController().ClientCacheIsValid(second));
+  EXPECT_TRUE(ClientCacheIsValid(first));
+  EXPECT_TRUE(ClientCacheIsValid(second));
 
   InvalidateAll();
-  EXPECT_FALSE(GetPaintController().ClientCacheIsValid(first));
-  EXPECT_FALSE(GetPaintController().ClientCacheIsValid(second));
+  EXPECT_FALSE(ClientCacheIsValid(first));
+  EXPECT_FALSE(ClientCacheIsValid(second));
 }
 
 TEST_P(PaintControllerTest, UpdateSwapOrderWithChildren) {
@@ -1653,7 +1653,7 @@ TEST_P(PaintControllerTest, SkipCache) {
   }
 
   // Draw again with nothing invalidated.
-  EXPECT_TRUE(GetPaintController().ClientCacheIsValid(multicol));
+  EXPECT_TRUE(ClientCacheIsValid(multicol));
   DrawRect(context, multicol, kBackgroundType, FloatRect(100, 200, 100, 100));
 
   GetPaintController().BeginSkippingCache();
@@ -1762,7 +1762,7 @@ TEST_P(PaintControllerTest, PartialSkipCache) {
   EXPECT_NE(record1, record2);
 
   // Content's cache is invalid because it has display items skipped cache.
-  EXPECT_FALSE(GetPaintController().ClientCacheIsValid(content));
+  EXPECT_FALSE(ClientCacheIsValid(content));
   EXPECT_EQ(PaintInvalidationReason::kFull,
             content.GetPaintInvalidationReason());
 
@@ -2046,6 +2046,8 @@ class PaintControllerUnderInvalidationTest
     DrawRect(context, first, kBackgroundType, FloatRect(100, 100, 300, 300));
     DrawRect(context, first, kForegroundType, FloatRect(100, 100, 300, 300));
     GetPaintController().CommitNewDisplayItems();
+
+    InitRootChunk();
     first.SetVisualRect(LayoutRect(200, 200, 300, 300));
     DrawRect(context, first, kBackgroundType, FloatRect(200, 200, 300, 300));
     DrawRect(context, first, kForegroundType, FloatRect(100, 100, 300, 300));
@@ -2059,6 +2061,8 @@ class PaintControllerUnderInvalidationTest
     InitRootChunk();
     DrawRect(context, first, kBackgroundType, FloatRect(100, 100, 300, 300));
     GetPaintController().CommitNewDisplayItems();
+
+    InitRootChunk();
     DrawRect(context, first, kBackgroundType, FloatRect(100, 100, 300, 300));
     DrawRect(context, first, kForegroundType, FloatRect(100, 100, 300, 300));
     GetPaintController().CommitNewDisplayItems();
@@ -2072,6 +2076,7 @@ class PaintControllerUnderInvalidationTest
     DrawRect(context, first, kBackgroundType, FloatRect(100, 100, 300, 300));
     DrawRect(context, first, kForegroundType, FloatRect(100, 100, 300, 300));
     GetPaintController().CommitNewDisplayItems();
+
     InitRootChunk();
     DrawRect(context, first, kBackgroundType, FloatRect(100, 100, 300, 300));
     GetPaintController().CommitNewDisplayItems();
@@ -2084,7 +2089,6 @@ class PaintControllerUnderInvalidationTest
     GraphicsContext context(GetPaintController());
 
     InitRootChunk();
-
     {
       SubsequenceRecorder r(context, container);
       DrawRect(context, container, kBackgroundType,
@@ -2093,7 +2097,6 @@ class PaintControllerUnderInvalidationTest
     GetPaintController().CommitNewDisplayItems();
 
     InitRootChunk();
-
     EXPECT_FALSE(SubsequenceRecorder::UseCachedSubsequenceIfPossible(
         context, container));
     {
@@ -2119,6 +2122,7 @@ class PaintControllerUnderInvalidationTest
   void TestChangeDrawingInSubsequence() {
     FakeDisplayItemClient first("first");
     GraphicsContext context(GetPaintController());
+    InitRootChunk();
     {
       SubsequenceRecorder r(context, first);
       first.SetVisualRect(LayoutRect(100, 100, 300, 300));
@@ -2126,6 +2130,8 @@ class PaintControllerUnderInvalidationTest
       DrawRect(context, first, kForegroundType, FloatRect(100, 100, 300, 300));
     }
     GetPaintController().CommitNewDisplayItems();
+
+    InitRootChunk();
     {
       EXPECT_FALSE(
           SubsequenceRecorder::UseCachedSubsequenceIfPossible(context, first));
@@ -2141,12 +2147,14 @@ class PaintControllerUnderInvalidationTest
     FakeDisplayItemClient first("first");
     GraphicsContext context(GetPaintController());
 
+    InitRootChunk();
     {
       SubsequenceRecorder r(context, first);
       DrawRect(context, first, kBackgroundType, FloatRect(100, 100, 300, 300));
     }
     GetPaintController().CommitNewDisplayItems();
 
+    InitRootChunk();
     {
       EXPECT_FALSE(
           SubsequenceRecorder::UseCachedSubsequenceIfPossible(context, first));
@@ -2161,6 +2169,7 @@ class PaintControllerUnderInvalidationTest
     FakeDisplayItemClient first("first");
     GraphicsContext context(GetPaintController());
 
+    InitRootChunk();
     {
       SubsequenceRecorder r(context, first);
       DrawRect(context, first, kBackgroundType, FloatRect(100, 100, 300, 300));
@@ -2168,6 +2177,7 @@ class PaintControllerUnderInvalidationTest
     }
     GetPaintController().CommitNewDisplayItems();
 
+    InitRootChunk();
     {
       EXPECT_FALSE(
           SubsequenceRecorder::UseCachedSubsequenceIfPossible(context, first));
@@ -2182,6 +2192,7 @@ class PaintControllerUnderInvalidationTest
     FakeDisplayItemClient content("content");
     GraphicsContext context(GetPaintController());
 
+    InitRootChunk();
     {
       SubsequenceRecorder r(context, container);
       { ClipPathRecorder clip_path_recorder(context, container, Path()); }
@@ -2191,6 +2202,7 @@ class PaintControllerUnderInvalidationTest
     }
     GetPaintController().CommitNewDisplayItems();
 
+    InitRootChunk();
     {
       EXPECT_FALSE(SubsequenceRecorder::UseCachedSubsequenceIfPossible(
           context, container));
@@ -2233,12 +2245,14 @@ class PaintControllerUnderInvalidationTest
     FakeDisplayItemClient target("target");
     GraphicsContext context(GetPaintController());
 
+    InitRootChunk();
     {
       SubsequenceRecorder r(context, target);
       DrawRect(context, target, kBackgroundType, FloatRect(100, 100, 300, 300));
     }
     GetPaintController().CommitNewDisplayItems();
 
+    InitRootChunk();
     {
       EXPECT_FALSE(
           SubsequenceRecorder::UseCachedSubsequenceIfPossible(context, target));
@@ -2249,18 +2263,18 @@ class PaintControllerUnderInvalidationTest
 };
 
 TEST_F(PaintControllerUnderInvalidationTest, ChangeDrawing) {
-  EXPECT_DEATH(TestChangeDrawing(), "");
+  EXPECT_DEATH(TestChangeDrawing(), "under-invalidation: display item changed");
 }
 
 TEST_F(PaintControllerUnderInvalidationTest, MoreDrawing) {
-  EXPECT_DEATH(TestMoreDrawing(), "");
+  // We don't detect under-invalidation in this case, and PaintController can
+  // also handle the case gracefully.
+  TestMoreDrawing();
 }
 
 TEST_F(PaintControllerUnderInvalidationTest, LessDrawing) {
   // We don't detect under-invalidation in this case, and PaintController can
-  // also handle the case gracefully. However, less drawing at one time often
-  // means more-drawing at another time, so eventually we'll detect such
-  // under-invalidations.
+  // also handle the case gracefully.
   TestLessDrawing();
 }
 
@@ -2270,23 +2284,28 @@ TEST_F(PaintControllerUnderInvalidationTest, NoopPairsInSubsequence) {
 }
 
 TEST_F(PaintControllerUnderInvalidationTest, ChangeDrawingInSubsequence) {
-  EXPECT_DEATH(TestChangeDrawingInSubsequence(), "");
+  EXPECT_DEATH(TestChangeDrawingInSubsequence(),
+               "In cached subsequence for first.*"
+               "under-invalidation: display item changed");
 }
 
 TEST_F(PaintControllerUnderInvalidationTest, MoreDrawingInSubsequence) {
-  EXPECT_DEATH(TestMoreDrawingInSubsequence(), "");
+  // TODO(wangxianzhu): Detect more drawings at the end of a subsequence.
+  TestMoreDrawingInSubsequence();
 }
 
 TEST_F(PaintControllerUnderInvalidationTest, LessDrawingInSubsequence) {
-  // We allow invalidated display item clients as long as they would produce the
-  // same display items. The cases of changed display items are tested by other
-  // test cases.
-  EXPECT_DEATH(TestLessDrawingInSubsequence(), "");
+  EXPECT_DEATH(TestLessDrawingInSubsequence(),
+               "In cached subsequence for first.*"
+               "under-invalidation: new subsequence wrong length");
 }
 
 TEST_F(PaintControllerUnderInvalidationTest, ChangeNonDrawingInSubsequence) {
-  if (!RuntimeEnabledFeatures::SlimmingPaintV175Enabled())
-    EXPECT_DEATH(TestChangeNonDrawingInSubsequence(), "");
+  if (RuntimeEnabledFeatures::SlimmingPaintV175Enabled())
+    return;
+  EXPECT_DEATH(TestChangeNonDrawingInSubsequence(),
+               "In cached subsequence for first.*"
+               "under-invalidation: new subsequence wrong length");
 }
 
 TEST_F(PaintControllerUnderInvalidationTest, InvalidationInSubsequence) {
@@ -2297,7 +2316,9 @@ TEST_F(PaintControllerUnderInvalidationTest, InvalidationInSubsequence) {
 }
 
 TEST_F(PaintControllerUnderInvalidationTest, SubsequenceBecomesEmpty) {
-  EXPECT_DEATH(TestSubsequenceBecomesEmpty(), "");
+  EXPECT_DEATH(TestSubsequenceBecomesEmpty(),
+               "In cached subsequence for target.*"
+               "under-invalidation: new subsequence wrong length");
 }
 
 TEST_F(PaintControllerUnderInvalidationTest, SkipCacheInSubsequence) {

third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.h

@@ -74,6 +74,15 @@ class PaintControllerTestBase : public testing::Test {
     return paint_controller_->GetSubsequenceMarkers(client);
   }
 
+  static bool ClientCacheIsValid(const PaintController& paint_controller,
+                                 const DisplayItemClient& client) {
+    return paint_controller.ClientCacheIsValid(client);
+  }
+
+  bool ClientCacheIsValid(const DisplayItemClient& client) const {
+    return ClientCacheIsValid(*paint_controller_, client);
+  }
+
  private:
   FakeDisplayItemClient root_paint_property_client_;
   PaintChunk::Id root_paint_chunk_id_;

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

@@ -20,7 +20,7 @@ TEST_F(PaintRecordBuilderTest, TransientPaintController) {
   FakeDisplayItemClient client("client", LayoutRect(10, 10, 20, 20));
   DrawRect(context, client, kBackgroundType, FloatRect(10, 10, 20, 20));
   DrawRect(context, client, kForegroundType, FloatRect(15, 15, 10, 10));
-  EXPECT_FALSE(context.GetPaintController().ClientCacheIsValid(client));
+  EXPECT_FALSE(ClientCacheIsValid(context.GetPaintController(), client));
 
   MockPaintCanvas canvas;
   PaintFlags flags;
@@ -30,7 +30,7 @@ TEST_F(PaintRecordBuilderTest, TransientPaintController) {
   EXPECT_DISPLAY_LIST(context.GetPaintController().GetDisplayItemList(), 2,
                       TestDisplayItem(client, kBackgroundType),
                       TestDisplayItem(client, kForegroundType));
-  EXPECT_FALSE(context.GetPaintController().ClientCacheIsValid(client));
+  EXPECT_FALSE(ClientCacheIsValid(context.GetPaintController(), client));
 }
 
 TEST_F(PaintRecordBuilderTest, LastingPaintController) {
@@ -46,13 +46,13 @@ TEST_F(PaintRecordBuilderTest, LastingPaintController) {
   FakeDisplayItemClient client("client", LayoutRect(10, 10, 20, 20));
   DrawRect(context, client, kBackgroundType, FloatRect(10, 10, 20, 20));
   DrawRect(context, client, kForegroundType, FloatRect(15, 15, 10, 10));
-  EXPECT_FALSE(GetPaintController().ClientCacheIsValid(client));
+  EXPECT_FALSE(ClientCacheIsValid(client));
 
   MockPaintCanvas canvas;
   PaintFlags flags;
   EXPECT_CALL(canvas, drawPicture(_)).Times(1);
   builder.EndRecording(canvas);
-  EXPECT_TRUE(GetPaintController().ClientCacheIsValid(client));
+  EXPECT_TRUE(ClientCacheIsValid(client));
 
   EXPECT_DISPLAY_LIST(GetPaintController().GetDisplayItemList(), 2,
                       TestDisplayItem(client, kBackgroundType),
@@ -73,7 +73,7 @@ TEST_F(PaintRecordBuilderTest, LastingPaintController) {
   EXPECT_DISPLAY_LIST(GetPaintController().GetDisplayItemList(), 2,
                       TestDisplayItem(client, kBackgroundType),
                       TestDisplayItem(client, kForegroundType));
-  EXPECT_TRUE(GetPaintController().ClientCacheIsValid(client));
+  EXPECT_TRUE(ClientCacheIsValid(client));
 }
 
 TEST_F(PaintRecordBuilderTest, TransientAndAnotherPaintController) {
@@ -87,7 +87,7 @@ TEST_F(PaintRecordBuilderTest, TransientAndAnotherPaintController) {
   EXPECT_DISPLAY_LIST(GetPaintController().GetDisplayItemList(), 2,
                       TestDisplayItem(client, kBackgroundType),
                       TestDisplayItem(client, kForegroundType));
-  EXPECT_TRUE(GetPaintController().ClientCacheIsValid(client));
+  EXPECT_TRUE(ClientCacheIsValid(client));
 
   PaintRecordBuilder builder;
   EXPECT_NE(&builder.Context().GetPaintController(), &GetPaintController());
@@ -97,9 +97,9 @@ TEST_F(PaintRecordBuilderTest, TransientAndAnotherPaintController) {
 
   // The transient PaintController in PaintRecordBuilder doesn't affect the
   // client's cache status in another PaintController.
-  EXPECT_TRUE(GetPaintController().ClientCacheIsValid(client));
+  EXPECT_TRUE(ClientCacheIsValid(client));
   EXPECT_FALSE(
-      builder.Context().GetPaintController().ClientCacheIsValid(client));
+      ClientCacheIsValid(builder.Context().GetPaintController(), client));
 }
 
 }  // namespace blink