Move icon fallbacks into ExtensionAction.

chrome/common/extensions/extension_action.cc

Index: chrome/common/extensions/extension_action.cc
diff --git a/chrome/common/extensions/extension_action.cc b/chrome/common/extensions/extension_action.cc
index 1e247210e4d4df856467317b18a664c9ebab4f92..353848f64674b7bdd0b73a3ea163256f3738e892 100644
--- a/chrome/common/extensions/extension_action.cc
+++ b/chrome/common/extensions/extension_action.cc
@@ -9,6 +9,7 @@
 #include "base/logging.h"
 #include "chrome/common/badge_util.h"
 #include "googleurl/src/gurl.h"
+#include "grit/theme_resources.h"
 #include "grit/ui_resources.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkCanvas.h"
@@ -191,11 +192,35 @@ GURL ExtensionAction::GetPopupUrl(int tab_id) const {
 }
 
 void ExtensionAction::SetIcon(int tab_id, const SkBitmap& bitmap) {
-  SetValue(&icon_, tab_id, bitmap);
+  SetValue(&icon_, tab_id, gfx::Image(bitmap));
 }
 
-SkBitmap ExtensionAction::GetIcon(int tab_id) const {
-  return GetValue(&icon_, tab_id);
+gfx::Image ExtensionAction::GetIcon(int tab_id,
+                                    const PathToIconCache& cache) const {

[not at google - send to devlin, 2012/07/26 02:29:44]

strange to pass in the cache then not use it sometime. Pulling this logic into a
separate class would be cleaner, I think.

[Jeffrey Yasskin, 2012/07/26 21:11:47]

Are you referring to the fact that if an icon is set as an icon instead of a
path, we don't need the cache? That's true, but the goal of this method is that
the caller shouldn't need to know which cases need a cache vs which cases don't.

I think making PathToIconCache a class is overkill since this and some map<>
methods would be its only operations, and there's no real gain from either
encapsulating a few of map<>'s methods or moving the logic away from this file.
When we write the interface to let common/ code load resources, the cache will
move into an ExtensionAction member, and this logic will just need to move back
into ExtensionAction, yielding more churn.

[not at google - send to devlin, 2012/07/27 03:16:12]

Of course. That's not what I meant; the caching should definitely return the
icon transparently.

I just meant that it's odd from an OO perspective to be passing an object into
another one, when all state behaviour could be encapsulated in the thing that's
being passed. I.e. make the map a little bit smarter.
Mm, I think it would end up being simpler, and easier to understand / less
boilerplatey for calling code. The class isn't exactly complicated, and it
encapsulates the purpose of what you're trying to do. I wrote a comment in
ExtensionAction.

To put it another way, all calling code cares about is getting an icon. It has a
tab ID. The callers shouldn't need to depend on there being a map from path ->
icon somewhere, that's an implementation detail.

[Jeffrey Yasskin, 2012/07/31 14:40:01]

Done, I think.

+  // Check if a specific icon is set for this tab.
+  gfx::Image icon = GetValue(&icon_, tab_id);
+  if (icon.IsEmpty()) {
+    // Need to find an icon from a path.
+    const std::string* path = NULL;
+    // Check if one of the elements of icon_path() was selected.
+    int icon_index = GetIconIndex(tab_id);
+    if (icon_index >= 0) {
+      path = &icon_paths()->at(icon_index);
+    } else {
+      // Otherwise, use the default icon.
+      path = &default_icon_path();
+    }
+
+    PathToIconCache::const_iterator cached_icon = cache.find(*path);
+    if (cached_icon != cache.end()) {
+      icon = cached_icon->second;
+    } else {
+      icon = ui::ResourceBundle::GetSharedInstance().GetImageNamed(
+          IDR_EXTENSIONS_FAVICON);
+    }
+  }
+
+  return ApplyIconAnimation(tab_id, icon);
 }
 
 void ExtensionAction::SetIconIndex(int tab_id, int index) {
@@ -218,6 +243,12 @@ void ExtensionAction::ClearAllValuesForTab(int tab_id) {
   icon_animation_.erase(tab_id);
 }
 
+static int Width(const gfx::Image& image) {
+  if (image.IsEmpty())
+    return 0;
+  return image.ToSkBitmap()->width();
+}

[not at google - send to devlin, 2012/07/26 02:29:44]

anonymous namespaces preferred over static methods, iirc for debugging/stack
trace purposes?

that said it's only used in the one place.

[Jeffrey Yasskin, 2012/07/26 21:11:47]

Sure, done. Is there any practical effect beyond spelling the function as
namespace::(anonymous)::Function instead of namespace::Function?

[not at google - send to devlin, 2012/07/27 03:16:12]

No idea. Reviewer once made me do it, said something about optimisations bleh.
Just seems to be the general common practice in chrome though, much as I prefer
the "static" keyword (Java background; plus for little utility methods, it's
cleaner).

+
 void ExtensionAction::PaintBadge(gfx::Canvas* canvas,
                                  const gfx::Rect& bounds,
                                  int tab_id) {
@@ -248,10 +279,10 @@ void ExtensionAction::PaintBadge(gfx::Canvas* canvas,
   // Calculate badge size. It is clamped to a min width just because it looks
   // silly if it is too skinny.
   int badge_width = SkScalarFloor(text_width) + kPadding * 2;
-  int icon_width = GetIcon(tab_id).width();
+  int icon_width = Width(GetValue(&icon_, tab_id));
   // Force the pixel width of badge to be either odd (if the icon width is odd)
   // or even otherwise. If there is a mismatch you get http://crbug.com/26400.
-  if (icon_width != 0 && (badge_width % 2 != GetIcon(tab_id).width() % 2))
+  if (icon_width != 0 && (badge_width % 2 != icon_width % 2))
     badge_width += 1;
   badge_width = std::max(kBadgeHeight, badge_width);
 
@@ -315,6 +346,15 @@ base::WeakPtr<ExtensionAction::IconAnimation> ExtensionAction::GetIconAnimation(
                                        : base::WeakPtr<IconAnimation>();
 }
 
+gfx::Image ExtensionAction::ApplyIconAnimation(int tab_id,
+                                               const gfx::Image& orig) const {
+  std::map<int, linked_ptr<IconAnimationWrapper> >::const_iterator it =
+      icon_animation_.find(tab_id);
+  if (it == icon_animation_.end())
+    return orig;
+  return gfx::Image(it->second->animation()->Apply(*orig.ToSkBitmap()));
+}
+
 void ExtensionAction::RunIconAnimation(int tab_id) {
   IconAnimationWrapper* icon_animation =
       new IconAnimationWrapper(this, tab_id);