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::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() {
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // 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.
 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 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;
     }
 
     // Decode the image 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
     // 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.
     *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;
   }
 
  private:
   friend class base::RefCountedThreadSafe<ImageLoader>;
   ~ImageLoader() {}
 
   // The tracker we are loading the image 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 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)
     : 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));
+  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.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;
 
+  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;
     }
 
     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);
+      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;
     }
 
     // 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;

[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,
     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,
+    const ImageInfo& 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);
   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.
     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.
   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;
     }
   }
 }