Remove duplication of encoded image data

third_party/WebKit/Source/core/fetch/ImageResource.cpp

Index: third_party/WebKit/Source/core/fetch/ImageResource.cpp
diff --git a/third_party/WebKit/Source/core/fetch/ImageResource.cpp b/third_party/WebKit/Source/core/fetch/ImageResource.cpp
index 1214c2ca111d30ddcbf52936e152a59ed2f5f6fa..0aaab6ee31f5eb01df4e3f58227731ec7eefd540 100644
--- a/third_party/WebKit/Source/core/fetch/ImageResource.cpp
+++ b/third_party/WebKit/Source/core/fetch/ImageResource.cpp
@@ -81,7 +81,6 @@ ImageResource::ImageResource(blink::Image* image, const ResourceLoaderOptions& o
 ImageResource::~ImageResource()
 {
     WTF_LOG(Timers, "~ImageResource %p", this);
-    clearImage();
 }
 
 DEFINE_TRACE(ImageResource)
@@ -124,10 +123,7 @@ void ImageResource::addObserver(ImageResourceObserver* observer)
     if (isCacheValidator())
         return;
 
-    if (m_data && !m_image && !errorOccurred()) {
-        createImage();
-        m_image->setData(m_data, true);
-    }
+    DCHECK(!m_data || m_image);
 
     if (m_image && !m_image->isNull()) {
         observer->imageChanged(this);
@@ -190,18 +186,15 @@ bool ImageResource::isSafeToUnlock() const
 
 void ImageResource::destroyDecodedDataForFailedRevalidation()
 {
-    m_image = nullptr;
-    setDecodedSize(0);
+    m_image->destroyDecodedData();

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

I think we have to clear |m_image| here, because we have to re-create |m_image|
in failed revalidation -- and it means we lack tests for that case. I'll add a
test.

[hiroshige, 2016/06/29 08:20:52]

Created the tests. https://codereview.chromium.org/2101103004/

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

Done.

 }
 
 void ImageResource::destroyDecodedDataIfPossible()
 {
-    if (!hasClientsOrObservers() && !isLoading() && (!m_image || (m_image->hasOneRef() && m_image->isBitmapImage()))) {
-        m_image = nullptr;
-        setDecodedSize(0);
-    } else if (m_image && !errorOccurred()) {
+    if (!m_image)
+        return;
+    if ((!hasClientsOrObservers() && !isLoading() && m_image->hasOneRef() && m_image->isBitmapImage()) || !errorOccurred())

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

I'm not sure, but can we simplify this condition to just |!errorOccurred()|?

The condition |!hasClientsOrObservers() && !isLoading() && (!m_image
||m_image->hasOneRef() && m_image->isBitmapImage()))| looks like for disposing
|m_image|, and this CL makes |m_image| to be not disposal.

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

As we talked offline, let's keep this condition as it is. If we need
refactoring, we'll do it in another CL.

         m_image->destroyDecodedData();
-    }
 }
 
 void ImageResource::doResetAnimation()
@@ -226,6 +219,16 @@ void ImageResource::allClientsAndObserversRemoved()
     Resource::allClientsAndObserversRemoved();
 }
 
+PassRefPtr<SharedBuffer> ImageResource::resourceBuffer() const
+{
+    RefPtr<SharedBuffer> data = Resource::resourceBuffer();
+    if (data)
+        return data;

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

Nit: return data.release();

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

Done.

+    if (m_image)
+        return m_image->data();
+    return nullptr;
+}
+
 void ImageResource::appendData(const char* data, size_t length)
 {
     if (m_multipartParser) {
@@ -329,16 +332,16 @@ void ImageResource::notifyObservers(const IntRect* changeRect)
 void ImageResource::clear()
 {
     prune();
-    clearImage();

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

Perhaps, we need to call clearImage() here, because clear() is called when an
error occurs. Clearing |m_image| in case of error looks reasonable for me.

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

Done.

     m_data.clear();
     setEncodedSize(0);
 }
 
 inline void ImageResource::createImage()
 {
-    // Create the image if it doesn't yet exist.
+    // When |m_image| exists here, this is a previous part of a multipart
+    // response. Recreate it.
     if (m_image)
-        return;
+        m_image = nullptr;

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

It's not obvious to me how this relates to the rest of the change, and I don't
know enough about a multi-part response to judge whether this makes sense.

One place where this seems wrong to me if the following is true:
1. No m_multipartParser
2. We have already appended some data (but not all), so m_image is not NULL, and
neither is m_data.
3. More data is appended in appendData()

This will call updateImage(false).
This method will clear m_image.

This means we'll have to recreate m_image from scratch, and previously we would
have reused the old one.

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

I think it's better to call clearImage() explicitly when a multipart image needs
to clear |m_image|, than clearing |m_image| here implicitly depending on
|m_image|. (Generally, implicit assumptions on the status of Resources are
frequently wrong...)

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

Done.

 
     if (m_response.mimeType() == "image/svg+xml") {
         m_image = SVGImage::create(this);
@@ -347,17 +350,6 @@ inline void ImageResource::createImage()
     }
 }
 
-inline void ImageResource::clearImage()
-{
-    if (!m_image)
-        return;
-
-    // If our Image has an observer, it's always us so we need to clear the back pointer
-    // before dropping our reference.
-    m_image->clearImageObserver();

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

Why is it okay to drop this line? My first thought was that we're about to
delete m_image, which would clear the observer anyway, but some other class
could hold a reference to m_image (e.g. by calling getImage() [1]), and I assume
we still want to clear the observer?

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Ima...

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

Ah, you're right. I'll revert this function and callers. 

-    m_image.clear();
-}
-
 void ImageResource::updateImage(bool allDataReceived)
 {
     TRACE_EVENT0("blink", "ImageResource::updateImage");
@@ -370,7 +362,7 @@ void ImageResource::updateImage(bool allDataReceived)
     // Have the image update its data from its internal buffer.
     // It will not do anything now, but will delay decoding until
     // queried for info (like size or specific image frames).
-    if (m_image)
+    if (m_image && m_data)

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

If we have m_data, aren't we guaranteed to also have m_image, since
createImage() would have been called above?

Could you change this to:

if (m_data) {
  DCHECK(m_image);
  sizeAvailable = m_image->setData(m_data, allDataReceived);
}

?

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

Right, now m_image's lifetime should include m_data's. Done.

         sizeAvailable = m_image->setData(m_data, allDataReceived);
 
     // Go ahead and tell our observers to try to draw if we have either
@@ -390,13 +382,9 @@ void ImageResource::updateImage(bool allDataReceived)
         // (decoding delayed until painting) that seems hard.
         notifyObservers();
     }
-}
 
-void ImageResource::updateImageAndClearBuffer()
-{
-    clearImage();
-    updateImage(true);
-    m_data.clear();
+    if (allDataReceived)
+        m_data.clear();
 }
 
 void ImageResource::finish(double loadFinishTime)
@@ -404,7 +392,7 @@ void ImageResource::finish(double loadFinishTime)
     if (m_multipartParser) {
         m_multipartParser->finish();
         if (m_data)
-            updateImageAndClearBuffer();
+            updateImage(true);
     } else {
         updateImage(true);
     }
@@ -518,7 +506,7 @@ void ImageResource::reloadIfLoFi(ResourceFetcher* fetcher)
     if (isLoading())
         m_loader->cancel();
     else
-        updateImageAndClearBuffer();
+        updateImage(true);
     setStatus(NotStarted);
     fetcher->startLoad(this);
 }
@@ -540,7 +528,7 @@ void ImageResource::onePartInMultipartReceived(const ResourceResponse& response)
         m_multipartParsingState = MultipartParsingState::ParsingFirstPart;
         return;
     }
-    updateImageAndClearBuffer();
+    updateImage(true);
 
     if (m_multipartParsingState == MultipartParsingState::ParsingFirstPart) {
         m_multipartParsingState = MultipartParsingState::FinishedParsingFirstPart;