[ImageResource 4] Split ImageResource into Resource and Image parts

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

Index: third_party/WebKit/Source/core/fetch/ImageResource.h
diff --git a/third_party/WebKit/Source/core/fetch/ImageResource.h b/third_party/WebKit/Source/core/fetch/ImageResource.h
index 2d2b657db0bf936115f91bebc31fa7911502798c..f28b46eea73b69e73c84c22c601f033610d89870 100644
--- a/third_party/WebKit/Source/core/fetch/ImageResource.h
+++ b/third_party/WebKit/Source/core/fetch/ImageResource.h
@@ -26,87 +26,42 @@
 #include "core/CoreExport.h"
 #include "core/fetch/MultipartImageResourceParser.h"
 #include "core/fetch/Resource.h"

[yhirano, 2016/12/05 09:02:03]

+platform/Timer.h
+platform/heap/Handle.h

[hiroshige, 2016/12/06 09:32:50]

Done.

-#include "platform/geometry/IntRect.h"
-#include "platform/geometry/IntSizeHash.h"
-#include "platform/geometry/LayoutSize.h"
-#include "platform/graphics/Image.h"
-#include "platform/graphics/ImageObserver.h"
-#include "platform/graphics/ImageOrientation.h"
-#include "wtf/HashMap.h"
 #include <memory>
 
 namespace blink {
 
 class FetchRequest;
-class ImageResourceObserver;
-class MemoryCache;
 class ResourceClient;
 class ResourceFetcher;
 class SecurityOrigin;
+class ImageResourceContent;

[yhirano, 2016/12/05 09:02:03]

alphabetical order?

[hiroshige, 2016/12/06 09:32:51]

Done.

 
-// ImageResource class represents an image type resource.
+// ImageResource is the resource-related part of image loading.

[kouhei (in TOK), 2016/12/05 07:13:06]

ImageResource implements blink::Resource interface for loading images.

[hiroshige, 2016/12/05 09:11:19]

Done.

+// Image-related things (blink::Image and ImageResourceObserver) are handled by
+// ImageResourceContent.
+// Most users should use ImageResourceContent instead of ImageResource.
+// https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8bE/edit?usp=sharing
 //
-// As for the lifetimes of m_image and m_data, see this document:
+// As for the lifetimes of ImageResourceContent::m_image and m_data, see this
+// document:
 // https://docs.google.com/document/d/1v0yTAZ6wkqX2U_M6BNIGUJpM1s0TIw1VsqpxoL7aciY/edit?usp=sharing
 class CORE_EXPORT ImageResource final
     : public Resource,
-      public ImageObserver,
       public MultipartImageResourceParser::Client {
-  friend class MemoryCache;
   USING_GARBAGE_COLLECTED_MIXIN(ImageResource);
 
  public:
   using ClientType = ResourceClient;
 
+  // Use ImageResourceContent::fetch() unless ImageResource is required.

[kouhei (in TOK), 2016/12/05 07:13:06]

Can we make this private && friend ImageResourceContent?

[hiroshige, 2016/12/05 09:11:19]

Currently No, because this is used from e.g. DocumentLoader.
I'd like to clean up related code so that this can be private in the near
future, but not in this CL.

[kouhei (in TOK), 2016/12/05 09:22:05]

Optional: Please consider adding TODO(hiroshige) to make this private.

[hiroshige, 2016/12/06 09:32:50]

Done.

   static ImageResource* fetch(FetchRequest&, ResourceFetcher*);
 
-  static ImageResource* create(blink::Image* image) {
-    return new ImageResource(image, ResourceLoaderOptions());
-  }
-
-  static ImageResource* create(const ResourceRequest& request) {
-    return new ImageResource(request, ResourceLoaderOptions(), false);
-  }
+  // TODO(hiroshige): Make create() test-only by refactoring ImageDocument.
+  static ImageResource* create(const ResourceRequest&);
 
   ~ImageResource() override;
 
-  blink::Image*
-  getImage();  // Returns the nullImage() if the image is not available yet.
-  bool hasImage() const { return m_image.get(); }
-
-  static std::pair<blink::Image*, float> brokenImage(
-      float deviceScaleFactor);  // Returns an image and the image's resolution
-                                 // scale factor.
-  bool willPaintBrokenImage() const;
-
-  bool usesImageContainerSize() const;
-  bool imageHasRelativeSize() const;
-  // The device pixel ratio we got from the server for this image, or 1.0.
-  float devicePixelRatioHeaderValue() const {
-    return m_devicePixelRatioHeaderValue;
-  }
-  bool hasDevicePixelRatioHeaderValue() const {
-    return m_hasDevicePixelRatioHeaderValue;
-  }
-
-  enum SizeType {
-    // Report the intrinsic size.
-    IntrinsicSize,
-
-    // Report the intrinsic size corrected to account for image density.
-    IntrinsicCorrectedToDPR,
-  };
-
-  // This method takes a zoom multiplier that can be used to increase the
-  // natural size of the image by the zoom.
-  LayoutSize imageSize(
-      RespectImageOrientationEnum shouldRespectImageOrientation,
-      float multiplier,
-      SizeType = IntrinsicSize);
-
-  bool isAccessAllowed(SecurityOrigin*);
-
-  void updateImageAnimationPolicy();
+  ImageResourceContent* getContent() const;
 
   enum class ReloadCachePolicy {
     UseExistingPolicy = 0,  // Don't modify the request's cache policy.
@@ -122,9 +77,6 @@ class CORE_EXPORT ImageResource final
 
   void didAddClient(ResourceClient*) override;
 
-  void addObserver(ImageResourceObserver*);
-  void removeObserver(ImageResourceObserver*);
-
   ResourcePriority priorityFromObservers() override;
 
   void allClientsAndObserversRemoved() override;
@@ -141,13 +93,6 @@ class CORE_EXPORT ImageResource final
 
   bool isImage() const override { return true; }
 
-  // ImageObserver
-  void decodedSizeChangedTo(const blink::Image*, size_t newSize) override;
-
-  bool shouldPauseAnimation(const blink::Image*) override;
-  void animationAdvanced(const blink::Image*) override;
-  void changedInRect(const blink::Image*, const IntRect&) override;
-
   // MultipartImageResourceParser::Client
   void onePartInMultipartReceived(const ResourceResponse&) final;
   void multipartDataReceived(const char*, size_t) final;
@@ -159,14 +104,13 @@ class CORE_EXPORT ImageResource final
     return m_isPlaceholder && willPaintBrokenImage();
   }
 
-  void setNotRefetchableDataFromDiskCache() {
-    m_isRefetchableDataFromDiskCache = false;
-  }
-
   DECLARE_VIRTUAL_TRACE();
 
  private:
-  explicit ImageResource(blink::Image*, const ResourceLoaderOptions&);
+  // Only for ImageResourceInfoImpl.

[yhirano, 2016/12/05 09:02:03]

Can you move these functions declarations below the constructor?

[hiroshige, 2016/12/06 09:32:50]

Done.

+  void decodeError(bool allDataReceived);
+  bool isAccessAllowed(SecurityOrigin*,
+                       bool doesCurrentFrameHasSingleSecurityOrigin) const;
 
   enum class MultipartParsingState : uint8_t {
     WaitingForFirstPart,
@@ -174,48 +118,35 @@ class CORE_EXPORT ImageResource final
     FinishedParsingFirstPart,
   };
 
+  class ImageResourceInfoImpl;
   class ImageResourceFactory;
 
   ImageResource(const ResourceRequest&,
                 const ResourceLoaderOptions&,
+                ImageResourceContent*,
                 bool isPlaceholder);
 
-  bool hasClientsOrObservers() const override {
-    return Resource::hasClientsOrObservers() || !m_observers.isEmpty() ||
-           !m_finishedObservers.isEmpty();
-  }
-  void clear();
+  bool hasClientsOrObservers() const override;
 
-  void createImage();
-  void updateImage(bool allDataReceived);
   void updateImageAndClearBuffer();
-  void clearImage();
-  enum NotifyFinishOption { ShouldNotifyFinish, DoNotNotifyFinish };
-  // If not null, changeRect is the changed part of the image.
-  void notifyObservers(NotifyFinishOption, const IntRect* changeRect = nullptr);
-
-  void ensureImage();
 
   void checkNotify() override;
-  void notifyObserversInternal();
-  void markObserverFinished(ImageResourceObserver*);
-
-  void doResetAnimation();
 
   void destroyDecodedDataIfPossible() override;
   void destroyDecodedDataForFailedRevalidation() override;
 
   void flushImageIfNeeded(TimerBase*);
 
+  bool willPaintBrokenImage() const;
+
+  Member<ImageResourceContent> m_content;

[yhirano, 2016/12/05 09:02:03]

[optional] const Member<ImageResourceContent> m_content;

[hiroshige, 2016/12/06 09:32:51]

I'll keep |m_content| non-const because it will be non-const soon after this CL.

+
   float m_devicePixelRatioHeaderValue;

[yhirano, 2016/12/05 09:02:03]

Is it desirable to move this member variable to ImageResourceContent and update
the value in ImageResource::responseReceived?

[hiroshige, 2016/12/06 09:32:50]

Moving |m_devicePixelRatioHeaderValue| to ImageResourceContent and make
ImageResource to update it is desirable, because it is more natural (the members
looks image-related) and removes the corresponding interfaces in
ImageResourceInfo.

However, I'd like to update it in updateImage(), not in a new
ImageResourceConent method called in responseReceived(), because I'd like to
make updateImage() the single point of state change of ImageResourceContent
caused by ImageResource (we don't have updateImage() call in
responseReceived()), to simplify timing constraints between ImageResource and
its content.
Older Patch Set (e.g. Patch Set 7) does this: updateImage() has
|devicePixelRatioHeaderValue| argument, and ImageResourceContent updates its
|m_devicePixelRatioHeaderValue| in updateImage().
I felt it is over-complex at this time, but I'll re-apply that change if it
looks good. (Added a TODO anyway in the latest PatchSet).

 
   Member<MultipartImageResourceParser> m_multipartParser;
-  RefPtr<blink::Image> m_image;
   MultipartParsingState m_multipartParsingState =
       MultipartParsingState::WaitingForFirstPart;
   bool m_hasDevicePixelRatioHeaderValue;

[yhirano, 2016/12/05 09:02:03]

ditto

[hiroshige, 2016/12/06 09:32:50]

ditto.

-  HashCountedSet<ImageResourceObserver*> m_observers;
-  HashCountedSet<ImageResourceObserver*> m_finishedObservers;
 
   // Indicates if the ImageResource is currently scheduling a reload, e.g.
   // because reloadIfLoFi() was called.
@@ -227,11 +158,6 @@ class CORE_EXPORT ImageResource final
 
   Timer<ImageResource> m_flushTimer;
   double m_lastFlushTime = 0.;
-  Image::SizeAvailability m_sizeAvailable = Image::SizeUnavailable;
-
-  // Indicates if this resource's encoded image data can be purged and refetched
-  // from disk cache to save memory usage. See crbug/664437.
-  bool m_isRefetchableDataFromDiskCache;
 };
 
 DEFINE_RESOURCE_TYPE_CASTS(Image);