Clean up flags and assumptions of ImageLoader |
|||
Issue descriptionCurrently the flags and image_ (and other states) are set/updated in somehow inconsistent/confusing ways. - image_ - has_pending_load_event_ - image_complete_ This issue aims to make them cleaner and consnstent, to unblock refactoring and put stronger assertions.
,
May 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d5af742666d2acde551ac33e82f3d8662ffa0f15 commit d5af742666d2acde551ac33e82f3d8662ffa0f15 Author: hiroshige <hiroshige@chromium.org> Date: Fri May 12 01:56:02 2017 Split ImageLoader::SetImage() and set flags consistently in tests Previously, ImageLoader::SetImage() is used in three ways: 1. SetImage(nullptr), 2. SetImage(non-null) for ImageDocument, and 3. SetImage(non-null) for unit tests for non-ImageDocument. and SetImage() sets has_pending_load_event_ = false and image_complete_ = true for all of these. This flag setting is consistent for 1., but not for 3., and causes assertion failures when we apply stronger assertions in [1]. This CL fixes this by splitting SetImage() for separate methods: 1. ClearImage(), 2. SetImageForImageDocument(), and 3. SetImageForTest(), and introducing UpdateImageState() that sets: - image_ - has_pending_load_event_ - image_complete_ This CL - Doesn't change non-test behavior, i.e. keeps the behavior for Cases 1 and 2. Particularly, this leaves the flag values that are apparently inconsistent but needed for the current implementation in Case 2 in SetImageForImageDocument(). - Changes the test-only behavior of Case 3, to set has_pending_load_event_ = true and image_complete_ = false in SetImageForTest(). This doesn't affect the tested behavior but fixes the assertion failures in [1]. [1] https://codereview.chromium.org/2859383002 BUG= 624697 , 719759 Review-Url: https://codereview.chromium.org/2864253003 Cr-Commit-Position: refs/heads/master@{#471177} [modify] https://crrev.com/d5af742666d2acde551ac33e82f3d8662ffa0f15/third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp [modify] https://crrev.com/d5af742666d2acde551ac33e82f3d8662ffa0f15/third_party/WebKit/Source/core/html/HTMLImageElement.h [modify] https://crrev.com/d5af742666d2acde551ac33e82f3d8662ffa0f15/third_party/WebKit/Source/core/html/HTMLObjectElement.cpp [modify] https://crrev.com/d5af742666d2acde551ac33e82f3d8662ffa0f15/third_party/WebKit/Source/core/loader/ImageLoader.cpp [modify] https://crrev.com/d5af742666d2acde551ac33e82f3d8662ffa0f15/third_party/WebKit/Source/core/loader/ImageLoader.h
,
May 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f33a3ba6de05eaed0115636c6dd552b9f8c80333 commit f33a3ba6de05eaed0115636c6dd552b9f8c80333 Author: hiroshige <hiroshige@chromium.org> Date: Fri May 12 19:18:35 2017 Add layout tests for ImageLoader::UpdateFromElement(kUpdateForcedReload) HTMLImageElement::ForceReload() and ImageLoader::UpdateFromElement(kUpdateForcedReload) were introduced by [1] but are not covered by any tests. This CL adds layout tests for this code path. This is also confirming the correctness of [2] and [3]. [1] https://codereview.chromium.org/1112513005/ [2] https://codereview.chromium.org/2877583002/ [3] https://codereview.chromium.org/2859383002/ BUG=485295, 624697 , 719759 Review-Url: https://codereview.chromium.org/2874073003 Cr-Commit-Position: refs/heads/master@{#471401} [add] https://crrev.com/f33a3ba6de05eaed0115636c6dd552b9f8c80333/third_party/WebKit/LayoutTests/http/tests/cache/resources/random-cached-image.php [add] https://crrev.com/f33a3ba6de05eaed0115636c6dd552b9f8c80333/third_party/WebKit/LayoutTests/http/tests/images/force-reload-image-document.html [add] https://crrev.com/f33a3ba6de05eaed0115636c6dd552b9f8c80333/third_party/WebKit/LayoutTests/http/tests/images/force-reload.html [modify] https://crrev.com/f33a3ba6de05eaed0115636c6dd552b9f8c80333/third_party/WebKit/Source/core/testing/Internals.cpp [modify] https://crrev.com/f33a3ba6de05eaed0115636c6dd552b9f8c80333/third_party/WebKit/Source/core/testing/Internals.h [modify] https://crrev.com/f33a3ba6de05eaed0115636c6dd552b9f8c80333/third_party/WebKit/Source/core/testing/Internals.idl
,
May 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1b070387595668ec438ffec65185e61017300e3c commit 1b070387595668ec438ffec65185e61017300e3c Author: hiroshige <hiroshige@chromium.org> Date: Fri May 12 22:21:22 2017 Add CHECK()s about ImageLoader::has_pending_load_event_ As preparation for [1], this CL checks some assumptions to hold that are helpful for proving the equivalences in [1]. [1] https://codereview.chromium.org/2859093003/ BUG= 624697 , 719759 Review-Url: https://codereview.chromium.org/2859383002 Cr-Commit-Position: refs/heads/master@{#471464} [modify] https://crrev.com/1b070387595668ec438ffec65185e61017300e3c/third_party/WebKit/Source/core/loader/ImageLoader.cpp
,
May 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9d5bcf58d9932d151f12231a6544b67903780fed commit 9d5bcf58d9932d151f12231a6544b67903780fed Author: hiroshige <hiroshige@chromium.org> Date: Sat May 13 18:03:28 2017 Revert of Add CHECK()s about ImageLoader::has_pending_load_event_ (patchset #12 id:220001 of https://codereview.chromium.org/2859383002/ ) Reason for revert: Assertion failures in the wild and on clusterfuzz. BUG= 721994 Original issue's description: > Add CHECK()s about ImageLoader::has_pending_load_event_ > > As preparation for [1], this CL checks some assumptions to hold that > are helpful for proving the equivalences in [1]. > > [1] https://codereview.chromium.org/2859093003/ > > BUG= 624697 , 719759 > > Review-Url: https://codereview.chromium.org/2859383002 > Cr-Commit-Position: refs/heads/master@{#471464} > Committed: https://chromium.googlesource.com/chromium/src/+/1b070387595668ec438ffec65185e61017300e3c TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 624697 , 719759 Review-Url: https://codereview.chromium.org/2878263002 Cr-Commit-Position: refs/heads/master@{#471598} [modify] https://crrev.com/9d5bcf58d9932d151f12231a6544b67903780fed/third_party/WebKit/Source/core/loader/ImageLoader.cpp
,
May 15 2017
,
May 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1c5d3f11a80d88d65f5fae8398ed97a7b5ea1508 commit 1c5d3f11a80d88d65f5fae8398ed97a7b5ea1508 Author: hiroshige <hiroshige@chromium.org> Date: Mon May 15 22:28:51 2017 Partial reland of "Add CHECK()s about ImageLoader::has_pending_load_event_" Original CL: https://codereview.chromium.org/2859383002/#ps220001 This CL relands the CHECK()s except for the failing one in DispatchPendingErrorEvent(). Issue 722500 tracks the issue around DispatchPendingErrorEvent() and relanding of the remaining CHECK(). Original CL description: > As preparation for [1], this CL checks some assumptions to hold that > are helpful for proving the equivalences in [1]. > > [1] https://codereview.chromium.org/2859093003/ > > BUG= 624697 , 719759 > > Review-Url: https://codereview.chromium.org/2859383002 > Cr-Commit-Position: refs/heads/master@{#471464} > Committed: https://chromium.googlesource.com/chromium/src/+/1b070387595668ec438ffec65185e61017300e3c BUG= 624697 , 719759 , 722500 Review-Url: https://codereview.chromium.org/2884773002 Cr-Commit-Position: refs/heads/master@{#471925} [modify] https://crrev.com/1c5d3f11a80d88d65f5fae8398ed97a7b5ea1508/third_party/WebKit/Source/core/loader/ImageLoader.cpp
,
May 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/84fb9e0a3e168b81e538a5ff8ce79e3690396a10 commit 84fb9e0a3e168b81e538a5ff8ce79e3690396a10 Author: hiroshige <hiroshige@chromium.org> Date: Fri May 19 16:53:20 2017 Make ImageLoader have at most one pending error event on EventSender Relanding the CHECK() that asserts ImageLoader::DispatchPendingErrorEvent() should always called when has_pending_error_event_ is true. Previously, we sometimes schedule multiple error events on EventSender: 1. DispatchErrorEvent() is called. 2. DispatchErrorEvent() is called. 3. DispatchPendingErrorEvent() is called (corresponding to Step 1) and actual processing is done. 4. DispatchPendingErrorEvent() is called (corresponding to Step 2) but ignored because has_pending_error_event_ is already false. This CL makes DispatchErrorEvent() cancel previous scheduled error events if any, ensures there can be at most scheduled error event on EventSender, and makes the assertion hold. The scenario above will be processed as follows, and thus this CL shouldn't change any observable behavior. 1. DispatchErrorEvent() is called. 2. DispatchErrorEvent() is called. The error event scheduled by Step 1 is cancelled. 3. DispatchPendingErrorEvent() is called (corresponding to Step 2) and actual processing is done. [1] https://codereview.chromium.org/2859383002/ [2] https://codereview.chromium.org/2884773002/ BUG= 624697 , 719759 , 722500 Review-Url: https://codereview.chromium.org/2893993002 Cr-Commit-Position: refs/heads/master@{#473214} [add] https://crrev.com/84fb9e0a3e168b81e538a5ff8ce79e3690396a10/third_party/WebKit/LayoutTests/images/multiple-inflight-error-event-crash.html [modify] https://crrev.com/84fb9e0a3e168b81e538a5ff8ce79e3690396a10/third_party/WebKit/Source/core/loader/ImageLoader.cpp
,
May 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2e6f72c80e0106f348fd9437536edc2e9f3b7acc commit 2e6f72c80e0106f348fd9437536edc2e9f3b7acc Author: hiroshige <hiroshige@chromium.org> Date: Mon May 22 20:49:24 2017 Change the semantics of ImageLoader::has_pending_load_event_ ImageLoader works like: (1) Image loading is started (AddObserver()), (2) Image loading is finished and the image element's load event is scheduled (ImageNotifyFinished()), and then (3) The load event is fired. Previously, has_pending_load_event_ was true from (1) to (3). This CL makes has_pending_load_event_ true only from (2) to (3), i.e. only while there is a pending task on EventSender. Because "image_ && !image_complete_" is true from (1) to (2), The previous "has_pending_load_event_" is equivalent to the current "(image_ && !image_complete_) || has_pending_load_event_". This CL, however, leaves most of "has_pending_load_event_" usage unchanged, because in such cases they are equivalent, given the assumptions around image_ and image_complete_, some of which are confirmed by [1]. This is preparation for [2] that replaces has_pending_load_event_ with a reference to a task that corresponds to the pending load event. [1] https://codereview.chromium.org/2859383002/ [2] https://codereview.chromium.org/2863763003/ BUG= 624697 , 719759 Review-Url: https://codereview.chromium.org/2859093003 Cr-Commit-Position: refs/heads/master@{#473684} [modify] https://crrev.com/2e6f72c80e0106f348fd9437536edc2e9f3b7acc/third_party/WebKit/Source/core/loader/ImageLoader.cpp [modify] https://crrev.com/2e6f72c80e0106f348fd9437536edc2e9f3b7acc/third_party/WebKit/Source/core/loader/ImageLoader.h
,
Sep 4 2017
|
|||
►
Sign in to add a comment |
|||
Comment 1 by hirosh...@chromium.org
, May 9 2017