Skip to content

Commit

Permalink
LCP Mouseover heuristics to include background images
Browse files Browse the repository at this point in the history
Currently the heuristics to detect mouseover-triggered LCP don't include
zoom widgets that are based on background images. This CL expands the
heuristic to also take such LCP entries into account.

Change-Id: I603068c7e3d267ea988c7c11febc4cbbcd036f57
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4062033
Reviewed-by: Noam Rosenthal <nrosenthal@chromium.org>
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1076740}
  • Loading branch information
Yoav Weiss authored and Chromium LUCI CQ committed Nov 29, 2022
1 parent 4c93d7c commit cdf23f0
Show file tree
Hide file tree
Showing 9 changed files with 90 additions and 34 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,23 @@
document.body.appendChild(zoom);
}
};
const loadBackgroundImage = size => {
return event => {
const div = document.createElement("div");
const [width, height] = size.split("x");
div.style = `background-image: url(/images/green-${size}.png); width: ${width}; height: ${height}`;
document.body.appendChild(div);
}
};

// Turn off BFCache to see if it helps with BFCache bot flakes.
window.addEventListener("unload", () => { console.log("Turn off BFCache"); });

image.addEventListener("mouseover", loadImage("100x50"));
span.addEventListener("mouseover", loadImage("256x256"));
const registerMouseover = background => {
const func = background ? loadBackgroundImage : loadImage;
image.addEventListener("mouseover", func("100x50"));
span.addEventListener("mouseover", func("256x256"));
}

const dispatch_mouseover = () => {
span.dispatchEvent(new Event("mouseover"))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,8 @@ IN_PROC_BROWSER_TEST_F(IsAnimatedLCPTest,
/*expected=*/true, /*entries=*/0);
}

class MouseoverLCPTest : public MetricIntegrationTest {
class MouseoverLCPTest : public MetricIntegrationTest,
public testing::WithParamInterface<bool> {
public:
void test_mouseover(const char* html_name,
blink::LargestContentfulPaintType flag_set,
Expand All @@ -358,6 +359,11 @@ class MouseoverLCPTest : public MetricIntegrationTest {
waiter->AddMinimumCompleteResourcesExpectation(2);
Start();
Load(html_name);
std::string background = GetParam() ? "true" : "false";
EXPECT_EQ(EvalJs(web_contents()->GetPrimaryMainFrame(),
"registerMouseover(" + background + ")")
.error,
"");
EXPECT_EQ(
EvalJs(web_contents()->GetPrimaryMainFrame(), "run_test(1)").error, "");

Expand Down Expand Up @@ -421,7 +427,9 @@ class MouseoverLCPTest : public MetricIntegrationTest {
}
};

IN_PROC_BROWSER_TEST_F(MouseoverLCPTest,
INSTANTIATE_TEST_SUITE_P(All, MouseoverLCPTest, ::testing::Values(false, true));

IN_PROC_BROWSER_TEST_P(MouseoverLCPTest,
LargestContentfulPaint_MouseoverOverLCPImage) {
test_mouseover("/mouseover.html",
blink::LargestContentfulPaintType::kAfterMouseover,
Expand All @@ -432,7 +440,7 @@ IN_PROC_BROWSER_TEST_F(MouseoverLCPTest,
/*expected=*/true);
}

IN_PROC_BROWSER_TEST_F(MouseoverLCPTest,
IN_PROC_BROWSER_TEST_P(MouseoverLCPTest,
LargestContentfulPaint_MouseoverOverLCPImageReplace) {
test_mouseover("/mouseover.html?replace",
blink::LargestContentfulPaintType::kAfterMouseover,
Expand All @@ -443,7 +451,7 @@ IN_PROC_BROWSER_TEST_F(MouseoverLCPTest,
/*expected=*/true);
}

IN_PROC_BROWSER_TEST_F(MouseoverLCPTest,
IN_PROC_BROWSER_TEST_P(MouseoverLCPTest,
LargestContentfulPaint_MouseoverOverBody) {
test_mouseover("/mouseover.html",
blink::LargestContentfulPaintType::kAfterMouseover,
Expand All @@ -454,7 +462,7 @@ IN_PROC_BROWSER_TEST_F(MouseoverLCPTest,
/*expected=*/false);
}

IN_PROC_BROWSER_TEST_F(MouseoverLCPTest,
IN_PROC_BROWSER_TEST_P(MouseoverLCPTest,
LargestContentfulPaint_MouseoverOverLCPImageThenBody) {
test_mouseover("/mouseover.html?dispatch",
blink::LargestContentfulPaintType::kAfterMouseover,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,8 @@ bool ImagePaintTimingDetector::RecordImage(
const MediaTiming& media_timing,
const PropertyTreeStateOrAlias& current_paint_chunk_properties,
const StyleFetchedImage* style_image,
const gfx::Rect& image_border) {
const gfx::Rect& image_border,
const bool is_loaded_after_mouseover) {
Node* node = object.GetNode();

if (!node)
Expand All @@ -321,7 +322,8 @@ bool ImagePaintTimingDetector::RecordImage(
image_border, mapped_visual_rect, intrinsic_size,
current_paint_chunk_properties, object, media_timing);
records_manager_.MaybeUpdateLargestIgnoredImage(
record_id, rect_size, image_border, mapped_visual_rect);
record_id, rect_size, image_border, mapped_visual_rect,
is_loaded_after_mouseover);
}
return false;
}
Expand Down Expand Up @@ -365,7 +367,8 @@ bool ImagePaintTimingDetector::RecordImage(
: 0.0;

bool added_pending = records_manager_.RecordFirstPaintAndReturnIsPending(
record_id, rect_size, image_border, mapped_visual_rect, bpp);
record_id, rect_size, image_border, mapped_visual_rect, bpp,
is_loaded_after_mouseover);
if (!added_pending)
return false;

Expand Down Expand Up @@ -524,12 +527,13 @@ void ImageRecordsManager::MaybeUpdateLargestIgnoredImage(
const RecordId& record_id,
const uint64_t& visual_size,
const gfx::Rect& frame_visual_rect,
const gfx::RectF& root_visual_rect) {
const gfx::RectF& root_visual_rect,
bool is_loaded_after_mouseover) {
if (visual_size && (!largest_ignored_image_ ||
visual_size > largest_ignored_image_->recorded_size)) {
largest_ignored_image_ =
CreateImageRecord(*record_id.first, record_id.second, visual_size,
frame_visual_rect, root_visual_rect);
largest_ignored_image_ = CreateImageRecord(
*record_id.first, record_id.second, visual_size, frame_visual_rect,
root_visual_rect, is_loaded_after_mouseover);
largest_ignored_image_->load_time = base::TimeTicks::Now();
}
}
Expand All @@ -539,7 +543,8 @@ bool ImageRecordsManager::RecordFirstPaintAndReturnIsPending(
const uint64_t& visual_size,
const gfx::Rect& frame_visual_rect,
const gfx::RectF& root_visual_rect,
double bpp) {
double bpp,
bool is_loaded_after_mouseover) {
// Don't process the image yet if it is invisible, as it may later become
// visible, and potentially eligible to be an LCP candidate.
if (visual_size == 0u) {
Expand All @@ -557,9 +562,9 @@ bool ImageRecordsManager::RecordFirstPaintAndReturnIsPending(
return false;
}

std::unique_ptr<ImageRecord> record =
CreateImageRecord(*record_id.first, record_id.second, visual_size,
frame_visual_rect, root_visual_rect);
std::unique_ptr<ImageRecord> record = CreateImageRecord(
*record_id.first, record_id.second, visual_size, frame_visual_rect,
root_visual_rect, is_loaded_after_mouseover);
size_ordered_set_.insert(record->AsWeakPtr());
pending_images_.insert(record_id, std::move(record));
return true;
Expand All @@ -570,12 +575,14 @@ std::unique_ptr<ImageRecord> ImageRecordsManager::CreateImageRecord(
const MediaTiming* media_timing,
const uint64_t& visual_size,
const gfx::Rect& frame_visual_rect,
const gfx::RectF& root_visual_rect) {
const gfx::RectF& root_visual_rect,
bool is_loaded_after_mouseover) {
DCHECK_GT(visual_size, 0u);
Node* node = object.GetNode();
DOMNodeId node_id = DOMNodeIds::IdForNode(node);
std::unique_ptr<ImageRecord> record = std::make_unique<ImageRecord>(
node_id, media_timing, visual_size, frame_visual_rect, root_visual_rect);
node_id, media_timing, visual_size, frame_visual_rect, root_visual_rect,
is_loaded_after_mouseover);
return record;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,12 @@ class ImageRecord : public base::SupportsWeakPtr<ImageRecord> {
const MediaTiming* new_media_timing,
uint64_t new_recorded_size,
const gfx::Rect& frame_visual_rect,
const gfx::RectF& root_visual_rect)
const gfx::RectF& root_visual_rect,
bool is_loaded_after_mouseover_input)
: node_id(new_node_id),
media_timing(new_media_timing),
recorded_size(new_recorded_size) {
recorded_size(new_recorded_size),
is_loaded_after_mouseover(is_loaded_after_mouseover_input) {
static unsigned next_insertion_index_ = 1;
insertion_index = next_insertion_index_++;
if (PaintTimingVisualizer::IsTracingEnabled()) {
Expand Down Expand Up @@ -83,6 +85,8 @@ class ImageRecord : public base::SupportsWeakPtr<ImageRecord> {
// Images that come from origin-dirty styles should have some limitations on
// what they report.
bool origin_clean = true;

bool is_loaded_after_mouseover = false;
};

typedef std::pair<const LayoutObject*, const MediaTiming*> RecordId;
Expand Down Expand Up @@ -127,7 +131,8 @@ class CORE_EXPORT ImageRecordsManager {
const uint64_t& visual_size,
const gfx::Rect& frame_visual_rect,
const gfx::RectF& root_visual_rect,
double bpp);
double bpp,
bool is_loaded_after_mouseover);
bool IsRecordedImage(const RecordId& record_id) const {
return recorded_images_.Contains(record_id);
}
Expand Down Expand Up @@ -159,7 +164,8 @@ class CORE_EXPORT ImageRecordsManager {
void MaybeUpdateLargestIgnoredImage(const RecordId&,
const uint64_t& visual_size,
const gfx::Rect& frame_visual_rect,
const gfx::RectF& root_visual_rect);
const gfx::RectF& root_visual_rect,
bool is_loaded_after_mouseover);
void ReportLargestIgnoredImage(unsigned current_frame_index);

void AssignPaintTimeToRegisteredQueuedRecords(
Expand All @@ -177,7 +183,8 @@ class CORE_EXPORT ImageRecordsManager {
const MediaTiming* media_timing,
const uint64_t& visual_size,
const gfx::Rect& frame_visual_rect,
const gfx::RectF& root_visual_rect);
const gfx::RectF& root_visual_rect,
bool is_loaded_after_mouseover);
inline void QueueToMeasurePaintTime(const RecordId& record_id,
base::WeakPtr<ImageRecord>& record,
unsigned current_frame_index) {
Expand Down Expand Up @@ -264,7 +271,8 @@ class CORE_EXPORT ImagePaintTimingDetector final
const MediaTiming&,
const PropertyTreeStateOrAlias& current_paint_properties,
const StyleFetchedImage*,
const gfx::Rect& image_border);
const gfx::Rect& image_border,
const bool is_loaded_after_mouseover);
void NotifyImageFinished(const LayoutObject&, const MediaTiming*);
void OnPaintFinished();
void NotifyImageRemoved(const LayoutObject&, const MediaTiming*);
Expand Down
8 changes: 7 additions & 1 deletion third_party/blink/renderer/core/paint/timing/paint_timing.cc
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,12 @@ PaintTiming& PaintTiming::From(Document& document) {
return *timing;
}

// static
const PaintTiming* PaintTiming::From(const Document& document) {
PaintTiming* timing = Supplement<Document>::From<PaintTiming>(document);
return timing;
}

void PaintTiming::MarkFirstPaint() {
// Test that |first_paint_| is non-zero here, as well as in setFirstPaint, so
// we avoid invoking monotonicallyIncreasingTime() on every call to
Expand Down Expand Up @@ -458,7 +464,7 @@ void PaintTiming::OnRestoredFromBackForwardCache() {
index));
}

bool PaintTiming::IsLCPMouseoverDispatchedRecently() {
bool PaintTiming::IsLCPMouseoverDispatchedRecently() const {
static constexpr base::TimeDelta kRecencyDelta = base::Milliseconds(500);
return (
!lcp_mouse_over_dispatch_time_.is_null() &&
Expand Down
3 changes: 2 additions & 1 deletion third_party/blink/renderer/core/paint/timing/paint_timing.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ class CORE_EXPORT PaintTiming final : public GarbageCollected<PaintTiming>,
virtual ~PaintTiming() = default;

static PaintTiming& From(Document&);
static const PaintTiming* From(const Document&);

// Mark*() methods record the time for the given paint event and queue a
// presentation promise to record the |first_*_presentation_| timestamp. These
Expand Down Expand Up @@ -177,7 +178,7 @@ class CORE_EXPORT PaintTiming final : public GarbageCollected<PaintTiming>,

// Indicates whether a mouseover event was recently dispatched over an
// HTMLImageElement LCP element.
bool IsLCPMouseoverDispatchedRecently();
bool IsLCPMouseoverDispatchedRecently() const;
void SetLCPMouseoverDispatched();

void Trace(Visitor*) const override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ bool PaintTimingDetector::NotifyBackgroundImagePaint(

return image_paint_timing_detector.RecordImage(
*object, image.Size(), *cached_image, current_paint_chunk_properties,
&style_image, image_border);
&style_image, image_border, style_image.IsLoadedAfterMouseover());
}

// static
Expand All @@ -181,9 +181,14 @@ bool PaintTimingDetector::NotifyImagePaint(
if (!image_paint_timing_detector.IsRecordingLargestImagePaint())
return false;

Node* image_node = object.GetNode();
HTMLImageElement* element = DynamicTo<HTMLImageElement>(image_node);
bool is_loaded_after_mouseover =
element && element->IsChangedShortlyAfterMouseover();

return image_paint_timing_detector.RecordImage(
object, intrinsic_size, media_timing, current_paint_chunk_properties,
nullptr, image_border);
nullptr, image_border, is_loaded_after_mouseover);
}

void PaintTimingDetector::NotifyImageFinished(const LayoutObject& object,
Expand Down Expand Up @@ -295,10 +300,7 @@ bool PaintTimingDetector::NotifyIfChangedLargestImagePaint(
lcp_details_.largest_contentful_paint_type_ =
blink::LargestContentfulPaintType::kNone;
if (image_record) {
Node* image_node = DOMNodeIds::NodeForId(image_record->node_id);
HTMLImageElement* element = DynamicTo<HTMLImageElement>(image_node);
if (element && !image_node->IsInShadowTree() &&
element->IsChangedShortlyAfterMouseover()) {
if (image_record->is_loaded_after_mouseover) {
lcp_details_.largest_contentful_paint_type_ |=
blink::LargestContentfulPaintType::kAfterMouseover;
}
Expand Down
5 changes: 5 additions & 0 deletions third_party/blink/renderer/core/style/style_fetched_image.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#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/timing/image_element_timing.h"
#include "third_party/blink/renderer/core/paint/timing/paint_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"
Expand All @@ -52,6 +53,10 @@ StyleFetchedImage::StyleFetchedImage(ImageResourceContent* image,
is_image_resource_ = true;
is_lazyload_possibly_deferred_ = is_lazyload_possibly_deferred;

const PaintTiming* paint_timing = PaintTiming::From(document);
is_loaded_after_mouseover_ =
paint_timing && paint_timing->IsLCPMouseoverDispatchedRecently();

image_ = image;
image_->AddObserver(this);
// ResourceFetcher is not determined from StyleFetchedImage and it is
Expand Down
8 changes: 8 additions & 0 deletions third_party/blink/renderer/core/style/style_fetched_image.h
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ class CORE_EXPORT StyleFetchedImage final : public StyleImage,

bool IsOriginClean() const { return origin_clean_; }

bool IsLoadedAfterMouseover() const { return is_loaded_after_mouseover_; }

private:
bool IsEqual(const StyleImage&) const override;
void Prefinalize();
Expand All @@ -100,6 +102,12 @@ class CORE_EXPORT StyleFetchedImage final : public StyleImage,

// Whether this was created by an ad-related CSSParserContext.
const bool is_ad_related_;

// This indicates that the style image was loaded after a recent mouseover
// event. This is used for LCP heuristics to ignore zoom widgets as LCP
// candidates. StyleFetchedImage is the best place to save this state, as it
// relates to the reason the image was fetched.
bool is_loaded_after_mouseover_ = false;
};

template <>
Expand Down

0 comments on commit cdf23f0

Please sign in to comment.