Cleanup ImageResource::decodeError() |
|||
Issue description
decodeError() has some issues:
- It calls ResourceLoader::didFinishLoading() directly, which is not clean.
- It makes Resource an error state (DecodeError), but also handles the Resource as if loading is completed (as didFinishLoading() is called), which is inconsistent.
- ImageResourceContent::updateImage() is reentered via decodeError():
Stack trace:
#3 0x00000cb7d860 blink::ImageResourceContent::updateImage()
#4 0x00000cb738f1 blink::ImageResource::finish()
#5 0x00000b994e1f blink::ResourceFetcher::handleLoaderFinish()
#6 0x00000cb73677 blink::ImageResource::decodeError()
#7 0x00000cb7def1 blink::ImageResourceContent::updateImage()
Test CL: https://codereview.chromium.org/2605733002
I think we should make it cleaner.
I did some trials in https://codereview.chromium.org/2487763003/, basically by treating decodeError() as a complete failure (e.g. by calling ResourceLoader::cancel() instead of didFinishLoading()), but some tests (e.g. those for inspector) failed.
,
Jan 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/25ff90033184e53f7fd47b44511397da29a1ce27 commit 25ff90033184e53f7fd47b44511397da29a1ce27 Author: hiroshige <hiroshige@chromium.org> Date: Wed Jan 25 01:51:04 2017 Check imageChangedCount() in DecodeError-related unit tests BUG=667641, 677188 Review-Url: https://codereview.chromium.org/2653493005 Cr-Commit-Position: refs/heads/master@{#445908} [modify] https://crrev.com/25ff90033184e53f7fd47b44511397da29a1ce27/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
,
Feb 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6d03fad48660f6a6a767c6cc70c621ff10f8fcfa commit 6d03fad48660f6a6a767c6cc70c621ff10f8fcfa Author: hiroshige <hiroshige@chromium.org> Date: Fri Feb 10 23:08:59 2017 Add unit tests for placeholder reloading on didFinishLoading ImageResourceTest.ReloadIfLoFiOrPlaceholderViaResourceFetcher tests reloading in ResourceFetcher::reloadLoFiImages(). ImageResourceTest.PartialContentWithoutDimensions tests that DecodeError with non-empty body in ResourceLoader::didFinishLoading(). ImageResourceTest.FetchAllowPlaceholderPartialContentWithoutDimensions tests the case where DecodeError with non-empty body in ResourceLoader::didFinishLoading() causes reloading. BUG=667641, 677188 Review-Url: https://codereview.chromium.org/2648073002 Cr-Commit-Position: refs/heads/master@{#449776} [modify] https://crrev.com/6d03fad48660f6a6a767c6cc70c621ff10f8fcfa/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
,
Feb 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cfa9dd423a171356f71478605f903b6410fe7861 commit cfa9dd423a171356f71478605f903b6410fe7861 Author: hiroshige <hiroshige@chromium.org> Date: Sat Feb 11 00:03:38 2017 Temporarily introduce ImageResourceInfo::setIsPlaceholder() This is for https://codereview.chromium.org/2543073002, and setIsPlaceholder() will be removed later in the refactoring. BUG= 657995 , 667641, 677188 Review-Url: https://codereview.chromium.org/2638383008 Cr-Commit-Position: refs/heads/master@{#449795} [modify] https://crrev.com/cfa9dd423a171356f71478605f903b6410fe7861/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp [modify] https://crrev.com/cfa9dd423a171356f71478605f903b6410fe7861/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp [modify] https://crrev.com/cfa9dd423a171356f71478605f903b6410fe7861/third_party/WebKit/Source/core/loader/resource/ImageResourceInfo.h
,
Feb 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bd8c80c437ceaa8427186bbbbd3040ae17d6d13d commit bd8c80c437ceaa8427186bbbbd3040ae17d6d13d Author: hiroshige <hiroshige@chromium.org> Date: Wed Feb 22 11:22:03 2017 Phase II Step 1: Remove updateImage() reentrancy around decodeError() Design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8bE/edit#heading=h.1rxkos5samv7 Previously, ImageResourceContent::updateImage() is reentered when DecodeError occurred in ImageResource. This CL removes the reentrancy and ImageResourceInfo::decodeError() by 1. making ImageResourceContent::updateImage() to return ShouldDecodeError without calling notifyObservers() when an decode error occurs, 2. then making ImageResource::updateImage() to call ImageResource::decodeError(), and 3. then making ImageResource::decodeError() to call notifyObservers() (via ImageResourceContent::updateImage()) if necessary. This CL also adds DCHECK() for prohibiting reentering to ImageResourceContent::updateImage(). In the case of a decode error inside didReceiveData(), this CL removes one imageChanged() call, which looks unnecessary. BUG=667641, 677188 Review-Url: https://codereview.chromium.org/2642823005 Cr-Commit-Position: refs/heads/master@{#451996} [modify] https://crrev.com/bd8c80c437ceaa8427186bbbbd3040ae17d6d13d/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp [modify] https://crrev.com/bd8c80c437ceaa8427186bbbbd3040ae17d6d13d/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp [modify] https://crrev.com/bd8c80c437ceaa8427186bbbbd3040ae17d6d13d/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h [modify] https://crrev.com/bd8c80c437ceaa8427186bbbbd3040ae17d6d13d/third_party/WebKit/Source/core/loader/resource/ImageResourceInfo.h [modify] https://crrev.com/bd8c80c437ceaa8427186bbbbd3040ae17d6d13d/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
,
Mar 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/810f7ecd76b0aba74f0a215dbd00b8e8cc5705ba commit 810f7ecd76b0aba74f0a215dbd00b8e8cc5705ba Author: hiroshige <hiroshige@chromium.org> Date: Tue Mar 07 23:23:14 2017 Phase II Step 2: Remove setIsPlaceholder() in updateImage() Design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8bE/edit#heading=h.1rxkos5samv7 Previsouly, |m_isPlaceholder| is cleared in updateImage(), if: - isEntireResource() is true, AND - (1) The HTTP status code is not 4XX or 5XX OR (2) the response has no DecodeError, to prevent creating of placeholder and reloading. However, this causes timing and code dependencies from ImageResourceContent::updateImage() to |ImageResource::m_isPlaceholder| and ImageResource::reloadIfLoFiOrPlaceholderImage(). This CL removes the dependencies and thus ImageResourceInfo::setIsPlaceholder(), by making |ImageResource::m_isPlaceholder| a tri-state enum (renamed as |ImageResource::placeholderOption|): - DoNotReloadPlaceholder (previously m_isPlaceholder == false) - ReloadPlaceholderOnDecodeError (new state) - ShowAndReloadPlaceholderAlways (previously m_isPlaceholder == true) and setting it when receiving ResourceResponse. This CL preserves the behavior of a placeholder ImageResource: If isEntireResource() && (1): Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder(). Previously: because |m_isPlaceholder| is cleared in updateImage(). Now: because |m_placeholderOption| is set to |DoNotReloadPlaceholder|. If isEntireResource() && not (1) && (2): Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder(). Previously: because |m_isPlaceholder| is cleared in updateImage(). Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError| and DecodeError does NOT occur because (2) is true. If isEntireResource() && not (1) && not (2): Behavior: Placeholder is not created && shouldReloadBrokenPlaceholder(). Previously: because updateImage() creates placeholder only if (2) is true and |m_isPlaceholder| is NOT cleared in updateImage(). Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError| and DecodeError does occur because (2) is false. Otherwise: Not affected. BUG=667641, 677188 Review-Url: https://codereview.chromium.org/2650113002 Cr-Commit-Position: refs/heads/master@{#455282} [modify] https://crrev.com/810f7ecd76b0aba74f0a215dbd00b8e8cc5705ba/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp [modify] https://crrev.com/810f7ecd76b0aba74f0a215dbd00b8e8cc5705ba/third_party/WebKit/Source/core/loader/resource/ImageResource.h [modify] https://crrev.com/810f7ecd76b0aba74f0a215dbd00b8e8cc5705ba/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp [modify] https://crrev.com/810f7ecd76b0aba74f0a215dbd00b8e8cc5705ba/third_party/WebKit/Source/core/loader/resource/ImageResourceInfo.h [modify] https://crrev.com/810f7ecd76b0aba74f0a215dbd00b8e8cc5705ba/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
,
Mar 8 2018
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 14 2018
Loading bug triage. 'Started' looks a right status. hiroshige@: did you finish this, or have any plan to submit more? |
|||
►
Sign in to add a comment |
|||
Comment 1 by bugdroid1@chromium.org
, Jan 25 2017