Remove some theme images that are not used any more.

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.h>
 #include <stddef.h>
 
 #include <limits>
@@ skipping 10 matching lines @@
 #include "base/threading/thread_restrictions.h"
 #include "base/values.h"
 #include "build/build_config.h"
 #include "chrome/browser/themes/theme_properties.h"
 #include "chrome/common/extensions/manifest_handlers/theme_handler.h"
 #include "chrome/grit/theme_resources.h"
 #include "components/crx_file/id_util.h"
 #include "content/public/browser/browser_thread.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "ui/base/resource/data_pack.h"
-#include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/geometry/safe_integer_conversions.h"
 #include "ui/gfx/geometry/size_conversions.h"
 #include "ui/gfx/image/canvas_image_source.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_skia_operations.h"
 #include "ui/gfx/skia_util.h"
 #include "ui/resources/grit/ui_resources.h"
@@ skipping 1190 matching lines @@
       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();
-
   // Create all the output images in a separate cache and move them back into
   // the input images because there can be name collisions.
   ImageCache temp_output;
 
   for (size_t i = 0; i < arraysize(kFrameTintMap); ++i) {
     int prs_id = kFrameTintMap[i].key;
     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.
-    int prs_base_id = 0;

[Evan Stade, 2016/12/13 17:40:11]

explanation of changes to this function.

1) This was a rather pointless assignment and then check that's always true.
2) we only need/use prs_base_id if there's no image for prs_id

-
-    if (!prs_base_id) {
+    if (images->count(prs_id)) {
+      frame = (*images)[prs_id];
+    } else {
+      // 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 theme colors.

[Peter Kasting, 2016/12/14 04:27:54]

Nit: The last sentence isn't really about what this code does, but what some
caller does.  If it says anything, it should maybe be "If that's not provided,
the caller will fall back to using the frame colors instead."

[Evan Stade, 2016/12/15 03:31:14]

Done.

+      int prs_base_id = prs_id;
       if (prs_id == PRS_THEME_FRAME_INCOGNITO_INACTIVE) {
         prs_base_id = images->count(PRS_THEME_FRAME_INCOGNITO) ?
                       PRS_THEME_FRAME_INCOGNITO : PRS_THEME_FRAME;
       } else if (prs_id == PRS_THEME_FRAME_OVERLAY_INACTIVE) {
         prs_base_id = PRS_THEME_FRAME_OVERLAY;
       } else if (prs_id == PRS_THEME_FRAME_INACTIVE) {
         prs_base_id = PRS_THEME_FRAME;
       } else if (prs_id == PRS_THEME_FRAME_INCOGNITO &&
                  !images->count(PRS_THEME_FRAME_INCOGNITO)) {
         prs_base_id = PRS_THEME_FRAME;
-      } else {
-        prs_base_id = prs_id;
       }

[Peter Kasting, 2016/12/14 04:27:54]

Nit: This isn't much better (it's just as long), but it does avoid duplicating
the logic for "if there is no incognito image, fall back to the normal image",
which seems nice.

      if (prs_base_id == PRS_THEME_FRAME_OVERLAY_INACTIVE) {
        prs_base_id = PRS_THEME_FRAME_OVERLAY;
      } else if (prs_base_id == PRS_THEME_FRAME_INACTIVE) {
        prs_base_id = PRS_THEME_FRAME;
      } else {
        if (prs_base_id == PRS_THEME_FRAME_INCOGNITO_INACTIVE)
          prs_base_id = PRS_THEME_FRAME_INCOGNITO;
        if (prs_base_id == PRS_THEME_FRAME_INCOGNITO &&
            !images->count(PRS_THEME_FRAME_INCOGNITO))
          prs_base_id = PRS_THEME_FRAME;
      }

[Evan Stade, 2016/12/15 03:31:14]

I further refined this for simplicity.

+      if (prs_base_id != prs_id && images->count(prs_base_id))
+        frame = (*images)[prs_base_id];
     }
-    if (images->count(prs_id)) {
-      frame = (*images)[prs_id];
-    } else if (prs_base_id != prs_id && images->count(prs_base_id)) {
-      frame = (*images)[prs_base_id];
-    } else if (prs_base_id == PRS_THEME_FRAME_OVERLAY) {
-      if (images->count(PRS_THEME_FRAME)) {
-        // 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 = gfx::Image();

[Evan Stade, 2016/12/13 17:40:11]

3) this was a pointless assignment. This entire branch could be rewritten

else if (prs_base_id != PRS_THEME_FRAME_OVERLAY) {
  frame = rb.GetImageNamed(IDR_THEME_FRAME);
}

4) however, even that I found pointless. I tried dozens of seemingly
frame-image-less themes and couldn't find one that didn't specify either a frame
image or a frame color (themes with solid color frames usually provide an
image). The only one I could find that was even hitting this code path was [1]
and it does fine without it, falling back to frame color.

bonus justification: for the use_ash condition, this was already using
notused.png, i.e. not doing anything, and to my knowledge we don't have any
outstanding bug reports.

[1]
https://chrome.google.com/webstore/detail/kalis-lake-meade-simple-t/lajcphnmk...

-      }
-    } else {
-      // If the theme doesn't specify an image, then apply the tint to
-      // the default frame.
-      frame = rb.GetImageNamed(IDR_THEME_FRAME);
-    }
+
     if (!frame.IsEmpty()) {
       temp_output[prs_id] = CreateHSLShiftedImage(
           frame, GetTintInternal(kFrameTintMap[i].value));
     }
   }
   MergeImageCaches(temp_output, images);
 }
 
 void BrowserThemePack::CreateTabBackgroundImages(ImageCache* images) const {
   ImageCache temp_output;
@@ skipping 179 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);
   }
 }