|
|
DescriptionRemove 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 #
Messages
Total messages: 84 (69 generated)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Remove EventSender from ImageLoader BUG= ========== to ========== Remove EventSender from ImageLoader Previous attempt: https://codereview.chromium.org/1833303002/ BUG= ==========
Description was changed from ========== Remove EventSender from ImageLoader Previous attempt: https://codereview.chromium.org/1833303002/ BUG= ========== to ========== Remove EventSender from ImageLoader Previous attempt: https://codereview.chromium.org/1833303002/ BUG=624697 ==========
Description was changed from ========== Remove EventSender from ImageLoader Previous attempt: https://codereview.chromium.org/1833303002/ BUG=624697 ========== to ========== Remove EventSender from ImageLoader Previous attempt: https://codereview.chromium.org/1833303002/ https://codereview.chromium.org/1515963005/ BUG=624697 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Remove EventSender from ImageLoader Previous attempt: https://codereview.chromium.org/1833303002/ https://codereview.chromium.org/1515963005/ BUG=624697 ========== to ========== 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. 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 ==========
hiroshige@chromium.org changed reviewers: + japhet@chromium.org, kinuko@chromium.org, kouhei@chromium.org, tzik@chromium.org, yhirano@chromium.org
PTAL japhet@, yhirano@, kinuko@, kouhei@ for loading. tzik@, is TaskHandle::IsActive() guaranteed to be false (or true?) when the corresponding cancellable task is being executed?
Description was changed from ========== 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. 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 ========== to ========== 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Looking good. https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html (right): https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html:16: // We emulate window load event to signal testharness.js that we don't nit: that -> so that? https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/resources/srcset-helper.js (right): https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/resources/srcset-helper.js:34: }, 300); This intuitively feels a bit sketchy, is 300 chosen arbitrarily? Would it be hard to make this deterministic?
On 2017/05/08 09:24:24, kinuko wrote: > Looking good. (And really great to see the CL that's going to remove EventSender!) > https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html > (right): > > https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html:16: // We > emulate window load event to signal testharness.js that we don't > nit: that -> so that? > > https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/http/tests/resources/srcset-helper.js > (right): > > https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/resources/srcset-helper.js:34: }, > 300); > This intuitively feels a bit sketchy, is 300 chosen arbitrarily? Would it be > hard to make this deterministic?
lgtm https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html (right): https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html:16: // We emulate window load event to signal testharness.js that we don't On 2017/05/08 09:24:24, kinuko wrote: > nit: that -> so that? "that" seems correct here? https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/resources/srcset-helper.js (right): https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/resources/srcset-helper.js:34: }, 300); On 2017/05/08 09:24:24, kinuko wrote: > This intuitively feels a bit sketchy, is 300 chosen arbitrarily? Would it be > hard to make this deterministic? Tip: use requestAnimationFrame?
https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html (right): https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html:16: // We emulate window load event to signal testharness.js that we don't On 2017/05/08 11:54:25, kouhei (on transit) wrote: > On 2017/05/08 09:24:24, kinuko wrote: > > nit: that -> so that? > "that" seems correct here? Either looks fine to me (with different but acceptable meanings) but I'm not a English expert. https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/resources/srcset-helper.js (right): https://codereview.chromium.org/2863763003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/resources/srcset-helper.js:34: }, 300); On 2017/05/08 11:54:25, kouhei (on transit) wrote: > On 2017/05/08 09:24:24, kinuko wrote: > This intuitively feels a bit sketchy, is 300 chosen arbitrarily? Would it be > hard to make this deterministic? I think we can make this deterministic by using iframes, i.e. set scale factor in the main HTML and then after that start loading the iframe that contains the tests. However, this requires changes to 9 test HTMLs under http/tests/loading that use srcset-helper.js. Also, about 40 tests are using srcset-helper.js under fast/. (They are not affected by this CL because they are not interested in preloading though) Therefore I chose to add workaround here not to block this CL by the test refactoring. > Tip: use requestAnimationFrame? requestAnimationFrame didn't work :( setTimeout(, 0) works.
k, lgtm. thanks!
k, lgtm. thanks!
On 2017/05/05 20:18:40, hiroshige wrote: > PTAL > japhet@, yhirano@, kinuko@, kouhei@ for loading. > > tzik@, is TaskHandle::IsActive() guaranteed to be false > (or true?) when the corresponding cancellable task is > being executed? LGTM. Yes, it's guaranteed to be *false* there.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== 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 ========== to ========== 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 ==========
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, kouhei@chromium.org, tzik@chromium.org Link to the patchset: https://codereview.chromium.org/2863763003/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by hiroshige@chromium.org
The CQ bit was checked by hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, kouhei@chromium.org, tzik@chromium.org Link to the patchset: https://codereview.chromium.org/2863763003/#ps180001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #11 (id:200001) has been deleted
The CQ bit was unchecked by hiroshige@chromium.org
The CQ bit was checked by hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, kouhei@chromium.org, tzik@chromium.org Link to the patchset: https://codereview.chromium.org/2863763003/#ps180001 (title: "Rebase")
The CQ bit was unchecked by hiroshige@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hiroshige@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495577657498910, "parent_rev": "789f8d1d41b5dbba8a252d8b69390f9bd67c83db", "commit_rev": "da70e32d811fffe4e15728ba858dbdfa3180ed59"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/da70e32d811fffe4e15728ba858d... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/da70e32d811fffe4e15728ba858d...
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... . |