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

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 "webkit/glue/image_decoder.h"
 
 using content::BrowserThread;
 using extensions::Extension;
 
+namespace {
+
+struct ComponentExtensionResource {
+  const char* extension_id;
+  const int resource_id;
+};
+
+const ComponentExtensionResource kSpecialComponentExtensionResources[] = {
+  { extension_misc::kWebStoreAppId, IDR_WEBSTORE_ICON },
+  { extension_misc::kWebStoreAppId, IDR_PRODUCT_LOGO_128 },

[Aaron Boodman, 2012/08/02 17:50:56]

Why are there two that have the same id?

[tbarzic, 2012/08/02 20:12:52]

looking at the old code, it looks like the second one should be kChromeAppId

+};
+
+// 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() {
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // ImageLoadingTracker::PendingLoadInfo
 
 ImageLoadingTracker::PendingLoadInfo::PendingLoadInfo()
   : extension(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 ||

[Aaron Boodman, 2012/08/02 17:50:56]

If it is the exact same size, you shouldn't resample, right?

[tbarzic, 2012/08/02 20:12:52]

Yep, also it may be safer not to rely on ResizeMethod enum having only two
values.

+        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(

[Aaron Boodman, 2012/08/02 17:50:56]

It's weird layering to call ExtensionIconSource here. Can you just call
ResourceBundle::GetInstance().GetImageNamed() instead?

[tbarzic, 2012/08/02 20:12:52]

Done.

         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
 
 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();
 }
 
+bool ImageLoadingTracker::CanLoadImage(const extensions::Extension* extension,

[Aaron Boodman, 2012/08/02 17:50:56]

This method is weird. First of all, it doesn't even answer the question of
whether the image can be loaded.

Secondly, it doesn't seem like we should even have it. Why does the calling code
need to ask this question? It seems like it should be ignorant of whether the
resource it is asking about is legit, and just handle the error in the callback.
That way all error handling is in the same place.

[tbarzic, 2012/08/02 20:12:52]

makes sense.. the version fallbacks to the SCALE_FACTOR_100P
if we fail to load image, not only if the resource for the other scale is not
legit (given this is not the only thing that can go wrong);

Also, I'll remove ExtensionIconImage::isNull() since it can be true only when we
detect desired resource is invalid (using this method) in ExtensionIconImage
ctor(I didn't really find it useful in the first place; and I'm not too sure it
is correct: if there is no resource for 1x, that does not mean there won't be
one for 2x)

+                                       const ExtensionResource& resource) {
+  if (FindSpecialExtensionResourceId(extension->id(), NULL))
+    return true;
+
+  return !resource.relative_path().empty();
+}
+
 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::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();
   int id = next_id_++;
   load_map_[id] = load_info;
 
-  for (std::vector<ImageInfo>::const_iterator it = info_list.begin();
+  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 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);
   }
 }
 
 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) {
+    info->image_skia.AddRepresentation(gfx::ImageSkiaRep(*bitmap,
+        image_info.scale_factor));

[Aaron Boodman, 2012/08/02 17:50:56]

Incorrect indentation. First character of each line of params should left-align.

[tbarzic, 2012/08/02 20:12:52]

Done.

+  }
 
-  // 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 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);
 
-    // 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;
     }
   }
 }