Load 2x resources on demand

ui/gfx/image/image_skia.cc

Index: ui/gfx/image/image_skia.cc
diff --git a/ui/gfx/image/image_skia.cc b/ui/gfx/image/image_skia.cc
index 65f2d48741233911458e18986c57f6ea12c4d56c..7d5470dde32027d834bbc8ebb419e85f7f7bc7d3 100644
--- a/ui/gfx/image/image_skia.cc
+++ b/ui/gfx/image/image_skia.cc
@@ -10,6 +10,7 @@
 
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/threading/non_thread_safe.h"
 #include "ui/gfx/image/image_skia_operations.h"
 #include "ui/gfx/image/image_skia_source.h"
 #include "ui/gfx/rect.h"
@@ -48,11 +49,13 @@ class Matcher {
 // A helper class such that ImageSkia can be cheaply copied. ImageSkia holds a
 // refptr instance of ImageSkiaStorage, which in turn holds all of ImageSkia's
 // information.
-class ImageSkiaStorage : public base::RefCounted<ImageSkiaStorage> {
+class ImageSkiaStorage : public base::RefCounted<ImageSkiaStorage>,
+                         public base::NonThreadSafe {
  public:
   ImageSkiaStorage(ImageSkiaSource* source, const gfx::Size& size)
       : source_(source),
-        size_(size) {
+        size_(size),
+        read_only_(false) {
   }
 
   bool has_source() const { return source_.get() != NULL; }
@@ -61,6 +64,31 @@ class ImageSkiaStorage : public base::RefCounted<ImageSkiaStorage> {
 
   const gfx::Size& size() const { return size_; }
 
+  bool read_only() const { return read_only_; }
+
+  void DeleteSource() {
+    source_.reset();
+  }
+
+  void SetReadOnly() {
+    read_only_ = true;
+    DetachFromThread();
+  }
+
+  void DetachFromThread() {
+    base::NonThreadSafe::DetachFromThread();
+  }
+
+  // Checks if the current thread can safely modify the storage.
+  void AssertModify() {
+    CHECK(!read_only_ && CalledOnValidThread());
+  }
+
+  // Checks if the current thread can safely read the storage.
+  void AssertRead() {
+    CHECK(read_only_ || CalledOnValidThread());
+  }
+
   // Returns the iterator of the image rep whose density best matches
   // |scale_factor|. If the image for the |scale_factor| doesn't exist
   // in the storage and |storage| is set, it fetches new image by calling
@@ -96,6 +124,9 @@ class ImageSkiaStorage : public base::RefCounted<ImageSkiaStorage> {
     }
 
     if (fetch_new_image && source_.get()) {
+      DCHECK(CalledOnValidThread()) <<
+          "An ImageSkia with the source must be accessed by the same thread.";
+
       ImageSkiaRep image = source_->GetImageForScale(scale_factor);
 
       // If the source returned the new image, store it.
@@ -120,7 +151,10 @@ class ImageSkiaStorage : public base::RefCounted<ImageSkiaStorage> {
   }
 
  private:
-  ~ImageSkiaStorage() {
+  virtual ~ImageSkiaStorage() {
+    // We only care if the storage is modified by the same thread.
+    // Don't blow up even if someone else deleted the ImageSkia.
+    DetachFromThread();
   }
 
   // Vector of bitmaps and their associated scale factor.
@@ -131,6 +165,8 @@ class ImageSkiaStorage : public base::RefCounted<ImageSkiaStorage> {
   // Size of the image in DIP.
   const gfx::Size size_;
 
+  bool read_only_;
+
   friend class base::RefCounted<ImageSkiaStorage>;
 };
 
@@ -168,6 +204,23 @@ ImageSkia& ImageSkia::operator=(const SkBitmap& other) {
 ImageSkia::~ImageSkia() {
 }
 
+ImageSkia ImageSkia::DeepCopy() const {
+  ImageSkia copy;
+  if (isNull())
+    return copy;
+
+  std::vector<gfx::ImageSkiaRep>& reps = storage_->image_reps();
+  for (std::vector<gfx::ImageSkiaRep>::iterator iter = reps.begin();
+       iter != reps.end(); ++iter) {
+    copy.AddRepresentation(*iter);
+  }
+  // The copy has its own storage. Detach the copy from the current
+  // thread so that other thread can use this.
+  if (!copy.isNull())
+    copy.storage_->DetachFromThread();
+  return copy;
+}
+
 bool ImageSkia::BackedBySameObjectAs(const gfx::ImageSkia& other) const {
   return storage_.get() == other.storage_.get();
 }
@@ -175,15 +228,24 @@ bool ImageSkia::BackedBySameObjectAs(const gfx::ImageSkia& other) const {
 void ImageSkia::AddRepresentation(const ImageSkiaRep& image_rep) {
   DCHECK(!image_rep.is_null());
 
-  if (isNull())
+  // TODO(oshima): This method should be called |SetRepresentation|
+  // and replace the existing rep if there is already one with the
+  // same scale factor so that we can guarantee that a ImageSkia
+  // instance contians only one image rep per scale factor. This is
+  // not possible now as ImageLoadingTracker currently stores need
+  // this feature, but this needs to be fixed.
+  if (isNull()) {
     Init(image_rep);
-  else
+  } else {
+    storage_->AssertModify();

[sky, 2012/08/24 18:25:48]

One other thought, should we take a copy on write approach? That is, if
AddRepresentation is invoked we clone the storage? Seems like that would make
things less fragile.

[oshima, 2012/08/24 19:06:45]

Doing so in a thread safe way require locking and will make it more complicated
IMO. Another issue is that the current semantics of shallow
copy (which is used very often) is that the copy shares the storage,
and copy on write changes this semantics. I'm afraid that it will break
a lot of places.

     storage_->image_reps().push_back(image_rep);
+  }
 }
 
 void ImageSkia::RemoveRepresentation(ui::ScaleFactor scale_factor) {
   if (isNull())
     return;
+  storage_->AssertModify();
 
   ImageSkiaReps& image_reps = storage_->image_reps();
   ImageSkiaReps::iterator it =
@@ -196,6 +258,8 @@ bool ImageSkia::HasRepresentation(ui::ScaleFactor scale_factor) const {
   if (isNull())
     return false;
 
+  storage_->AssertRead();
+
   ImageSkiaReps::iterator it =
       storage_->FindRepresentation(scale_factor, false);
   return (it != storage_->image_reps().end() &&
@@ -207,6 +271,8 @@ const ImageSkiaRep& ImageSkia::GetRepresentation(
   if (isNull())
     return NullImageRep();
 
+  storage_->AssertRead();
+
   ImageSkiaReps::iterator it = storage_->FindRepresentation(scale_factor, true);
   if (it == storage_->image_reps().end())
     return NullImageRep();
@@ -214,6 +280,15 @@ const ImageSkiaRep& ImageSkia::GetRepresentation(
   return *it;
 }
 
+void ImageSkia::SetReadOnly() {
+  CHECK(storage_);
+  storage_->SetReadOnly();

[sky, 2012/08/24 18:25:48]

Should this invoke DeleteSource?

[oshima, 2012/08/24 19:06:45]

Maybe: I kept it separated because I wanted to make the image returned by
ResourceBundle read only,
but still allow source to generate the image. Bad use will be caught by DCHECK()
in ImageSkiaSource::FindRepresentation, and such code should use
EnsureRepsForSupportedScaleFactors & DeleteSource.

I now think that it's probably not intuitive and am thinking of introducing two
read only mode,
one that prevent modification by client (thus keep source),
and another one to make it thread safe (thus, generate images and remove
source).
What do you think?

+}
+
+bool ImageSkia::IsReadOnly() const {
+  return !storage_ || storage_->read_only();
+}
+
 #if defined(OS_MACOSX)
 
 std::vector<ImageSkiaRep> ImageSkia::GetRepresentations() const {
@@ -223,14 +298,9 @@ std::vector<ImageSkiaRep> ImageSkia::GetRepresentations() const {
   if (!storage_->has_source())
     return image_reps();
 
-  // Attempt to generate image reps for as many scale factors supported by
-  // this platform as possible.
-  // Do not build return array here because the mapping from scale factor to
-  // image rep is one to many in some cases.
-  std::vector<ui::ScaleFactor> supported_scale_factors =
-      ui::GetSupportedScaleFactors();
-  for (size_t i = 0; i < supported_scale_factors.size(); ++i)
-    storage_->FindRepresentation(supported_scale_factors[i], true);
+  storage_->AssertRead();
+
+  EnsureRepsForSupportedScaleFactors();
 
   return image_reps();
 }
@@ -253,6 +323,8 @@ std::vector<ImageSkiaRep> ImageSkia::image_reps() const {
   if (isNull())
     return std::vector<ImageSkiaRep>();
 
+  storage_->AssertRead();
+
   ImageSkiaReps internal_image_reps = storage_->image_reps();
   // Create list of image reps to return, skipping null image reps which were
   // added for caching purposes only.
@@ -266,6 +338,20 @@ std::vector<ImageSkiaRep> ImageSkia::image_reps() const {
   return image_reps;
 }
 
+void ImageSkia::DeleteSource() {
+  if (storage_)
+    storage_->DeleteSource();
+}
+
+void ImageSkia::EnsureRepsForSupportedScaleFactors() const {
+  if (storage_ && storage_->has_source()) {
+    std::vector<ui::ScaleFactor> supported_scale_factors =
+        ui::GetSupportedScaleFactors();
+    for (size_t i = 0; i < supported_scale_factors.size(); ++i)
+      storage_->FindRepresentation(supported_scale_factors[i], true);
+  }
+}
+
 void ImageSkia::Init(const ImageSkiaRep& image_rep) {
   // TODO(pkotwicz): The image should be null whenever image rep is null.
   if (image_rep.sk_bitmap().empty()) {