[webkit-reviews] review granted: [Bug 243601] Images with loading="lazy" have uncontrollable gray border while loading : [Attachment 461989] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Aug 29 14:07:20 PDT 2022

Darin Adler <darin at apple.com> has granted Rob Buis <rbuis at igalia.com>'s request
for review:
Bug 243601: Images with loading="lazy" have uncontrollable gray border while

Attachment 461989: Patch


--- Comment #13 from Darin Adler <darin at apple.com> ---
Comment on attachment 461989
  --> https://bugs.webkit.org/attachment.cgi?id=461989

View in context: https://bugs.webkit.org/attachment.cgi?id=461989&action=review

> +Do not paint border while an image is in deferred state.
> +The test image-loading-lazy-slow.html covers this. However, the current
> +test runner logic always stops page loads before pixel dumping, causing the
> +image to be painted as a broken image instead of the empty image at the
> +time of calling takeScreenshot. To fix this, postpone the stopping of page
> +loads (except when the test tests printing) and instead always stop page
> +loads when reseting after the test.

The code doesn’t actually match this comment. The patch postpones the stopping
of the page loads only when the test is dumping a pixel result, so the special
case is dumping pixels, not printing. I am guessing this comment documents an
earlier plan.

> Source/WebCore/rendering/RenderImage.cpp:486
> +	   if (!context.contenfulPaintDetected() && cachedImage() &&
cachedImage()->canRender(this, deviceScaleFactor) && !contentSize.isEmpty())

Just noticed a typo in the code (the unmodified part): contenfulPaintDetected.
Someone could fix that later.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:553
> +    if (!m_pixelResultIsPending)
> +	   page()->stopLoading();

I think this is not obviously correct, so it needs a comment. Why ever stop
loading? Why make an exception for pixel results?

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:408
> +    stopLoading();

Seems unclear exactly why we *need* this here, although obviously we need it
for the pixel result case. But it seems we could instead stop loading at the
same time we set m_pixelResultIsPending to false. The trick is to keep this
code as logical as possible so we don’t get more confused in the future.

Might even need a comment here that explains why this is usually a no-op.

> LayoutTests/TestExpectations:-803
t/image-loading-lazy-slow.html [ ImageOnlyFailure ]

Given we are changing WebKitTestRunner and not DumpRenderTree, it seems likely
this is still broken under Legacy WebKit. Do we have the correct test
expectation for that?

More information about the webkit-reviews mailing list