Bug 142825 – Refactor ImageLoader's setting of CachedImage

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yoav Weiss]

Refactor ImageLoader's setting of CachedImage

[Yoav Weiss]

Created attachment 248932 [details]
Patch

[Chris Dumez]

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"

[Chris Dumez]

Comment on attachment 248932 [details]
Patch

Please fix the typos.

[Yoav Weiss]

Created attachment 248941 [details]
Patch

[Yoav Weiss]

Fixed the typo. Apologies for CQing it. I missed the comment due to conference WiFi slowness :/

[Chris Dumez]

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.

[Yoav Weiss]

Created attachment 249025 [details]
Patch

[Yoav Weiss]

Comment on attachment 249025 [details]
Patch

Sorry about that. Fixed the second one as well.

[Darin Adler]

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?

[Yoav Weiss]

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.

[Yoav Weiss]

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.

[WebKit Commit Bot]

Comment on attachment 249025 [details]
Patch

Clearing flags on attachment: 249025

Committed r181849: <http://trac.webkit.org/changeset/181849>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.