Save a bitmap copy when advancing to dependent GIF and WebP animation frames

third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp

Index: third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
diff --git a/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp b/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
index 530aee3d338b7529ad0aed1f37fa86912648c35f..02fc0739d8b2edec800eae076d67059dd9818594 100644
--- a/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
+++ b/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
@@ -95,8 +95,11 @@ void NotificationImageLoader::didFinishLoading(unsigned long resourceIdentifier,
             decoder->setData(m_data.get(), true /* allDataReceived */);
             // The |ImageFrame*| is owned by the decoder.
             ImageFrame* imageFrame = decoder->frameBufferAtIndex(0);
-            if (imageFrame) {
-                (*m_imageCallback)(imageFrame->bitmap());
+            DCHECK(decoder->failed() || imageFrame->getStatus() == ImageFrame::FrameComplete);

[Peter Kasting, 2016/08/30 06:59:40]

This check isn't safe if somehow failed() is false but |imageFrame| is null.

If that should never happen, I'd change this to:

    if (imageFrame && imageFrame->getStatus() == ImageFrame::FrameComplete) {
        ...
    } else {
        DCHECK(decoder->failed());
    }

[aleksandar.stojiljkovic, 2016/09/21 20:56:57]

Done. Reverted. As allDataReceived is set to true it is expected that
either it is complete or failed. Didn't see in code how it could be null.

+            if (imageFrame && imageFrame->getStatus() == ImageFrame::FrameComplete) {
+                SkBitmap bitmap = imageFrame->bitmap();
+                bitmap.setImmutable();

[Peter Kasting, 2016/08/30 06:59:40]

Why is setting the bitmap as immutable here important?  At the very least this
needs a comment.

This question basically applies to everywhere in this CL that does this.  I was
under the impression only people getting an SkImage needed to do this (to avoid
a copy), but we seem to be doing it many other places.

If this is because we're passing the bitmap to places that will turn it into an
image, can we change to passing an SkImage now?

[aleksandar.stojiljkovic, 2016/09/21 20:56:57]

That was my plan too, with all the cases where SkBitmap was ported to SkImage.
After this patch, only remaining case where we do this is with
WebImage(SkBitmap) construction:
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebIm...
and there is WebImage::getSkBitmap API that would need to be replaced with
SkImage variant.
The reason I left the SkBitmap/setImmutable used here is that it is WebKit
public/platform API and don't know the deprecation policy here / if anybody else
outside chromium codebase is using it.

[aleksandar.stojiljkovic, 2016/09/21 20:56:57]

Reverted this to original implementation - in ImageSkiaRep::ImageSkiaRep(const
SkBitmap& src, float scale) where this code leads to, bitmap is set to
immutable.
[1]
https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia_rep.cc?rcl=14744...

[Peter Kasting, 2016/09/21 21:54:49]

Blink is Chrome's fork, so we assume that we only want things Chrome is using.

So you should always go ahead and rip out things that you see Chromium code no
longer needing.

[aleksandar.stojiljkovic, 2016/09/22 09:41:19]

Checked in details - a lot of layout tests are using WebImage and the code needs
SkBitmap for direct access to pixels - so I left SkBitmap but there in the
WebImage::getSkBitmap API, and seems to be no need to call setImmutable - on
rare places where it is converted to SkImage it is already happening  on client
side of the call through [1]
https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia_rep.cc?rcl=14744...

+                (*m_imageCallback)(bitmap);
                 return;
             }
         }