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
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

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.