[ElementTiming] Replace responseEnd with loadTime

This CL replaces responseEnd with loadTime in ElementTiming. Before,
responseEnd would be used and would be problematic for memory cached
images and for inline images (data scheme). We change it to loadTime,
the time at which LayoutObject::ImageNotifyFinished is called. This is
the same time as when LargestContentfulPaint computes loadTime. As they
are both under runtime flags and the hooks are not unified, for
simplicity we compute the timestamp again. This will not be too
expensive for ElementTiming, as the timestamp is gated on the existence
of the elementtiming attribute.

To store the loadTime, we change images_notified_ to be a HashMap which
now stores the timestamp corresponding to loadTime and whether the image
has painted (after being loaded).

Bug: 982046, 879270
Change-Id: I69da42c9250cc36c567df5da285ef1a9a357b554
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1710840
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#680117}

third_party/blink/renderer/core/layout/layout_object.cc

@@ -3517,6 +3517,12 @@ void LayoutObject::ImageNotifyFinished(ImageResourceContent* image) {
   if (AXObjectCache* cache = GetDocument().ExistingAXObjectCache())
     cache->ImageLoaded(this);
 
+  if (RuntimeEnabledFeatures::ElementTimingEnabled(&GetDocument())) {
+    LocalDOMWindow* window = GetDocument().domWindow();
+    if (window) {
+      ImageElementTiming::From(*window).NotifyImageFinished(*this, image);
+    }
+  }
   if (RuntimeEnabledFeatures::FirstContentfulPaintPlusPlusEnabled()) {
     if (LocalFrameView* frame_view = GetFrameView()) {
       frame_view->GetPaintTimingDetector().NotifyImageFinished(*this, image);

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

@@ -65,6 +65,16 @@ ImageElementTiming::ImageElementTiming(LocalDOMWindow& window)
       GetSupplementable()->document()));
 }
 
+void ImageElementTiming::NotifyImageFinished(
+    const LayoutObject& layout_object,
+    const ImageResourceContent* cached_image) {
+  if (!internal::IsExplicitlyRegisteredForTiming(&layout_object))
+    return;
+
+  images_notified_.Set(std::make_pair(&layout_object, cached_image),
+                       ImageInfo(base::TimeTicks::Now()));
+}
+
 void ImageElementTiming::NotifyImagePainted(
     const LayoutObject* layout_object,
     const ImageResourceContent* cached_image,
@@ -74,11 +84,13 @@ void ImageElementTiming::NotifyImagePainted(
   if (!internal::IsExplicitlyRegisteredForTiming(layout_object))
     return;
 
-  auto result =
-      images_notified_.insert(std::make_pair(layout_object, cached_image));
-  if (result.is_new_entry && cached_image) {
+  auto it = images_notified_.find(std::make_pair(layout_object, cached_image));
+  DCHECK(it != images_notified_.end());
+  if (!it->value.is_painted_ && cached_image) {
+    it->value.is_painted_ = true;
     NotifyImagePaintedInternal(layout_object->GetNode(), *layout_object,
-                               *cached_image, current_paint_chunk_properties);
+                               *cached_image, current_paint_chunk_properties,
+                               it->value.load_time_);
   }
 }
 
@@ -86,7 +98,8 @@ void ImageElementTiming::NotifyImagePaintedInternal(
     Node* node,
     const LayoutObject& layout_object,
     const ImageResourceContent& cached_image,
-    const PropertyTreeState& current_paint_chunk_properties) {
+    const PropertyTreeState& current_paint_chunk_properties,
+    base::TimeTicks load_time) {
   LocalFrame* frame = GetSupplementable()->GetFrame();
   DCHECK(frame == layout_object.GetDocument().GetFrame());
   DCHECK(node);
@@ -134,7 +147,7 @@ void ImageElementTiming::NotifyImagePaintedInternal(
       // Create an entry with a |startTime| of 0.
       performance->AddElementTiming(
           ImagePaintString(), url.GetString(), intersection_rect,
-          base::TimeTicks(), cached_image.LoadResponseEnd(), attr,
+          base::TimeTicks(), load_time, attr,
           cached_image.IntrinsicSize(kDoNotRespectImageOrientation), id,
           element);
     }
@@ -149,7 +162,7 @@ void ImageElementTiming::NotifyImagePaintedInternal(
                                 ? url.GetString().Left(kInlineImageMaxChars)
                                 : url.GetString();
   element_timings_.emplace_back(MakeGarbageCollected<ElementTimingInfo>(
-      image_url, intersection_rect, cached_image.LoadResponseEnd(), attr,
+      image_url, intersection_rect, load_time, attr,
       cached_image.IntrinsicSize(kDoNotRespectImageOrientation), id, element));
   // Only queue a swap promise when |element_timings_| was empty. All of the
   // records in |element_timings_| will be processed when the promise succeeds
@@ -180,11 +193,20 @@ void ImageElementTiming::NotifyBackgroundImagePainted(
   if (!cached_image || !cached_image->IsLoaded())
     return;
 
-  auto result =
-      images_notified_.insert(std::make_pair(layout_object, cached_image));
-  if (result.is_new_entry) {
-    NotifyImagePaintedInternal(node, *layout_object, *cached_image,
-                               current_paint_chunk_properties);
+  std::pair<const LayoutObject*, const ImageResourceContent*> pair =
+      std::make_pair(layout_object, cached_image);
+  auto it = images_notified_.find(pair);
+  // TODO(crbug.com/986891): ideally |images_notified_| would always be able to
+  // find the pair here. However, for some background images that is currently
+  // not possible. Therefore, in those cases we create the entry here with
+  // loadTime of 0. Once the bug is fixed, we should replace that with a DCHECK.
+  if (it == images_notified_.end())
+    images_notified_.Set(pair, ImageInfo(base::TimeTicks()));
+  if (!it->value.is_painted_ && cached_image) {
+    it->value.is_painted_ = true;
+    NotifyImagePaintedInternal(layout_object->GetNode(), *layout_object,
+                               *cached_image, current_paint_chunk_properties,
+                               it->value.load_time_);
   }
 }
 

third_party/blink/renderer/core/paint/image_element_timing.h

@@ -40,6 +40,8 @@ class CORE_EXPORT ImageElementTiming final
 
   static ImageElementTiming& From(LocalDOMWindow&);
 
+  void NotifyImageFinished(const LayoutObject&, const ImageResourceContent*);
+
   // Called when the LayoutObject has been painted. This method might queue a
   // swap promise to compute and report paint timestamps.
   void NotifyImagePainted(
@@ -64,7 +66,8 @@ class CORE_EXPORT ImageElementTiming final
       Node*,
       const LayoutObject&,
       const ImageResourceContent& cached_image,
-      const PropertyTreeState& current_paint_chunk_properties);
+      const PropertyTreeState& current_paint_chunk_properties,
+      base::TimeTicks load_time);
 
   // Callback for the swap promise. Reports paint timestamps.
   void ReportImagePaintSwapTime(WebWidgetClient::SwapResult,
@@ -107,11 +110,20 @@ class CORE_EXPORT ImageElementTiming final
   // Vector containing the element timing infos that will be reported during the
   // next swap promise callback.
   HeapVector<Member<ElementTimingInfo>> element_timings_;
+  struct ImageInfo {
+    // HashMap values require default constructor so we set default value for
+    // |load_time|.
+    ImageInfo(base::TimeTicks load_time = base::TimeTicks())
+        : load_time_(load_time), is_painted_(false) {}
+
+    base::TimeTicks load_time_;
+    bool is_painted_;
+  };
+  typedef std::pair<const LayoutObject*, const ImageResourceContent*> RecordId;
   // Hashmap of pairs of elements, LayoutObjects (for the elements) and
   // ImageResourceContent (for the src) which correspond to either images or
   // background images whose paint has been observed.
-  WTF::HashSet<std::pair<const LayoutObject*, const ImageResourceContent*>>
-      images_notified_;
+  WTF::HashMap<RecordId, ImageInfo> images_notified_;
 
   DISALLOW_COPY_AND_ASSIGN(ImageElementTiming);
 };

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

@@ -55,10 +55,16 @@ class ImageElementTimingTest : public testing::Test {
     return layout_image;
   }
 
-  const WTF::HashSet<
-      std::pair<const LayoutObject*, const ImageResourceContent*>>&
-  GetImagesNotified() {
-    return ImageElementTiming::From(*GetDoc()->domWindow()).images_notified_;
+  bool ImagesNotifiedContains(
+      const std::pair<const LayoutObject*, const ImageResourceContent*>&
+          record_id) {
+    return ImageElementTiming::From(*GetDoc()->domWindow())
+        .images_notified_.Contains(record_id);
+  }
+
+  unsigned ImagesNotifiedSize() {
+    return ImageElementTiming::From(*GetDoc()->domWindow())
+        .images_notified_.size();
   }
 
   Document* GetDoc() {
@@ -143,7 +149,7 @@ TEST_F(ImageElementTimingTest, IgnoresUnmarkedElement) {
   LayoutImage* layout_image = SetImageResource("target", 5, 5);
   ASSERT_TRUE(layout_image);
   UpdateAllLifecyclePhases();
-  EXPECT_FALSE(GetImagesNotified().Contains(
+  EXPECT_FALSE(ImagesNotifiedContains(
       std::make_pair(layout_image, layout_image->CachedImage())));
 }
 
@@ -165,7 +171,7 @@ TEST_F(ImageElementTimingTest, ImageInsideSVG) {
   UpdateAllLifecyclePhases();
 
   // |layout_image| should have had its paint notified to ImageElementTiming.
-  EXPECT_TRUE(GetImagesNotified().Contains(
+  EXPECT_TRUE(ImagesNotifiedContains(
       std::make_pair(layout_image, layout_image->CachedImage())));
 }
 
@@ -178,13 +184,13 @@ TEST_F(ImageElementTimingTest, ImageRemoved) {
   LayoutImage* layout_image = SetImageResource("target", 5, 5);
   ASSERT_TRUE(layout_image);
   UpdateAllLifecyclePhases();
-  EXPECT_TRUE(GetImagesNotified().Contains(
+  EXPECT_TRUE(ImagesNotifiedContains(
       std::make_pair(layout_image, layout_image->CachedImage())));
 
   GetDoc()->getElementById("target")->remove();
   // |layout_image| should no longer be part of |images_notified| since it will
   // be destroyed.
-  EXPECT_TRUE(GetImagesNotified().IsEmpty());
+  EXPECT_EQ(ImagesNotifiedSize(), 0u);
 }
 
 TEST_F(ImageElementTimingTest, SVGImageRemoved) {
@@ -198,13 +204,13 @@ TEST_F(ImageElementTimingTest, SVGImageRemoved) {
   LayoutSVGImage* layout_image = SetSVGImageResource("target", 5, 5);
   ASSERT_TRUE(layout_image);
   UpdateAllLifecyclePhases();
-  EXPECT_TRUE(GetImagesNotified().Contains(std::make_pair(
+  EXPECT_TRUE(ImagesNotifiedContains(std::make_pair(
       layout_image, layout_image->ImageResource()->CachedImage())));
 
   GetDoc()->getElementById("target")->remove();
   // |layout_image| should no longer be part of |images_notified| since it will
   // be destroyed.
-  EXPECT_TRUE(GetImagesNotified().IsEmpty());
+  EXPECT_EQ(ImagesNotifiedSize(), 0u);
 }
 
 TEST_F(ImageElementTimingTest, BackgroundImageRemoved) {
@@ -224,11 +230,11 @@ TEST_F(ImageElementTimingTest, BackgroundImageRemoved) {
   ImageResourceContent* content =
       object->Style()->BackgroundLayers().GetImage()->CachedImage();
   UpdateAllLifecyclePhases();
-  EXPECT_EQ(GetImagesNotified().size(), 1u);
-  EXPECT_TRUE(GetImagesNotified().Contains(std::make_pair(object, content)));
+  EXPECT_EQ(ImagesNotifiedSize(), 1u);
+  EXPECT_TRUE(ImagesNotifiedContains(std::make_pair(object, content)));
 
   GetDoc()->getElementById("target")->remove();
-  EXPECT_TRUE(GetImagesNotified().IsEmpty());
+  EXPECT_EQ(ImagesNotifiedSize(), 0u);
 }
 
 }  // namespace blink

third_party/blink/renderer/core/timing/performance_element_timing.cc

@@ -15,7 +15,7 @@ PerformanceElementTiming* PerformanceElementTiming::Create(
     const String& url,
     const FloatRect& intersection_rect,
     DOMHighResTimeStamp render_time,
-    DOMHighResTimeStamp response_end,
+    DOMHighResTimeStamp load_time,
     const AtomicString& identifier,
     int naturalWidth,
     int naturalHeight,
@@ -27,7 +27,7 @@ PerformanceElementTiming* PerformanceElementTiming::Create(
   DCHECK_GE(naturalHeight, 0);
   DCHECK(element);
   return MakeGarbageCollected<PerformanceElementTiming>(
-      name, url, intersection_rect, render_time, response_end, identifier,
+      name, url, intersection_rect, render_time, load_time, identifier,
       naturalWidth, naturalHeight, id, element);
 }
 
@@ -36,7 +36,7 @@ PerformanceElementTiming::PerformanceElementTiming(
     const String& url,
     const FloatRect& intersection_rect,
     DOMHighResTimeStamp render_time,
-    DOMHighResTimeStamp response_end,
+    DOMHighResTimeStamp load_time,
     const AtomicString& identifier,
     int naturalWidth,
     int naturalHeight,
@@ -46,7 +46,7 @@ PerformanceElementTiming::PerformanceElementTiming(
       element_(element),
       intersection_rect_(DOMRectReadOnly::FromFloatRect(intersection_rect)),
       render_time_(render_time),
-      response_end_(response_end),
+      load_time_(load_time),
       identifier_(identifier),
       naturalWidth_(naturalWidth),
       naturalHeight_(naturalHeight),
@@ -72,7 +72,15 @@ Element* PerformanceElementTiming::element() const {
 
 void PerformanceElementTiming::BuildJSONValue(V8ObjectBuilder& builder) const {
   PerformanceEntry::BuildJSONValue(builder);
+  builder.Add("renderTime", render_time_);
+  builder.Add("loadTime", load_time_);
   builder.Add("intersectionRect", intersection_rect_);
+  builder.Add("identifier", identifier_);
+  builder.Add("naturalWidth", naturalWidth_);
+  builder.Add("naturalHeight", naturalHeight_);
+  builder.Add("id", id_);
+  builder.Add("element", element());
+  builder.Add("url", url_);
 }
 
 void PerformanceElementTiming::Trace(blink::Visitor* visitor) {

third_party/blink/renderer/core/timing/performance_element_timing.h

@@ -25,7 +25,7 @@ class CORE_EXPORT PerformanceElementTiming final : public PerformanceEntry {
                                           const String& url,
                                           const FloatRect& intersection_rect,
                                           DOMHighResTimeStamp render_time,
-                                          DOMHighResTimeStamp response_end,
+                                          DOMHighResTimeStamp load_time,
                                           const AtomicString& identifier,
                                           int naturalWidth,
                                           int naturalHeight,
@@ -35,7 +35,7 @@ class CORE_EXPORT PerformanceElementTiming final : public PerformanceEntry {
                            const String& url,
                            const FloatRect& intersection_rect,
                            DOMHighResTimeStamp render_time,
-                           DOMHighResTimeStamp response_end,
+                           DOMHighResTimeStamp load_time,
                            const AtomicString& identifier,
                            int naturalWidth,
                            int naturalHeight,
@@ -49,7 +49,7 @@ class CORE_EXPORT PerformanceElementTiming final : public PerformanceEntry {
 
   DOMRectReadOnly* intersectionRect() const { return intersection_rect_; }
   DOMHighResTimeStamp renderTime() const { return render_time_; }
-  DOMHighResTimeStamp responseEnd() const { return response_end_; }
+  DOMHighResTimeStamp loadTime() const { return load_time_; }
   AtomicString identifier() const { return identifier_; }
   unsigned naturalWidth() const { return naturalWidth_; }
   unsigned naturalHeight() const { return naturalHeight_; }
@@ -65,7 +65,7 @@ class CORE_EXPORT PerformanceElementTiming final : public PerformanceEntry {
   WeakMember<Element> element_;
   Member<DOMRectReadOnly> intersection_rect_;
   DOMHighResTimeStamp render_time_;
-  DOMHighResTimeStamp response_end_;
+  DOMHighResTimeStamp load_time_;
   AtomicString identifier_;
   unsigned naturalWidth_;
   unsigned naturalHeight_;

third_party/blink/renderer/core/timing/performance_element_timing.idl

@@ -6,7 +6,7 @@
 [RuntimeEnabled=ElementTiming]
 interface PerformanceElementTiming : PerformanceEntry {
     readonly attribute DOMHighResTimeStamp renderTime;
-    readonly attribute DOMHighResTimeStamp responseEnd;
+    readonly attribute DOMHighResTimeStamp loadTime;
     readonly attribute DOMRectReadOnly intersectionRect;
     readonly attribute DOMString identifier;
     readonly attribute unsigned long naturalWidth;

third_party/blink/renderer/core/timing/window_performance.cc

@@ -391,7 +391,7 @@ void WindowPerformance::AddElementTiming(const AtomicString& name,
                                          const String& url,
                                          const FloatRect& rect,
                                          base::TimeTicks start_time,
-                                         base::TimeTicks response_end,
+                                         base::TimeTicks load_time,
                                          const AtomicString& identifier,
                                          const IntSize& intrinsic_size,
                                          const AtomicString& id,
@@ -399,7 +399,7 @@ void WindowPerformance::AddElementTiming(const AtomicString& name,
   DCHECK(RuntimeEnabledFeatures::ElementTimingEnabled(GetExecutionContext()));
   PerformanceElementTiming* entry = PerformanceElementTiming::Create(
       name, url, rect, MonotonicTimeToDOMHighResTimeStamp(start_time),
-      MonotonicTimeToDOMHighResTimeStamp(response_end), identifier,
+      MonotonicTimeToDOMHighResTimeStamp(load_time), identifier,
       intrinsic_size.Width(), intrinsic_size.Height(), id, element);
   if (HasObserverFor(PerformanceEntry::kElement)) {
     UseCounter::Count(GetExecutionContext(),

third_party/blink/renderer/core/timing/window_performance.h

@@ -78,7 +78,7 @@ class CORE_EXPORT WindowPerformance final : public Performance,
                         const String& url,
                         const FloatRect& rect,
                         base::TimeTicks start_time,
-                        base::TimeTicks response_end,
+                        base::TimeTicks load_time,
                         const AtomicString& identifier,
                         const IntSize& intrinsic_size,
                         const AtomicString& id,

third_party/blink/web_tests/external/wpt/element-timing/images-repeated-resource.html

@@ -13,8 +13,10 @@ body {
 <script>
   let beforeRender;
   let numEntries = 0;
-  let responseEnd1;
-  let responseEnd2;
+  let loadTime1;
+  let loadTime2;
+  let renderTime1;
+  let renderTime2;
   let img;
   let img2;
   const index = window.location.href.lastIndexOf('/');
@@ -26,23 +28,33 @@ body {
     }
     const observer = new PerformanceObserver(
       t.step_func(function(entryList) {
-        entryList.getEntries().forEach(entry => {
-          // Easier to check the |element| attribute here since element ID is the same for both images.
-          checkElement(entry, pathname, entry.identifier, 'image_id', beforeRender, null);
-          checkNaturalSize(entry, 100, 100);
-          if (entry.identifier === 'my_image') {
-            ++numEntries;
-            responseEnd1 = entry.responseEnd;
-            assert_equals(entry.element, img);
-          }
-          else if (entry.identifier === 'my_image2') {
-            ++numEntries;
-            responseEnd2 = entry.responseEnd;
-            assert_equals(entry.element, img2);
-          }
-        });
+        assert_equals(entryList.getEntries().length, 1);
+        const entry = entryList.getEntries()[0];
+        // Easier to check the |element| attribute here since element ID is the same for both images.
+        checkElement(entry, pathname, entry.identifier, 'image_id', beforeRender, null);
+        checkNaturalSize(entry, 100, 100);
+        if (entry.identifier === 'my_image') {
+          ++numEntries;
+          loadTime1 = entry.loadTime;
+          renderTime1 = entry.renderTime;
+          assert_equals(entry.element, img);
+
+          img2 = document.createElement('img');
+          img2.src = 'resources/square100.png';
+          img2.setAttribute('elementtiming', 'my_image2');
+          img2.setAttribute('id', 'image_id');
+          document.body.appendChild(img2);
+          beforeRender = performance.now();
+        }
+        else if (entry.identifier === 'my_image2') {
+          ++numEntries;
+          loadTime2 = entry.loadTime;
+          renderTime2 = entry.renderTime;
+          assert_equals(entry.element, img2);
+        }
         if (numEntries == 2) {
-          assert_equals(responseEnd1, responseEnd2);
+          assert_greater_than(loadTime2, loadTime1, 'Second image loads after first.');
+          assert_greater_than(renderTime2, renderTime1, 'Second image renders after first');
           t.done();
         }
       })
@@ -57,16 +69,9 @@ body {
       img.setAttribute('elementtiming', 'my_image');
       img.setAttribute('id', 'image_id');
       document.body.appendChild(img);
-
-      img2 = document.createElement('img');
-      img2.src = 'resources/square100.png';
-      img2.setAttribute('elementtiming', 'my_image2');
-      img2.setAttribute('id', 'image_id');
-      document.body.appendChild(img2);
-
       beforeRender = performance.now();
     };
-  }, 'Element with elementtiming attribute is observable.');
+  }, 'Elements with elementtiming and same src are observable.');
 </script>
 
 </body>

third_party/blink/web_tests/external/wpt/element-timing/resources/element-timing-helpers.js

@@ -22,7 +22,8 @@ function checkElement(entry, expectedUrl, expectedIdentifier, expectedID, before
   assert_equals(entry.name, 'image-paint');
   const rt_entries = performance.getEntriesByName(expectedUrl, 'resource');
   assert_equals(rt_entries.length, 1);
-  assert_equals(rt_entries[0].responseEnd, entry.responseEnd);
+  assert_greater_than_equal(entry.loadTime, rt_entries[0].responseEnd,
+    'Image loadTime is after the resource responseEnd');
 }
 
 function checkElementWithoutResourceTiming(entry, expectedUrl, expectedIdentifier,
@@ -30,8 +31,8 @@ function checkElementWithoutResourceTiming(entry, expectedUrl, expectedIdentifie
   checkElementInternal(entry, expectedUrl, expectedIdentifier, expectedID, beforeRender,
       expectedElement);
   assert_equals(entry.name, 'image-paint');
-  // No associated resource from ResourceTiming, so the responseEnd should be 0.
-  assert_equals(entry.responseEnd, 0);
+  // No associated resource from ResourceTiming, so not much to compare loadTime with.
+  assert_greater_than(entry.loadTime, 0);
 }
 
 // Checks that the rect matches the desired values [left right top bottom].
@@ -57,5 +58,5 @@ function checkTextElement(entry, expectedIdentifier, expectedID, beforeRender,
   checkElementInternal(entry, '', expectedIdentifier, expectedID, beforeRender,
       expectedElement);
   assert_equals(entry.name, 'text-paint');
-  assert_equals(entry.responseEnd, 0);
+  assert_equals(entry.loadTime, 0);
 }

third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt

@@ -5439,10 +5439,10 @@ interface PerformanceElementTiming : PerformanceEntry
     getter id
     getter identifier
     getter intersectionRect
+    getter loadTime
     getter naturalHeight
     getter naturalWidth
     getter renderTime
-    getter responseEnd
     getter url
     method constructor
     method toJSON