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

[bugzilla-daemon at webkit.org]

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
loading
https://bugs.webkit.org/show_bug.cgi?id=243601

Attachment 461989: Patch

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




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

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

> COMMIT_MESSAGE:12
> +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
>
-imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-elemen
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?