Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(379)

Issue 654253003: Error event shouldn't fire when image is updated on environment changes. (Closed)

Created:
6 years, 2 months ago by Yoav Weiss
Modified:
6 years, 1 month ago
Reviewers:
Nate Chapin, Mike West
CC:
blink-reviews, Nate Chapin, gavinp+loader_chromium.org, simonp
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Error event shouldn't fire when image is updated on environment changes. When environment changes triggered an image load, the error event was wrongfully triggered. This is the wrong behavior for Web compat and it made related tests flaky. This CL fixes that. BUG=410268, 411672 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184917

Patch Set 1 #

Total comments: 3

Patch Set 2 : Review comments #

Patch Set 3 : style #

Unified diffs Side-by-side diffs Delta from patch set Stats (+9 lines, -3 lines) Patch
M LayoutTests/fast/dom/HTMLImageElement/image-srcset-w-onerror-expected.txt View 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/loader/ImageLoader.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/loader/ImageLoader.cpp View 1 2 3 chunks +8 lines, -2 lines 0 comments Download

Messages

Total messages: 12 (3 generated)
Yoav Weiss
6 years, 2 months ago (2014-10-17 13:06:04 UTC) #2
Mike West
Code change looks reasonable, but I don't understand why the error event occurs twice in ...
6 years, 2 months ago (2014-10-17 13:26:47 UTC) #4
Yoav Weiss
On 2014/10/17 13:26:47, Mike West wrote: > Code change looks reasonable, but I don't understand ...
6 years, 2 months ago (2014-10-17 13:38:37 UTC) #5
Mike West
Hrm. So, step 10 of the algorithm you point to does indeed suggest that we ...
6 years, 2 months ago (2014-10-17 14:12:40 UTC) #6
Yoav Weiss
On 2014/10/17 14:12:40, Mike West wrote: > Hrm. So, step 10 of the algorithm you ...
6 years, 2 months ago (2014-10-21 11:50:32 UTC) #7
Yoav Weiss
On 2014/10/21 11:50:32, Yoav Weiss wrote: > > For the rest, let's wait for zcorpan ...
6 years, 1 month ago (2014-11-06 14:23:48 UTC) #8
Mike West
Still LGTM. Thanks for looping folks in and waiting on landing the patch.
6 years, 1 month ago (2014-11-06 14:31:20 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/654253003/40001
6 years, 1 month ago (2014-11-06 15:42:28 UTC) #11
commit-bot: I haz the power
6 years, 1 month ago (2014-11-06 15:44:29 UTC) #12
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as 184917

Powered by Google App Engine
This is Rietveld 408576698