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

chrome/browser/extensions/extension_icon_image.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/extension_icon_image.h"
+
+#include <vector>
+
+#include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/extensions/extension.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"
+
+namespace {
+
+ExtensionResource GetExtensionIconResource(
+    const extensions::Extension* extension,
+    const ExtensionIconSet& icons,
+    int size,
+    ExtensionIconSet::MatchType match_type) {
+  std::string path = icons.Get(size, match_type);
+  if (path.empty())
+    return ExtensionResource();
+
+  return extension->GetResource(path);
+}
+
+}  // namespace
+
+namespace extensions {
+
+////////////////////////////////////////////////////////////////////////////////
+// ExtensionIconImage::Source
+
+class IconImage::Source : public gfx::ImageSkiaSource {
+ public:
+  explicit Source(IconImage* host);
+  virtual ~Source();
+
+  void ResetHost();
+
+ private:
+  // gfx::ImageSkiaSource overrides:
+  virtual gfx::ImageSkiaRep GetImageForScale(
+      ui::ScaleFactor scale_factor) OVERRIDE;
+
+  IconImage* host_;
+
+  DISALLOW_COPY_AND_ASSIGN(Source);
+};
+
+IconImage::Source::Source(IconImage* host) : host_(host) {
+}
+
+IconImage::Source::~Source() {
+}
+
+void IconImage::Source::ResetHost() {
+  host_ = NULL;
+}
+
+gfx::ImageSkiaRep IconImage::Source::GetImageForScale(
+    ui::ScaleFactor scale_factor) {
+  if (host_)
+    host_->LoadImageForScaleFactor(scale_factor);
+  return gfx::ImageSkiaRep();
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// ExtensionIconImage
+
+IconImage::IconImage(
+    const Extension* extension,
+    const ExtensionIconSet& icon_set,
+    int resource_size_in_dip,
+    ExtensionIconSet::MatchType resource_match_type,
+    const gfx::Size& desired_size_in_dip,
+    ImageLoadingTracker::CacheParam cache_param,
+    Observer* observer)
+    : extension_(extension),
+      icon_set_(icon_set),
+      resource_size_in_dip_(resource_size_in_dip),

[Aaron Boodman, 2012/08/10 21:42:08]

I don't understand why resource_size_in_dip is necessary. Why not just tell this
class the desired size and let it pass this through to ILT?

[tbarzic, 2012/08/10 23:01:23]

semantically, those have different meanings:
resource_size_in_dip is the size of the extension resource we are loading,
while desired_size_in_dip is the final size of the loaded image.

If you think we can assume these values should always be equal, I'd be happy to
get rid of one of those two :)
(I'm not to confident about this claim: it could easily happen that we e.g. need
17 px icon and load 16 px resource for it; or that we want non-square icon)

+      resource_match_type_(resource_match_type),
+      desired_size_in_dip_(desired_size_in_dip),
+      cache_param_(cache_param),
+      observer_(observer),
+      source_(NULL),
+      ALLOW_THIS_IN_INITIALIZER_LIST(tracker_(this)) {
+  source_ = new Source(this);
+  image_skia_ = gfx::ImageSkia(source_, desired_size_in_dip_);
+
+  registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNLOADED,
+                 content::NotificationService::AllSources());
+}
+
+IconImage::~IconImage() {
+  // |source_| could be NULL if resource does not exist.
+  if (source_)
+    source_->ResetHost();
+}
+
+void IconImage::LoadImageForScaleFactor(ui::ScaleFactor scale_factor) {
+  // Do nothing if extension is unloaded.
+  if (!extension_)
+    return;
+
+  const float scale = ui::GetScaleFactorScale(scale_factor);
+  const int resource_size_in_pixel =
+      static_cast<int>(resource_size_in_dip_ * scale);
+
+  ExtensionResource resource = GetExtensionIconResource(
+      extension_, icon_set_, resource_size_in_pixel, resource_match_type_);
+
+  int id = tracker_.next_id();
+  load_map_[id] = scale_factor;
+
+  std::vector<ImageLoadingTracker::ImageRepresentation> info_list;
+  info_list.push_back(ImageLoadingTracker::ImageRepresentation(
+      resource,
+      ImageLoadingTracker::ImageRepresentation::ALWAYS_RESIZE,
+      desired_size_in_dip_.Scale(scale),
+      scale_factor));
+  tracker_.LoadImages(extension_, info_list, cache_param_);
+}
+
+void IconImage::OnImageLoaded(const gfx::Image& image,
+                              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;
+
+  load_map_.erase(load_map_it);
+
+  // If we were unable to load desired image, fallback to 1x scale factor.
+  if (image.IsEmpty()) {

[Aaron Boodman, 2012/08/10 21:42:08]

I think that resource_match_type_ can handle this fallback for us naturally.
There's no need for a separate fallback.

[xiyuan, 2012/08/10 22:03:44]

If |resource_match_type_| could handle the fallback, ILT would load something
and image.IsEmpty() would not be true here. However, that is not always the
case. 

e.g. suppose extension has only 16x16 and 24x24 icons and we asks for 16x16
(DIP) with MATCH_BIGGER. For 1x, we get 16x16 correctly. Then for 2x, ILT would
fail because we would ask for 32x32 (pixel) with MATCH_BIGGER and that does not
exist.

If we want to use |resource_match_type_| for the fallback, I imagine it might be
something like trying other match types when current one does not give us
anything. i.e. if MATCH_BIGGER does not give us anything, try MATCH_SMALLER
here. 

Think we need a fallback way here. But not sure which way is better though.

+    if (scale_factor != ui::SCALE_FACTOR_100P &&
+        !image_skia_.HasRepresentation(ui::SCALE_FACTOR_100P)) {
+      LoadImageForScaleFactor(ui::SCALE_FACTOR_100P);
+    } else if (scale_factor == ui::SCALE_FACTOR_100P) {
+      // TODO(tbarzic): We should probably use a default image at this point.
+      LOG(WARNING) << "Unable to load icon for the extension " << extension_id;
+    }
+    return;
+  }
+
+  DCHECK(image.ToImageSkia()->HasRepresentation(scale_factor));
+  gfx::ImageSkiaRep rep = image.ToImageSkia()->GetRepresentation(scale_factor);
+  DCHECK(!rep.is_null());
+  image_skia_.AddRepresentation(rep);
+
+  if (observer_)
+    observer_->OnExtensionIconImageChanged(this);
+}
+
+void IconImage::Observe(int type,
+                        const content::NotificationSource& source,
+                        const content::NotificationDetails& details) {
+  DCHECK_EQ(type, chrome::NOTIFICATION_EXTENSION_UNLOADED);
+
+  const Extension* extension =
+      content::Details<extensions::UnloadedExtensionInfo>(details)->extension;
+
+  if (extension_ == extension)
+    extension_ = NULL;
+}
+
+}  // namespace extensions