[Extensions] Fix lifetime bug in ExtensionAction/IconImage

chrome/browser/extensions/extension_action_manager.cc

Index: chrome/browser/extensions/extension_action_manager.cc
diff --git a/chrome/browser/extensions/extension_action_manager.cc b/chrome/browser/extensions/extension_action_manager.cc
index 6d197c9046067590785fb780f6053df66dd2aa7f..30b5c7605f84c95024634cef18c0ac2a316c22b6 100644
--- a/chrome/browser/extensions/extension_action_manager.cc
+++ b/chrome/browser/extensions/extension_action_manager.cc
@@ -4,14 +4,17 @@
 
 #include "chrome/browser/extensions/extension_action_manager.h"
 
+#include "base/memory/ptr_util.h"
 #include "chrome/browser/extensions/api/system_indicator/system_indicator_manager_factory.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "extensions/browser/extension_icon_image.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/extensions_browser_client.h"
 #include "extensions/common/constants.h"
+#include "ui/gfx/image/image_skia.h"
 
 namespace extensions {
 
@@ -107,8 +110,16 @@ ExtensionAction* GetOrCreateOrNull(
     return nullptr;
   }
 
-  std::unique_ptr<ExtensionAction> action(
-      new ExtensionAction(extension, action_type, *action_info));
+  auto action =
+      base::MakeUnique<ExtensionAction>(extension, action_type, *action_info);
+
+  if (action->default_icon()) {
+    action->SetDefaultIconImage(base::MakeUnique<IconImage>(
+        profile, &extension, *action->default_icon(),
+        ExtensionAction::ActionIconSize(),
+        ExtensionAction::DefaultIcon().AsImageSkia(), nullptr));

[asargent_no_longer_on_chrome, 2016/11/18 00:55:27]

The naming here is pretty confusing and I really had to think things through to
understand it, as there are 3 different things named something like "default
icon":

ExtensionAction::DefaultIcon() [static method]
- returns gfx::Image of puzzle piece

ExtensionAction::default_icon() [instance method]
- returns ExtensionIconSet of icons from "icons" section of manifest, if no
specific browser/page action icons were specified


ExtensionAction::default_icon_image() [instance method]
- returns IconImage created with default_icon_ (or maybe DefaultIcon() in some
cases?)


Can we rename one or two of these? For instance, how about calling DefaultIcon
"FallbackIcon" or "LastResortIcon" or something like that?


Also, as an aside, if we only call SetDefaultIconImage when default_icon() is
non-null, will the IconImage ever use the DefaultIcon().AsImageSkia() we pass to
it? (I guess maybe if default_icon was non-null but also empty?)

[Devlin, 2016/11/18 01:56:47]

Rename DefaultIcon() to FallbackIcon() - I agree that makes things a bit more
clear.  Now default icon only refers to the default icon for the extension.

Re the default icon that's passed to the IconImage, it's used when the icon
loading fails somehow (either it's empty or the resource doesn't load properly,
etc).  We might be able to clean it up a bit, but it seems out of scope of this
CL.

[asargent_no_longer_on_chrome, 2016/11/18 18:29:33]

Yeah, agreed. I guess to complete the thought, I was wondering if we could just
pass an empty ImageSkia, but I guess it is possible that the actual icon loading
would fail somehow. 

+  }
+
   ExtensionAction* raw_action = action.get();
   (*map)[extension.id()] = std::move(action);
   return raw_action;