Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(6)

Issue 1047563002: Fire load event for an image even if it was the last image we loaded successfully. (Closed)

Created:
5 years, 7 months ago by rhogan
Modified:
4 years, 6 months ago
CC:
blink-reviews, gavinp+loader_chromium.org, Nate Chapin, tyoshino+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Fire load event for an image even if it was the last image we loaded successfully. When we updated image loading to handle alt content using the shadow dom we made most image loads asynchronous, including loads where the src is null. src='' is a common idiom for resetting the content of the image but if a user now does "src=''; src='www.example.com/someimage.gif;" the asynchronous image load doesn't get time to reset the current image stored in memory before the subsequent image loads. We have a strange rule in our image loading code that tells us not to fire a load event if the new image we've loaded is the same as the old one. https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element says that we should fire load events when we load an image, even if its cached. This means our practice of not firing load events when we already have an image loaded is not consistent with the spec or what other browsers do, so always fire a load event if we load an image. The only exception is if the load is in response to a viewport resize event and we haven't chosen a new image to load from srcset. BUG=472193, 7731 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192974

Patch Set 1 #

Patch Set 2 : Updated #

Patch Set 3 : Updated #

Total comments: 6

Patch Set 4 : Updated #

Total comments: 3

Patch Set 5 : Updated #

Patch Set 6 : Updated #

Total comments: 8

Messages

Total messages: 17 (5 generated)
rhogan
5 years, 7 months ago (2015-03-30 20:50:13 UTC) #2
esprehn
https://codereview.chromium.org/1047563002/diff/40001/LayoutTests/fast/images/onload-event-when-reloading-image-after-interrupted-broken-image-load.html File LayoutTests/fast/images/onload-event-when-reloading-image-after-interrupted-broken-image-load.html (right): https://codereview.chromium.org/1047563002/diff/40001/LayoutTests/fast/images/onload-event-when-reloading-image-after-interrupted-broken-image-load.html#newcode14 LayoutTests/fast/images/onload-event-when-reloading-image-after-interrupted-broken-image-load.html:14: document.getElementById('result').innerHTML = "PASS"; textContent = instead of innerHTML https://codereview.chromium.org/1047563002/diff/40001/LayoutTests/fast/images/onload-event-when-reloading-image-after-interrupted-null-src-load.html ...
5 years, 7 months ago (2015-03-31 00:34:54 UTC) #3
rhogan
https://codereview.chromium.org/1047563002/diff/40001/Source/core/loader/ImageLoader.cpp File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/1047563002/diff/40001/Source/core/loader/ImageLoader.cpp#newcode294 Source/core/loader/ImageLoader.cpp:294: if (resizingToViewport && !selectedNewImage) { On 2015/03/31 at 00:34:54, ...
5 years, 6 months ago (2015-03-31 19:03:20 UTC) #4
esprehn
lgtm, but please use js-test.js in the test. https://codereview.chromium.org/1047563002/diff/60001/LayoutTests/fast/images/onload-event-when-reloading-image-after-interrupted-broken-image-load.html File LayoutTests/fast/images/onload-event-when-reloading-image-after-interrupted-broken-image-load.html (right): https://codereview.chromium.org/1047563002/diff/60001/LayoutTests/fast/images/onload-event-when-reloading-image-after-interrupted-broken-image-load.html#newcode13 LayoutTests/fast/images/onload-event-when-reloading-image-after-interrupted-broken-image-load.html:13: document.getElementById('console').innerHTML ...
5 years, 6 months ago (2015-03-31 21:53:21 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1047563002/100001
5 years, 6 months ago (2015-04-01 19:30:52 UTC) #8
commit-bot: I haz the power
Committed patchset #6 (id:100001) as https://src.chromium.org/viewvc/blink?view=rev&revision=192974
5 years, 6 months ago (2015-04-01 23:20:23 UTC) #9
Yoav Weiss
https://codereview.chromium.org/1047563002/diff/100001/LayoutTests/fast/images/onload-event-when-reloading-image-after-interrupted-broken-image-load.html File LayoutTests/fast/images/onload-event-when-reloading-image-after-interrupted-broken-image-load.html (right): https://codereview.chromium.org/1047563002/diff/100001/LayoutTests/fast/images/onload-event-when-reloading-image-after-interrupted-broken-image-load.html#newcode31 LayoutTests/fast/images/onload-event-when-reloading-image-after-interrupted-broken-image-load.html:31: img.src= resetImage; Is the first src setting relevant here? ...
5 years, 6 months ago (2015-04-02 07:52:10 UTC) #11
rhogan
https://codereview.chromium.org/1047563002/diff/100001/Source/core/loader/ImageLoader.cpp File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/1047563002/diff/100001/Source/core/loader/ImageLoader.cpp#newcode292 Source/core/loader/ImageLoader.cpp:292: if (updateBehavior == UpdateSizeChanged && m_element->layoutObject() && m_element->layoutObject()->isImage() && ...
5 years, 6 months ago (2015-04-02 18:15:16 UTC) #12
Yoav Weiss
On 2015/04/02 18:15:16, rhogan wrote: > https://codereview.chromium.org/1047563002/diff/100001/Source/core/loader/ImageLoader.cpp > File Source/core/loader/ImageLoader.cpp (right): > > https://codereview.chromium.org/1047563002/diff/100001/Source/core/loader/ImageLoader.cpp#newcode292 > ...
5 years, 6 months ago (2015-04-03 06:03:15 UTC) #13
rhogan
On 2015/04/03 at 06:03:15, yoav wrote: > What I'm saying is that UpdateSizeChanged wasn't intended ...
5 years, 6 months ago (2015-04-03 17:28:19 UTC) #14
esprehn
https://codereview.chromium.org/1047563002/diff/100001/LayoutTests/fast/images/onload-event-when-reloading-image-after-successful-image-load.html File LayoutTests/fast/images/onload-event-when-reloading-image-after-successful-image-load.html (right): https://codereview.chromium.org/1047563002/diff/100001/LayoutTests/fast/images/onload-event-when-reloading-image-after-successful-image-load.html#newcode6 LayoutTests/fast/images/onload-event-when-reloading-image-after-successful-image-load.html:6: testRunner.dumpAsText(); js-test already sets dumpAsText, and you can do ...
5 years, 6 months ago (2015-04-06 20:31:36 UTC) #15
Jimmy Jo
4 years, 6 months ago (2016-04-28 13:02:33 UTC) #17
Message was sent while issue was closed.
https://codereview.chromium.org/1047563002/diff/100001/Source/core/loader/Ima...
File Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/1047563002/diff/100001/Source/core/loader/Ima...
Source/core/loader/ImageLoader.cpp:323: oldImage->removeClient(this);
I have a question.
If newImage and oldImage are equal, is this line same
newImage->removeClient(this)?

Powered by Google App Engine
This is Rietveld 408576698