Fire load event for an image even if it was the last image we loaded successfully.

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

[rhogan, 2015-03-30 20:50:13]

robhogan@gmail.com changed reviewers:
+ esprehn@chromium.org

[esprehn, 2015-03-31 00:34:54]

https://codereview.chromium.org/1047563002/diff/40001/LayoutTests/fast/images...
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...
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...
File
LayoutTests/fast/images/onload-event-when-reloading-image-after-interrupted-null-src-load.html
(right):

https://codereview.chromium.org/1047563002/diff/40001/LayoutTests/fast/images...
LayoutTests/fast/images/onload-event-when-reloading-image-after-interrupted-null-src-load.html:1:
<script>
add doctype.

https://codereview.chromium.org/1047563002/diff/40001/LayoutTests/fast/images...
File
LayoutTests/fast/images/onload-event-when-reloading-image-after-successful-image-load.html
(right):

https://codereview.chromium.org/1047563002/diff/40001/LayoutTests/fast/images...
LayoutTests/fast/images/onload-event-when-reloading-image-after-successful-image-load.html:1:
<script>
ditto

https://codereview.chromium.org/1047563002/diff/40001/Source/core/loader/Imag...
File Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/1047563002/diff/40001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:293: bool selectedNewImage = newImage !=
oldImage;
I would just inline the newImage != oldImage check in the two places. It's just
a ptr compare.

https://codereview.chromium.org/1047563002/diff/40001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:294: if (resizingToViewport &&
!selectedNewImage) {
I might add a method like isLayoutImage()

and then do:

if (updateBehavior == UpdateSizeChanged && isLayoutImage() && newImage ==
oldImage) {
   ...
} else {
   if (newImage != oldImage)
      sourceImageChanged()
}

[rhogan, 2015-03-31 19:03:20]

https://codereview.chromium.org/1047563002/diff/40001/Source/core/loader/Imag...
File Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/1047563002/diff/40001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:294: if (resizingToViewport &&
!selectedNewImage) {
On 2015/03/31 at 00:34:54, esprehn wrote:
> I might add a method like isLayoutImage()

Mm, I passed on this. Hope that's OK.

[esprehn, 2015-03-31 21:53:21]

lgtm, but please use js-test.js in the test.

https://codereview.chromium.org/1047563002/diff/60001/LayoutTests/fast/images...
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...
LayoutTests/fast/images/onload-event-when-reloading-image-after-interrupted-broken-image-load.html:13:
document.getElementById('console').innerHTML += "Image onload event fired for "
+ img.src.substring(img.src.lastIndexOf('/') + 1) + "<br>";
Why not just use js-test.js and then you can use debug() and such in the test.

https://codereview.chromium.org/1047563002/diff/60001/LayoutTests/fast/images...
LayoutTests/fast/images/onload-event-when-reloading-image-after-interrupted-broken-image-load.html:21:
setTimeout("img.src='" + resetImage + "';img.src = '" + testImage + "';", 0);
don't use the eval, use a function.

https://codereview.chromium.org/1047563002/diff/60001/LayoutTests/fast/images...
File
LayoutTests/fast/images/onload-event-when-reloading-image-after-successful-image-load.html
(right):

https://codereview.chromium.org/1047563002/diff/60001/LayoutTests/fast/images...
LayoutTests/fast/images/onload-event-when-reloading-image-after-successful-image-load.html:21:
setTimeout("img.src='" + resetImage + "';img.src = '" + testImage + "';", 0);
use functions instead.

setTimeout(function() {
   ...
})

[rhogan, 2015-04-01 19:29:56]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2015-04-01 19:29:56]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1047563002/#ps100001
(title: "Updated")

[commit-bot: I haz the power, 2015-04-01 19:30:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1047563002/100001

[commit-bot: I haz the power, 2015-04-01 23:20:23]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192974

[Yoav Weiss, 2015-04-02 07:52:09]

yoav@yoav.ws changed reviewers:
+ yoav@yoav.ws

[Yoav Weiss, 2015-04-02 07:52:10]

https://codereview.chromium.org/1047563002/diff/100001/LayoutTests/fast/image...
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/image...
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? (since this image load is async)

https://codereview.chromium.org/1047563002/diff/100001/LayoutTests/fast/image...
File
LayoutTests/fast/images/onload-event-when-reloading-image-after-successful-image-load.html
(right):

https://codereview.chromium.org/1047563002/diff/100001/LayoutTests/fast/image...
LayoutTests/fast/images/onload-event-when-reloading-image-after-successful-image-load.html:31:
img.src= resetImage;
same question

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:292: if (updateBehavior == UpdateSizeChanged
&& m_element->layoutObject() && m_element->layoutObject()->isImage() && newImage
== oldImage) {
Hmm, I'm not sure that's the cleanest way to do that. The UpdateSizeChanged
behavior is there to update the image intrinsic size as a result of environment
changes. If that triggers onload in your test cases, I suspect it's a
side-effect :|

[rhogan, 2015-04-02 18:15:16]

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:292: if (updateBehavior == UpdateSizeChanged
&& m_element->layoutObject() && m_element->layoutObject()->isImage() && newImage
== oldImage) {
On 2015/04/02 at 07:52:10, Yoav Weiss wrote:
> Hmm, I'm not sure that's the cleanest way to do that. The UpdateSizeChanged
behavior is there to update the image intrinsic size as a result of environment
changes. If that triggers onload in your test cases, I suspect it's a
side-effect :|

It's not material for the test cases added by my CL, but there is coverage in
LayoutTests for the case where the viewport changes and srcset resizes the image
but doesn't select a new one. In that situation the spec tells me I don't want
to trigger a load event so that's what I'm doing here.

[Yoav Weiss, 2015-04-03 06:03:15]

On 2015/04/02 18:15:16, rhogan wrote:
>
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:292: if (updateBehavior ==
UpdateSizeChanged
> && m_element->layoutObject() && m_element->layoutObject()->isImage() &&
newImage
> == oldImage) {
> On 2015/04/02 at 07:52:10, Yoav Weiss wrote:
> > Hmm, I'm not sure that's the cleanest way to do that. The UpdateSizeChanged
> behavior is there to update the image intrinsic size as a result of
environment
> changes. If that triggers onload in your test cases, I suspect it's a
> side-effect :|
> 
> It's not material for the test cases added by my CL, but there is coverage in
> LayoutTests for the case where the viewport changes and srcset resizes the
image
> but doesn't select a new one. In that situation the spec tells me I don't want
> to trigger a load event so that's what I'm doing here.

I didn't say there's no coverage or that your change is not aligned with the
spec :) 
What I'm saying is that UpdateSizeChanged wasn't intended to handle cases
outside of the realm of env changes, and the fact that it does (hence making
your new tests pass), is a side effect.
I'm not against keeping that change in, but we should probably refactor
ImageLoader and make sure that its code is explicit about what it does, (and
doesn't do what we didn't intend for it to do).

[rhogan, 2015-04-03 17:28:19]

On 2015/04/03 at 06:03:15, yoav wrote:
> What I'm saying is that UpdateSizeChanged wasn't intended to handle cases
outside of the realm of env changes, 

Can you clarify how I'm introducing that here? The logic I've introduced boils
down to: if you loaded an image fire a load event, unless you went to load an
image in response to an environment change and when all was said and done you
ended up keeping the old image. So as far as I can tell UpdateSizeChanged is
still doing its old job and hasn't been given a new one. :)

> and the fact that it does (hence making your new tests pass), is a side
effect.
It's not having that effect to my understanding - I'm just keeping the reponsive
image path working as expected while allowing other cases to fire a load event
when they ought to, which is always unless they failed to load an image.

If I'm still missing your point hopefully you can see where I'm misapprehending
you!

[esprehn, 2015-04-06 20:31:36]

https://codereview.chromium.org/1047563002/diff/100001/LayoutTests/fast/image...
File
LayoutTests/fast/images/onload-event-when-reloading-image-after-successful-image-load.html
(right):

https://codereview.chromium.org/1047563002/diff/100001/LayoutTests/fast/image...
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 something like jsTestAsync =
true and it'll deal with the waitUntilDone().

Then you do finishJSTest() in the async function to finish the test.

https://codereview.chromium.org/1047563002/diff/100001/LayoutTests/fast/image...
LayoutTests/fast/images/onload-event-when-reloading-image-after-successful-image-load.html:18:
testRunner.notifyDone();
finishJSTest(), you don't even need to mention testRunner anywhere in this test.

https://codereview.chromium.org/1047563002/diff/100001/LayoutTests/fast/image...
LayoutTests/fast/images/onload-event-when-reloading-image-after-successful-image-load.html:36:
crbug.com/469678: We should fire an image load event even if the image is the
last one we loaded successfully.<br>
Generally this goes in the description("") call instead of inline like this.

[Jimmy Jo, 2016-04-28 13:02:32]

jincheol.jo@navercorp.com changed reviewers:
+ jincheol.jo@navercorp.com

[Jimmy Jo, 2016-04-28 13:02:33]

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)?