Only load theme images for the scale factors that use them

chrome/browser/themes/browser_theme_pack.cc

Index: chrome/browser/themes/browser_theme_pack.cc
diff --git a/chrome/browser/themes/browser_theme_pack.cc b/chrome/browser/themes/browser_theme_pack.cc
index 3a8fb68df6d7987446647d55572ff96063c37d1f..f7ce52bb665405bfd147e9707334903689aa3757 100644
--- a/chrome/browser/themes/browser_theme_pack.cc
+++ b/chrome/browser/themes/browser_theme_pack.cc
@@ -480,6 +480,102 @@ class ThemeImageSource: public gfx::ImageSkiaSource {
   DISALLOW_COPY_AND_ASSIGN(ThemeImageSource);
 };
 
+// An ImageSkiaSouce that delays decoding PNG data into bitmaps until

[pkotwicz, 2013/06/18 15:23:43]

Nit: ImageSkiaSource

[sschmitz, 2013/06/18 18:20:29]

Done.

+// needed and, if necessary, generates bitmaps for scale factors for
+// which no PNG was provided. Once a PNG is decoded the resulting
+// bitmap is stored for later use.
+class ThemeImagePngSource : public gfx::ImageSkiaSource {
+ public:
+  ThemeImagePngSource(const base::WeakPtr<BrowserThemePack>& btp, int id) :
+      browser_theme_pack_(btp),
+      idr_id_(id) {
+  }
+
+  virtual ~ThemeImagePngSource() {}
+
+ private:
+  virtual gfx::ImageSkiaRep GetImageForScale(
+      ui::ScaleFactor scale_factor) OVERRIDE {
+    // Look up the scale factor in the bitmap map. If found return it.
+    BitmapMap::const_iterator exact_bitmap = bitmap_map_.find(scale_factor);

[pkotwicz, 2013/06/18 15:23:43]

I personally like suffixing iterator variable names with "_it" to make it clear
that they are iterators.

For instance, I find it confusing that |available_bitmap| is an iterator but
|available_scale_factor| is not 

[sschmitz, 2013/06/18 18:20:29]

Done.

+    if (exact_bitmap != bitmap_map_.end())
+      return gfx::ImageSkiaRep(exact_bitmap->second, scale_factor);
+
+    if (!browser_theme_pack_.get())
+      return gfx::ImageSkiaRep(SkBitmap(), scale_factor);

[pkotwicz, 2013/06/18 15:23:43]

Nit: just use the empty constructor for ImageSkiaRep()

[sschmitz, 2013/06/18 18:20:29]

Done.

+
+    // Look up the scale factor in the browser theme pack. If found,

[pkotwicz, 2013/06/18 15:23:43]

How about: "Look up the raw PNG data for |idr_id_| and |scale_factor| in the
browser theme pack. If found ..."

[sschmitz, 2013/06/18 18:20:29]

Done.

+    // decode it, store the result in the bitmap map and return it.
+    scoped_refptr<base::RefCountedMemory> exact_memory =
+        browser_theme_pack_->GetRawData(idr_id_, scale_factor);
+    if (exact_memory.get()) {
+      SkBitmap bitmap;
+      if (!gfx::PNGCodec::Decode(exact_memory->front(),
+                                 exact_memory->size(),
+                                 &bitmap)) {
+        NOTREACHED();
+        return gfx::ImageSkiaRep(SkBitmap(), scale_factor);

[pkotwicz, 2013/06/18 15:23:43]

Nit: use the empty constructor

[sschmitz, 2013/06/18 18:20:29]

Done.

+      }
+      bitmap_map_[scale_factor] = bitmap;
+      return gfx::ImageSkiaRep(bitmap, scale_factor);
+    }
+
+    // No exact match was found, find an available png with highest scale.

[pkotwicz, 2013/06/18 15:23:43]

Nit: the highest scale factor

[sschmitz, 2013/06/18 18:20:29]

I changed to: "for the scale factor that corresponds to the highest scale."

+    ui::ScaleFactor available_scale_factor = ui::SCALE_FACTOR_NONE;
+    scoped_refptr<base::RefCountedMemory> available_png =
+        browser_theme_pack_->GetRawDataWithHighestScale(
+            idr_id_,
+            &available_scale_factor);
+    // No png found for this id, return null rep.
+    if (!available_png.get())
+      return gfx::ImageSkiaRep(SkBitmap(), scale_factor);
+
+    // Look up the found scale factor in the bitmap map. If not found, decode
+    // the corresponding png and store the result in bitmap map.

[pkotwicz, 2013/06/18 15:23:43]

Nit: "the bitmap map" or "|bitmap_map_|".

[sschmitz, 2013/06/18 18:20:29]

Done.

+    BitmapMap::const_iterator available_bitmap =
+        bitmap_map_.find(available_scale_factor);
+    if (available_bitmap == bitmap_map_.end()) {
+      SkBitmap bitmap;
+      if (!gfx::PNGCodec::Decode(available_png->front(),
+                                 available_png->size(),
+                                 &bitmap)) {
+        NOTREACHED();
+        return gfx::ImageSkiaRep(SkBitmap(), scale_factor);
+      }
+      bitmap_map_[available_scale_factor] = bitmap;
+      available_bitmap = bitmap_map_.find(available_scale_factor);
+    }
+
+    // Use available bitmap to create bitmap for desired scale factor by
+    // scaling, store the result in the bitmap map and return it.
+    gfx::Size scaled_size = gfx::ToCeiledSize(

[pkotwicz, 2013/06/18 15:23:43]

Nit: Pull out the resizing code into a function in the anonymous namespace. Call
the method CreateLowQualityResizedBitmap();

[sschmitz, 2013/06/18 18:20:29]

Done.

+        gfx::ScaleSize(gfx::Size(available_bitmap->second.width(),
+                                 available_bitmap->second.height()),
+                       ui::GetScaleFactorScale(scale_factor) /
+                       ui::GetScaleFactorScale(available_scale_factor)));
+    SkBitmap scaled_bitmap;
+    scaled_bitmap.setConfig(SkBitmap::kARGB_8888_Config,
+                            scaled_size.width(),
+                            scaled_size.height());
+    if (!scaled_bitmap.allocPixels())
+      SK_CRASH();
+    scaled_bitmap.eraseARGB(0, 0, 0, 0);
+    SkCanvas canvas(scaled_bitmap);
+    SkRect scaled_bounds = RectToSkRect(gfx::Rect(scaled_size));
+    canvas.drawBitmapRect(available_bitmap->second, NULL, scaled_bounds);
+    bitmap_map_[scale_factor] = scaled_bitmap;
+    return gfx::ImageSkiaRep(scaled_bitmap, scale_factor);
+  }
+
+ private:
+  typedef std::map<ui::ScaleFactor, SkBitmap> BitmapMap;
+  base::WeakPtr<BrowserThemePack> browser_theme_pack_;
+  const int idr_id_;
+  BitmapMap bitmap_map_;

[pkotwicz, 2013/06/18 15:23:43]

Nit: Move the typedef to the line above |bitmap_map_|.

[sschmitz, 2013/06/18 18:20:29]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(ThemeImagePngSource);
+};
+
 class TabBackgroundImageSource: public gfx::CanvasImageSource {
  public:
   TabBackgroundImageSource(const gfx::ImageSkia& image_to_tint,
@@ -520,6 +616,14 @@ class TabBackgroundImageSource: public gfx::CanvasImageSource {
   DISALLOW_COPY_AND_ASSIGN(TabBackgroundImageSource);
 };
 
+class OrderByScale {
+ public:
+  bool operator()(const ui::ScaleFactor& a,
+                  const ui::ScaleFactor& b) const {
+    return ui::GetScaleFactorScale(a) > ui::GetScaleFactorScale(b);
+  }
+};
+
 }  // namespace
 
 BrowserThemePack::~BrowserThemePack() {
@@ -749,35 +853,12 @@ gfx::Image BrowserThemePack::GetImageNamed(int idr_id) {
   if (image_iter != images_on_ui_thread_.end())
     return image_iter->second;
 
-  // TODO(pkotwicz): Do something better than loading the bitmaps
-  // for all the scale factors associated with |idr_id|.
-  // See crbug.com/243831.
-  gfx::ImageSkia source_image_skia;
-  for (size_t i = 0; i < scale_factors_.size(); ++i) {
-    scoped_refptr<base::RefCountedMemory> memory =
-        GetRawData(idr_id, scale_factors_[i]);
-    if (memory.get()) {
-      // Decode the PNG.
-      SkBitmap bitmap;
-      if (!gfx::PNGCodec::Decode(memory->front(), memory->size(),
-                                 &bitmap)) {
-        NOTREACHED() << "Unable to decode theme image resource " << idr_id
-                     << " from saved DataPack.";
-        continue;
-      }
-      source_image_skia.AddRepresentation(
-          gfx::ImageSkiaRep(bitmap, scale_factors_[i]));
-    }
-  }
-
-  if (!source_image_skia.isNull()) {
-    ThemeImageSource* source = new ThemeImageSource(source_image_skia);
-    gfx::ImageSkia image_skia(source, source_image_skia.size());
-    gfx::Image ret = gfx::Image(image_skia);
-    images_on_ui_thread_[prs_id] = ret;
-    return ret;
-  }
-  return gfx::Image();
+  gfx::ImageSkia image_skia(

[pkotwicz, 2013/06/18 15:23:43]

Nit: Comment that the gfx::ImageSkia machinery will make |ret| IsEmpty() if
there is no data for |idr_id| in the data pack.

[sschmitz, 2013/06/18 18:20:29]

Done.

+      new ThemeImagePngSource(weak_ptr_factory_.GetWeakPtr(), idr_id),
+      ui::SCALE_FACTOR_100P);
+  gfx::Image ret = gfx::Image(image_skia);
+  images_on_ui_thread_[prs_id] = ret;
+  return ret;
 }
 
 base::RefCountedMemory* BrowserThemePack::GetRawData(
@@ -801,6 +882,21 @@ base::RefCountedMemory* BrowserThemePack::GetRawData(
   return memory;
 }
 
+base::RefCountedMemory* BrowserThemePack::GetRawDataWithHighestScale(

[pkotwicz, 2013/06/18 15:23:43]

This is only used by ThemeImagePngSource. Can this be a member of
ThemeImagePNGSource? You may need to add an accessor for |scale_factors_|.

[sschmitz, 2013/06/18 18:20:29]

Done.

+    int idr_id,
+    ui::ScaleFactor* scale_factor) const {
+  for (std::vector<ui::ScaleFactor>::const_reverse_iterator it =
+           scale_factors_ordered_by_scale_.rbegin();
+       it != scale_factors_ordered_by_scale_.rend(); ++it) {
+    if (base::RefCountedMemory* memory = GetRawData(idr_id, *it)) {
+      if (scale_factor)
+        *scale_factor = *it;
+      return memory;
+    }
+  }
+  return NULL;
+}
+
 // static
 void BrowserThemePack::GetThemeableImageIDRs(std::set<int>* result) {
   if (!result)
@@ -837,8 +933,13 @@ BrowserThemePack::BrowserThemePack()
       tints_(NULL),
       colors_(NULL),
       display_properties_(NULL),
-      source_images_(NULL) {
+      source_images_(NULL),
+      weak_ptr_factory_(this) {
   scale_factors_ = ui::GetSupportedScaleFactors();

[pkotwicz, 2013/06/18 15:23:43]

Nit: The result of ui::GetSupportedScaleFactors() is already sorted in ascending
order.

[sschmitz, 2013/06/18 18:20:29]

Done.

+  scale_factors_ordered_by_scale_ = scale_factors_;
+  std::sort(scale_factors_ordered_by_scale_.begin(),
+            scale_factors_ordered_by_scale_.end(),
+            OrderByScale());
 }
 
 void BrowserThemePack::BuildHeader(const Extension* extension) {