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

Issue 2863763003: Remove EventSender from ImageLoader (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 5 days ago by hiroshige
Modified:
19 hours, 17 minutes ago
CC:
chromium-reviews, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews, rwlbuis
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove EventSender from ImageLoader Instead of using EventSender, this CL schedules ImageLoader's load and error events using PostCancellableTask. has_pending_load_event_ and has_pending_error_event_ booleans are replaced with TaskHandle to the cancellable tasks and IsActive() calls. This CL removes the dependencies between Document's load event and <img> load events via EventSender. This CL removes EventSender.h as ImageLoader was the last user of that. Test changes: stop-loading.html: testharness.js waits for the window load event before finishing. Previously, we call window.stop() in <img> load event, but window load event is fired, because the <img> load event is fired from Document::ImplicitClose() via EventSender. At that time window.stop() doesn't stop the window load event. After this CL, <img> load event is fired separately from Document::ImplicitClose(), and thus window.stop() stops the window load event. To work around this, this CL triggers window load event explicitly just to signal testharness.js to finish. http/tests/loading/preload-image-sizes-2x.html and http/tests/loading/preload-picture-sizes-2x.html: The tests uses srcset-helper.js that reloads the page with empty cache in the window load event. Because this CL changes the timing of the window load event, this causes some asynchronous tasks that cause new image loading to be scheduled before window load event and executed after that, causing some images left on the cache before reloading starts. This CL works around the issue by waiting a little bit for those tasks to be executed, and then clearing the cache and reloading the page. BUG=624697, 720268 Review-Url: https://codereview.chromium.org/2863763003 Cr-Commit-Position: refs/heads/master@{#474086} Committed: https://chromium.googlesource.com/chromium/src/+/da70e32d811fffe4e15728ba858dbdfa3180ed59

Patch Set 1 #

Patch Set 2 : adRebase #

Patch Set 3 : fix tests #

Total comments: 6

Patch Set 4 : Rebase #

Patch Set 5 : Rebase #

Patch Set 6 : format #

Patch Set 7 : Rebase error fix #

Patch Set 8 : Rebase #

Patch Set 9 : Rebase #

Patch Set 10 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+74 lines, -221 lines) Patch
M third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html View 1 2 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/resources/srcset-helper.js View 1 2 1 chunk +11 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.cpp View 1 2 3 4 5 6 7 8 9 2 chunks +0 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/events/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -1 line 0 comments Download
D third_party/WebKit/Source/core/events/EventSender.h View 1 1 chunk +0 lines, -120 lines 0 comments Download
M third_party/WebKit/Source/core/loader/ImageLoader.h View 1 2 3 4 5 5 chunks +6 lines, -16 lines 0 comments Download
M third_party/WebKit/Source/core/loader/ImageLoader.cpp View 1 2 3 4 5 6 7 8 9 16 chunks +51 lines, -73 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 84 (69 generated)
hiroshige
PTAL japhet@, yhirano@, kinuko@, kouhei@ for loading. tzik@, is TaskHandle::IsActive() guaranteed to be false (or ...
2 weeks, 4 days ago (2017-05-05 20:18:40 UTC) #14
kinuko
Looking good. https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html File third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html (right): https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html#newcode16 third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html:16: // We emulate window load event to ...
2 weeks, 2 days ago (2017-05-08 09:24:24 UTC) #22
kinuko
On 2017/05/08 09:24:24, kinuko wrote: > Looking good. (And really great to see the CL ...
2 weeks, 2 days ago (2017-05-08 09:25:21 UTC) #23
kouhei (back in TOK)
lgtm https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html File third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html (right): https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html#newcode16 third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html:16: // We emulate window load event to signal ...
2 weeks, 2 days ago (2017-05-08 11:54:26 UTC) #24
hiroshige
https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html File third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html (right): https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html#newcode16 third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html:16: // We emulate window load event to signal testharness.js ...
2 weeks, 1 day ago (2017-05-08 19:23:11 UTC) #25
kinuko
k, lgtm. thanks!
2 weeks, 1 day ago (2017-05-09 00:02:57 UTC) #26
kinuko
k, lgtm. thanks!
2 weeks, 1 day ago (2017-05-09 00:03:03 UTC) #27
tzik
On 2017/05/05 20:18:40, hiroshige wrote: > PTAL > japhet@, yhirano@, kinuko@, kouhei@ for loading. > ...
2 weeks, 1 day ago (2017-05-09 03:13:10 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2863763003/160001
21 hours, 58 minutes ago (2017-05-23 19:50:50 UTC) #62
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/221636) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, ...
21 hours, 55 minutes ago (2017-05-23 19:54:23 UTC) #64
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2863763003/180001
21 hours, 44 minutes ago (2017-05-23 20:05:26 UTC) #70
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2863763003/180001
19 hours, 35 minutes ago (2017-05-23 22:13:59 UTC) #78
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2863763003/180001
19 hours, 33 minutes ago (2017-05-23 22:15:55 UTC) #80
commit-bot: I haz the power
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/da70e32d811fffe4e15728ba858dbdfa3180ed59
19 hours, 17 minutes ago (2017-05-23 22:31:48 UTC) #83
yosin_UTC9
13 hours, 43 minutes ago (2017-05-24 04:06:12 UTC) #84
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2903773002/ by yosin@chromium.org.

The reason for reverting is: This patch probably causes object leaks:

See details in "webkit_tests" of
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty...
.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06