Remove duplication of encoded image data

third_party/WebKit/Source/platform/graphics/BitmapImage.cpp

Index: third_party/WebKit/Source/platform/graphics/BitmapImage.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp b/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp
index 17d815c5049c96f11bf0829b3427eecc716e092f..2664b5bc20938b063dc73568382a1c317958726c 100644
--- a/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp
+++ b/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp
@@ -118,6 +118,18 @@ void BitmapImage::destroyDecodedData()
     notifyMemoryChanged();
 }
 
+PassRefPtr<SharedBuffer> BitmapImage::data()

[hiroshige, 2016/06/29 07:04:06]

The name data() here (and ImageSource) looks like just an accessor, but actually
it re-creates SharedBuffer if needed. How about renaming it to a name that looks
like more costly?

[hajimehoshi, 2016/07/04 10:46:25]

This method is overriding Image::data. Do you mean we should rename Image::data
and its all usages?

+{
+    if (RefPtr<SharedBuffer> data = cachedData())
+        return data;

[f(malita), 2016/06/30 18:21:46]

Nit: return data.release();

[hajimehoshi, 2016/07/04 10:46:25]

Done.

+    return m_source.data();

[f(malita), 2016/06/30 18:21:46]

IIUC we clear the cached data as soon as we can, and m_source.data() allocates a
new SharedBuffer and makes a copy of the decoder data.

So while previously Resource::resourceBuffer() calls used to be always cheap,
now that's no longer the case for lazy-decoded images.  Is this going to impact
performance?

[hajimehoshi, 2016/07/04 10:46:25]

IIUC the situation where this function is limited: clipboard, serializing to
MHTML or WebGL. Among them, I think only WebGL can be a problem. I'll try to
check this with Telemetry.

+}
+
+PassRefPtr<SharedBuffer> BitmapImage::cachedData()

[scroggo_chromium, 2016/06/28 15:33:44]

A couple of thoughts on this method:

It is very simple, and we could almost inline it at the callsites, except that
you want to call it from a test.

I find the name confusing. It's not obvious to me which one is "cached" and
which one is not. I guess we call it cached because Image may throw away its
data?

[hiroshige, 2016/06/29 07:04:06]

FYI using "cached" prefix near loading code might be confusing; There are
prefixes "cached" in loader code (e.g. |cachedImage|), but most of them are just
due to historical reasons (CachedImage is the old class name of ImageResource)
and don't have much meaning.

[hajimehoshi, 2016/07/04 10:46:25]

Removed.

+{
+    return Image::data();
+}
+
 void BitmapImage::notifyMemoryChanged()
 {
     if (getImageObserver())
@@ -218,8 +230,16 @@ bool BitmapImage::dataChanged(bool allDataReceived)
 
     // Feed all the data we've seen so far to the image decoder.
     m_allDataReceived = allDataReceived;
-    ASSERT(data());
-    m_source.setData(*data(), allDataReceived);
+    if (cachedData()) {
+        m_source.setData(*cachedData(), allDataReceived);
+        if (m_source.hasData()) {
+            // Remove the cached data (encoded image data) from BitmapImage. Now
+            // there is an encoded image data in DefferedImageDecoder on a skia

[scroggo_chromium, 2016/06/28 15:33:44]

nit: "there is encoded image data" (no need for "an" here)

nit: *DeferredImageDecoder (one 'f', two 'r's)

nit: I don't think "on a skia side" adds useful information. Maybe it's more
clear to say something like "in DeferredImageDecoder's SkRWBuffer"?

[hajimehoshi, 2016/07/04 10:46:25]

Done.

+            // side, we won't need the same data in BitmapImage.
+            resetCachedData();

[scroggo_chromium, 2016/06/28 15:33:44]

Just to make sure I understand - this will drop the Image's reference to the
SharedBuffer (m_encodedImageData), but the actual SharedBuffer will only get
freed (thus saving memory) if the caller (e.g. ImageResource) also drops its
reference?

Is it possible the Image will drop its reference, resulting in having to copy
from the SkRWBuffer, in cases where the SharedBuffer is still around, so we
could have skipped copying from the SkRWBuffer without holding onto extra
memory? If so, would it be possible to only reset it if we know that that the
ImageResource *also* dropped its reference? (Or does this CL already do that and
I'm missing something?)

[hajimehoshi, 2016/07/04 10:46:25]

Yes.
Hmm, I think it's possible but it'd be very hard since we'd need hack
Resource::m_data without breaking other resource classes (hiroshige, do you
agree?) If we need this, I want to do it in another CL.

+            DCHECK(!cachedData());
+        }
+    }
 
     m_haveFrameCount = false;
     return isSizeAvailable();