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);
-    }
-

[hajimehoshi, 2016/06/10 13:30:28]

I think it is safe to remove since this was introduced with purgable buffer, and
now an Image holds the encode image data just temporarily and can generate
encoded image data from SkRWBuffer on the fly.

[hiroshige, 2016/06/15 13:27:01]

So can we add
|CHECK(!m_data || m_image)|
here?

[hajimehoshi, 2016/06/17 09:25:15]

Done.

     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 243 matching lines @@
     m_data.clear();
 }
 
 void ImageResource::finish(double loadFinishTime)
 {
     if (m_multipartParser) {
         m_multipartParser->finish();
         if (m_data)
             updateImageAndClearBuffer();
     } else {
-        updateImage(true);
+        updateImageAndClearBuffer();

[f(malita), 2016/06/10 14:26:43]

My understanding of the current impl is that image resources (resources in
general) are cached in discardable memory for reuse.

What are the caching/data sharing implications of this change for

a) multiple image instances with the same source

b) same-source images with non-overlapping lifespans - say across page reload or
navigation.

If I understand this CL correctly, we're now moving the data to the image
decoder at the expense of the resource cache.

[hajimehoshi, 2016/06/13 09:10:47]

I think this doesn't happen with this CL.
ImageResource has an blink::Image as a RefPtr and the Image should be owned by
another object in this case.
Did you mean the resource cache is ResourceFetcher::cachedResource
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Res...
?

I think this CL doesn't affect the cache resource management. So, there is no
performance regression.

     }
     Resource::finish(loadFinishTime);
 }
 
 void ImageResource::error(const ResourceError& error)
 {
     if (m_multipartParser)
         m_multipartParser->cancel();
     clear();
     Resource::error(error);
@@ skipping 88 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()) {
         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