Only load theme images for the scale factors that use them

chrome/browser/themes/browser_theme_pack.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/themes/browser_theme_pack.h"
 
 #include <limits>
 
 #include "base/memory/ref_counted_memory.h"
 #include "base/memory/scoped_ptr.h"
@@ skipping 462 matching lines @@
     canvas.drawBitmapRect(rep_100p.sk_bitmap(), NULL, resized_bounds);
     return gfx::ImageSkiaRep(resized_bitmap, scale_factor);
   }
 
  private:
   const gfx::ImageSkia source_;
 
   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,
                            const gfx::ImageSkia& overlay,
                            const color_utils::HSL& hsl_shift,
                            int vertical_offset)
       : gfx::CanvasImageSource(image_to_tint.size(), false),
         image_to_tint_(image_to_tint),
         overlay_(overlay),
         hsl_shift_(hsl_shift),
@@ skipping 20 matching lines @@
 
  private:
   const gfx::ImageSkia image_to_tint_;
   const gfx::ImageSkia overlay_;
   const color_utils::HSL hsl_shift_;
   const int vertical_offset_;
 
   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() {
   if (!data_pack_.get()) {
     delete header_;
     delete [] tints_;
     delete [] colors_;
     delete [] display_properties_;
     delete [] source_images_;
   }
@@ skipping 209 matching lines @@
 gfx::Image BrowserThemePack::GetImageNamed(int idr_id) {
   int prs_id = GetPersistentIDByIDR(idr_id);
   if (prs_id == -1)
     return gfx::Image();
 
   // Check if the image is cached.
   ImageCache::const_iterator image_iter = images_on_ui_thread_.find(prs_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(
     int idr_id,
     ui::ScaleFactor scale_factor) const {
   base::RefCountedMemory* memory = NULL;
   int prs_id = GetPersistentIDByIDR(idr_id);
   int raw_id = GetRawIDByPersistentID(prs_id, scale_factor);
 
   if (raw_id != -1) {
     if (data_pack_.get()) {
       memory = data_pack_->GetStaticMemory(raw_id);
     } else {
       RawImages::const_iterator it = image_memory_.find(raw_id);
       if (it != image_memory_.end()) {
         memory = it->second.get();
       }
     }
   }
 
   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)
     return;
 
   result->clear();
   for (size_t i = 0; i < kPersistingImagesLength; ++i)
     result->insert(kPersistingImages[i].idr_id);
 
 #if defined(OS_WIN) && defined(USE_AURA)
@@ skipping 16 matching lines @@
   return false;
 }
 
 // private:
 
 BrowserThemePack::BrowserThemePack()
     : header_(NULL),
       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) {
   header_ = new BrowserThemePackHeader;
   header_->version = kThemePackVersion;
 
   // TODO(erg): Need to make this endian safe on other computers. Prerequisite
   // is that ui::DataPack removes this same check.
 #if defined(__BYTE_ORDER)
   // Linux check
@@ skipping 674 matching lines @@
                                            false,
                                            &bitmap_data)) {
       NOTREACHED() << "Unable to encode theme image for prs_id="
                    << prs_id << " for scale_factor=" << scale_factors_[i];
       break;
     }
     image_memory_[scaled_raw_id] =
         base::RefCountedBytes::TakeVector(&bitmap_data);
   }
 }