Remove duplication of encoded image data

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

 /*
     Copyright (C) 1998 Lars Knoll (knoll@mpi-hd.mpg.de)
     Copyright (C) 2001 Dirk Mueller (mueller@kde.org)
     Copyright (C) 2002 Waldo Bastian (bastian@kde.org)
     Copyright (C) 2006 Samuel Weinig (sam.weinig@gmail.com)
     Copyright (C) 2004, 2005, 2006, 2007 Apple Inc. All rights reserved.
 
     This library is free software; you can redistribute it and/or
     modify it under the terms of the GNU Library General Public
     License as published by the Free Software Foundation; either
@@ skipping 105 matching lines @@
 
 void ImageResource::addObserver(ImageResourceObserver* observer)
 {
     willAddClientOrObserver();
 
     m_observers.add(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);
     }
 
     if (isLoaded()) {
         observer->imageNotifyFinished(this);
         if (m_observers.contains(observer)) {
             m_finishedObservers.add(observer);
             m_observers.remove(observer);
@@ skipping 42 matching lines @@
 }
 
 bool ImageResource::isSafeToUnlock() const
 {
     // Note that |m_image| holds a reference to |m_data| in addition to the one held by the Resource parent class.
     return !m_image || (m_image->hasOneRef() && m_data->refCount() == 2);
 }
 
 void ImageResource::destroyDecodedDataForFailedRevalidation()
 {
-    m_image = nullptr;
-    setDecodedSize(0);
+    m_image->destroyDecodedData();
 }
 
 void ImageResource::destroyDecodedDataIfPossible()
 {
-    if (!hasClientsOrObservers() && !isLoading() && (!m_image || (m_image->hasOneRef() && m_image->isBitmapImage()))) {
-        m_image = nullptr;
-        setDecodedSize(0);
+    if (!hasClientsOrObservers() && !isLoading() && m_image && (m_image->hasOneRef() && m_image->isBitmapImage())) {

[scroggo_chromium, 2016/06/20 19:00:43]

Are the parentheses around

  m_image->hasOneRef() && m_image->isBitmapImage()

still necessary? It looks like you just have &&'s, so the order should not
matter?

[hajimehoshi, 2016/06/22 09:42:29]

Done.

+        m_image->destroyDecodedData();
     } else if (m_image && !errorOccurred()) {
         m_image->destroyDecodedData();

[scroggo_chromium, 2016/06/20 19:00:43]

This else statement appears to do the exact same thing as the if case. Should
this be one giant if?

[hajimehoshi, 2016/06/22 09:42:29]

Done.

     }
 }
 
 void ImageResource::allClientsAndObserversRemoved()
 {
     if (m_image && !errorOccurred())
         m_image->resetAnimation();
     if (m_multipartParser)
         m_multipartParser->cancel();
     Resource::allClientsAndObserversRemoved();
 }
 
+PassRefPtr<SharedBuffer> ImageResource::resourceBuffer() const
+{
+    RefPtr<SharedBuffer> data = Resource::resourceBuffer();
+    if (data)
+        return data;
+    if (m_image)
+        return m_image->data();
+    return nullptr;
+}
+
 void ImageResource::appendData(const char* data, size_t length)
 {
     if (m_multipartParser) {
         m_multipartParser->appendData(data, length);
     } else {
         Resource::appendData(data, length);
         updateImage(false);
     }
 }
 
@@ skipping 123 matching lines @@
     TRACE_EVENT0("blink", "ImageResource::updateImage");
 
     if (m_data)
         createImage();
 
     bool sizeAvailable = false;
 
     // 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)

[hajimehoshi, 2016/06/22 09:42:29]

Found that there is a case |m_data| is null (ImageResource::finish now clears
|m_data| as of this CL). I've changed the condition.

+    if (m_image) {
+        DCHECK(m_data);
         sizeAvailable = m_image->setData(m_data, allDataReceived);
+    }
 
     // Go ahead and tell our observers to try to draw if we have either
     // received all the data or the size is known. Each chunk from the
     // network causes observers to repaint, which will force that chunk
     // to decode.
     if (sizeAvailable || allDataReceived) {
         if (!m_image || m_image->isNull()) {
             if (!errorOccurred())
                 setStatus(DecodeError);
             clear();
             if (memoryCache()->contains(this))
                 memoryCache()->remove(this);
         }
 
         // It would be nice to only redraw the decoded band of the image, but with the current design
         // (decoding delayed until painting) that seems hard.
         notifyObservers();
     }
+
+    if (allDataReceived)
+        m_data.clear();
 }
 
 void ImageResource::updateImageAndClearBuffer()
 {
     clearImage();
     updateImage(true);
-    m_data.clear();
 }
 
 void ImageResource::finish(double loadFinishTime)
 {
     if (m_multipartParser) {
         m_multipartParser->finish();
         if (m_data)
             updateImageAndClearBuffer();
     } else {
         updateImage(true);
@@ skipping 98 matching lines @@
         m_image->setAnimationPolicy(newPolicy);
     }
 }
 
 void ImageResource::reloadIfLoFi(ResourceFetcher* fetcher)
 {
     if (!m_response.httpHeaderField("chrome-proxy").contains("q=low"))
         return;
     m_resourceRequest.setCachePolicy(WebCachePolicy::BypassingCache);
     m_resourceRequest.setLoFiState(WebURLRequest::LoFiOff);
-    if (isLoading())
+    if (isLoading()) {

[scroggo_chromium, 2016/06/20 19:00:43]

nit: My understanding of the style guide is that this brace should not be here.

[hajimehoshi, 2016/06/22 09:42:29]

Done.

         m_loader->cancel();
-    else
-        updateImageAndClearBuffer();
+    }
     setStatus(NotStarted);
     fetcher->startLoad(this);
 }
 
 void ImageResource::changedInRect(const blink::Image* image, const IntRect& rect)
 {
     if (!image || image != m_image)
         return;
     notifyObservers(&rect);
 }
@@ skipping 32 matching lines @@
     if (response().wasFetchedViaServiceWorker())
         return response().serviceWorkerResponseType() != WebServiceWorkerResponseTypeOpaque;
     if (!getImage()->currentFrameHasSingleSecurityOrigin())
         return false;
     if (passesAccessControlCheck(securityOrigin))
         return true;
     return !securityOrigin->taintsCanvas(response().url());
 }
 
 } // namespace blink