901146 - Fix misleading comment in nsImageFrame.cpp

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Seth Fowler [:seth] [:s2h]]

(From bug 825771 comment 36:)

The current behavior doesn't make sense because BuildDisplayList will display the image if the first frame is partially decoded, but if it wasn't available when BuildDisplayList was called so that we end up calling DisplayAltFeedback when painting instead, then DisplayAltFeedback discovers that the first frame is now partially decoded, it won't actually paint it unless the first frame is completely decoded. Whether the image gets displayed before the first frame is fully decoded is therefore determined by a race on the timing of the call to BuildDisplayList.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Make nsImageFrame display the image in DisplayAltFeedback when it would in BuildDisplayList.

Proposed patch. This is a one-liner that makes us display the image in DisplayAltFeedback if we have STATUS_SIZE_AVAILABLE, rather than waiting for STATUS_FRAME_COMPLETE. This matches the BuildDisplayList code.

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=b3f5beb781a7

Comment 2

[Timothy Nikkel (:tnikkel)]

The only caller of DisplayAltFeedback is here http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsImageFrame.cpp#1064 which shows that aRequest here is either gIconLoad->mLoadingImage or gIconLoad->mBrokenImage, not the request of the image we are asked to display for this image frame. So it's a different request from the request in BuildDisplayList.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: Fix misleading comment in nsImageFrame.cpp.

Wow, good catch. Alright, let's update the comment at the top of that block to clarify what the situation is.

This also makes it clear that I don't need to touch that code at all in bug 825771, so I'll remove that portion of the patch on that bug. (And request review from you, just to make sure I don't let any other mistakes slip through!)

Comment 4

[Seth Fowler [:seth] [:s2h]]

Wow, this somehow got removed from my patch queue without getting pushed. Nine months ago.

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d71e43379733

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d71e43379733