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

chrome/browser/extensions/image_loading_tracker.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/image_loading_tracker.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "chrome/browser/ui/webui/extensions/extension_icon_source.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/extensions/extension_resource.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "grit/component_extension_resources_map.h"
 #include "grit/theme_resources.h"
 #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;
 
 ////////////////////////////////////////////////////////////////////////////////
 // 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() {
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // ImageLoadingTracker::PendingLoadInfo
 
 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) {
 }
 
 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:
   explicit ImageLoader(ImageLoadingTracker* tracker)
       : tracker_(tracker) {
     CHECK(BrowserThread::GetCurrentThreadIdentifier(&callback_thread_id_));
     DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::FILE));
   }
 
   // Lets this class know that the tracker is no longer interested in the
   // results.
   void StopTracking() {
     tracker_ = NULL;
   }
 
   // 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_;
 
   // The thread that we need to call back on to report that we are done.
   BrowserThread::ID callback_thread_id_;
 
   DISALLOW_COPY_AND_ASSIGN(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)
     : observer_(observer),
       next_id_(0) {
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNLOADED,
                  content::NotificationService::AllSources());
 }
 
 ImageLoadingTracker::~ImageLoadingTracker() {
   // The loader is created lazily and is NULL if the tracker is destroyed before
   // 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;
+
+  int id = next_id_++;
+  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.
+  observer_->OnImageLoaded(gfx::Image(load_info.image_skia),

[pkotwicz, 2012/07/18 00:07:25]

You need to validate that you are capable of generating a non null image. This
is not necessarily the case as Extension::GetIconResource can generate an empty
IconResource.
In particular, you want to return an empty gfx::Image if
Extension::GetIconResource fails. (Check out line 246 on the left side of the
diff and line 65 in launcher_app_icon_loader)
The only suggestion I have is to load immediately at least one scale factor and
decide that if you can't load any of those scale factors that you won't able to
load any other scale factors.

[pkotwicz, 2012/07/18 14:17:53]

The validation is super cheap so you can also validate whether you can generate
a Bitmap for at least one scale factor here. If you can't you can return an
empty image instead.

[xiyuan, 2012/07/18 17:28:32]

Good catch. Added a CanLoadImage to check whether resource could be loaded and
returns an empty Image if we could not.

Also fixed OnBitmapLoaded where we should return an empty Image if loaded
ImageSkia is empty (i.e. has no reps).

+                           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.extension_id = extension->id();
   load_info.cache = cache;
-  load_info.extension_id = extension->id();
-  load_info.pending_count = info_list.size();
+
   int id = next_id_++;
   load_map_[id] = load_info;
 
-  for (std::vector<ImageInfo>::const_iterator it = info_list.begin();
+  DoLoadImage(id, info_list);
+}
+
+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) {
     // 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);
+      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;
     }
 
     // Instruct the ImageLoader to load this on the File thread. LoadImage and
     // LoadResource do not block.
     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(
+      resource_size_in_pixel,
+      load_info->resource_match_type);

[oshima, 2012/07/18 15:16:28]

you can move this to previous line.

[xiyuan, 2012/07/18 17:28:32]

Done.

+
+  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,
     int& resource_id) const {
   if (extension->location() != Extension::COMPONENT)
     return false;
 
   FilePath directory_path = extension->path();
   FilePath relative_path = directory_path.BaseName().Append(
       resource.relative_path());
 
   for (size_t i = 0; i < kComponentExtensionResourcesSize; ++i) {
     FilePath resource_path =
         FilePath().AppendASCII(kComponentExtensionResources[i].name);
     resource_path = resource_path.NormalizePathSeparators();
 
     if (relative_path == resource_path) {
       resource_id = kComponentExtensionResources[i].value;
       return true;
     }
   }
   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) {
   LoadMap::iterator load_map_it = load_map_.find(id);
   DCHECK(load_map_it != load_map_.end());
   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) {

[oshima, 2012/07/18 15:16:28]

how about add dcheck to make sure image_skia dones't have ImageSkiaRep for
image_info.scale_factor?

[xiyuan, 2012/07/18 17:28:32]

Should not do that because ImageLoadingTracker is also used to load multiple
images to construct icons (on windows and mac I think), where scale_factor is
always 1x for those bitmaps.

[oshima, 2012/07/18 20:41:49]

Can you point me the code? We shouldn't add multiple image reps with the same
scale factor.

[xiyuan, 2012/07/18 20:45:01]

The one I mentioned is AppShortcutManager::InstallApplicationShortcuts. It seems
like only care about raw pixel sized resource.

http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...

+    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;
+    gfx::Image image(info->image_skia);
     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_source == NULL)
+      load_map_.erase(load_map_it);
 
-    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);
   }
 }
 
 void ImageLoadingTracker::Observe(int type,
                                   const content::NotificationSource& source,
                                   const content::NotificationDetails& details) {
   DCHECK(type == chrome::NOTIFICATION_EXTENSION_UNLOADED);
 
   const Extension* extension =
       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) {
       info->extension = NULL;
       info->cache = DONT_CACHE;
     }
   }
 }