Skip to content

Commit

Permalink
[M103] [Paint Preview] Ignore Paint Timing while capturing
Browse files Browse the repository at this point in the history
WebLocalFrameImpl::CapturePaintPreview can cause LCP to select elements
that are off-screen. Avoid this by skipping paint timings while it is
running.

(cherry picked from commit 3e66b23)

Bug: 1323073
Change-Id: I549fd7ae853e656534f2e768454d9bf5f01dedf1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3661154
Reviewed-by: Calder Kitagawa <ckitagawa@chromium.org>
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1006953}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3671535
Reviewed-by: Krishna Govind <govind@chromium.org>
Owners-Override: Krishna Govind <govind@chromium.org>
Commit-Queue: Krishna Govind <govind@chromium.org>
Cr-Commit-Position: refs/branch-heads/5060@{#422}
Cr-Branched-From: b83393d-refs/heads/main@{#1002911}
  • Loading branch information
Daniel Rubery authored and Chromium LUCI CQ committed May 31, 2022
1 parent c4fbea8 commit 50d9535
Show file tree
Hide file tree
Showing 3 changed files with 140 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
<script src="resources/testharness.js"></script>
<script>
// Tell testharness.js to not wait for 'real' tests; we only want
// testharness.js for its assertion helpers.
setup({'output': false});
</script>

<script>
// 'AsyncBuffer' serves as a helper to buffer LCP reports asynchronously.
class AsyncBuffer {
constructor() {
// 'pending' is an array that will buffer entries reported through the
// PerformanceObserver and can be collected with 'pop'.
this.pending = [];

// 'resolve_fn' is a reference to the 'resolve' function of a
// Promise that blocks for new entries to arrive via 'push()'. Calling
// the function resolves the promise and unblocks calls to 'pop()'.
this.resolve_fn = null;
}

// Concatenates the given 'entries' list to this AsyncBuffer.
push(entries) {
if (entries.length == 0) {
throw new Error("Must not push an empty list of entries!");
}
this.pending = this.pending.concat(entries);

// If there are calls to 'pop' that are blocked waiting for items, signal
// that they can continue.
if (this.resolve_fn != null) {
this.resolve_fn();
this.resolve_fn = null;
}
}

// Takes the current pending entries from this AsyncBuffer. If there are no
// entries queued already, this will block until some show up.
async pop() {
if (this.pending.length == 0) {
// Need to instantiate a promise to block on. The next call to 'push'
// will resolve the promise once it has queued the entries.
await new Promise(resolve => {
this.resolve_fn = resolve;
});
}
assert_true(this.pending.length > 0);

const result = this.pending;
this.pending = [];
return result;
}
}

const buffer = new AsyncBuffer();
const po = new PerformanceObserver(entryList => {
buffer.push(entryList.getEntries());
});
po.observe({type: 'largest-contentful-paint', buffered: true});

const block_for_next_lcp = async () => {
const seen_events = await buffer.pop();
assert_equals(seen_events.length, 1);
return seen_events[0].size;
};

const trigger_repaint_and_block_for_next_lcp = async () => {
let new_div = document.createElement("div");
document.getElementById('firstDiv').appendChild(new_div);
new_div.innerText = "Medium text";
return await block_for_next_lcp();
};
</script>

<div id="firstDiv" style='width: 600px; height: 10000px'>Short text</div>
<div style='width: 600px; height: 500px'>Loooooooooooong offscreen text that shouldn't count for LCP computation</div>
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "build/build_config.h"
#include "chrome/test/base/ui_test_utils.h"
#include "components/page_load_metrics/browser/page_load_metrics_test_waiter.h"
#include "components/paint_preview/buildflags/buildflags.h"
#include "content/public/browser/render_widget_host_view.h"
#include "content/public/test/browser_test.h"
#include "services/metrics/public/cpp/ukm_builders.h"
Expand All @@ -23,6 +24,12 @@
#endif
#include "ui/events/test/event_generator.h"

#if BUILDFLAG(ENABLE_PAINT_PREVIEW)
// `gn check` doesn't recognize that this is included conditionally, with the
// same condition as the dependencies.
#include "components/paint_preview/browser/paint_preview_client.h" // nogncheck
#endif

using trace_analyzer::Query;
using trace_analyzer::TraceAnalyzer;
using trace_analyzer::TraceEvent;
Expand Down Expand Up @@ -158,6 +165,57 @@ IN_PROC_BROWSER_TEST_F(MetricIntegrationTest,
EXPECT_EQ(EvalJs(sub, "test_step_2()").value.GetString(), "green-16x16.png");
}

#if BUILDFLAG(ENABLE_PAINT_PREVIEW)
IN_PROC_BROWSER_TEST_F(MetricIntegrationTest,
LargestContentfulPaintPaintPreview) {
Start();
StartTracing({"loading"});
Load("/largest_contentful_paint_paint_preview.html");

content::EvalJsResult lcp_before_paint_preview =
EvalJs(web_contents(), "block_for_next_lcp()");
EXPECT_EQ("", lcp_before_paint_preview.error);

paint_preview::PaintPreviewClient::CreateForWebContents(
web_contents()); // Is a singleton.
auto* client =
paint_preview::PaintPreviewClient::FromWebContents(web_contents());

paint_preview::PaintPreviewClient::PaintPreviewParams params(
paint_preview::RecordingPersistence::kMemoryBuffer);
params.inner.clip_rect = gfx::Rect(0, 0, 1, 1);
params.inner.is_main_frame = true;
params.inner.capture_links = false;
params.inner.max_capture_size = 50 * 1024 * 1024;
params.inner.max_decoded_image_size_bytes = 50 * 1024 * 1024;
params.inner.skip_accelerated_content = true;

base::RunLoop run_loop;
client->CapturePaintPreview(
params, web_contents()->GetMainFrame(),
base::BindOnce(
[](base::OnceClosure callback, base::UnguessableToken,
paint_preview::mojom::PaintPreviewStatus,
std::unique_ptr<paint_preview::CaptureResult>) {
std::move(callback).Run();
},
run_loop.QuitClosure()));
run_loop.Run();

content::EvalJsResult lcp_after_paint_preview =
EvalJs(web_contents(), "trigger_repaint_and_block_for_next_lcp()");
EXPECT_EQ("", lcp_after_paint_preview.error);

// When PaintPreview creates new LCP candidates, we compare the short text and
// the long text here, which will fail. But in order to consistently get the
// new LCP candidate in that case, we always add a medium text in
// `trigger_repaint_and_block_for_next_lcp`. So use a soft comparison here
// that would permit the medium text, but not the long text.
EXPECT_LT(lcp_after_paint_preview.value.GetDouble(),
2 * lcp_before_paint_preview.value.GetDouble());
}
#endif

class PageViewportInLCPTest : public MetricIntegrationTest {
public:
void SetUpCommandLine(base::CommandLine* command_line) override {
Expand Down
6 changes: 6 additions & 0 deletions third_party/blink/renderer/core/frame/web_local_frame_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@
#include "third_party/blink/renderer/platform/graphics/color.h"
#include "third_party/blink/renderer/platform/graphics/graphics_context.h"
#include "third_party/blink/renderer/platform/graphics/paint/drawing_recorder.h"
#include "third_party/blink/renderer/platform/graphics/paint/ignore_paint_timing_scope.h"
#include "third_party/blink/renderer/platform/graphics/paint/paint_record_builder.h"
#include "third_party/blink/renderer/platform/graphics/paint/scoped_paint_chunk_properties.h"
#include "third_party/blink/renderer/platform/graphics/skia/skia_utils.h"
Expand Down Expand Up @@ -1823,6 +1824,11 @@ bool WebLocalFrameImpl::CapturePaintPreview(const gfx::Rect& bounds,
bool skip_accelerated_content) {
bool success = false;
{
// Ignore paint timing while capturing a paint preview as it can change LCP
// see crbug.com/1323073.
IgnorePaintTimingScope scope;
IgnorePaintTimingScope::IncrementIgnoreDepth();

Document::PaintPreviewScope paint_preview(
*GetFrame()->GetDocument(),
skip_accelerated_content
Expand Down

0 comments on commit 50d9535

Please sign in to comment.