[ElementTiming] Fix background image loadTime

This change adds a map |background_image_timestamps_| of load timestamps of
background images tracked by StyleFetchedImage, which are the background images
with url. The map only tracks the load time, as |images_notified_| must still be
used to track the background image paints. Otherwise we'd only report an entry
per background image even when applied to multiple elements.

This change also makes computations a bit more efficient by only calling
base::TimeTicks::Now() on the first time we load an entry. It also fixes a
problem of calling Set() in between usages of an iterator, which is not allowed.
Before this change, the added test would crash due to this problem.

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

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

@@ -19,6 +19,7 @@
 #include "third_party/blink/renderer/core/style/border_edge.h"
 #include "third_party/blink/renderer/core/style/computed_style.h"
 #include "third_party/blink/renderer/core/style/shadow_list.h"
+#include "third_party/blink/renderer/core/style/style_fetched_image.h"
 #include "third_party/blink/renderer/platform/geometry/layout_rect.h"
 #include "third_party/blink/renderer/platform/graphics/graphics_context_state_saver.h"
 #include "third_party/blink/renderer/platform/graphics/paint/paint_controller.h"
@@ -563,7 +564,7 @@ inline bool PaintFastBottomLayer(Node* node,
     LocalDOMWindow* window = node->GetDocument().domWindow();
     DCHECK(window);
     ImageElementTiming::From(*window).NotifyBackgroundImagePainted(
-        node, info.image,
+        node, To<StyleFetchedImage>(info.image.Get()),
         context.GetPaintController().CurrentPaintChunkProperties());
   }
   return true;
@@ -695,7 +696,7 @@ void PaintFillLayerBackground(GraphicsContext& context,
       LocalDOMWindow* window = node->GetDocument().domWindow();
       DCHECK(window);
       ImageElementTiming::From(*window).NotifyBackgroundImagePainted(
-          node, info.image,
+          node, To<StyleFetchedImage>(info.image.Get()),
           context.GetPaintController().CurrentPaintChunkProperties());
     }
   }

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

@@ -8,7 +8,7 @@
 #include "third_party/blink/renderer/core/loader/resource/image_resource_content.h"
 #include "third_party/blink/renderer/core/page/chrome_client.h"
 #include "third_party/blink/renderer/core/paint/element_timing_utils.h"
-#include "third_party/blink/renderer/core/style/style_image.h"
+#include "third_party/blink/renderer/core/style/style_fetched_image.h"
 #include "third_party/blink/renderer/core/timing/dom_window_performance.h"
 #include "third_party/blink/renderer/core/timing/window_performance.h"
 #include "third_party/blink/renderer/platform/graphics/paint/property_tree_state.h"
@@ -71,8 +71,18 @@ void ImageElementTiming::NotifyImageFinished(
   if (!internal::IsExplicitlyRegisteredForTiming(&layout_object))
     return;
 
-  images_notified_.Set(std::make_pair(&layout_object, cached_image),
-                       ImageInfo(base::TimeTicks::Now()));
+  const auto& insertion_result = images_notified_.insert(
+      std::make_pair(&layout_object, cached_image), ImageInfo());
+  if (insertion_result.is_new_entry)
+    insertion_result.stored_value->value.load_time_ = base::TimeTicks::Now();
+}
+
+void ImageElementTiming::NotifyBackgroundImageFinished(
+    const StyleFetchedImage* style_image) {
+  const auto& insertion_result =
+      background_image_timestamps_.insert(style_image, base::TimeTicks());
+  if (insertion_result.is_new_entry)
+    insertion_result.stored_value->value = base::TimeTicks::Now();
 }
 
 void ImageElementTiming::NotifyImagePainted(
@@ -177,7 +187,7 @@ void ImageElementTiming::NotifyImagePaintedInternal(
 
 void ImageElementTiming::NotifyBackgroundImagePainted(
     Node* node,
-    const StyleImage* background_image,
+    const StyleFetchedImage* background_image,
     const PropertyTreeState& current_paint_chunk_properties) {
   DCHECK(node);
   DCHECK(background_image);
@@ -193,20 +203,18 @@ void ImageElementTiming::NotifyBackgroundImagePainted(
   if (!cached_image || !cached_image->IsLoaded())
     return;
 
-  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;
+  auto it = background_image_timestamps_.find(background_image);
+  DCHECK(it != background_image_timestamps_.end());
+
+  ImageInfo& info =
+      images_notified_
+          .insert(std::make_pair(layout_object, cached_image), ImageInfo())
+          .stored_value->value;
+  if (!info.is_painted_) {
+    info.is_painted_ = true;
     NotifyImagePaintedInternal(layout_object->GetNode(), *layout_object,
                                *cached_image, current_paint_chunk_properties,
-                               it->value.load_time_);
+                               it->value);
   }
 }
 
@@ -233,6 +241,7 @@ void ImageElementTiming::NotifyImageRemoved(const LayoutObject* layout_object,
 
 void ImageElementTiming::Trace(blink::Visitor* visitor) {
   visitor->Trace(element_timings_);
+  visitor->Trace(background_image_timestamps_);
   Supplement<LocalDOMWindow>::Trace(visitor);
 }
 

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

@@ -8,7 +8,9 @@
 #include <utility>
 
 #include "third_party/blink/public/web/web_widget_client.h"
+#include "third_party/blink/renderer/core/dom/element.h"
 #include "third_party/blink/renderer/core/frame/local_dom_window.h"
+#include "third_party/blink/renderer/platform/heap/heap_allocator.h"
 #include "third_party/blink/renderer/platform/supplementable.h"
 #include "third_party/blink/renderer/platform/wtf/functional.h"
 #include "third_party/blink/renderer/platform/wtf/hash_set.h"
@@ -19,7 +21,7 @@ namespace blink {
 
 class ImageResourceContent;
 class PropertyTreeState;
-class StyleImage;
+class StyleFetchedImage;
 
 // ImageElementTiming is responsible for tracking the paint timings for <img>
 // elements for a given window.
@@ -42,6 +44,8 @@ class CORE_EXPORT ImageElementTiming final
 
   void NotifyImageFinished(const LayoutObject&, const ImageResourceContent*);
 
+  void NotifyBackgroundImageFinished(const StyleFetchedImage*);
+
   // Called when the LayoutObject has been painted. This method might queue a
   // swap promise to compute and report paint timestamps.
   void NotifyImagePainted(
@@ -51,7 +55,7 @@ class CORE_EXPORT ImageElementTiming final
 
   void NotifyBackgroundImagePainted(
       Node*,
-      const StyleImage* background_image,
+      const StyleFetchedImage* background_image,
       const PropertyTreeState& current_paint_chunk_properties);
 
   void NotifyImageRemoved(const LayoutObject*,
@@ -111,20 +115,24 @@ class CORE_EXPORT ImageElementTiming final
   // 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) {}
+    ImageInfo() {}
 
     base::TimeTicks load_time_;
-    bool is_painted_;
+    bool is_painted_ = false;
   };
   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.
+  // background images whose paint has been observed. For background images,
+  // only the |is_painted_| bit is used, as the timestamp needs to be tracked by
+  // |background_image_timestamps_|.
   WTF::HashMap<RecordId, ImageInfo> images_notified_;
 
+  // Hashmap of background images which contain information about the load time
+  // of the background image.
+  HeapHashMap<WeakMember<const StyleFetchedImage>, base::TimeTicks>
+      background_image_timestamps_;
+
   DISALLOW_COPY_AND_ASSIGN(ImageElementTiming);
 };
 

third_party/blink/renderer/core/style/style_fetched_image.cc

@@ -30,11 +30,13 @@
 #include "third_party/blink/renderer/core/frame/local_frame_client.h"
 #include "third_party/blink/renderer/core/frame/settings.h"
 #include "third_party/blink/renderer/core/loader/resource/image_resource_content.h"
+#include "third_party/blink/renderer/core/paint/image_element_timing.h"
 #include "third_party/blink/renderer/core/style/computed_style.h"
 #include "third_party/blink/renderer/core/svg/graphics/svg_image.h"
 #include "third_party/blink/renderer/core/svg/graphics/svg_image_for_container.h"
 #include "third_party/blink/renderer/platform/geometry/layout_size.h"
 #include "third_party/blink/renderer/platform/graphics/placeholder_image.h"
+#include "third_party/blink/renderer/platform/runtime_enabled_features.h"
 
 namespace blink {
 
@@ -140,6 +142,12 @@ void StyleFetchedImage::ImageNotifyFinished(ImageResourceContent*) {
     image_->UpdateImageAnimationPolicy();
   }
 
+  if (document_ && RuntimeEnabledFeatures::ElementTimingEnabled(document_)) {
+    if (LocalDOMWindow* window = document_->domWindow()) {
+      ImageElementTiming::From(*window).NotifyBackgroundImageFinished(this);
+    }
+  }
+
   // Oilpan: do not prolong the Document's lifetime.
   document_.Clear();
 }

third_party/blink/web_tests/external/wpt/element-timing/first-letter-background.html

+<!DOCTYPE HTML>
+<meta charset=utf-8>
+<title>Element Timing: observe element with background image in its first letter</title>
+<body>
+<style>
+#target::first-letter {
+  background-image: url('/images/black-rectangle.png');
+}
+</style>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="resources/element-timing-helpers.js"></script>
+<script>
+  let beforeRender = performance.now();
+  async_test(function (t) {
+    if (!window.PerformanceElementTiming) {
+      assert_unreached("PerformanceElementTiming is not implemented");
+    }
+    const div = document.getElementById('target');
+    let textObserved = false;
+    let imageObserved = false;
+    const observer = new PerformanceObserver(
+      t.step_func(function(entryList) {
+        entryList.getEntries().forEach(entry => {
+          if (entry.name === 'text-paint') {
+            checkTextElement(entry, 'my_div', 'target', beforeRender, div);
+            textObserved = true;
+          }
+          else {
+            assert_equals(entry.name, 'image-paint');
+            const pathname = window.location.origin + '/images/black-rectangle.png';
+            checkElement(entry, pathname, 'my_div', 'target', beforeRender, div);
+            checkNaturalSize(entry, 100, 50);
+            imageObserved = true;
+          }
+        });
+        if (textObserved && imageObserved)
+          t.done();
+      })
+    );
+    observer.observe({entryTypes: ['element']});
+  }, 'Element with elementtiming attribute and background image in first-letter is observable.');
+</script>
+<div id='target' elementtiming='my_div'>A</div>
+</body>