In Skia's ImageFrame, only set the isOpaque flag when the frame is complete
Created attachment 168712 [details] Patch
Comment on attachment 168712 [details] Patch Attachment 168712 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14292761
Looks like this is missing some .h changes? +abarth for his image decoder expertise.
Created attachment 168724 [details] Patch
forgot to upload change to ImageDecoder.h
Comment on attachment 168724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168724&action=review This looks reasonable to me, but I'll leave for Adam in case he has any comments. > Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:55 > + setHasAlpha(other.hasAlpha()); Is it important that you do the new special logic here? If so, it seems that m_status must be initialized before this is called. Perhaps you should add a comment to that effect.
> This looks reasonable to me, but I'll leave for Adam in case he has any comments. Looks reasonable to me too.
Created attachment 168731 [details] Patch
patch #3 adds comment about calling setHasAlpha after calling setStatus.
Comment on attachment 168731 [details] Patch OK (bots willing). r=me
Comment on attachment 168731 [details] Patch Rejecting attachment 168731 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/14291783
Created attachment 168776 [details] Patch
reloaded with OOPS. PTAL
Comment on attachment 168776 [details] Patch Once more, with feeling. r=me (Sadly, I think you might need the exclamation mark too).
Created attachment 168777 [details] Patch
OOPS!
Comment on attachment 168777 [details] Patch <insert Bill Murray here> r=me
Comment on attachment 168777 [details] Patch Clearing flags on attachment: 168777 Committed r131370: <http://trac.webkit.org/changeset/131370>
All reviewed patches have been landed. Closing bug.
(In reply to comment #19) > All reviewed patches have been landed. Closing bug. And landed by the time this got to me.
Comment on attachment 168777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168777&action=review > Source/WebCore/ChangeLog:12 > + Really don't understand these comments (lines 8-11); the tests are passing last I looked. > Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:74 > + m_hasAlpha = true; So if we returned and had to paint the image buffer here, would the opaque setting of m_bitmap be correct given you set m_hasAlpha = true here, viz., would m_bitmap.bitmap().isOpaque() return false at this time. It's not clear to me where the opaque value was initialized for the m_bitmap, but one of this function or the class constructor seems wrong.
Comment on attachment 168777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168777&action=review >> Source/WebCore/ChangeLog:12 >> + > > Really don't understand these comments (lines 8-11); the tests are passing last I looked. This is a chicken-egg problem. We have a hack in skia that works around this bug. If I remove that hack, DRT has tests that will fail, so I have to first fix the webkit code and then do the 2nd half of the fix in skia. If I did the changes in the other order, the tests would fail.. until I also landed the webkit change. >> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:74 >> + m_hasAlpha = true; > > So if we returned and had to paint the image buffer here, would the opaque setting of m_bitmap be correct given you set m_hasAlpha = true here, viz., would m_bitmap.bitmap().isOpaque() return false at this time. It's not clear to me where the opaque value was initialized for the m_bitmap, but one of this function or the class constructor seems wrong. We will *never* set skia's opaque bit until the image-decode statis FrameComplete, so yes, isOpaque will be false if we stopped right after zeroing, but didn't complete the decode. I guess we could call setHasAlpha(true) instead of setting this field directly. Might be clearer. I'm not sure that there is a real bug, but I'd be fine with making that change as a new CL.