Move icon fallbacks into ExtensionAction.

chrome/browser/ui/views/browser_action_view.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/ui/views/browser_action_view.h"
 
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/api/commands/command_service_factory.h"
 #include "chrome/browser/extensions/extension_context_menu_model.h"
@@ skipping 77 matching lines @@
     std::string relative_path = browser_action_->default_icon_path();
     if (!relative_path.empty()) {
       // LoadImage is not guaranteed to be synchronous, so we might see the
       // callback OnImageLoaded execute immediately. It (through UpdateState)
       // expects parent() to return the owner for this button, so this
       // function is as early as we can start this request.
       tracker_.LoadImage(extension_, extension_->GetResource(relative_path),
                          gfx::Size(Extension::kBrowserActionIconMaxSize,
                                    Extension::kBrowserActionIconMaxSize),
                          ImageLoadingTracker::DONT_CACHE);
-    } else {
-      // Set the icon to be the default extensions icon.
-      default_icon_ = *ui::ResourceBundle::GetSharedInstance().GetImageNamed(
-          IDR_EXTENSIONS_FAVICON).ToSkBitmap();
-      UpdateState();

[sky, 2012/07/31 16:25:19]

Are you sure you don't still need the call to UpdateState here?

[Jeffrey Yasskin, 2012/07/31 19:53:20]

Pretty sure, but not 100%. I believe the only time the call to UpdateState()
matters is when this is called indirectly from
BrowserActionView::BrowserActionView(), which calls UpdateState() itself. Is
there some other time I've missed?

     }
 
     MaybeRegisterExtensionCommand();
   }
 
   MenuButton::ViewHierarchyChanged(is_add, parent, child);
 }
 
 bool BrowserActionButton::CanHandleAccelerators() const {
   // View::CanHandleAccelerators() checks to see if the view is visible before
@@ skipping 33 matching lines @@
       views::MenuRunner::MENU_DELETED)
     return;
 
   SetButtonNotPushed();
   context_menu_ = NULL;
 }
 
 void BrowserActionButton::OnImageLoaded(const gfx::Image& image,
                                         const std::string& extension_id,
                                         int index) {
-  if (!image.IsEmpty())
-    default_icon_ = *image.ToSkBitmap();
+  browser_action_->CacheIcon(browser_action_->default_icon_path(), image);
 
   // Call back to UpdateState() because a more specific icon might have been set
   // while the load was outstanding.
   UpdateState();
 }
 
 void BrowserActionButton::UpdateState() {
   int tab_id = panel_->GetCurrentTabId();
   if (tab_id < 0)
     return;
 
   if (!IsEnabled(tab_id)) {
     SetState(views::CustomButton::BS_DISABLED);
   } else {
     SetState(menu_visible_ ?
              views::CustomButton::BS_PUSHED :
              views::CustomButton::BS_NORMAL);
   }
 
-  SkBitmap icon(browser_action()->GetIcon(tab_id));
-  if (icon.isNull())
-    icon = default_icon_;
+  SkBitmap icon(*browser_action()->GetIcon(tab_id).ToSkBitmap());

[sky, 2012/07/31 16:25:19]

I believe all this code should be using ImageSkia.

[Jeffrey Yasskin, 2012/07/31 19:53:20]

I believe you're right. However, I don't think this is the right CL to do that
migration because it's primarily focused on something else.

By the way, is there a porting guide for migrating SkBitmap to gfx::Image? I
tried to avoid Image->SkBitmap->Image conversions in this CL, but I was stumped
by things like finding the analog of SkDevice. Also, is there a guideline for
picking a scale factor when building an SkCanvas out of an ImageSkia?

   if (!icon.isNull()) {
     if (!browser_action()->GetIsVisible(tab_id))
       icon = MakeTransparent(icon);
     SkPaint paint;
     paint.setXfermode(SkXfermode::Create(SkXfermode::kSrcOver_Mode));
     ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
 
     SkBitmap bg;
     rb.GetBitmapNamed(IDR_BROWSER_ACTION)->copyTo(&bg,
         SkBitmap::kARGB_8888_Config);
@@ skipping 195 matching lines @@
 }
 
 BrowserActionView::~BrowserActionView() {
   RemoveChildView(button_);
   button_->Destroy();
 }
 
 gfx::Canvas* BrowserActionView::GetIconWithBadge() {
   int tab_id = panel_->GetCurrentTabId();
 
-  SkBitmap icon = button_->extension()->browser_action()->GetIcon(tab_id);
-  if (icon.isNull())
-    icon = button_->default_icon();
+  SkBitmap icon = *button_->extension()->browser_action()->GetIcon(
+      tab_id).ToSkBitmap();
 
   // Dim the icon if our button is disabled.
   if (!button_->IsEnabled(tab_id))
     icon = MakeTransparent(icon);
 
   gfx::Canvas* canvas =
       new gfx::Canvas(gfx::ImageSkiaRep(icon, ui::SCALE_FACTOR_100P), false);
 
   if (tab_id >= 0) {
     gfx::Rect bounds(icon.width(), icon.height() + ToolbarView::kVertSpacing);
@@ skipping 21 matching lines @@
   state->role = ui::AccessibilityTypes::ROLE_GROUPING;
 }
 
 void BrowserActionView::PaintChildren(gfx::Canvas* canvas) {
   View::PaintChildren(canvas);
   ExtensionAction* action = button()->browser_action();
   int tab_id = panel_->GetCurrentTabId();
   if (tab_id >= 0)
     action->PaintBadge(canvas, gfx::Rect(width(), height()), tab_id);
 }