Revert of Fix ImageLoader::m_hasPendingLoadEvent/m_imageComplete in ImageDocument (issue 2278953002 by johnme@chromium.org)

0 views
Skip to first unread message

joh...@chromium.org

unread,
Aug 25, 2016, 8:32:24 AM8/25/16
to jap...@chromium.org, yhi...@chromium.org, hiro...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, loading...@chromium.org, tyoshin...@chromium.org, jap...@chromium.org, gavinp...@chromium.org
Reviewers: Nate Chapin, yhirano, hiroshige
CL: https://codereview.chromium.org/2278953002/

Message:
Created Revert of Fix ImageLoader::m_hasPendingLoadEvent/m_imageComplete in
ImageDocument

Description:
Revert of Fix ImageLoader::m_hasPendingLoadEvent/m_imageComplete in
ImageDocument (patchset #4 id:60001 of
https://codereview.chromium.org/2208073004/ )

Reason for revert:
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"

Original issue's 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}

TBR=jap...@chromium.org,yhi...@chromium.org,hiro...@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=632495

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+27, -82 lines):
D third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document.html
D third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document-expected.png
D third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document-expected.txt
M third_party/WebKit/Source/core/loader/ImageLoader.h
M third_party/WebKit/Source/core/loader/ImageLoader.cpp


commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 25, 2016, 8:33:27 AM8/25/16
to joh...@chromium.org, jap...@chromium.org, yhi...@chromium.org, hiro...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, loading...@chromium.org, tyoshin...@chromium.org, jap...@chromium.org, gavinp...@chromium.org

hiro...@chromium.org

unread,
Aug 25, 2016, 9:41:48 AM8/25/16
to joh...@chromium.org, jap...@chromium.org, yhi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, loading...@chromium.org, tyoshin...@chromium.org, jap...@chromium.org, gavinp...@chromium.org
On 2016/08/25 12:33:17, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2278953002/1

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 25, 2016, 11:26:16 AM8/25/16
to joh...@chromium.org, jap...@chromium.org, yhi...@chromium.org, hiro...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, loading...@chromium.org, tyoshin...@chromium.org, jap...@chromium.org, gavinp...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 25, 2016, 11:30:40 AM8/25/16
to joh...@chromium.org, jap...@chromium.org, yhi...@chromium.org, hiro...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, loading...@chromium.org, tyoshin...@chromium.org, jap...@chromium.org, gavinp...@chromium.org
Committed patchset #1 (id:1)

https://codereview.chromium.org/2278953002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 25, 2016, 11:33:02 AM8/25/16
to joh...@chromium.org, jap...@chromium.org, yhi...@chromium.org, hiro...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, loading...@chromium.org, tyoshin...@chromium.org, jap...@chromium.org, gavinp...@chromium.org
Patchset 1 (id:??) landed as
https://crrev.com/8b3f5df1a65187e662fcf2fd5ead1ccb89226f31
Cr-Commit-Position: refs/heads/master@{#414441}

https://codereview.chromium.org/2278953002/

har...@chromium.org

unread,
Aug 25, 2016, 8:53:06 PM8/25/16
to joh...@chromium.org, jap...@chromium.org, yhi...@chromium.org, hiro...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, loading...@chromium.org, tyoshin...@chromium.org, jap...@chromium.org, gavinp...@chromium.org
Reply all
Reply to author
Forward
0 new messages