Supply default icon to extension icon image.

chrome/browser/extensions/extension_icon_image.cc

Index: chrome/browser/extensions/extension_icon_image.cc
diff --git a/chrome/browser/extensions/extension_icon_image.cc b/chrome/browser/extensions/extension_icon_image.cc
index dcd096815c03ab6c26653705b041539a73703acc..42aae616f37982220dadf4881a1e46d6d038a296 100644
--- a/chrome/browser/extensions/extension_icon_image.cc
+++ b/chrome/browser/extensions/extension_icon_image.cc
@@ -9,10 +9,48 @@
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/extensions/extension.h"
 #include "content/public/browser/notification_service.h"
+#include "ui/gfx/canvas.h"
+#include "ui/gfx/image/canvas_image_source.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/image/image_skia_source.h"
 #include "ui/gfx/size.h"
 
+/**
+ * The file contains extensions::IconImage implementation, which provides

[Jeffrey Yasskin, 2012/09/01 01:10:27]

Shorter comments are better, all else equal, to prevent TL;DR syndrome. I think
you can replace the first three sentences with: "The ImageSkia provided by
extensions::IconImage contains ImageSkiaReps that are computed and updated using
the following algorithm:" Try to keep your other comments as concise as possible
too.

[tbarzic, 2012/09/04 21:07:09]

Done.

+ * extension icons to be displayed in the UI.
+ * The icon provided by IconImage is ImageSkia icon defined with custom
+ * ImageSkiaSource. When a new image representation is requesetd from the image

[Jeffrey Yasskin, 2012/09/01 01:10:27]

sp: requesetd

[tbarzic, 2012/09/04 21:07:09]

Done.

+ * source the following happens:
+ * - If the extension doesn't have a icon resource needed for the image

[Jeffrey Yasskin, 2012/09/01 01:10:27]

There's some complex behavior around which sizes are allowed to satisfy a given
scale request. You should refer to LoadImageForScaleFactor() for the definition
of that load. Perhaps:

"- LoadImageForScaleFactor() searches the extension for an icon of an
appropriate size.
"- If the extension doesn't have ..."

[tbarzic, 2012/09/04 21:07:09]

Done.

+ *   representation, default icon's representation for the requested scale
+ *   factor is returned by ImageSkiaSource. If the default icon is empty,

[Jeffrey Yasskin, 2012/09/01 01:10:27]

You wind up saying the empty-default fallback 3 times in this description.
Instead, say something like, "If the default icon is empty, a transparent image
is used as the default." in the comment header and just refer to the default
image thereafter.

[tbarzic, 2012/09/04 21:07:09]

Done.

+ *   transparent image representation is returned.
+ * - If the extension has the resource, IconImage tries to load it using
+ *   ImageLoadingTracker.
+ * - |ImageLoaderTracker| may return both syncronously and asyncronously.

[Jeffrey Yasskin, 2012/09/01 01:10:27]

sp: syncronously 2x

[tbarzic, 2012/09/04 21:07:09]

Done.

+ *   IconImage behaviour depends on this.
+ * 1. |ImageLoaderTracker| is synchronous.
+ *  - If image representation resurce is successfully loaded, the

[Jeffrey Yasskin, 2012/09/01 01:10:27]

sp: resurce

[tbarzic, 2012/09/04 21:07:09]

Done.

+ *    representation returned by ImageSkiaSource is created from the loaded
+ *    bitmap.
+ *  - If resource loading fails, ImageSkiaSource returns either default icon's
+ *    representation, or a blank image representation; depending on whether the
+ *    supplied default icon was non empty.
+ * 2. |ImageLoadingTracker| is asyncronous.
+ *  - ImageSkiaSource will initially return transparent image resource of the
+ *    desired size.
+ *  - The image will be manually updated when the |ImageLoadingTracker|

[Jeffrey Yasskin, 2012/09/01 01:10:27]

Nothing's manual in code. I think you mean that we use AddRepresentation to set
the resulting image into the ImageSkia, and tell the Observer to repaint by
calling OnExtensionIconImageChanged.

[tbarzic, 2012/09/04 21:07:09]

Done.

+ *    finishes.
+ *  - If image resource is successfully loaded, the image is updated with the
+ *    representation created from the loaded bitmap (the old representation is
+ *    removed from the image). Also, the observer is notified the image has
+ *    changed.
+ *  - If resource load failed and non empty default icon was supplied, image
+ *    is updated by the corresponding representatin from the default icon and

[Jeffrey Yasskin, 2012/09/01 01:10:27]

sp: representatin

[tbarzic, 2012/09/04 21:07:09]

Done.

+ *    the obesrver is notifeid image has changed.
+ *  - If resource load failed and an empty default icon was supplied, nothing
+ *    is done. The image will keep initial, transparent representation.
+**/
 namespace {
 
 const int kMatchBiggerTreshold = 32;
@@ -29,6 +67,22 @@ ExtensionResource GetExtensionIconResource(
   return extension->GetResource(path);
 }
 
+class BlankImageSource : public gfx::CanvasImageSource {
+ public:
+  explicit BlankImageSource(const gfx::Size& size_in_dip)
+      : CanvasImageSource(size_in_dip, false) {

[Jeffrey Yasskin, 2012/09/01 01:10:27]

I prefer writing something like /*is_opaque=*/false so that future readers don't
have to look up CanvasImageSource to figure out what the parameter means.

[tbarzic, 2012/09/04 21:07:09]

yeah, I've started doing this, but I still occasionally miss some places

+  }
+  virtual ~BlankImageSource() {}
+
+ private:
+  // gfx::CanvasImageSource overrides:
+  virtual void Draw(gfx::Canvas* canvas) OVERRIDE {
+    canvas->DrawColor(SkColorSetARGB(0, 0, 0, 0));
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(BlankImageSource);
+};
+
 }  // namespace
 
 namespace extensions {
@@ -38,7 +92,7 @@ namespace extensions {
 
 class IconImage::Source : public gfx::ImageSkiaSource {
  public:
-  explicit Source(IconImage* host);
+  Source(IconImage* host, const gfx::Size& size_in_dip);
   virtual ~Source();
 
   void ResetHost();
@@ -48,12 +102,23 @@ class IconImage::Source : public gfx::ImageSkiaSource {
   virtual gfx::ImageSkiaRep GetImageForScale(
       ui::ScaleFactor scale_factor) OVERRIDE;
 
+  // IconImage object that is hosting this image source. It is used to load

[Jeffrey Yasskin, 2012/09/01 01:10:27]

Shorter: "Used to load images, possibly asynchronously. NULLed out when the
IconImage is destroyed."

[tbarzic, 2012/09/04 21:07:09]

Done.

+  // image representation for the extension icon.
+  // |host_| will be reset once the referenced icon image dies. Since |this|
+  // can outlive the IconImage, NULL check should be made on |host_| before
+  // using it.
   IconImage* host_;
 
+  // Image whose representations will be used until |host_| load the real
+  // representations for the image.
+  gfx::ImageSkia blank_image_;
+
   DISALLOW_COPY_AND_ASSIGN(Source);
 };
 
-IconImage::Source::Source(IconImage* host) : host_(host) {
+IconImage::Source::Source(IconImage* host, const gfx::Size& size_in_dip)
+    : host_(host),
+      blank_image_(new BlankImageSource(size_in_dip), size_in_dip) {
 }
 
 IconImage::Source::~Source() {
@@ -65,9 +130,14 @@ void IconImage::Source::ResetHost() {
 
 gfx::ImageSkiaRep IconImage::Source::GetImageForScale(
     ui::ScaleFactor scale_factor) {
+  gfx::ImageSkiaRep representation;
   if (host_)
-    host_->LoadImageForScaleFactor(scale_factor);
-  return gfx::ImageSkiaRep();
+    representation = host_->LoadImageForScaleFactor(scale_factor);
+
+  if (!representation.is_null())
+    return representation;
+
+  return blank_image_.GetRepresentation(scale_factor);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -77,31 +147,31 @@ IconImage::IconImage(
     const Extension* extension,
     const ExtensionIconSet& icon_set,
     int resource_size_in_dip,
+    const gfx::ImageSkia& default_icon,
     Observer* observer)
     : extension_(extension),
       icon_set_(icon_set),
       resource_size_in_dip_(resource_size_in_dip),
-      desired_size_in_dip_(resource_size_in_dip, resource_size_in_dip),
       observer_(observer),
       source_(NULL),
+      default_icon_(default_icon),
       ALLOW_THIS_IN_INITIALIZER_LIST(tracker_(this)) {
-  source_ = new Source(this);
-  image_skia_ = gfx::ImageSkia(source_, desired_size_in_dip_);
+  gfx::Size resource_size(resource_size_in_dip, resource_size_in_dip);
+  source_ = new Source(this, resource_size);
+  image_skia_ = gfx::ImageSkia(source_, resource_size);
 
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNLOADED,
                  content::NotificationService::AllSources());
 }
 
 IconImage::~IconImage() {

[Jeffrey Yasskin, 2012/09/01 01:10:27]

This still needs to call ResetHost(), right? If this passed your testing, you're
missing a test.

[tbarzic, 2012/09/04 21:07:09]

jeez, I'm sometimes amazed with things I do on Friday afternoons...

-  // |source_| could be NULL if resource does not exist.
-  if (source_)
-    source_->ResetHost();
 }
 
-void IconImage::LoadImageForScaleFactor(ui::ScaleFactor scale_factor) {
+gfx::ImageSkiaRep IconImage::LoadImageForScaleFactor(
+    ui::ScaleFactor scale_factor) {
   // Do nothing if extension is unloaded.
   if (!extension_)
-    return;
+    return gfx::ImageSkiaRep();
 
   const float scale = ui::GetScaleFactorScale(scale_factor);
   const int resource_size_in_pixel =
@@ -120,48 +190,68 @@ void IconImage::LoadImageForScaleFactor(ui::ScaleFactor scale_factor) {
         resource_size_in_pixel, ExtensionIconSet::MATCH_SMALLER);
   }
 
-  // If there is no resource found, bail out and notify observer of failure.
-  if (resource.empty()) {
-    if (observer_)
-      observer_->OnIconImageLoadFailed(this, scale_factor);
-    return;
-  }
+  // If there is no resource found, return default icon.
+  if (resource.empty())
+    return default_icon_.GetRepresentation(scale_factor);
 
   int id = tracker_.next_id();
-  load_map_[id] = scale_factor;
+  load_map_[id].scale_factor = scale_factor;
+  load_map_[id].is_async = false;
 
   std::vector<ImageLoadingTracker::ImageRepresentation> info_list;
   info_list.push_back(ImageLoadingTracker::ImageRepresentation(
       resource,
       ImageLoadingTracker::ImageRepresentation::RESIZE_WHEN_LARGER,
-      desired_size_in_dip_.Scale(scale),
+      gfx::Size(resource_size_in_dip_, resource_size_in_dip_).Scale(scale),
       scale_factor));
   tracker_.LoadImages(extension_, info_list, ImageLoadingTracker::DONT_CACHE);
+
+  // If we have not received |OnImageLoaded|, image load request is
+  // asynchronous.
+  if (load_map_.find(id) != load_map_.end())
+    load_map_[id].is_async = true;
+
+  // If LoadImages returned synchronously and the requested image rep is cached
+  // in the extension, return the cached image rep.
+  if (image_skia_.HasRepresentation(scale_factor))
+    return image_skia_.GetRepresentation(scale_factor);
+
+  return gfx::ImageSkiaRep();
 }
 
-void IconImage::OnImageLoaded(const gfx::Image& image,
+void IconImage::OnImageLoaded(const gfx::Image& image_in,
                               const std::string& extension_id,
                               int index) {
   LoadMap::iterator load_map_it = load_map_.find(index);
   DCHECK(load_map_it != load_map_.end());
 
-  ui::ScaleFactor scale_factor = load_map_it->second;
+  ui::ScaleFactor scale_factor = load_map_it->second.scale_factor;
+  bool is_async = load_map_it->second.is_async;
 
   load_map_.erase(load_map_it);
 
-  if (image.IsEmpty()) {
-    // There waas an error loading the image.
-    if (observer_)
-      observer_->OnIconImageLoadFailed(this, scale_factor);
+  const gfx::ImageSkia* image =
+      image_in.IsEmpty() ? &default_icon_ : image_in.ToImageSkia();
+
+  // If the image resource has been loaded successfully, the returned image
+  // should have the representation for the requested scale factor.
+  DCHECK(image_in.IsEmpty() || image->HasRepresentation(scale_factor));

[Jeffrey Yasskin, 2012/09/01 01:10:27]

Your comment says GetRepresentation should work, not that HasRepresentation
should return true, so please write a test for the case where the default image
computes its representations.

[tbarzic, 2012/09/04 21:07:09]

Done.

+
+  // Maybe default icon was not set.
+  if (image->isNull())
     return;
-  }
 
-  DCHECK(image.ToImageSkia()->HasRepresentation(scale_factor));
-  gfx::ImageSkiaRep rep = image.ToImageSkia()->GetRepresentation(scale_factor);
+  gfx::ImageSkiaRep rep = image->GetRepresentation(scale_factor);
   DCHECK(!rep.is_null());
+
+  // Remove old representation if there is one.
+  image_skia_.RemoveRepresentation(rep.scale_factor());
   image_skia_.AddRepresentation(rep);
 
-  if (observer_)
+  // If |tracker_| called us synchronously the image did not really change from
+  // the observers perspective, since the initial image representation is
+  // returned synchronously.
+  if (is_async && observer_)
     observer_->OnExtensionIconImageChanged(this);
 }