Don't error out if we successfully loaded an image

• Component: Images
• Status: NEW ---

[Ahmad Saleem]

Hi Team,

While going through Blink's commit, I came across one potential merge:

Blink Commit: https://chromium.googlesource.com/chromium/blink/+/c1fdb4cd2099a27ae10dba405fe2869121c50545

"The spec shows that this behaviour is OK for the object element but not for the
img element, where its silence implies we shoud load and display the image if we
got one - even if the response was a 404."

WebKit Source: https://searchfox.org/wubkat/source/Source/WebCore/html/HTMLImageLoader.cpp#83

Chromium / Blink still has modified test case.

Just wanted to raise to get input, so we can fix it (if needed).

Thanks!

[Ahmad Saleem]

For <object> - Due to the algorithm above, the contents of object elements act as fallback content, used only when referenced resources can't be shown (e.g. because it returned a 404 error.

https://html.spec.whatwg.org/#the-object-element

Can't find anything about 'img' element. CCing - @Anne (to check for his expertise on web-spec).

[Anne van Kesteren]

That code looks wrong indeed. We should not fail to load an image due to an HTTP status code.

I thought WPT had coverage for this as well, but maybe not?

[Karl Dubost]

Yup and that should be an easy fix.

[Ahmad Saleem]

PR - https://github.com/WebKit/WebKit/pull/22100

[Ahmad Saleem]

We do have this WPT - http://wpt.live/html/semantics/embedded-content/the-img-element/404-response-with-actual-image-data.html

and we are passing it. :-?

[Radar WebKit Bug Importer]

<rdar://problem/120188957>

[Ahmad Saleem]

Reading code bit better - I agree with Chris's comment on PR:

"I don't think this code change actually causes any web facing behavior change? The chromium code change made more sense because loadError was used for various element types.

Here the only thing you changed is whether we report the extra memory to the GC or not it seems."

---

Plus we are already passing WPT test, so we don't need to do this.

Only benefit - we can take from this commit would be to update 'test' to 'ref-test'. Although I am in favor of deleting it since we have WPT test coverage.

@Anne & @Karl - any input, I am closing my PR but I can use this bug to delete local test. :-)

[Anne van Kesteren]

Removing local tests that are (near) identical to imported WPT tests seems fine.

I don't understand why our code still has a 400 check in it though. Even with your change that was kept, but I believe the HTML Standard has no such check for image fetches.