Update browserAction.setIcon and pageAction.setIcon for hidpi

chrome/browser/extensions/api/extension_action/extension_actions_api.cc

Index: chrome/browser/extensions/api/extension_action/extension_actions_api.cc
diff --git a/chrome/browser/extensions/api/extension_action/extension_actions_api.cc b/chrome/browser/extensions/api/extension_action/extension_actions_api.cc
index 7b4cd1f1ec81085d8d53513249ccdd3b1fce8854..e88f8ef4bdab6fc25abeac6951b1e3e4dec0cb68 100644
--- a/chrome/browser/extensions/api/extension_action/extension_actions_api.cc
+++ b/chrome/browser/extensions/api/extension_action/extension_actions_api.cc
@@ -44,6 +44,20 @@ const char kNoExtensionActionError[] =
 const char kNoTabError[] = "No tab with id: *.";
 const char kIconIndexOutOfBounds[] = "Page action icon index out of bounds.";
 
+struct IconRepresentationInfo {
+  // Size as a string that will be used to retrieve represantation value from

[Jeffrey Yasskin, 2012/08/17 23:20:28]

sp: represantation -> representation

[tbarzic, 2012/08/21 00:22:07]

Done.

+  // SetIcon function arguments.
+  const char* size_string;
+  // Scale factor for which the represantion should be used.
+  ui::ScaleFactor scale;
+};
+
+
+const IconRepresentationInfo kIconSizes[] = {
+    { "19", ui::SCALE_FACTOR_100P },
+    { "38", ui::SCALE_FACTOR_200P }
+};
+
 // Conversion function for reading/writing to storage.
 SkColor RawStringToSkColor(const std::string& str) {
   uint64 value = 0;
@@ -71,9 +85,11 @@ bool StringToSkBitmap(const std::string& str, SkBitmap* bitmap) {
 }
 
 // Conversion function for reading/writing to storage.
-std::string ImageToString(const gfx::Image& image) {
+std::string RepresentationToString(const gfx::ImageSkia& image,
+                                   ui::ScaleFactor scale) {
+  SkBitmap bitmap = image.GetRepresentation(scale).sk_bitmap();
   IPC::Message bitmap_pickle;
-  IPC::WriteParam(&bitmap_pickle, image.AsBitmap());
+  IPC::WriteParam(&bitmap_pickle, bitmap);
   std::string raw_str(static_cast<const char*>(bitmap_pickle.data()),
                       bitmap_pickle.size());
   std::string base64_str;
@@ -83,12 +99,13 @@ std::string ImageToString(const gfx::Image& image) {
 }
 
 // Set |action|'s default values to those specified in |dict|.
-void SetDefaultsFromValue(const base::DictionaryValue* dict,
+void SetDefaultsFromValue(base::DictionaryValue* dict,

[Jeffrey Yasskin, 2012/08/17 23:20:28]

Ew @ modifying the dict. I think you can avoid this by making icon_value a const
base::DictionaryValue*?

[tbarzic, 2012/08/21 00:22:07]

Done.

                           ExtensionAction* action) {
   const int kTabId = ExtensionAction::kDefaultTabId;
   std::string str_value;
   int int_value;
   SkBitmap bitmap;
+  gfx::ImageSkia icon;
 
   if (dict->GetString(kPopupUrlStorageKey, &str_value))
     action->SetPopupUrl(kTabId, GURL(str_value));
@@ -103,10 +120,17 @@ void SetDefaultsFromValue(const base::DictionaryValue* dict,
   if (dict->GetInteger(kAppearanceStorageKey, &int_value))
     action->SetAppearance(kTabId,
                           static_cast<ExtensionAction::Appearance>(int_value));
-  if (dict->GetString(kIconStorageKey, &str_value) &&
-      StringToSkBitmap(str_value, &bitmap)) {
-    CHECK(!bitmap.isNull());
-    action->SetIcon(kTabId, gfx::Image(bitmap));
+
+  base::DictionaryValue* icon_value = NULL;
+  if (dict->GetDictionary(kIconStorageKey, &icon_value)) {
+    for (size_t i = 0; i < arraysize(kIconSizes); i++) {
+      if (icon_value->GetString(kIconSizes[i].size_string, &str_value) &&
+          StringToSkBitmap(str_value, &bitmap)) {
+        CHECK(!bitmap.isNull());
+        icon.AddRepresentation(gfx::ImageSkiaRep(bitmap, kIconSizes[i].scale));
+      }
+    }
+    action->SetIcon(kTabId, gfx::Image(icon));
   }
 }
 
@@ -126,11 +150,42 @@ scoped_ptr<base::DictionaryValue> DefaultsToValue(ExtensionAction* action) {
   dict->SetInteger(kAppearanceStorageKey,
                    action->GetIsVisible(kTabId) ?
                        ExtensionAction::ACTIVE : ExtensionAction::INVISIBLE);
-  dict->SetString(kIconStorageKey, ImageToString(action->GetIcon(kTabId)));
 
+  gfx::ImageSkia icon = action->GetExplicitlySetIcon(kTabId);
+  if (!icon.empty()) {
+    base::DictionaryValue* icon_value = new base::DictionaryValue();
+    for (size_t i = 0; i < arraysize(kIconSizes); i++) {
+      if (icon.HasRepresentation(kIconSizes[i].scale)) {
+        icon_value->SetString(
+            kIconSizes[i].size_string,
+            RepresentationToString(icon, kIconSizes[i].scale));
+      }
+    }
+    dict->Set(kIconStorageKey, icon_value);
+  }
   return dict.Pass();
 }
 
+// Updates |icon| with a new image representation for |scale_factor|. The
+// representation is found in dictionary |representations| under |size_key|. If
+// there is no image representation under desired key, |icon| remains unchanged.
+bool UpdateIconWithRepresentation(const char* size_key,

[Jeffrey Yasskin, 2012/08/17 23:20:28]

This function doesn't seem to be pulling its weight. The behavior of both
pulling a sized image out of a dictionary and adding an ImageSkiaRep don't feel
like they logically belong together, and the function's only called once, so
you'd actually save code lines by inlining it.

[tbarzic, 2012/08/21 00:22:07]

Done.

+                                  base::DictionaryValue* representations,
+                                  ui::ScaleFactor scale_factor,
+                                  gfx::ImageSkia* icon) {
+  base::BinaryValue* binary;
+  if (representations->GetBinary(size_key, &binary)) {
+    IPC::Message bitmap_pickle(binary->GetBuffer(), binary->GetSize());
+    PickleIterator iter(bitmap_pickle);
+    SkBitmap bitmap;
+    if (!IPC::ReadParam(&bitmap_pickle, &iter, &bitmap))
+      return false;
+    CHECK(!bitmap.isNull());
+    icon->AddRepresentation(gfx::ImageSkiaRep(bitmap, scale_factor));
+  }
+  return true;
+}
+
 }  // namespace
 
 namespace extensions {
@@ -402,17 +457,17 @@ bool ExtensionActionHideFunction::RunExtensionAction() {
 }
 
 bool ExtensionActionSetIconFunction::RunExtensionAction() {
-  // setIcon can take a variant argument: either a canvas ImageData, or an
-  // icon index.
-  base::BinaryValue* binary = NULL;
+  // setIcon can take a variant argument: either a dictionary of canvases
+  // ImageDataSet, or an icon index.
   int icon_index;
-  if (details_->GetBinary("imageData", &binary)) {
-    IPC::Message bitmap_pickle(binary->GetBuffer(), binary->GetSize());
-    PickleIterator iter(bitmap_pickle);
-    SkBitmap bitmap;
-    EXTENSION_FUNCTION_VALIDATE(IPC::ReadParam(&bitmap_pickle, &iter, &bitmap));
-    CHECK(!bitmap.isNull());
-    extension_action_->SetIcon(tab_id_, gfx::Image(bitmap));
+  base::DictionaryValue* canvas_set = NULL;
+  if (details_->GetDictionary("imageDataSet", &canvas_set)) {
+    gfx::ImageSkia icon;
+    for (size_t i = 0; i < arraysize(kIconSizes); i++) {
+      EXTENSION_FUNCTION_VALIDATE(UpdateIconWithRepresentation(
+          kIconSizes[i].size_string, canvas_set, kIconSizes[i].scale, &icon));
+    }
+    extension_action_->SetIcon(tab_id_, gfx::Image(icon));
   } else if (details_->GetInteger("iconIndex", &icon_index)) {
     // If --enable-script-badges is on there might legitimately be an iconIndex
     // set. Until we decide what to do with that, ignore.