chromeos: Fix pixelated icons in app list and launcher (part 2)

chrome/browser/extensions/image_loading_tracker.cc

Index: chrome/browser/extensions/image_loading_tracker.cc
diff --git a/chrome/browser/extensions/image_loading_tracker.cc b/chrome/browser/extensions/image_loading_tracker.cc
index ea84c34fe5dc55d490b8af4ea2a5971c38ecc3e1..7f81a33eb16c166fc4174730c23c2e0312d11b70 100644
--- a/chrome/browser/extensions/image_loading_tracker.cc
+++ b/chrome/browser/extensions/image_loading_tracker.cc
@@ -21,27 +21,60 @@
 #include "skia/ext/image_operations.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/image/image.h"
-#include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_skia_rep.h"
+#include "ui/gfx/image/image_skia_source.h"
+#include "ui/gfx/screen.h"
 #include "webkit/glue/image_decoder.h"
 
 using content::BrowserThread;
 using extensions::Extension;
 
+namespace {
+
+struct ComponentExtensionResource {
+  const char* extension_id;
+  const int resource_id;
+} const kSpecialComponentExtensionResources[] = {

[pkotwicz, 2012/07/19 01:37:45]

Nit: New line after end of struct definition

[xiyuan, 2012/07/20 15:00:13]

Done.

+  { extension_misc::kWebStoreAppId, IDR_WEBSTORE_ICON },
+  { extension_misc::kWebStoreAppId, IDR_PRODUCT_LOGO_128 },
+};
+
+// Finds special component extension resource id for given extension id.
+bool FindSpecialExtensionResourceId(const std::string& extension_id,
+                                    int* out_resource_id) {
+  for (size_t i = 0; i < arraysize(kSpecialComponentExtensionResources); ++i) {
+    if (extension_id == kSpecialComponentExtensionResources[i].extension_id) {
+      if (out_resource_id)
+        *out_resource_id = kSpecialComponentExtensionResources[i].resource_id;
+      return true;
+    }
+  }
+
+  return false;
+}
+
+}  // namespace
+
 ////////////////////////////////////////////////////////////////////////////////
 // ImageLoadingTracker::Observer
 
 ImageLoadingTracker::Observer::~Observer() {}
 
 ////////////////////////////////////////////////////////////////////////////////
-// ImageLoadingTracker::ImageInfo
+// ImageLoadingTracker::ImageRepInfo
 
-ImageLoadingTracker::ImageInfo::ImageInfo(
-    const ExtensionResource& resource, gfx::Size max_size)
-    : resource(resource), max_size(max_size) {
+ImageLoadingTracker::ImageRepInfo::ImageRepInfo(
+    const ExtensionResource& resource,
+    ResizeMethod resize_method,
+    const gfx::Size& desired_size,
+    ui::ScaleFactor scale_factor)
+    : resource(resource),
+      resize_method(resize_method),
+      desired_size(desired_size),
+      scale_factor(scale_factor) {
 }
 
-ImageLoadingTracker::ImageInfo::~ImageInfo() {
+ImageLoadingTracker::ImageRepInfo::~ImageRepInfo() {
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -49,6 +82,9 @@ ImageLoadingTracker::ImageInfo::~ImageInfo() {
 
 ImageLoadingTracker::PendingLoadInfo::PendingLoadInfo()
   : extension(NULL),
+    resource_size_in_dip(ExtensionIconSet::EXTENSION_ICON_INVALID),
+    resource_match_type(ExtensionIconSet::MATCH_BIGGER),
+    image_source(NULL),
     cache(CACHE),
     pending_count(0) {
 }
@@ -58,8 +94,8 @@ ImageLoadingTracker::PendingLoadInfo::~PendingLoadInfo() {}
 ////////////////////////////////////////////////////////////////////////////////
 // ImageLoadingTracker::ImageLoader
 
-// A RefCounted class for loading images on the File thread and reporting back
-// on the UI thread.
+// A RefCounted class for loading bitmaps/image reps on the File thread and
+// reporting back on the UI thread.
 class ImageLoadingTracker::ImageLoader
     : public base::RefCountedThreadSafe<ImageLoader> {
  public:
@@ -76,106 +112,98 @@ class ImageLoadingTracker::ImageLoader
   }
 
   // Instructs the loader to load a task on the File thread.
-  void LoadImage(const ExtensionResource& resource,
-                 const gfx::Size& max_size,
-                 int id) {
+  void LoadImage(const ImageRepInfo& image_info, int id) {
     DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::FILE));
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
-        base::Bind(&ImageLoader::LoadOnFileThread, this, resource,
-                   max_size, id));
+        base::Bind(&ImageLoader::LoadOnFileThread, this, image_info, id));
   }
 
-  void LoadOnFileThread(const ExtensionResource& resource,
-                        const gfx::Size& max_size,
-                        int id) {
+  void LoadOnFileThread(const ImageRepInfo& image_info, int id) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
     // Read the file from disk.
     std::string file_contents;
-    FilePath path = resource.GetFilePath();
+    FilePath path = image_info.resource.GetFilePath();
     if (path.empty() || !file_util::ReadFileToString(path, &file_contents)) {
-      ReportBack(NULL, resource, gfx::Size(), id);
+      ReportBack(NULL, image_info, gfx::Size(), id);
       return;
     }
 
-    // Decode the image using WebKit's image decoder.
+    // Decode the bitmap using WebKit's image decoder.
     const unsigned char* data =
         reinterpret_cast<const unsigned char*>(file_contents.data());
     webkit_glue::ImageDecoder decoder;
     scoped_ptr<SkBitmap> decoded(new SkBitmap());
-    // Note: This class only decodes images from extension resources. Chrome
+    // Note: This class only decodes bitmaps from extension resources. Chrome
     // doesn't (for security reasons) directly load extension resources provided
     // by the extension author, but instead decodes them in a separate
     // locked-down utility process. Only if the decoding succeeds is the image
     // saved from memory to disk and subsequently used in the Chrome UI.
-    // Chrome is therefore decoding images here that were generated by Chrome.
+    // Chrome is therefore decoding bitmaps here that were generated by Chrome.
     *decoded = decoder.Decode(data, file_contents.length());
     if (decoded->empty()) {
-      ReportBack(NULL, resource, gfx::Size(), id);
+      ReportBack(NULL, image_info, gfx::Size(), id);
       return;  // Unable to decode.
     }
 
     gfx::Size original_size(decoded->width(), decoded->height());
 
-    if (decoded->width() > max_size.width() ||
-        decoded->height() > max_size.height()) {
+    if (image_info.resize_method == ImageRepInfo::ALWAYS_RESIZE ||
+        decoded->width() > image_info.desired_size.width() ||
+        decoded->height() > image_info.desired_size.height()) {
       // The bitmap is too big, re-sample.
       *decoded = skia::ImageOperations::Resize(
           *decoded, skia::ImageOperations::RESIZE_LANCZOS3,
-          max_size.width(), max_size.height());
+          image_info.desired_size.width(), image_info.desired_size.height());
     }
 
-    ReportBack(decoded.release(), resource, original_size, id);
+    ReportBack(decoded.release(), image_info, original_size, id);
   }
 
   // Instructs the loader to load a resource on the File thread.
-  void LoadResource(const ExtensionResource& resource,
-                    const gfx::Size& max_size,
-                    int id,
-                    int resource_id) {
+  void LoadResource(const ImageRepInfo& image_info, int id, int resource_id) {
     DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::FILE));
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
-        base::Bind(&ImageLoader::LoadResourceOnFileThread, this, resource,
-                   max_size, id, resource_id));
+        base::Bind(&ImageLoader::LoadResourceOnFileThread, this, image_info,
+                   id, resource_id));
   }
 
-  void LoadResourceOnFileThread(const ExtensionResource& resource,
-                                const gfx::Size& max_size,
+  void LoadResourceOnFileThread(const ImageRepInfo& image_info,
                                 int id,
                                 int resource_id) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-    SkBitmap* image = ExtensionIconSource::LoadImageByResourceId(
+    SkBitmap* bitmap = ExtensionIconSource::LoadImageByResourceId(
         resource_id);
-    ReportBack(image, resource, max_size, id);
+    ReportBack(bitmap, image_info, image_info.desired_size, id);
   }
 
-  void ReportBack(SkBitmap* image, const ExtensionResource& resource,
+  void ReportBack(SkBitmap* bitmap, const ImageRepInfo& image_info,
                   const gfx::Size& original_size, int id) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
     BrowserThread::PostTask(
         callback_thread_id_, FROM_HERE,
         base::Bind(&ImageLoader::ReportOnUIThread, this,
-                   image, resource, original_size, id));
+                   bitmap, image_info, original_size, id));
   }
 
-  void ReportOnUIThread(SkBitmap* image, const ExtensionResource& resource,
+  void ReportOnUIThread(SkBitmap* bitmap, const ImageRepInfo& image_info,
                         const gfx::Size& original_size, int id) {
     DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
     if (tracker_)
-      tracker_->OnImageLoaded(image, resource, original_size, id, true);
+      tracker_->OnBitmapLoaded(bitmap, image_info, original_size, id, true);
 
-    delete image;
+    delete bitmap;
   }
 
  private:
   friend class base::RefCountedThreadSafe<ImageLoader>;
   ~ImageLoader() {}
 
-  // The tracker we are loading the image for. If NULL, it means the tracker is
+  // The tracker we are loading the bitmap for. If NULL, it means the tracker is
   // no longer interested in the reply.
   ImageLoadingTracker* tracker_;
 
@@ -186,6 +214,53 @@ class ImageLoadingTracker::ImageLoader
 };
 
 ////////////////////////////////////////////////////////////////////////////////
+// ImageLoadingTracker::ImageSource
+
+// An ImageSkiaSource to load bitmap/image rep for additional scale factors.
+class ImageLoadingTracker::ImageSource : public gfx::ImageSkiaSource {
+ public:
+  ImageSource(ImageLoadingTracker* tracker, int id);
+  virtual ~ImageSource();
+
+  void StopTracking();
+
+  // gfx::ImageSkiaSource overrides:
+  virtual gfx::ImageSkiaRep GetImageForScale(
+      ui::ScaleFactor scale_factor) OVERRIDE;
+
+ private:
+  ImageLoadingTracker* tracker_;
+  int id_;
+
+  DISALLOW_COPY_AND_ASSIGN(ImageSource);
+};
+
+ImageLoadingTracker::ImageSource::ImageSource(ImageLoadingTracker* tracker,
+                                              int id)
+    : tracker_(tracker),
+      id_(id) {
+}
+
+ImageLoadingTracker::ImageSource::~ImageSource() {
+}
+
+void ImageLoadingTracker::ImageSource::StopTracking() {
+  tracker_ = NULL;
+}
+
+gfx::ImageSkiaRep ImageLoadingTracker::ImageSource::GetImageForScale(
+    ui::ScaleFactor scale_factor) {
+  // Asks tracker to load new bitmap/image rep for |scale_factor|.
+  if (tracker_)
+    tracker_->LoadImageForScaleFactor(id_, scale_factor);
+
+  // Returns an empty representation for |scale_factor| here. If |tracker_| is
+  // valid, it loads the bitmap asynchronously. When loading is done, it updates
+  // existing ImageSkia and notifies its observer.
+  return gfx::ImageSkiaRep();
+}
+
+////////////////////////////////////////////////////////////////////////////////
 // ImageLoadingTracker
 
 ImageLoadingTracker::ImageLoadingTracker(Observer* observer)
@@ -200,59 +275,127 @@ ImageLoadingTracker::~ImageLoadingTracker() {
   // any valid image load tasks have been posted.
   if (loader_)
     loader_->StopTracking();
+
+  for (LoadMap::iterator it = load_map_.begin(); it != load_map_.end(); ++it) {
+    if (it->second.image_source)
+      it->second.image_source->StopTracking();
+  }
 }
 
 void ImageLoadingTracker::LoadImage(const Extension* extension,
                                     const ExtensionResource& resource,
                                     const gfx::Size& max_size,
                                     CacheParam cache) {
-  std::vector<ImageInfo> info_list;
-  info_list.push_back(ImageInfo(resource, max_size));
+  std::vector<ImageRepInfo> info_list;
+  info_list.push_back(ImageRepInfo(resource,
+                                   ImageRepInfo::RESIZE_WHEN_LARGER,
+                                   max_size,
+                                   ui::SCALE_FACTOR_100P));
   LoadImages(extension, info_list, cache);
 }
 
+void ImageLoadingTracker::LoadImageSkia(
+    const Extension* extension,
+    int resource_size,
+    ExtensionIconSet::MatchType resource_match_type,
+    const gfx::Size& desired_size,
+    CacheParam cache) {
+  PendingLoadInfo load_info;
+  load_info.extension = extension;
+  load_info.extension_id = extension->id();
+  load_info.cache = cache;
+
+  load_info.resource_size_in_dip = resource_size;
+  load_info.resource_match_type = resource_match_type;
+  load_info.desired_size_in_dip = desired_size;
+
+  const bool can_load = CanLoadImage(load_info);

[pkotwicz, 2012/07/19 01:37:45]

Nit: Why don't you call OnImageLoaded(gfx::Image(), load_info.extension_id, id)
here immediately if can_load is false and return?

[xiyuan, 2012/07/20 15:00:13]

Done.

+
+  int id = next_id_++;
+
+  if (can_load) {
+    load_info.image_source = new ImageSource(this, id);
+    load_info.image_skia = gfx::ImageSkia(load_info.image_source,
+                                          load_info.desired_size_in_dip);
+  }
+
+  load_map_[id] = load_info;
+
+  // Notify |observer_| with an image that has ImageSource. Relevant bitmap
+  // resources  would be loaded when ImageSource is asked to provide a
+  // representation for certain scale factor.
+  gfx::Image image;
+  if (can_load)
+    image = gfx::Image(load_info.image_skia);
+  observer_->OnImageLoaded(image, load_info.extension_id, id);
+}
+
 void ImageLoadingTracker::LoadImages(const Extension* extension,
-                                     const std::vector<ImageInfo>& info_list,
+                                     const std::vector<ImageRepInfo>& info_list,
                                      CacheParam cache) {
   PendingLoadInfo load_info;
   load_info.extension = extension;
-  load_info.cache = cache;
   load_info.extension_id = extension->id();
-  load_info.pending_count = info_list.size();
+  load_info.cache = cache;
+
   int id = next_id_++;
   load_map_[id] = load_info;
 
-  for (std::vector<ImageInfo>::const_iterator it = info_list.begin();
+  DoLoadImage(id, info_list);
+}
+
+bool ImageLoadingTracker::CanLoadImage(const PendingLoadInfo& load_info) {

[pkotwicz, 2012/07/19 01:37:45]

Can you rename this to CanLoadImageSkia as this is specific to loading to
LoadImageSkia?

[xiyuan, 2012/07/20 15:00:13]

Done.

+  if (FindSpecialExtensionResourceId(load_info.extension_id, NULL))
+    return true;
+
+  // Check if 1x resource exists.
+  const int resource_size_in_pixel = load_info.resource_size_in_dip;
+  ExtensionResource resource = load_info.extension->GetIconResource(
+      resource_size_in_pixel,
+      load_info.resource_match_type);
+  return !resource.relative_path().empty();
+}
+
+void ImageLoadingTracker::DoLoadImage(
+    int id,
+    const std::vector<ImageRepInfo>& info_list) {
+  LoadMap::iterator load_map_it = load_map_.find(id);
+  DCHECK(load_map_it != load_map_.end());
+
+  PendingLoadInfo* load_info = &load_map_it->second;
+
+  // Count |info_list| requests as additional requests to existing load
+  // requests to handle the case LoadImageForScaleFactor is called before
+  // previous pending loads are finished.
+  load_info->pending_count += info_list.size();
+
+  const extensions::Extension* extension = load_info->extension;
+  for (std::vector<ImageRepInfo>::const_iterator it = info_list.begin();
        it != info_list.end(); ++it) {
+    int resource_id = -1;
+
     // Load resources for special component extensions.
-    if (load_info.extension_id == extension_misc::kWebStoreAppId) {
+    if (FindSpecialExtensionResourceId(load_info->extension_id, &resource_id)) {
       if (!loader_)
         loader_ = new ImageLoader(this);
-      loader_->LoadResource(it->resource, it->max_size, id, IDR_WEBSTORE_ICON);
-      continue;
-    } else if (load_info.extension_id == extension_misc::kChromeAppId) {
-      if (!loader_)
-        loader_ = new ImageLoader(this);
-      loader_->LoadResource(it->resource,
-                            it->max_size,
-                            id,
-                            IDR_PRODUCT_LOGO_128);
+      loader_->LoadResource(*it, id, resource_id);
       continue;
     }
 
     // If we don't have a path we don't need to do any further work, just
     // respond back.
     if (it->resource.relative_path().empty()) {
-      OnImageLoaded(NULL, it->resource, it->max_size, id, false);
+      OnBitmapLoaded(NULL, *it, it->desired_size, id, false);
       continue;
     }
 
     DCHECK(extension->path() == it->resource.extension_root());
 
-    // See if the extension has the image already.
-    if (extension->HasCachedImage(it->resource, it->max_size)) {
-      SkBitmap image = extension->GetCachedImage(it->resource, it->max_size);
-      OnImageLoaded(&image, it->resource, it->max_size, id, false);
+    // See if the extension has the bitmap/image rep already.
+    if (extension->HasCachedImage(it->resource, it->desired_size)) {
+      SkBitmap bitmap = extension->GetCachedImage(it->resource,
+                                                  it->desired_size);
+      OnBitmapLoaded(&bitmap, *it, it->desired_size, id, false);
       continue;
     }
 
@@ -261,14 +404,39 @@ void ImageLoadingTracker::LoadImages(const Extension* extension,
     if (!loader_)
       loader_ = new ImageLoader(this);
 
-    int resource_id;
     if (IsComponentExtensionResource(extension, it->resource, resource_id))
-      loader_->LoadResource(it->resource, it->max_size, id, resource_id);
+      loader_->LoadResource(*it, id, resource_id);
     else
-      loader_->LoadImage(it->resource, it->max_size, id);
+      loader_->LoadImage(*it, id);
   }
 }
 
+void ImageLoadingTracker::LoadImageForScaleFactor(
+    int id,
+    ui::ScaleFactor scale_factor) {
+  LoadMap::iterator load_map_it = load_map_.find(id);
+  DCHECK(load_map_it != load_map_.end());
+  PendingLoadInfo* load_info = &load_map_it->second;
+
+  // Do nothing if extension is unloaded.
+  if (load_info->extension == NULL)
+    return;
+
+  const float scale = ui::GetScaleFactorScale(scale_factor);
+  const int resource_size_in_pixel =
+      static_cast<int>(load_info->resource_size_in_dip * scale);
+
+  ExtensionResource resource = load_info->extension->GetIconResource(

[pkotwicz, 2012/07/19 01:37:45]

if load_info->image_skia has no reps and resource is not valid, you must fall
back to 1x here.
This may be the case for HiDPI mac when we never ask for the 1x scale factor and
the 2x version is not valid

[xiyuan, 2012/07/20 15:00:13]

Done.

+      resource_size_in_pixel, load_info->resource_match_type);
+
+  std::vector<ImageRepInfo> info_list;
+  info_list.push_back(ImageRepInfo(resource,
+                                   ImageRepInfo::ALWAYS_RESIZE,
+                                   load_info->desired_size_in_dip.Scale(scale),
+                                   scale_factor));
+  DoLoadImage(id, info_list);
+}
+
 bool ImageLoadingTracker::IsComponentExtensionResource(
     const Extension* extension,
     const ExtensionResource& resource,
@@ -293,9 +461,9 @@ bool ImageLoadingTracker::IsComponentExtensionResource(
   return false;
 }
 
-void ImageLoadingTracker::OnImageLoaded(
-    SkBitmap* image,
-    const ExtensionResource& resource,
+void ImageLoadingTracker::OnBitmapLoaded(
+    SkBitmap* bitmap,
+    const ImageRepInfo& image_info,
     const gfx::Size& original_size,
     int id,
     bool should_cache) {
@@ -304,38 +472,34 @@ void ImageLoadingTracker::OnImageLoaded(
   PendingLoadInfo* info = &load_map_it->second;
 
   // Save the pending results.
-  DCHECK(info->pending_count > 0);
+  DCHECK_GT(info->pending_count, 0u);
   info->pending_count--;
-  if (image)
-    info->bitmaps.push_back(*image);
+  if (bitmap) {
+    info->image_skia.AddRepresentation(gfx::ImageSkiaRep(*bitmap,
+        image_info.scale_factor));
+  }
 
-  // Add to the extension's image cache if requested.
+  // Add to the extension's bitmap cache if requested.
   DCHECK(info->cache != CACHE || info->extension);
   if (should_cache && info->cache == CACHE  &&
-      !info->extension->HasCachedImage(resource, original_size)) {
-    info->extension->SetCachedImage(resource, image ? *image : SkBitmap(),
+      !info->extension->HasCachedImage(image_info.resource, original_size)) {
+    info->extension->SetCachedImage(image_info.resource,
+                                    bitmap ? *bitmap : SkBitmap(),
                                     original_size);
   }
 
-  // If all pending images are done then report back.
+  // If all pending bitmaps/image reps are done then report back.
   if (info->pending_count == 0) {
     gfx::Image image;
     std::string extension_id = info->extension_id;
 
-    if (info->bitmaps.size() > 0) {
-      gfx::ImageSkia image_skia;
-      for (std::vector<SkBitmap>::const_iterator it = info->bitmaps.begin();
-           it != info->bitmaps.end(); ++it) {
-        // TODO(pkotwicz): Do something better but ONLY when DIP is enabled.
-        image_skia.AddRepresentation(
-            gfx::ImageSkiaRep(*it, ui::SCALE_FACTOR_100P));
-      }
-      image = gfx::Image(image_skia);
-    }
+    if (!info->image_skia.empty())
+      image = gfx::Image(info->image_skia);
 
-    load_map_.erase(load_map_it);
+    if (info->image_source == NULL)
+      load_map_.erase(load_map_it);
 
-    // ImageLoadingTracker might be deleted after the callback so don't
+    // ImageLoadingTracker might be deleted after the callback so don't do
     // anything after this statement.
     observer_->OnImageLoaded(image, extension_id, id);
   }
@@ -350,7 +514,7 @@ void ImageLoadingTracker::Observe(int type,
       content::Details<extensions::UnloadedExtensionInfo>(details)->extension;
 
   // Remove reference to this extension from all pending load entries. This
-  // ensures we don't attempt to cache the image when the load completes.
+  // ensures we don't attempt to cache the bitmap when the load completes.
   for (LoadMap::iterator i = load_map_.begin(); i != load_map_.end(); ++i) {
     PendingLoadInfo* info = &i->second;
     if (info->extension == extension) {