Bug 142825 - Refactor ImageLoader's setting of CachedImage
Summary: Refactor ImageLoader's setting of CachedImage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-18 08:15 PDT by Yoav Weiss
Modified: 2015-03-23 02:15 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.44 KB, patch)
2015-03-18 08:39 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (5.44 KB, patch)
2015-03-18 09:52 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (5.44 KB, patch)
2015-03-19 01:29 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yoav Weiss 2015-03-18 08:15:14 PDT
Refactor ImageLoader's setting of CachedImage
Comment 1 Yoav Weiss 2015-03-18 08:39:27 PDT
Created attachment 248932 [details]
Patch
Comment 2 Chris Dumez 2015-03-18 09:17:07 PDT
Comment on attachment 248932 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248932&action=review

r=me

> Source/WebCore/ChangeLog:11
> +        CachedImages and were not used as originalyl inteneded. That resulted in some dead code,

"originally intended"
Comment 3 Chris Dumez 2015-03-18 09:26:48 PDT
Comment on attachment 248932 [details]
Patch

Please fix the typos.
Comment 4 Yoav Weiss 2015-03-18 09:52:49 PDT
Created attachment 248941 [details]
Patch
Comment 5 Yoav Weiss 2015-03-18 09:55:23 PDT
Fixed the typo. Apologies for CQing it. I missed the comment due to conference WiFi slowness :/
Comment 6 Chris Dumez 2015-03-18 10:19:13 PDT
Comment on attachment 248941 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248941&action=review

> Source/WebCore/ChangeLog:11
> +        CachedImages and were not used as originally inteneded. That resulted in some dead code,

"intended". You fixed one out of 2 typos.
Comment 7 Yoav Weiss 2015-03-19 01:29:39 PDT
Created attachment 249025 [details]
Patch
Comment 8 Yoav Weiss 2015-03-19 01:30:23 PDT
Comment on attachment 249025 [details]
Patch

Sorry about that. Fixed the second one as well.
Comment 9 Darin Adler 2015-03-19 08:08:00 PDT
Comment on attachment 249025 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249025&action=review

> Source/WebCore/loader/ImageLoader.cpp:139
> +    if (oldImage) {

Can this instead be an early return? Is it important to call resetAnimation when m_image is null?
Comment 10 Yoav Weiss 2015-03-19 15:33:10 PDT
Comment on attachment 249025 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249025&action=review

>> Source/WebCore/loader/ImageLoader.cpp:139
>> +    if (oldImage) {
> 
> Can this instead be an early return? Is it important to call resetAnimation when m_image is null?

I'm not sure, TBH. Since resetAnimation is called on renderImageResource, which arrives from the element's renderer (so not necessarily null, and not related to m_image), it seems likely that we do need to reset any animations when setting the CachedImage to be null.
Comment 11 Yoav Weiss 2015-03-23 01:28:31 PDT
Comment on attachment 249025 [details]
Patch

Committing, but let me know if you think this can be further improved and I'll do that in a followup patch.
Comment 12 WebKit Commit Bot 2015-03-23 02:15:00 PDT
Comment on attachment 249025 [details]
Patch

Clearing flags on attachment: 249025

Committed r181849: <http://trac.webkit.org/changeset/181849>
Comment 13 WebKit Commit Bot 2015-03-23 02:15:05 PDT
All reviewed patches have been landed.  Closing bug.