Crop images from custom themes before storing them into the theme pack

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 29c306fc8004b09c36db08df386888fdbe1bfc87..7368e37ae963195b1241ae618d4f06b2a957d78c 100644
--- a/chrome/browser/themes/browser_theme_pack.cc
+++ b/chrome/browser/themes/browser_theme_pack.cc
@@ -329,7 +329,7 @@ struct IntToIntTable {
   int value;
 };
 
-// Mapping used in GenerateFrameImages() to associate frame images with the
+// Mapping used in CreateFrameImages() to associate frame images with the
 // tint ID that should maybe be applied to it.
 IntToIntTable kFrameTintMap[] = {
   { PRS_THEME_FRAME, ThemeProperties::TINT_FRAME },
@@ -360,22 +360,59 @@ IntToIntTable kTabBackgroundMap[] = {
 #endif
 };
 
+struct CropEntry {
+  int prs_id;
+
+  // The maximum useful height of the image at |prs_id|.
+  int max_height;
+
+  // Whether cropping the image at |prs_id| should be skipped on OSes which
+  // have a frame border to the left and right of the web contents.
+  // This should be true for images which can be used to decorate the border to
+  // the left and the right of the web contents.
+  bool skip_if_frame_border;
+};
+
+// The images which should be cropped before being saved to the data pack. The
+// maximum heights are meant to be conservative as to give room for the UI to
+// change without the maximum heights having to be modified.
+// |kThemePackVersion| must be incremented if any of the maximum heights below
+// are modified.
+struct CropEntry kImagesToCrop[] = {
+  { PRS_THEME_FRAME, 120, true },
+  { PRS_THEME_FRAME_INACTIVE, 120, true },
+  { PRS_THEME_FRAME_INCOGNITO, 120, true },
+  { PRS_THEME_FRAME_INCOGNITO_INACTIVE, 120, true },
+  { PRS_THEME_FRAME_OVERLAY, 120, true },
+  { PRS_THEME_FRAME_OVERLAY_INACTIVE, 120, true },
+  { PRS_THEME_TOOLBAR, 200, false },
+  { PRS_THEME_BUTTON_BACKGROUND, 60, false },
+  { PRS_THEME_WINDOW_CONTROL_BACKGROUND, 50, false }
+#if defined(OS_WIN) && defined(USE_AURA)
+  { PRS_THEME_TOOLBAR_WIN, 200, false }
+#endif
+};
+
 
 // A list of images that don't need tinting or any other modification and can
 // be byte-copied directly into the finished DataPack. This should contain the
-// persistent IDs for all themeable image IDs that aren't in kFrameTintMap or
-// kTabBackgroundMap.
+// persistent IDs for all themeable image IDs that aren't in kFrameTintMap,
+// kTabBackgroundMap or kImagesToCrop.
 const int kPreloadIDs[] = {
-  PRS_THEME_TOOLBAR,
   PRS_THEME_NTP_BACKGROUND,
-  PRS_THEME_BUTTON_BACKGROUND,
   PRS_THEME_NTP_ATTRIBUTION,
-  PRS_THEME_WINDOW_CONTROL_BACKGROUND,
-#if defined(OS_WIN) && defined(USE_AURA)
-  PRS_THEME_TOOLBAR_WIN,
-#endif
 };
 
+// Returns true if this OS uses a browser frame which has a non zero width to
+// the left and the right of the web contents.
+bool HasFrameBorder() {
+#if defined(OS_CHROMEOS) || defined(OS_MACOSX)
+  return false;
+#else
+  return true;
+#endif
+}
+
 // Returns a piece of memory with the contents of the file |path|.
 base::RefCountedMemory* ReadFileData(const base::FilePath& path) {
   if (!path.empty()) {
@@ -399,10 +436,10 @@ base::RefCountedMemory* ReadFileData(const base::FilePath& path) {
 
 // Shifts an image's HSL values. The caller is responsible for deleting
 // the returned image.
-gfx::Image* CreateHSLShiftedImage(const gfx::Image& image,
-                                  const color_utils::HSL& hsl_shift) {
+gfx::Image CreateHSLShiftedImage(const gfx::Image& image,
+                                 const color_utils::HSL& hsl_shift) {
   const gfx::ImageSkia* src_image = image.ToImageSkia();
-  return new gfx::Image(gfx::ImageSkiaOperations::CreateHSLShiftedImage(
+  return gfx::Image(gfx::ImageSkiaOperations::CreateHSLShiftedImage(
       *src_image, hsl_shift));
 }
 
@@ -492,9 +529,6 @@ BrowserThemePack::~BrowserThemePack() {
     delete [] display_properties_;
     delete [] source_images_;
   }
-
-  STLDeleteValues(&images_on_ui_thread_);
-  STLDeleteValues(&images_on_file_thread_);
 }
 
 // static
@@ -522,7 +556,7 @@ scoped_refptr<BrowserThemePack> BrowserThemePack::BuildFromExtension(
   if (!pack->LoadRawBitmapsTo(file_paths, &pack->images_on_ui_thread_))
     return NULL;
 
-  pack->CopyImagesTo(pack->images_on_ui_thread_, &pack->images_on_file_thread_);
+  pack->images_on_file_thread_ = pack->images_on_ui_thread_;
 
   pack->CreateImages(&pack->images_on_ui_thread_);
   pack->CreateImages(&pack->images_on_file_thread_);

[Elliot Glaysher, 2013/04/03 17:23:35]

I think that somethings gone a little screwy here. It looks like
|images_on_file_thread_| and |images_on_ui_thread_| are now identical. And
you're creating all the images twice?

[pkotwicz, 2013/04/03 20:27:22]

You are right that this is suboptimal. I will take a pass to fix this better
than I did in this patch set in another CL. In particular, I will do the fix
before we launch hidpi Windows.
We were doing lazy loading so it was not quite as bad as creating the images
twice sequentially (but still pretty bad).

@@ -532,7 +566,7 @@ scoped_refptr<BrowserThemePack> BrowserThemePack::BuildFromExtension(
   for (ImageCache::iterator it = pack->images_on_file_thread_.begin();
        it != pack->images_on_file_thread_.end(); ++it) {
     gfx::ImageSkia* image_skia =
-        const_cast<gfx::ImageSkia*>(it->second->ToImageSkia());
+        const_cast<gfx::ImageSkia*>(it->second.ToImageSkia());
     image_skia->MakeThreadSafe();
   }
 
@@ -686,10 +720,10 @@ bool BrowserThemePack::GetDisplayProperty(int id, int* result) const {
   return false;
 }
 
-const gfx::Image* BrowserThemePack::GetImageNamed(int idr_id) const {
+gfx::Image BrowserThemePack::GetImageNamed(int idr_id) {
   int prs_id = GetPersistentIDByIDR(idr_id);
   if (prs_id == -1)
-    return NULL;
+    return gfx::Image();
 
   // Check if the image is cached.
   ImageCache::const_iterator image_iter = images_on_ui_thread_.find(prs_id);
@@ -719,11 +753,11 @@ const gfx::Image* BrowserThemePack::GetImageNamed(int idr_id) const {
   if (!source_image_skia.isNull()) {
     ThemeImageSource* source = new ThemeImageSource(source_image_skia);
     gfx::ImageSkia image_skia(source, source_image_skia.size());
-    gfx::Image* ret = new gfx::Image(image_skia);
+    gfx::Image ret = gfx::Image(image_skia);
     images_on_ui_thread_[prs_id] = ret;
     return ret;
   }
-  return NULL;
+  return gfx::Image();
 }
 
 base::RefCountedMemory* BrowserThemePack::GetRawData(
@@ -1093,7 +1127,7 @@ bool BrowserThemePack::LoadRawBitmapsTo(
       if (gfx::PNGCodec::Decode(raw_data->front(), raw_data->size(),
                                 &bitmap)) {
         (*image_cache)[prs_id] =
-            new gfx::Image(gfx::ImageSkia::CreateFrom1xBitmap(bitmap));
+            gfx::Image(gfx::ImageSkia::CreateFrom1xBitmap(bitmap));
       } else {
         NOTREACHED() << "Unable to decode theme image resource " << it->first;
       }
@@ -1104,11 +1138,30 @@ bool BrowserThemePack::LoadRawBitmapsTo(
 }
 
 void BrowserThemePack::CreateImages(ImageCache* images) const {
+  CropImages(images);
   CreateFrameImages(images);
   CreateTintedButtons(GetTintInternal(ThemeProperties::TINT_BUTTONS), images);
   CreateTabBackgroundImages(images);
 }
 
+void BrowserThemePack::CropImages(ImageCache* images) const {
+  bool has_frame_border = HasFrameBorder();
+  for (size_t i = 0; i < arraysize(kImagesToCrop); ++i) {
+    if (has_frame_border && kImagesToCrop[i].skip_if_frame_border)
+      continue;
+
+    int prs_id = kImagesToCrop[i].prs_id;
+    ImageCache::iterator it = images->find(prs_id);
+    if (it == images->end())
+      continue;
+
+    int crop_height = kImagesToCrop[i].max_height;
+    gfx::ImageSkia image_skia = it->second.AsImageSkia();
+    (*images)[prs_id] = gfx::Image(gfx::ImageSkiaOperations::ExtractSubset(
+        image_skia, gfx::Rect(0, 0, image_skia.width(), crop_height)));
+  }
+}
+
 void BrowserThemePack::CreateFrameImages(ImageCache* images) const {
   ResourceBundle& rb = ResourceBundle::GetSharedInstance();
 
@@ -1118,7 +1171,7 @@ void BrowserThemePack::CreateFrameImages(ImageCache* images) const {
 
   for (size_t i = 0; i < arraysize(kFrameTintMap); ++i) {
     int prs_id = kFrameTintMap[i].key;
-    const gfx::Image* frame = NULL;
+    gfx::Image frame;
     // If there's no frame image provided for the specified id, then load
     // the default provided frame. If that's not provided, skip this whole
     // thing and just use the default images.
@@ -1163,22 +1216,22 @@ void BrowserThemePack::CreateFrameImages(ImageCache* images) const {
         // If there is no theme overlay, don't tint the default frame,
         // because it will overwrite the custom frame image when we cache and
         // reload from disk.
-        frame = NULL;
+        frame = gfx::Image();
       }
     } else {
       // If the theme doesn't specify an image, then apply the tint to
       // the default frame.
-      frame = &rb.GetImageNamed(IDR_THEME_FRAME);
+      frame = rb.GetImageNamed(IDR_THEME_FRAME);
 #if defined(OS_WIN) && defined(USE_AURA)
       if (prs_id >= PRS_THEME_FRAME_WIN &&
           prs_id <= PRS_THEME_FRAME_INCOGNITO_INACTIVE_WIN) {
-        frame = &rb.GetImageNamed(IDR_THEME_FRAME_WIN);
+        frame = rb.GetImageNamed(IDR_THEME_FRAME_WIN);
       }
 #endif
     }
-    if (frame) {
+    if (!frame.IsEmpty()) {
       temp_output[prs_id] = CreateHSLShiftedImage(
-          *frame, GetTintInternal(kFrameTintMap[i].value));
+          frame, GetTintInternal(kFrameTintMap[i].value));
     }
   }
   MergeImageCaches(temp_output, images);
@@ -1216,7 +1269,7 @@ void BrowserThemePack::CreateTabBackgroundImages(ImageCache* images) const {
     // with a PRS_THEME_FRAME.
     ImageCache::const_iterator it = images->find(prs_base_id);
     if (it != images->end()) {
-      const gfx::ImageSkia* image_to_tint = (it->second)->ToImageSkia();
+      gfx::ImageSkia image_to_tint = (it->second).AsImageSkia();
       color_utils::HSL hsl_shift = GetTintInternal(
           ThemeProperties::TINT_BACKGROUND_TAB);
       int vertical_offset = images->count(prs_id)
@@ -1225,13 +1278,13 @@ void BrowserThemePack::CreateTabBackgroundImages(ImageCache* images) const {
       gfx::ImageSkia overlay;
       ImageCache::const_iterator overlay_it = images->find(prs_id);
       if (overlay_it != images->end())
-        overlay = *overlay_it->second->ToImageSkia();
+        overlay = overlay_it->second.AsImageSkia();
 
       gfx::ImageSkiaSource* source = new TabBackgroundImageSource(
-          *image_to_tint, overlay, hsl_shift, vertical_offset);
+          image_to_tint, overlay, hsl_shift, vertical_offset);
       // ImageSkia takes ownership of |source|.
-      temp_output[prs_id] = new gfx::Image(gfx::ImageSkia(source,
-          image_to_tint->size()));
+      temp_output[prs_id] = gfx::Image(gfx::ImageSkia(source,
+          image_to_tint.size()));
     }
   }
   MergeImageCaches(temp_output, images);
@@ -1242,7 +1295,7 @@ void BrowserThemePack::RepackImages(const ImageCache& images,
   typedef std::vector<ui::ScaleFactor> ScaleFactors;
   for (ImageCache::const_iterator it = images.begin();
        it != images.end(); ++it) {
-    gfx::ImageSkia image_skia = *it->second->ToImageSkia();
+    gfx::ImageSkia image_skia = *it->second.ToImageSkia();
 
     typedef std::vector<gfx::ImageSkiaRep> ImageSkiaReps;
     ImageSkiaReps image_reps = image_skia.image_reps();
@@ -1268,22 +1321,10 @@ void BrowserThemePack::MergeImageCaches(
     const ImageCache& source, ImageCache* destination) const {
   for (ImageCache::const_iterator it = source.begin(); it != source.end();
        ++it) {
-    ImageCache::const_iterator image_it = destination->find(it->first);
-    if (image_it != destination->end())
-      delete image_it->second;
-
     (*destination)[it->first] = it->second;
   }
 }
 
-void BrowserThemePack::CopyImagesTo(const ImageCache& source,
-                                    ImageCache* destination) const {
-  for (ImageCache::const_iterator it = source.begin(); it != source.end();
-       ++it) {
-    (*destination)[it->first] = new gfx::Image(*it->second);
-  }
-}
-
 void BrowserThemePack::AddRawImagesTo(const RawImages& images,
                                       RawDataForWriting* out) const {
   for (RawImages::const_iterator it = images.begin(); it != images.end();