[Extensions] Fix lifetime bug in ExtensionAction/IconImage

chrome/browser/extensions/extension_action_manager.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_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 {
 
 namespace {
 
 // BrowserContextKeyedServiceFactory for ExtensionActionManager.
 class ExtensionActionManagerFactory : public BrowserContextKeyedServiceFactory {
  public:
   // BrowserContextKeyedServiceFactory implementation:
   static ExtensionActionManager* GetForBrowserContext(
@@ skipping 75 matching lines @@
     return nullptr;
 
   // Only create action info for enabled extensions.
   // This avoids bugs where actions are recreated just after being removed
   // in response to OnExtensionUnloaded().
   if (!ExtensionRegistry::Get(profile)
       ->enabled_extensions().Contains(extension.id())) {
     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;
 }
 
 }  // namespace
 
 ExtensionAction* ExtensionActionManager::GetPageAction(
     const Extension& extension) const {
   return GetOrCreateOrNull(&page_actions_, extension,
@@ skipping 40 matching lines @@
                            profile_);
 }
 
 ExtensionAction* ExtensionActionManager::GetExtensionAction(
     const Extension& extension) const {
   ExtensionAction* action = GetBrowserAction(extension);
   return action ? action : GetPageAction(extension);
 }
 
 }  // namespace extensions