Notify image observers for stale cached images during revalidation [chromium/src : main]

0 views

Skip to first unread message

jj (Gerrit)

unread,

May 19, 2026, 8:11:09 PM (7 hours ago) May 19

to jj, Nate Chapin, Fredrik Söderquist, Morten Stenshorne, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, blink-revie...@chromium.org, loading...@chromium.org, gavinp...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org

Attention needed from Fredrik Söderquist and Morten Stenshorne

jj added 3 comments

Patchset-level comments

File-level comment, Patchset 4:

Morten Stenshorne . resolved

I'm not the best reviewer for this. Adding f...@opera.com
Is there no way we can create a WPT test instead of a unit test?

Fredrik Söderquist

I guess the problem with a WPT may be that we want to load and then reload the same image resource, and the second time we'd want to perhaps incur a delay to be able to verify the result? If a WPT can't be done, then I'd prefer a unit test that better reflects the actual flow of events (i.e is less low-level).

I was able to get something working, that reliably fails before and passes after the fix (visually confirmed too).

File third_party/blink/renderer/core/layout/layout_image.cc

Line 418, Patchset 4:

  PhysicalNaturalSizingInfo sizing_info = GetNaturalDimensions();
  if (sizing_info.size.IsEmpty() && sizing_info.aspect_ratio.IsEmpty() &&
      image_resource_->HasImage() && !image_resource_->ErrorOccurred()) {
    // During cache revalidation, a stale image can be paintable before layout
    // has been notified that its natural dimensions are available.
    sizing_info = PhysicalNaturalSizingInfo::FromSizingInfo(
        image_resource_->GetNaturalDimensions(StyleRef().EffectiveZoom()));
  }

Fredrik Söderquist . resolved

This seems a bit fragile to me, and we're also reintroducing a mixed "push-pull" model for getting the natural dimensions. I also don't see why we'd want potentially different "views" for layout vs. paint - that seems like it could cause inconsistencies. Isn't the problem here that when the `LayoutImage` has the image resource (content) attached to it, it doesn't get an image changed notification and thus will not update its natural dimensions? It seems we should fix that instead - maybe by not muting those notifications, or by making sure we've updated the natural dimensions regardless (or some other similar option).

Agreed, thanks. I looked into it a bit more and the solution might be much simpler.

File third_party/blink/renderer/core/layout/layout_image_test.cc

Line 72, Patchset 4: EXPECT_FALSE(layout_image->ImageResource()->IsSizeAvailable());

Fredrik Söderquist . resolved

Is this really true in the real case?

No. (Well, at least not after this change.) Either way, I replaced this test with the WPT.

Open in Gerrit

Related details

Attention is currently required from:

Fredrik Söderquist
Morten Stenshorne

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages