Ensure that the extension icons show up in the omnibox at fractional scales.

extensions/browser/extension_icon_image.cc

 // Copyright 2014 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 "extensions/browser/extension_icon_image.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "content/public/browser/notification_service.h"
 #include "extensions/browser/image_loader.h"
 #include "extensions/browser/notification_types.h"
 #include "extensions/common/extension.h"
+#include "ui/base/layout.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/geometry/size.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_operations.h"
 #include "ui/gfx/image/image_skia_source.h"
 
 // The ImageSkia provided by extensions::IconImage contains ImageSkiaReps that
 // are computed and updated using the following algorithm (if no default icon
@@ skipping 192 matching lines @@
       image_in.IsEmpty() ? &default_icon_ : image_in.ToImageSkia();
 
   // Maybe default icon was not set.
   if (image->isNull())
     return;
 
   gfx::ImageSkiaRep rep = image->GetRepresentation(scale);
   DCHECK(!rep.is_null());
   DCHECK_EQ(scale, rep.scale());
 
-  // Remove old representation if there is one.
+  // Remove fractional scale image representations as they may have become
+  // stale here. These images are generated by ImageSkia on request from
+  // supported scales like 1x, 2x, etc.
+  // TODO(oshima)
+  // A better approach might be to set the |image_| member using the ImageSkia
+  // copy from the image passed in and set the |image_skia_| member using
+  // image_.ToImageSkia(). However that does not work correctly as the
+  // ImageSkia from the image does not contain a source which breaks requests
+  // for scaled images.
+  std::vector<gfx::ImageSkiaRep> reps = image_skia_.image_reps();
+  for (const auto rep : reps) {

[brucedawson, 2015/03/26 18:44:37]

The declaration of 'rep' should, I believe, be "const auto& rep" in order to
avoid a gratuitous copy of the ImageSkiaRep object. The copy isn't super
expensive, but still probably worth avoiding.

Also, 'rep' shadows the same-named variable in the outer scope. This isn't a bug
but it can lead to confusion about which variable was intended. Consider
renaming the inner 'rep'? I noticed this because of a new variable shadowing
warning from /analyze.

[ananta, 2015/03/26 18:48:20]

Will address these in a followup

Thanks
Ananta

+    if (!ui::IsSupportedScale(rep.scale()))
+      image_skia_.RemoveRepresentation(rep.scale());
+  }
+
   image_skia_.RemoveRepresentation(scale);
   image_skia_.AddRepresentation(rep);
 
   // Update the image to use the updated image skia.
   // It's a shame we have to do this because it means that all the other
   // representations stored on |image_| will be deleted, but unfortunately
   // there's no way to combine the storage of two images.
   image_ = gfx::Image(image_skia_);
 
   FOR_EACH_OBSERVER(Observer, observers_, OnExtensionIconImageChanged(this));
 }
 
 void IconImage::Observe(int type,
                         const content::NotificationSource& source,
                         const content::NotificationDetails& details) {
   DCHECK_EQ(type, extensions::NOTIFICATION_EXTENSION_REMOVED);
 
   const Extension* extension = content::Details<const Extension>(details).ptr();
 
   if (extension_ == extension)
     extension_ = NULL;
 }
 
 }  // namespace extensions