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

Issue 2905493005: Reland of "Remove EventSender from ImageLoader" (Closed)

Created:
3 years, 7 months ago by hiroshige
Modified:
3 years, 7 months ago
Reviewers:
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

Reland of "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 from issue 2863763003 at patchset 180001 (http://crrev.com/2863763003#ps180001)

Patch Set 1 #

Patch Set 2 : Use timer task runner #

Patch Set 3 : 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 2 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/resources/srcset-helper.js View 2 1 chunk +11 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.cpp View 1 2 2 chunks +0 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/events/BUILD.gn View 2 1 chunk +0 lines, -1 line 0 comments Download
D third_party/WebKit/Source/core/events/EventSender.h View 2 1 chunk +0 lines, -120 lines 0 comments Download
M third_party/WebKit/Source/core/loader/ImageLoader.h View 1 2 5 chunks +6 lines, -16 lines 0 comments Download
M third_party/WebKit/Source/core/loader/ImageLoader.cpp View 1 2 13 chunks +51 lines, -73 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 4 (4 generated)
hiroshige
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
3 years, 7 months ago (2017-05-24 21:42:45 UTC) #1
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2905493005/1
3 years, 7 months ago (2017-05-24 21:43:50 UTC) #2
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 7 months ago (2017-05-25 01:01:17 UTC) #3
commit-bot: I haz the power
3 years, 7 months ago (2017-05-25 01:01:18 UTC) #4
Dry run: This issue passed the CQ dry run.

Powered by Google App Engine
This is Rietveld 408576698