Fix bug in browser action icon scaling for non-1x scale factors.

chrome/browser/extensions/extension_action.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/extensions/extension_action.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/base64.h"
@@ skipping 111 matching lines @@
 
 void ExtensionAction::SetIcon(int tab_id, const gfx::Image& image) {
   SetValue(&icon_, tab_id, image);
 }
 
 bool ExtensionAction::ParseIconFromCanvasDictionary(
     const base::DictionaryValue& dict,
     gfx::ImageSkia* icon) {
   for (base::DictionaryValue::Iterator iter(dict); !iter.IsAtEnd();
        iter.Advance()) {
-    int icon_size = 0;
-    // Chrome helpfully scales the provided icon(s), but let's not go overboard.
-    const int kActionIconMaxSize = 10 * extension_misc::EXTENSION_ICON_ACTION;
-    if (!base::StringToInt(iter.key(), &icon_size) || icon_size <= 0 ||
-        icon_size > kActionIconMaxSize) {

[Devlin, 2016/01/26 19:08:48]

Do we not want any checks for ridiculous scaling?  (Agreed that the index into
the dictionary might not be useful, but we could check the bitmap size itself.)

[Evan Stade, 2016/01/26 19:50:07]

righto, baby rescued from bathwater

-      continue;
-    }
-
     const base::BinaryValue* image_data;
     std::string binary_string64;
     IPC::Message pickle;
     if (iter.value().GetAsBinary(&image_data)) {
       pickle = IPC::Message(image_data->GetBuffer(), image_data->GetSize());
     } else if (iter.value().GetAsString(&binary_string64)) {
       std::string binary_string;
       if (!base::Base64Decode(binary_string64, &binary_string))
         return false;
       pickle = IPC::Message(binary_string.c_str(), binary_string.length());
     } else {
       continue;
     }
     base::PickleIterator pickle_iter(pickle);
     SkBitmap bitmap;
     if (!IPC::ReadParam(&pickle, &pickle_iter, &bitmap))
       return false;
     CHECK(!bitmap.isNull());
+    if (bitmap.width() != bitmap.height())

[Devlin, 2016/01/26 19:08:48]

I wonder if we really *need* this to be a square.  But we've always kind of
assumed it was, so this is fine for now.

Could you add a
// TODO(devlin): Can we support rectangular icons?

[Evan Stade, 2016/01/26 19:50:07]

Why would we want to let developers do that? Is there any advantage?

If we determine we need to scale the icon, we'll scale it to square, which looks
bad if it's changing dimensions. If we don't think we need to scale (based on
width), we'll use it as is, which will look bad if height > width (the icon
would extend below the 16x16 box it's supposed to be inside).

[Devlin, 2016/01/26 19:58:25]

Well, we can scale it to be proportionate and maintain the aspect ratio, and
then center the icon.  This is already what any developer without a perfectly
square icon would do and then provide a square icon with a transparent
background, but we could save them that step.

But, low priority.

[Evan Stade, 2016/01/26 22:33:50]

IMO, better not to save them that step. It adds code complexity and hurts
performance.

+      continue;
     float scale =
-        static_cast<float>(icon_size) / ExtensionAction::ActionIconSize();
+        static_cast<float>(bitmap.width()) / ExtensionAction::ActionIconSize();
     icon->AddRepresentation(gfx::ImageSkiaRep(bitmap, scale));
   }
   return true;
 }
 
 gfx::Image ExtensionAction::GetExplicitlySetIcon(int tab_id) const {
   return GetValue(&icon_, tab_id);
 }
 
 bool ExtensionAction::SetIsVisible(int tab_id, bool new_visibility) {
@@ skipping 158 matching lines @@
   // If there is a default icon, the icon width will be set depending on our
   // action type.
   if (default_icon_)
     return ActionIconSize();
 
   // If no icon has been set and there is no default icon, we need favicon
   // width.
   return ui::ResourceBundle::GetSharedInstance().GetImageNamed(
           IDR_EXTENSIONS_FAVICON).Width();
 }