Task manager's apps and extensions should show the correct favicon

chrome/browser/task_management/providers/web_contents/extension_task.cc

 // Copyright 2015 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/task_management/providers/web_contents/extension_task.h"
 
 #include "base/strings/utf_string_conversions.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/browser/view_type_utils.h"
+#include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/manifest_handlers/icons_handler.h"
 #include "extensions/common/view_type.h"
 #include "grit/theme_resources.h"
 #include "ui/base/resource/resource_bundle.h"
 
 namespace task_management {
 
 namespace {
 
 gfx::ImageSkia* g_default_icon = nullptr;
 
@@ skipping 11 matching lines @@
 
 }  // namespace
 
 ExtensionTask::ExtensionTask(content::WebContents* web_contents,
                              const extensions::Extension* extension,
                              extensions::ViewType view_type)
     : RendererTask(GetExtensionTitle(web_contents, extension, view_type),
                    GetDefaultIcon(),
                    web_contents,
                    web_contents->GetRenderProcessHost()) {
+  LoadExtensionIcon(web_contents, extension);
 }
 
 ExtensionTask::~ExtensionTask() {
 }
 
 void ExtensionTask::OnTitleChanged(content::NavigationEntry* entry) {
   // The title of the extension should not change as a result of title change
   // in its WebContents, so we ignore this.
 }
 
 void ExtensionTask::OnFaviconChanged() {
-  // For now we never change the favicon of the extension, we always use the
-  // default one.
-  // TODO(afakhry): In the future use the extensions' favicons.
+  // We don't care about the favicon of the WebContents but rather of the
+  // extension.
 }
 
 Task::Type ExtensionTask::GetType() const {
   return Task::EXTENSION;
 }
 
+void ExtensionTask::OnExtensionIconImageChanged(extensions::IconImage* image) {
+  DCHECK_EQ(extension_icon_.get(), image);
+
+  if (image->image_skia().isNull())
+    return;

[Devlin, 2015/08/27 16:08:55]

nit: it'll depend on who you ask, but I generally prefer to only early-return if
it saves code/eliminates a big indent.  So in a case like this, I'd just do:
if (!image->image_skia().isNull())
  set_icon(image->image_skia());

[afakhry, 2015/08/27 21:48:15]

Done.

+
+  set_icon(image->image_skia());
+}
+
 base::string16 ExtensionTask::GetExtensionTitle(
     content::WebContents* web_contents,
     const extensions::Extension* extension,
     extensions::ViewType view_type) const {
   DCHECK(web_contents);
 
   base::string16 title = extension ?

[Devlin, 2015/08/27 16:08:55]

I don't know why I didn't see this before, but if |extension| is null here,
extension->is_app() will crash.  So we should either DCHECK extension, or handle
it being null properly.  Can it be null here?

[afakhry, 2015/08/27 21:48:15]

Yes it can, in some tests if you remember, which they don't add themselves to
the enabled extensions lists. We're not crashing these tests however since the
creation of the Extension task doesn't happen until the task manager starts
updating.

Thank you for spotting this!

       base::UTF8ToUTF16(extension->name()) :
       RendererTask::GetTitleFromWebContents(web_contents);
 
   bool is_background =
       view_type == extensions::VIEW_TYPE_EXTENSION_BACKGROUND_PAGE;
 
   return RendererTask::PrefixRendererTitle(
       title,
       extension->is_app(),
       true,  // is_extension
       web_contents->GetBrowserContext()->IsOffTheRecord(),
       is_background);
 }
 
+void ExtensionTask::LoadExtensionIcon(content::WebContents* web_contents,

[Devlin, 2015/08/27 16:08:55]

instead of passing in web_contents, just use web_contents().

[afakhry, 2015/08/27 21:48:15]

Done.

+                                      const extensions::Extension* extension) {
+  DCHECK(web_contents);
+
+  if (!extension)
+    return;
+
+  extension_icon_.reset(
+      new extensions::IconImage(web_contents->GetBrowserContext(),
+                                extension,
+                                extensions::IconsInfo::GetIcons(extension),
+                                extension_misc::EXTENSION_ICON_SMALL,
+                                icon(),
+                                this));
+
+  // Triggers actual image loading with 1x resources.
+  extension_icon_->image_skia().GetRepresentation(1.0f);
+  set_icon(extension_icon_->image_skia());
+}
+
 }  // namespace task_management