[FCP++, LastImage] Redefine "last" as last load from last added

Last Image Paint takes the last image based on the order of first paint index.
In the initial implementation, the first paint index was the time the image is
attached to DOM tree. As an alternative, the first paint index can also be the
time the image is loaded. By observation, we find that it's more natural for
users to regard the last image as the last one being loaded other than the last
one being attached to the tree, as the latter may not be visible to users yet.

Therefore, this CL is to change the definition of "last" in Last Image Paint to
the last image being loaded.

Bug: 869924
Change-Id: Ib5d0addf10c32668b38297c2efe5e73b7e1e7a33
Reviewed-on: https://chromium-review.googlesource.com/c/1316360
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605497}

third_party/blink/renderer/core/paint/image_paint_timing_detector.cc

@@ -269,12 +269,12 @@ void ImagePaintTimingDetector::RecordImage(const LayoutObject& object,
       }
       std::unique_ptr<ImageRecord> record = std::make_unique<ImageRecord>();
       record->node_id = node_id;
-      record->first_size = rect_size;
-      record->first_paint_index = ++first_paint_index_max_;
       record->image_url =
           !cachedImg ? "" : cachedImg->Url().StrippedForUseAsReferrer();
+      // Mind that first_size has to be assigned at the push of
+      // largest_image_heap_ since it's the sorting key.
+      record->first_size = rect_size;
       largest_image_heap_.push(record->AsWeakPtr());
-      latest_image_heap_.push(record->AsWeakPtr());
       id_record_map_.insert(node_id, std::move(record));
     } else {
       // for assessing whether kImageNodeNumberLimit is large enough for all
@@ -288,8 +288,17 @@ void ImagePaintTimingDetector::RecordImage(const LayoutObject& object,
   if (id_record_map_.Contains(node_id) &&
       IsJustLoaded(cachedImg, *id_record_map_.at(node_id))) {
     records_pending_timing_.push(node_id);
-    id_record_map_.at(node_id)->frame_index = frame_index_;
-    id_record_map_.at(node_id)->loaded = true;
+    ImageRecord* record = id_record_map_.at(node_id);
+    record->frame_index = frame_index_;
+    record->loaded = true;
+    // Latest image heap differs from largest image heap in that the former
+    // pushes a record when an image is loaded while the latter pushes when an
+    // image is attached to DOM. This causes last image paint to base its order
+    // on load time other than attachment time.
+    // Mind that first_paint_index has to be assigned at the push of
+    // latest_image_heap_ since it's the sorting key.
+    record->first_paint_index = ++first_paint_index_max_;
+    latest_image_heap_.push(record->AsWeakPtr());
   }
 }
 

third_party/blink/renderer/core/paint/image_paint_timing_detector_test.cc

@@ -12,6 +12,7 @@
 #include "third_party/blink/renderer/platform/graphics/static_bitmap_image.h"
 #include "third_party/blink/renderer/platform/runtime_enabled_features.h"
 #include "third_party/blink/renderer/platform/testing/runtime_enabled_features_test_helpers.h"
+#include "third_party/blink/renderer/platform/testing/wtf/scoped_mock_clock.h"
 #include "third_party/skia/include/core/SkImage.h"
 #include "third_party/skia/include/core/SkSurface.h"
 
@@ -313,41 +314,81 @@ TEST_F(ImagePaintTimingDetectorTest, LastImagePaint_OneImage) {
 }
 
 TEST_F(ImagePaintTimingDetectorTest, LastImagePaint_Last) {
+  WTF::ScopedMockClock clock;
   SetBodyInnerHTML(R"HTML(
     <div id="parent">
-      <img id="1"></img>
-      <img id="2"></img>
-      <img id="3"></img>
+      <img height="10" width="10" id="1"></img>
+      <img height="5" width="5" id="2"></img>
+      <img height="7" width="7" id="3"></img>
     </div>
   )HTML");
-  TimeTicks time1 = CurrentTimeTicks();
+  GetFrameView().UpdateAllLifecyclePhases();
   SetImageAndPaint("1", 10, 10);
-  UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
+  GetFrameView().UpdateAllLifecyclePhases();
+  clock.Advance(TimeDelta::FromSecondsD(1));
+  InvokeCallback();
+
   ImageRecord* record;
   record = FindLastPaintCandidate();
   EXPECT_TRUE(record);
-  EXPECT_GE(record->first_paint_time_after_loaded, time1);
+  EXPECT_EQ(record->first_size, 100);
+  EXPECT_EQ(record->first_paint_time_after_loaded,
+            base::TimeTicks() + TimeDelta::FromSecondsD(1));
 
-  TimeTicks time2 = CurrentTimeTicks();
   SetImageAndPaint("2", 5, 5);
-  UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
+  GetFrameView().UpdateAllLifecyclePhases();
+  clock.Advance(TimeDelta::FromSecondsD(1));
+  InvokeCallback();
+
   record = FindLastPaintCandidate();
   EXPECT_TRUE(record);
-  EXPECT_GE(record->first_paint_time_after_loaded, time2);
+#if defined(OS_MACOSX)
+  EXPECT_EQ(record->first_size, 30);
+#else
+  EXPECT_EQ(record->first_size, 25);
+#endif
+  EXPECT_EQ(record->first_paint_time_after_loaded,
+            base::TimeTicks() + TimeDelta::FromSecondsD(2));
 
-  TimeTicks time3 = CurrentTimeTicks();
   SetImageAndPaint("3", 7, 7);
-  UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
+  GetFrameView().UpdateAllLifecyclePhases();
+  clock.Advance(TimeDelta::FromSecondsD(1));
+  // 6th s
+  InvokeCallback();
   record = FindLastPaintCandidate();
   EXPECT_TRUE(record);
-  EXPECT_GE(record->first_paint_time_after_loaded, time3);
+  EXPECT_GE(record->first_paint_time_after_loaded,
+            base::TimeTicks() + TimeDelta::FromSecondsD(3));
 
   GetDocument().getElementById("parent")->RemoveChild(
       GetDocument().getElementById("3"));
   record = FindLastPaintCandidate();
   EXPECT_TRUE(record);
-  EXPECT_GE(record->first_paint_time_after_loaded, time2);
-  EXPECT_LE(record->first_paint_time_after_loaded, time3);
+  EXPECT_GE(record->first_paint_time_after_loaded,
+            base::TimeTicks() + TimeDelta::FromSecondsD(2));
+}
+
+TEST_F(ImagePaintTimingDetectorTest, LastImagePaint_LastBasedOnLoadTime) {
+  SetBodyInnerHTML(R"HTML(
+    <div id="parent">
+      <img height="5" width="5" id="1"></img>
+    </div>
+  )HTML");
+  Element* image = GetDocument().CreateRawElement(html_names::kImgTag);
+  image->setAttribute(html_names::kIdAttr, "2");
+  image->setAttribute(html_names::kHeightAttr, "10");
+  image->setAttribute(html_names::kWidthAttr, "10");
+  GetDocument().getElementById("parent")->appendChild(image);
+  SetImageAndPaint("2", 10, 10);
+  UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
+
+  SetImageAndPaint("1", 5, 5);
+  UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
+
+  ImageRecord* record;
+  record = FindLastPaintCandidate();
+  EXPECT_TRUE(record);
+  EXPECT_EQ(record->first_size, 25);
 }
 
 TEST_F(ImagePaintTimingDetectorTest, LastImagePaint_IgnoreTheRemoved) {