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..9033142db57f05bf01ca5e277d5f2198567584b2 100644
--- a/chrome/browser/extensions/image_loading_tracker.cc
+++ b/chrome/browser/extensions/image_loading_tracker.cc
@@ -21,8 +21,9 @@
 #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;
@@ -37,8 +38,12 @@ ImageLoadingTracker::Observer::~Observer() {}
 // ImageLoadingTracker::ImageInfo
 
 ImageLoadingTracker::ImageInfo::ImageInfo(
-    const ExtensionResource& resource, gfx::Size max_size)
-    : resource(resource), max_size(max_size) {
+    const ExtensionResource& resource,
+    const gfx::Size& max_size,
+    ui::ScaleFactor scale_factor)
+    : resource(resource),
+      max_size(max_size),
+      scale_factor(scale_factor) {
 }
 
 ImageLoadingTracker::ImageInfo::~ImageInfo() {
@@ -49,6 +54,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) {
 }
@@ -76,26 +84,21 @@ 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 ImageInfo& image_info, int id) {
     DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::FILE));
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,

[oshima, 2012/07/16 17:22:33]

this is not the scope of this change, but at some point, you'll need to change
this to use thread pool.

[xiyuan, 2012/07/16 20:11:27]

Agree. But I'd prefer to do it in a follow up CL so that we could focus.

-        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 ImageInfo& 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;
     }
 
@@ -112,61 +115,57 @@ class ImageLoadingTracker::ImageLoader
     // Chrome is therefore decoding images 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 (decoded->width() > image_info.max_size.width() ||
+        decoded->height() > image_info.max_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.max_size.width(), image_info.max_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 ImageInfo& 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 ImageInfo& image_info,
                                 int id,
                                 int resource_id) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
     SkBitmap* image = ExtensionIconSource::LoadImageByResourceId(
         resource_id);
-    ReportBack(image, resource, max_size, id);
+    ReportBack(image, image_info, image_info.max_size, id);
   }
 
-  void ReportBack(SkBitmap* image, const ExtensionResource& resource,
+  void ReportBack(SkBitmap* image, const ImageInfo& 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));
+                   image, image_info, original_size, id));
   }
 
-  void ReportOnUIThread(SkBitmap* image, const ExtensionResource& resource,
+  void ReportOnUIThread(SkBitmap* image, const ImageInfo& 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_->OnImageLoaded(image, image_info, original_size, id, true);
 
     delete image;
   }
@@ -186,6 +185,53 @@ class ImageLoadingTracker::ImageLoader
 };
 
 ////////////////////////////////////////////////////////////////////////////////
+// ImageLoadingTracker::ImageSource
+
+// An ImageSkiaSource to load image for additional scale factor.

[pkotwicz, 2012/07/16 17:33:52]

Nit: factors

[xiyuan, 2012/07/16 20:11:27]

Done.

+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 images for |scale_factor|.
+  if (tracker_)
+    tracker_->LoadImageForScaleFactor(id_, scale_factor);
+
+  // Returns an empty representation for |scale_factor| here. If |tracker_| is
+  // valid, it loads image asynchronously. When loading is done, it  updates
+  // existing ImageSkia and notifies its observer.
+  return gfx::ImageSkiaRep();
+}
+
+////////////////////////////////////////////////////////////////////////////////
 // ImageLoadingTracker
 
 ImageLoadingTracker::ImageLoadingTracker(Observer* observer)
@@ -200,6 +246,11 @@ 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,
@@ -207,43 +258,102 @@ void ImageLoadingTracker::LoadImage(const Extension* extension,
                                     const gfx::Size& max_size,
                                     CacheParam cache) {
   std::vector<ImageInfo> info_list;
-  info_list.push_back(ImageInfo(resource, max_size));
+  info_list.push_back(ImageInfo(resource, max_size, ui::SCALE_FACTOR_NONE));

[pkotwicz, 2012/07/16 17:33:52]

Just use SCALE_FACTOR_100P. (SCALE_FACTOR_NONE is value equivalent.

[xiyuan, 2012/07/16 20:11:27]

Done.

   LoadImages(extension, info_list, cache);
 }
 
+void ImageLoadingTracker::LoadImageInDIP(
+    const Extension* extension,
+    int resource_size,
+    ExtensionIconSet::MatchType resource_match_type,
+    const gfx::Size& max_size,
+    CacheParam cache) {
+  typedef std::vector<ui::ScaleFactor> ScaleFactors;
+
+  // TODO(xiyuan): Update the following.
+  // gfx::Screen::GetScaleFactorsInUse is in peter's pending CL.
+  // const ScaleFactors scale_factors_in_use =
+  //     gfx::Screen::GetScaleFactorsInUse();

[oshima, 2012/07/16 17:22:33]

I thought we're going to load the correct image for current scale factor? Am I
wrong?

[xiyuan, 2012/07/16 20:11:27]

You are correct. The code is in comment because
http://codereview.chromium.org/10702098/ is not landed. :)

+  ScaleFactors scale_factors_in_use;
+  if (gfx::Screen::IsDIPEnabled())
+    scale_factors_in_use.push_back(ui::SCALE_FACTOR_200P);
+  else
+    scale_factors_in_use.push_back(ui::SCALE_FACTOR_100P);
+
+  std::vector<ImageInfo> info_list;
+  for (ScaleFactors::const_iterator it = scale_factors_in_use.begin();
+       it != scale_factors_in_use.end(); ++it) {
+    const float scale = ui::GetScaleFactorScale(*it);
+
+    ExtensionResource resource = extension->GetIconResource(
+        static_cast<int>(resource_size * scale),
+        resource_match_type);
+
+    info_list.push_back(ImageInfo(resource, max_size.Scale(scale), *it));
+  }
+
+  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.max_size_in_dip = max_size;
+
+  int id = next_id_++;
+  load_info.image_source = new ImageSource(this, id);
+  load_info.images = gfx::ImageSkia(load_info.image_source,
+                                    load_info.max_size_in_dip);

[pkotwicz, 2012/07/16 17:33:52]

The loaded bitmap is not always resized to max_size. In particular, it is only
resized if it is greater than max size.
For dip images, the bitmaps should always be resized.
(We seem to be resizing the results of ImageLoadingTracker to the desired size
in the few cases I checked).
I suggest in ImageInfo, renaming max_size to desired_size
and adding a resize method field which would have values: RESIZE_WHEN_LARGER,
ALWAYS_RESIZE

[xiyuan, 2012/07/16 20:11:27]

Done.

+
+  load_map_[id] = load_info;
+
+  DoLoadImages(id, info_list);

[pkotwicz, 2012/07/16 17:33:52]

Call this DoLoadImage. You are still loading a single gfx::Image, though it is a
multiresolution image.

[xiyuan, 2012/07/16 20:11:27]

Done.

+}
+
 void ImageLoadingTracker::LoadImages(const Extension* extension,
                                      const std::vector<ImageInfo>& 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;
 
+  DoLoadImages(id, info_list);
+}
+
+void ImageLoadingTracker::DoLoadImages(
+    int id,
+    const std::vector<ImageInfo>& 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;
+  load_info->pending_count = info_list.size();
+  const extensions::Extension* extension = load_info->extension;
+
   for (std::vector<ImageInfo>::const_iterator it = info_list.begin();
        it != info_list.end(); ++it) {
     // Load resources for special component extensions.
-    if (load_info.extension_id == extension_misc::kWebStoreAppId) {
+    if (load_info->extension_id == extension_misc::kWebStoreAppId) {
       if (!loader_)
         loader_ = new ImageLoader(this);
-      loader_->LoadResource(it->resource, it->max_size, id, IDR_WEBSTORE_ICON);
+      loader_->LoadResource(*it, id, IDR_WEBSTORE_ICON);
       continue;
-    } else if (load_info.extension_id == extension_misc::kChromeAppId) {
+    } 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, IDR_PRODUCT_LOGO_128);
       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);
+      OnImageLoaded(NULL, *it, it->max_size, id, false);
       continue;
     }
 
@@ -252,7 +362,7 @@ void ImageLoadingTracker::LoadImages(const Extension* extension,
     // 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);
+      OnImageLoaded(&image, *it, it->max_size, id, false);

[pkotwicz, 2012/07/16 17:33:52]

Rename image->bitmap.
Can you add a TODO to support caching with LoadImageInDIP?

[xiyuan, 2012/07/16 20:11:27]

Rename done.

Caching is supported on the bitmap/image rep basis. When LoadImageSkia is
called, cached bitmap/image rep will be used if it is present. Only the
bitmap/image rep that does not exist in cache would go to |loader_| to get
loaded.

       continue;
     }
 
@@ -263,12 +373,38 @@ void ImageLoadingTracker::LoadImages(const Extension* extension,
 
     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;

[pkotwicz, 2012/07/16 17:33:52]

You need to check here if |load_info->pending_count==0| or do something else to
avoid duplicate requests.
In particular, if you get any requests from ImageSkiaSource before the loading
for the initial scale factors has completed.

[xiyuan, 2012/07/16 20:11:27]

Good catch. Think we should allow LoadImageForScaleFactor being called before
previous pending loads are finished. Thus, we should update
load_info->pending_count in DoLoadImage by adding the new requests count (i.e.
info_list.size) instead of replacing it. This should handle the case.

+
+  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(
+      resource_size_in_pixel,
+      load_info->resource_match_type);
+
+  std::vector<ImageInfo> info_list;
+  info_list.push_back(ImageInfo(resource,
+                                load_info->max_size_in_dip.Scale(scale),
+                                scale_factor));
+  DoLoadImages(id, info_list);
+}
+
 bool ImageLoadingTracker::IsComponentExtensionResource(
     const Extension* extension,
     const ExtensionResource& resource,
@@ -295,7 +431,7 @@ bool ImageLoadingTracker::IsComponentExtensionResource(
 
 void ImageLoadingTracker::OnImageLoaded(
     SkBitmap* image,
-    const ExtensionResource& resource,
+    const ImageInfo& image_info,
     const gfx::Size& original_size,
     int id,
     bool should_cache) {
@@ -306,34 +442,27 @@ void ImageLoadingTracker::OnImageLoaded(
   // Save the pending results.
   DCHECK(info->pending_count > 0);
   info->pending_count--;
-  if (image)
-    info->bitmaps.push_back(*image);
+  if (image) {

[oshima, 2012/07/16 17:22:33]

DCHECK(!images.HasRepresentation(image_info.scale_factor))

[xiyuan, 2012/07/16 20:11:28]

This DCHECK actually would fail because ImageLoadingTracker::ImageSource returns
an empty ImageSkiaRep in its GetImageForScale and expect ImageLoadingTracker to
loads the image and replace the empty ImageSkiaRep here.

The ImageSkaRep replacement logic is in pkotwicz's
http://codereview.chromium.org/10500015/. And this CL depends on that. :p

+    info->images.AddRepresentation(gfx::ImageSkiaRep(*image,
+        image_info.scale_factor));
+  }
 
   // Add to the extension's image 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,
+                                    image ? *image : SkBitmap(),

[pkotwicz, 2012/07/16 17:33:52]

Rename image->bitmap

[xiyuan, 2012/07/16 20:11:28]

Done.

                                     original_size);
   }
 
   // If all pending images are done then report back.
   if (info->pending_count == 0) {
-    gfx::Image image;
+    gfx::Image image(info->images);
     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);
-    }
-
-    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
     // anything after this statement.