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

Issue 2208073004: Fix ImageLoader::m_hasPendingLoadEvent/m_imageComplete in ImageDocument

Created:
4 years, 4 months ago by hiroshige
Modified:
4 years, 3 months ago
Reviewers:
Nate Chapin, yhirano
CC:
chromium-reviews, blink-reviews, loading-reviews_chromium.org, tyoshino+watch_chromium.org, Nate Chapin, gavinp+loader_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix ImageLoader::m_hasPendingLoadEvent/m_imageComplete in ImageDocument Case 1: When a image is loaded as a subresource, loading is stared by ImageResource::fetch() in ImageLoader::doUpdateFromElement(). Case 2: When a image is loaded as a main document, the start of the loading is emulated by setImage() in ImageLoader::updateFromElement() and ImageDocumentParser will do the subsequent loading steps. However in Case 2, |m_hasPendingLoadEvent| is set to false (in setImageWithoutConsideringPendingLoadEvent()), causing |imageStillLoading| to be false and ensureFallbackContent() to be called in HTMLImageElement::selectSourceURL(), and thus a box indicating a broken image is displayed during loading instead of the image displayed progressively. This is regression since https://codereview.chromium.org/1879793003. This CL makes the behavior of Case 1 and 2 consistent, by moving what is done in Case 1 to setImagePending(), and makes Case 1 and 2 both to call setImagePending(). This CL also adds a layout test to confirm that an image is displayed progressively when loaded as a main document. BUG=632495 Committed: https://crrev.com/576c7a048bfc30faf510eeda07f303fa9dd12898 Cr-Commit-Position: refs/heads/master@{#414008}

Patch Set 1 #

Patch Set 2 : Add comments #

Patch Set 3 : Add comments #

Patch Set 4 : Rebase (First Commit) #

Patch Set 5 : Rebase. #

Patch Set 6 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+51 lines, -30 lines) Patch
A + third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document.html View 1 2 2 chunks +10 lines, -3 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document-expected.png View 2 Binary file 0 comments Download
A + third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document-expected.txt View 2 0 chunks +-1 lines, --1 lines 0 comments Download
M third_party/WebKit/Source/core/loader/ImageLoader.h View 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/loader/ImageLoader.cpp View 1 2 3 4 3 chunks +37 lines, -28 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 23 (11 generated)
hiroshige
PTAL. I want to merge this into M-52 (stable).
4 years, 4 months ago (2016-08-04 15:24:49 UTC) #4
yhirano
lgtm
4 years, 4 months ago (2016-08-08 11:50:58 UTC) #6
hiroshige
japhet@, could you take a look?
4 years, 4 months ago (2016-08-10 13:52:06 UTC) #7
hiroshige
japhet@, friendly ping.
4 years, 4 months ago (2016-08-23 10:30:52 UTC) #8
Nate Chapin
lgtm
4 years, 4 months ago (2016-08-23 16:26:23 UTC) #9
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/2208073004/60001
4 years, 4 months ago (2016-08-23 19:00:51 UTC) #12
commit-bot: I haz the power
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_ng/builds/279331)
4 years, 4 months ago (2016-08-23 22:03:43 UTC) #14
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/2208073004/60001
4 years, 4 months ago (2016-08-24 04:25:34 UTC) #16
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 4 months ago (2016-08-24 07:45:43 UTC) #18
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/576c7a048bfc30faf510eeda07f303fa9dd12898 Cr-Commit-Position: refs/heads/master@{#414008}
4 years, 4 months ago (2016-08-24 07:47:35 UTC) #20
haraken
http/tests/images/png-partial-load-as-document.html has been leaking since the CL was landed. Would you take a look at ...
4 years, 4 months ago (2016-08-25 06:18:00 UTC) #21
johnme
4 years, 3 months ago (2016-08-25 12:32:23 UTC) #22
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2278953002/ by johnme@chromium.org.

The reason for reverting is: Reverting since
http/tests/images/png-partial-load-as-document.html consistently leaks on WebKit
Linux Leak and LeakExpectations says "Sheriff is expected to revert culprit CLs
instead of suppressing the leaks".

Powered by Google App Engine
This is Rietveld 408576698