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..82a86abf9a1e363c2d5680a5aa9083ef996492ff 100644
--- a/chrome/browser/extensions/extension_icon_image.cc
+++ b/chrome/browser/extensions/extension_icon_image.cc
@@ -6,13 +6,17 @@
 
 #include <vector>
 
+#include "base/message_loop_proxy.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/extensions/extension.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/image/image_skia_source.h"
 #include "ui/gfx/size.h"
 
+using content::BrowserThread;
+
 namespace {
 
 const int kMatchBiggerTreshold = 32;
@@ -38,7 +42,7 @@ namespace extensions {
 
 class IconImage::Source : public gfx::ImageSkiaSource {
  public:
-  explicit Source(IconImage* host);
+  explicit Source(IconImage* host, const gfx::ImageSkia& default_image);
   virtual ~Source();
 
   void ResetHost();
@@ -50,10 +54,15 @@ class IconImage::Source : public gfx::ImageSkiaSource {
 
   IconImage* host_;
 
+  // Image whose representation GetImageForScale will return if |host_| supplies
+  // a null representation.
+  gfx::ImageSkia default_image_;
+
   DISALLOW_COPY_AND_ASSIGN(Source);
 };
 
-IconImage::Source::Source(IconImage* host) : host_(host) {
+IconImage::Source::Source(IconImage* host, const gfx::ImageSkia& default_image)
+    : host_(host), default_image_(default_image) {
 }
 
 IconImage::Source::~Source() {
@@ -65,9 +74,15 @@ 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;
+
+  representation = default_image_.GetRepresentation(scale_factor);
+  return gfx::ImageSkiaRep(representation.sk_bitmap(), scale_factor);

[tbarzic, 2012/08/22 02:39:10]

pkotwicz: is it safe to assume that e.g.
ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(IDR_EXTENSIONS_FAVICON)
will have representations for all the scales we support?

Do you think we should do resize operation on representation if
representation/scale_factor doesn't match scale_factor, or just expect that
default_icon has all the representations we need?

[pkotwicz, 2012/08/22 23:17:55]

If the default icon comes from the resource bundle, it should be safe to assume
that it has all the scale factors we need.
Note that it MAY not right now but that's something which should be fixed.

 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -77,6 +92,7 @@ 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),
@@ -84,8 +100,9 @@ IconImage::IconImage(
       desired_size_in_dip_(resource_size_in_dip, resource_size_in_dip),
       observer_(observer),
       source_(NULL),
-      ALLOW_THIS_IN_INITIALIZER_LIST(tracker_(this)) {
-  source_ = new Source(this);
+      ALLOW_THIS_IN_INITIALIZER_LIST(tracker_(this)),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
+  source_ = new Source(this, default_icon);
   image_skia_ = gfx::ImageSkia(source_, desired_size_in_dip_);
 
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNLOADED,
@@ -98,10 +115,11 @@ IconImage::~IconImage() {
     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 =
@@ -122,13 +140,15 @@ void IconImage::LoadImageForScaleFactor(ui::ScaleFactor scale_factor) {
 
   // If there is no resource found, bail out and notify observer of failure.
   if (resource.empty()) {
-    if (observer_)
-      observer_->OnIconImageLoadFailed(this, scale_factor);
-    return;
+    base::MessageLoopProxy::current()->PostTask(FROM_HERE,
+        base::Bind(&IconImage::NotifyFail, weak_ptr_factory_.GetWeakPtr(),
+            scale_factor));
+    return gfx::ImageSkiaRep();
   }
 
   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(
@@ -137,6 +157,18 @@ void IconImage::LoadImageForScaleFactor(ui::ScaleFactor scale_factor) {
       desired_size_in_dip_.Scale(scale),
       scale_factor));
   tracker_.LoadImages(extension_, info_list, ImageLoadingTracker::DONT_CACHE);
+
+  // If requested image has been cached, we may already have the wanted
+  // representation.

[pkotwicz, 2012/08/22 23:17:55]

Nit: return representation if we have cached it a different scale factor.

[tbarzic, 2012/08/23 00:06:09]

are you sure, we could paint two different images in small time span (which is
just what Aaron had against returning default icon reps).

[pkotwicz, 2012/08/23 13:59:09]

I should have been clearer. My nit was about changing the wording of the
comment.

[pkotwicz, 2012/08/23 14:09:45]

Ok, I see what you are doing. Sorry about that. Change the comment to "If
LoadImages returned synchronously and the requested image rep is cached in the
extension return the cached image rep."

[tbarzic, 2012/08/23 17:41:53]

Done.

+  if (image_skia_.HasRepresentation(scale_factor))
+    return image_skia_.GetRepresentation(scale_factor);
+
+  // If we have not received |OnImageLoaded|, note that iamge load request is
+  // asynchronous.

[pkotwicz, 2012/08/22 23:17:55]

Nit: I think it might be a bit more understandable if you set the is_async flag
for both cases here.

[tbarzic, 2012/08/23 00:06:09]

Done.

+  if (load_map_.find(id) != load_map_.end())
+    load_map_[id].is_async = true;
+
+  return gfx::ImageSkiaRep();
 }
 
 void IconImage::OnImageLoaded(const gfx::Image& image,
@@ -145,26 +177,37 @@ void IconImage::OnImageLoaded(const gfx::Image& image,
   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);
+    base::MessageLoopProxy::current()->PostTask(FROM_HERE,
+        base::Bind(&IconImage::NotifyFail, weak_ptr_factory_.GetWeakPtr(),
+            scale_factor));
     return;
   }
 
   DCHECK(image.ToImageSkia()->HasRepresentation(scale_factor));
   gfx::ImageSkiaRep rep = image.ToImageSkia()->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 we still haven't returned the initial
+  // image representation.
+  if (is_async && observer_)
     observer_->OnExtensionIconImageChanged(this);
 }
 
+void IconImage::NotifyFail(ui::ScaleFactor scale_factor) {
+  if (observer_)
+    observer_->OnIconImageLoadFailed(this, scale_factor);
+}
+
 void IconImage::Observe(int type,
                         const content::NotificationSource& source,
                         const content::NotificationDetails& details) {