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

chrome/browser/extensions/image_loading_tracker.h

Index: chrome/browser/extensions/image_loading_tracker.h
diff --git a/chrome/browser/extensions/image_loading_tracker.h b/chrome/browser/extensions/image_loading_tracker.h
index b163651e9a551d4ae3b53533f4596a181d350ea6..f51c387fc8c07ce470942ed2add626cc7ca858d5 100644
--- a/chrome/browser/extensions/image_loading_tracker.h
+++ b/chrome/browser/extensions/image_loading_tracker.h
@@ -6,13 +6,18 @@
 #define CHROME_BROWSER_EXTENSIONS_IMAGE_LOADING_TRACKER_H_
 
 #include <map>
+#include <string>
+#include <vector>
 
 #include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
+#include "chrome/common/extensions/extension_icon_set.h"
 #include "chrome/common/extensions/extension_resource.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
+#include "ui/base/layout.h"
+#include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/size.h"
 
 class SkBitmap;
@@ -35,7 +40,15 @@ class Image;
 // Observer::OnImageLoaded and call:
 //   tracker_.LoadImage(extension, resource, max_size, false);
 // ... and wait for OnImageLoaded to be called back on you with a pointer to the
-// SkBitmap loaded.
+// ImageSkia loaded.
+//
+// To support DIP, LoadImageSkia should be used. It returns an ImageSkia that

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

support DIP -> support scale factors other than 1x

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

Done.

+// has ImageLoadingTracker::ImageSource. ImageSource would use this class to

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

How about: Calling ImageSkia::GetRepresentation(ui::ScaleFactor) either
explicitly or by gfx::Canvas::DrawImageInt will kick off the fetching the
resource and call ImageLoadingTracker::OnImageLoaded when finished.
ImageLoadingTracker must be kept around ...

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

Done.

+// load resource bitmap when it is asked for a representation of a scale factor.
+// One needs to keep ImageLoadingTracker around as long as the returned

[sky, 2012/07/18 23:50:57]

This is a weird restriction. Seems like we should allow destroying the tracker
once the image is obtained.

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

The tracker is kept around to allow additional bitmap resource to be loaded when
scale factor changes. This is needed for a couple of reasons:
1. We want to only load minimum resource that is going to be used instead of
loading all possible resources;
2. There is currently no mechanism for an ImageSkia to notify its host that it
is changed and needs repaint. The current thinking is to reuse the existing
async loading mechanism;

If we could figure out some way to do #2, then this restriction could be
removed.

+// ImageSkia is in use and expects OnImageLoaded to be called with updated
+// ImageSkia when actual resource bitmap is loaded.
+//
 // NOTE: if the image is available already (or the resource is not valid), the
 // Observer is notified immediately from the call to LoadImage. In other words,
 // by the time LoadImage returns the observer has been notified.
@@ -62,14 +75,33 @@ class ImageLoadingTracker : public content::NotificationObserver {
     virtual ~Observer();
   };
 
-  // Information about a single image to load from a extension resource.
-  struct ImageInfo {
-    ImageInfo(const ExtensionResource& resource, gfx::Size max_size);
-    ~ImageInfo();
+  // Information about a single bitmap/image rep to load from an extension
+  // resource.
+  struct ImageRepInfo {
+    // Enum values to indicate whether to resize loaded bitmap when it is larger
+    // than |desired_size| or always resize it.
+    enum ResizeMethod {
+      RESIZE_WHEN_LARGER,
+      ALWAYS_RESIZE,
+    };
+
+    ImageRepInfo(const ExtensionResource& resource,
+                 ResizeMethod resize_method,
+                 const gfx::Size& desired_size,
+                 ui::ScaleFactor scale_factor);
+    ~ImageRepInfo();
+
+    // Extension resource to load.
     ExtensionResource resource;
-    // If the loaded image is larger than |max_size| it will be resized to those
-    // dimensions.
-    gfx::Size max_size;
+
+    ResizeMethod resize_method;
+
+    // When |resize_metho| is ALWAYS_RESIZE or when the loaded image is larger
+    // than |desired_size| it will be resized to those dimensions.
+    gfx::Size desired_size;
+
+    // |scale_factor| is used to construct the loaded gfx::ImageSkia.
+    ui::ScaleFactor scale_factor;
   };
 
   explicit ImageLoadingTracker(Observer* observer);
@@ -79,16 +111,28 @@ class ImageLoadingTracker : public content::NotificationObserver {
   // |max_size| it will be resized to those dimensions. IMPORTANT NOTE: this
   // function may call back your observer synchronously (ie before it returns)
   // if the image was found in the cache.
+  // Note this method loads a raw bitmap from the resource. All sizes given are
+  // assumed to be in pixels. If you want to load a DIP-aware image, use
+  // |LoadImageSkia| instead.
   void LoadImage(const extensions::Extension* extension,
                  const ExtensionResource& resource,
                  const gfx::Size& max_size,
                  CacheParam cache);
 
+  // Similar to LoadImage above but loads an ImageSkia that supports DIP and
+  // always resize loaded image to |desired_size|.  |resource_size| and
+  // |desired_size| are in DIP coordinates.
+  void LoadImageSkia(const extensions::Extension* extension,

[sky, 2012/07/18 23:50:57]

Is there a way to avoid having the two methods? That is, can we make LoadImage
always do what LoadImageSkia is doing? If not, can we add another parameter to
LoadImage?

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

I think that we will end up always having two methods.
Here is the split that I envision:
Uses of ImageLoadingTracker where we load an image which is displayed in the UI.
Here it is ok to use an ImageSkiaSource
Uses of ImageLoadingTracker where we want to write the result to file.
app_shortcut_manager.cc for instance. Here ideally, we would pass in the desired
scale factors.
I really wouldn't want AppShortcutManager::OnImageLoaded to always be called
twice, and a second time with the desired data.

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

Peter says it.

+                     int resource_size,
+                     ExtensionIconSet::MatchType resource_match_type,
+                     const gfx::Size& desired_size,
+                     CacheParam cache);
+
   // Same as LoadImage() above except it loads multiple images from the same
   // extension. This is used to load multiple resolutions of the same image
   // type.
   void LoadImages(const extensions::Extension* extension,
-                  const std::vector<ImageInfo>& info_list,
+                  const std::vector<ImageRepInfo>& info_list,
                   CacheParam cache);
 
   // Returns the ID used for the next image that is loaded. That is, the return
@@ -97,33 +141,60 @@ class ImageLoadingTracker : public content::NotificationObserver {
   int next_id() const { return next_id_; }
 
  private:
-  // Information for pending image load operation for one or more images.
+  class ImageLoader;
+  class ImageSource;
+
+  // Information for pending resource load operation for one or more
+  // bitmaps/image reps.
   struct PendingLoadInfo {
     PendingLoadInfo();
     ~PendingLoadInfo();
 
     const extensions::Extension* extension;
-    // This is cached separate from |extension| in case the extension in
+    // This is cached separate from |extension| in case the extension is
     // unloaded.
     std::string extension_id;
+
+    // Size info from a LoadImage call.
+    int resource_size_in_dip;
+    ExtensionIconSet::MatchType resource_match_type;
+    gfx::Size desired_size_in_dip;
+
+    // ImageSource loads image for additional scale factor. This is only set if
+    // above size info is valid.
+    ImageSource* image_source;  // Owned by ImageSkiaStorage.
+
     CacheParam cache;
     size_t pending_count;
-    std::vector<SkBitmap> bitmaps;
+
+    gfx::ImageSkia image_skia;
   };
 
   // Maps an integer identifying a load request to a PendingLoadInfo.
   typedef std::map<int, PendingLoadInfo> LoadMap;
 
-  class ImageLoader;
+  // Checks whether a pending load could be performed. That is, whether the
+  // extension has the resource.
+  bool CanLoadImage(const PendingLoadInfo& load_info);
+
+  // Loads bitmap resources in |info_list| for load request identified by |id|.
+  void DoLoadImage(int id, const std::vector<ImageRepInfo>& info_list);
+
+  // Loads bitmap for additional scale factor for given load |id|.
+  void LoadImageForScaleFactor(int id, ui::ScaleFactor scale_factor);
 
-  // When an image has finished loaded and been resized on the file thread, it
+  // When a bitmap has finished loading and been resized on the file thread, it
   // is posted back to this method on the original thread.  This method then
-  // calls the observer's OnImageLoaded and deletes the ImageLoadingTracker if
-  // it was the last image in the list. The |original_size| should be the size
-  // of the image before any resizing was done.
-  // |image| may be null if the file failed to decode.
-  void OnImageLoaded(SkBitmap* image, const ExtensionResource& resource,
-                     const gfx::Size& original_size, int id, bool should_cache);
+  // calls the observer's OnImageLoaded  if it was the last image in the list.
+  // The observer could delete the ImageLoadingTracker if it does not care about
+  // DIP scale changes afterwards. The |original_size| should be the size of the
+  // bitmap before any resizing was done.
+  // |bitmap| may be null if the file failed to decode.
+  void OnBitmapLoaded(SkBitmap* bitmap,
+                      const ImageRepInfo& image_info,
+                      const gfx::Size& original_size,
+                      int id,
+                      bool should_cache);
 
   // Checks whether image is a component extension resource. Returns false
   // if a given |resource| does not have a corresponding image in bundled