ImageReplacement: Wait for image load and complete replacement [chromium/src : main]
0 views
Skip to first unread message
Adithya Srinivasan (Gerrit)
Apr 1, 2026, 1:29:12 PM (17 hours ago) Apr 1
to Jeremy Roman, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Jeremy Roman
New activity on the change![]( "Open in Gerrit")
Related details
Attention is currently required from:
- Jeremy Roman
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jeremy Roman (Gerrit)
Apr 1, 2026, 2:25:13 PM (16 hours ago) Apr 1
to Adithya Srinivasan, Jeremy Roman, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Adithya Srinivasan
Jeremy Roman voted and added 6 comments![]( "Open in Gerrit")
Votes added by Jeremy Roman
| Code-Review | +1 |
6 comments
File third_party/blink/renderer/core/html/html_image_element.cc
Line 1094, Patchset 4 (Latest):
return;Jeremy Roman . unresolved
not immediately clear to me why ResetLayoutDisposition isn't called in this case, is it because the layout disposition was already correct? can you add comments to clarify?
File third_party/blink/renderer/core/image_replacement/image_replacement.h
Line 52, Patchset 4 (Latest):
// Returns true if the replacement is pending (StartReplacement was
// called, but the replacement was delayed because the image was still
// loading).
bool IsWaitingForLoad() const;
// Resumes and completes a pending replacement. Should only be called when
// `IsWaitingForLoad()` is true. Returns true if replacement was completed,
// false otherwise (can happen if there was a loading error for example).
bool ResumeReplacement();Jeremy Roman . unresolved
If these are always called together (maybe you have plans for it to be otherwise?) maybe it should be one function, like ResumeReplacementAfterImageLoad, which returns a boolean with appropriate meaning?
Line 55, Patchset 4 (Latest):
bool IsWaitingForLoad() const;Jeremy Roman . unresolved
nit: IsWaitingForImageLoad? Just because it's otherwise unclear if we're waiting for that or the load of the replacement content.
File third_party/blink/renderer/core/image_replacement/image_replacement.cc
Line 199, Patchset 4 (Latest):
return image_element_ && image_element_->HasImageReplacement();Jeremy Roman . unresolved
Can `image_element_` actually be null here given it's `CHECK`ed earlier in this function? Do we expect `StartReplacement` to null it out?
File third_party/blink/renderer/core/image_replacement/image_replacement_test.cc
Line 387, Patchset 4 (Latest):
const unsigned char gif_bytes[] = {
0x47, 0x49, 0x46, 0x38, 0x39, 0x61, 0x01, 0x00, 0x01, 0x00, 0x80,
0x00, 0x00, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x21, 0xf9, 0x04,
0x01, 0x00, 0x00, 0x00, 0x00, 0x2c, 0x00, 0x00, 0x00, 0x00, 0x01,
0x00, 0x01, 0x00, 0x00, 0x02, 0x02, 0x44, 0x01, 0x00, 0x3b};Jeremy Roman . unresolved
nit: probably make it static const and call it `kGifBytes`? The compiler will probably optimize (and it's a test) but still, it's easy to do here.
Line 606, Patchset 4 (Latest):
test::RunPendingTasks();Jeremy Roman . unresolved
Is RunPendingTasks our best option here or is it possible without too much trouble to explicitly observe the error occurring? (RunPendingTasks can lead to flaky tests if details change subtly.)
Related details
Attention is currently required from:
- Adithya Srinivasan
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adithya Srinivasan (Gerrit)
Apr 1, 2026, 5:29:33 PM (13 hours ago) Apr 1
to Jeremy Roman, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Adithya Srinivasan voted and added 6 comments![]( "Open in Gerrit")
Votes added by Adithya Srinivasan
| Commit-Queue | +2 |
6 comments
File third_party/blink/renderer/core/html/html_image_element.cc
not immediately clear to me why ResetLayoutDisposition isn't called in this case, is it because the layout disposition was already correct? can you add comments to clarify?
Adithya Srinivasan
Yeah exactly. Added a comment.
File third_party/blink/renderer/core/image_replacement/image_replacement.h
Line 52, Patchset 4:
// Returns true if the replacement is pending (StartReplacement was
// called, but the replacement was delayed because the image was still
// loading).
bool IsWaitingForLoad() const;
// Resumes and completes a pending replacement. Should only be called when
// `IsWaitingForLoad()` is true. Returns true if replacement was completed,
// false otherwise (can happen if there was a loading error for example).
bool ResumeReplacement();Jeremy Roman . resolved
If these are always called together (maybe you have plans for it to be otherwise?) maybe it should be one function, like ResumeReplacementAfterImageLoad, which returns a boolean with appropriate meaning?
Adithya Srinivasan
It's used separately in tests, but honestly doesn't really need to be checked. I'll combine them like you've suggested.
nit: IsWaitingForImageLoad? Just because it's otherwise unclear if we're waiting for that or the load of the replacement content.
Adithya Srinivasan
Obsolete now.
File third_party/blink/renderer/core/image_replacement/image_replacement.cc
Line 199, Patchset 4:
return image_element_ && image_element_->HasImageReplacement();Jeremy Roman . resolved
Can `image_element_` actually be null here given it's `CHECK`ed earlier in this function? Do we expect `StartReplacement` to null it out?
Adithya Srinivasan
Do we expect StartReplacement to null it out?
If the image load errored, we will call ResetImageReplacement in StartReplacement and image_element_ will be nulled out. Added a comment.
File third_party/blink/renderer/core/image_replacement/image_replacement_test.cc
Line 387, Patchset 4:
const unsigned char gif_bytes[] = {
0x47, 0x49, 0x46, 0x38, 0x39, 0x61, 0x01, 0x00, 0x01, 0x00, 0x80,
0x00, 0x00, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x21, 0xf9, 0x04,
0x01, 0x00, 0x00, 0x00, 0x00, 0x2c, 0x00, 0x00, 0x00, 0x00, 0x01,
0x00, 0x01, 0x00, 0x00, 0x02, 0x02, 0x44, 0x01, 0x00, 0x3b};Jeremy Roman . resolved
nit: probably make it static const and call it `kGifBytes`? The compiler will probably optimize (and it's a test) but still, it's easy to do here.
Adithya Srinivasan
Done
Is RunPendingTasks our best option here or is it possible without too much trouble to explicitly observe the error occurring? (RunPendingTasks can lead to flaky tests if details change subtly.)
Adithya Srinivasan
Switched to listening to the error event.
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Apr 1, 2026, 7:38:03 PM (11 hours ago) Apr 1
to Adithya Srinivasan, Jeremy Roman, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/html/html_image_element.cc
Insertions: 4, Deletions: 1.
@@ -1090,7 +1090,10 @@
DocumentImageReplacements::FromIfExists(GetDocument())) {
if (ImageReplacement* replacement =
replacements->GetImageReplacement(this)) {
- if (replacement->IsWaitingForLoad() && replacement->ResumeReplacement()) {
+ if (replacement->ResumeReplacementAfterImageLoad()) {
+ // Replacement is now complete and the layout disposition is now
+ // kImageReplacement. We don't need to reset it, so we skip the call
+ // to ResetLayoutDisposition() below.
return;
}
}
```
```
The name of the file: third_party/blink/renderer/core/image_replacement/image_replacement.cc
Insertions: 6, Deletions: 6.
@@ -186,16 +186,16 @@
}
}
-bool ImageReplacement::IsWaitingForLoad() const {
- return pending_host_remote_.is_valid();
-}
-
-bool ImageReplacement::ResumeReplacement() {
+bool ImageReplacement::ResumeReplacementAfterImageLoad() {
+ if (!pending_host_remote_.is_valid()) {
+ return false;
+ }
CHECK(image_element_ && image_element_->complete());
- CHECK(IsWaitingForLoad());
mojo::PendingRemote<mojom::blink::ImageReplacementHost> remote =
std::move(pending_host_remote_);
StartReplacement(std::move(remote));
+ // Note: `image_element_` can be nullptr here if the image load failed with
+ // an error (StartReplacement will reset the image replacement in that case).
return image_element_ && image_element_->HasImageReplacement();
}
```
```
The name of the file: third_party/blink/renderer/core/image_replacement/image_replacement.h
Insertions: 3, Deletions: 8.
@@ -49,14 +49,9 @@
const AtomicString& OriginalImageSourceURL() const {
return original_image_source_url_;
}
- // Returns true if the replacement is pending (StartReplacement was
- // called, but the replacement was delayed because the image was still
- // loading).
- bool IsWaitingForLoad() const;
- // Resumes and completes a pending replacement. Should only be called when
- // `IsWaitingForLoad()` is true. Returns true if replacement was completed,
- // false otherwise (can happen if there was a loading error for example).
- bool ResumeReplacement();
+ // Resumes and completes a pending replacement if it was waiting for an image
+ // load. Returns true if replacement was completed, false otherwise.
+ bool ResumeReplacementAfterImageLoad();
void Trace(Visitor*) const;
```
```
The name of the file: third_party/blink/renderer/core/image_replacement/image_replacement_test.cc
Insertions: 24, Deletions: 5.
@@ -7,6 +7,7 @@
#include <memory>
#include "base/test/scoped_feature_list.h"
+#include "base/test/test_future.h"
#include "cc/paint/paint_op.h"
#include "cc/paint/paint_op_buffer_iterator.h"
#include "mojo/public/cpp/bindings/pending_receiver.h"
@@ -61,6 +62,21 @@
bool clicked_ = false;
};
+class ErrorEventListener : public NativeEventListener {
+ public:
+ explicit ErrorEventListener(base::OnceClosure quit_closure)
+ : quit_closure_(std::move(quit_closure)) {}
+
+ void Invoke(ExecutionContext*, Event* event) override {
+ if (quit_closure_) {
+ std::move(quit_closure_).Run();
+ }
+ }
+
+ private:
+ base::OnceClosure quit_closure_;
+};
+
} // namespace
class MockImageReplacementHost : public mojom::blink::ImageReplacementHost {
@@ -381,16 +397,15 @@
ImageReplacement* replacement =
DocumentImageReplacements::From(GetDocument()).GetImageReplacement(img);
ASSERT_TRUE(replacement);
- EXPECT_TRUE(replacement->IsWaitingForLoad());
// 1x1 transparent GIF
- const unsigned char gif_bytes[] = {
+ static const unsigned char kGifBytes[] = {
0x47, 0x49, 0x46, 0x38, 0x39, 0x61, 0x01, 0x00, 0x01, 0x00, 0x80,
0x00, 0x00, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x21, 0xf9, 0x04,
0x01, 0x00, 0x00, 0x00, 0x00, 0x2c, 0x00, 0x00, 0x00, 0x00, 0x01,
0x00, 0x01, 0x00, 0x00, 0x02, 0x02, 0x44, 0x01, 0x00, 0x3b};
Vector<char> gif_data;
- gif_data.append_range(gif_bytes);
+ gif_data.append_range(kGifBytes);
// Finish image load.
image_resource.Complete(gif_data);
test::RunPendingTasks();
@@ -599,11 +614,15 @@
ImageReplacement* replacement =
DocumentImageReplacements::From(GetDocument()).GetImageReplacement(img);
ASSERT_TRUE(replacement);
- EXPECT_TRUE(replacement->IsWaitingForLoad());
+
+ base::test::TestFuture<void> future;
+ auto* error_listener =
+ MakeGarbageCollected<ErrorEventListener>(future.GetCallback());
+ img->addEventListener(AtomicString("error"), error_listener);
// Finish the load (with invalid data to cause an error).
image_resource.Complete("invalid data");
- test::RunPendingTasks();
+ EXPECT_TRUE(future.Wait());
EXPECT_TRUE(img->CachedImage()->ErrorOccurred());
// Replacement should not have started since the image load failed.
```
Change information
Commit message:
ImageReplacement: Wait for image load and complete replacement
Instead of failing StartReplacement if the image has not finished
loading, we now wait for the load to finish and complete replacement.
Bug: 489470758
Change-Id: I27920420dc69fea245148e2d4bb963406b695b2b
Commit-Queue: Adithya Srinivasan <adit...@chromium.org>
Reviewed-by: Jeremy Roman <jbr...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1608858}
Files:
- M third_party/blink/renderer/core/html/html_image_element.cc
- M third_party/blink/renderer/core/html/html_image_element.h
- M third_party/blink/renderer/core/html/html_image_loader.cc
- M third_party/blink/renderer/core/image_replacement/image_replacement.cc
- M third_party/blink/renderer/core/image_replacement/image_replacement.h
- M third_party/blink/renderer/core/image_replacement/image_replacement_test.cc
Change size: M
Delta: 6 files changed, 127 insertions(+), 21 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Jeremy Roman
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages