From d91a61917396cc7cf77a44b26290758045d57ef3 Mon Sep 17 00:00:00 2001 From: afakhry Date: Fri, 28 Aug 2015 08:52:19 -0700 Subject: [PATCH] Task manager's apps and extensions should show the correct favicon We need to get the correct favicon of the app or extension so that it's easier to identify in the table. Before and after screenshots are available on the BUG thread. BUG=525293 TEST=browser_tests --gtest_filter=ExtensionTagsTest.* Review URL: https://codereview.chromium.org/1302423005 Cr-Commit-Position: refs/heads/master@{#346155} --- .../web_contents/extension_tag_browsertest.cc | 26 ++++++- .../providers/web_contents/extension_task.cc | 34 ++++++++- .../providers/web_contents/extension_task.h | 15 +++- .../browser/extension_icon_image_unittest.cc | 73 +++---------------- extensions/browser/test_image_loader.cc | 58 +++++++++++++++ extensions/browser/test_image_loader.h | 44 +++++++++++ extensions/extensions.gypi | 2 + 7 files changed, 180 insertions(+), 72 deletions(-) create mode 100644 extensions/browser/test_image_loader.cc create mode 100644 extensions/browser/test_image_loader.h diff --git a/chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc b/chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc index 969349b931190..6b38a76c42e52 100644 --- a/chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc +++ b/chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc @@ -6,6 +6,10 @@ #include "chrome/browser/extensions/extension_browsertest.h" #include "chrome/browser/task_management/task_management_browsertest_util.h" #include "chrome/common/chrome_switches.h" +#include "extensions/browser/test_image_loader.h" +#include "extensions/common/constants.h" +#include "ui/gfx/image/image.h" +#include "ui/gfx/skia_util.h" namespace task_management { @@ -56,7 +60,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionTagsTest, Basic) { } #if defined(OS_WIN) -// Test disabled due to flakiness on Windows. +// Test disabled due to flakiness on Windows XP. // See bug: http://crbug.com/519333 #define MAYBE_PreAndPostExistingTaskProviding \ DISABLED_PreAndPostExistingTaskProviding @@ -80,16 +84,30 @@ IN_PROC_BROWSER_TEST_F(ExtensionTagsTest, EXPECT_EQ(2U, tracked_tags().size()); EXPECT_TRUE(task_manager.tasks().empty()); + base::RunLoop run_loop; + run_loop.RunUntilIdle(); + // Start observing, pre-existing tasks will be provided. task_manager.StartObserving(); - EXPECT_EQ(2U, task_manager.tasks().size()); - EXPECT_EQ(Task::EXTENSION, task_manager.tasks().back()->GetType()); + ASSERT_EQ(2U, task_manager.tasks().size()); + const Task* extension_task = task_manager.tasks().back(); + EXPECT_EQ(Task::EXTENSION, extension_task->GetType()); + + SkBitmap expected_bitmap = + extensions::TestImageLoader::LoadAndGetExtensionBitmap( + extension, + "icon_128.png", + extension_misc::EXTENSION_ICON_SMALL); + ASSERT_FALSE(expected_bitmap.empty()); + + EXPECT_TRUE(gfx::BitmapsAreEqual(*extension_task->icon().bitmap(), + expected_bitmap)); // Unload the extension and expect that the task manager now shows only the // about:blank tab. UnloadExtension(extension->id()); EXPECT_EQ(1U, tracked_tags().size()); - EXPECT_EQ(1U, task_manager.tasks().size()); + ASSERT_EQ(1U, task_manager.tasks().size()); const Task* about_blank_task = task_manager.tasks().back(); EXPECT_EQ(Task::RENDERER, about_blank_task->GetType()); EXPECT_EQ(base::UTF8ToUTF16("Tab: about:blank"), about_blank_task->title()); diff --git a/chrome/browser/task_management/providers/web_contents/extension_task.cc b/chrome/browser/task_management/providers/web_contents/extension_task.cc index a63781a20de87..9df1366560d86 100644 --- a/chrome/browser/task_management/providers/web_contents/extension_task.cc +++ b/chrome/browser/task_management/providers/web_contents/extension_task.cc @@ -8,7 +8,9 @@ #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" @@ -40,6 +42,7 @@ ExtensionTask::ExtensionTask(content::WebContents* web_contents, GetDefaultIcon(), web_contents, web_contents->GetRenderProcessHost()) { + LoadExtensionIcon(extension); } ExtensionTask::~ExtensionTask() { @@ -51,15 +54,21 @@ void ExtensionTask::OnTitleChanged(content::NavigationEntry* entry) { } 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()) + set_icon(image->image_skia()); +} + base::string16 ExtensionTask::GetExtensionTitle( content::WebContents* web_contents, const extensions::Extension* extension, @@ -75,10 +84,27 @@ base::string16 ExtensionTask::GetExtensionTitle( return RendererTask::PrefixRendererTitle( title, - extension->is_app(), + extension && extension->is_app(), true, // is_extension web_contents->GetBrowserContext()->IsOffTheRecord(), is_background); } +void ExtensionTask::LoadExtensionIcon(const extensions::Extension* extension) { + 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 diff --git a/chrome/browser/task_management/providers/web_contents/extension_task.h b/chrome/browser/task_management/providers/web_contents/extension_task.h index 9ab3ecc0abd16..83c2d2b82b15e 100644 --- a/chrome/browser/task_management/providers/web_contents/extension_task.h +++ b/chrome/browser/task_management/providers/web_contents/extension_task.h @@ -6,6 +6,7 @@ #define CHROME_BROWSER_TASK_MANAGEMENT_PROVIDERS_WEB_CONTENTS_EXTENSION_TASK_H_ #include "chrome/browser/task_management/providers/web_contents/renderer_task.h" +#include "extensions/browser/extension_icon_image.h" #include "extensions/common/view_type.h" namespace extensions { @@ -15,7 +16,9 @@ class Extension; namespace task_management { // Defines a task manager representation for extensions. -class ExtensionTask : public RendererTask { +class ExtensionTask + : public RendererTask, + public extensions::IconImage::Observer { public: ExtensionTask(content::WebContents* web_contents, const extensions::Extension* extension, @@ -27,6 +30,9 @@ class ExtensionTask : public RendererTask { void OnFaviconChanged() override; Type GetType() const override; + // extensions::IconImage::Observer + void OnExtensionIconImageChanged(extensions::IconImage* image) override; + private: // If |extension| is nullptr, this method will get the title from // the |web_contents|. @@ -35,6 +41,13 @@ class ExtensionTask : public RendererTask { const extensions::Extension* extension, extensions::ViewType view_type) const; + // This is called upon the creation of this task to load the extension icon + // for the first time if any. + void LoadExtensionIcon(const extensions::Extension* extension); + + // The favicon of the extension represented by this task. + scoped_ptr extension_icon_; + DISALLOW_COPY_AND_ASSIGN(ExtensionTask); }; diff --git a/extensions/browser/extension_icon_image_unittest.cc b/extensions/browser/extension_icon_image_unittest.cc index 3fb4e691b711b..29bb1ff207c59 100644 --- a/extensions/browser/extension_icon_image_unittest.cc +++ b/extensions/browser/extension_icon_image_unittest.cc @@ -14,6 +14,7 @@ #include "content/public/test/test_browser_thread.h" #include "extensions/browser/extensions_test.h" #include "extensions/browser/image_loader.h" +#include "extensions/browser/test_image_loader.h" #include "extensions/common/extension.h" #include "extensions/common/extension_paths.h" #include "extensions/common/manifest.h" @@ -65,55 +66,6 @@ class MockImageSkiaSource : public gfx::ImageSkiaSource { gfx::ImageSkia image_; }; -// Helper class for synchronously loading extension image resource. -class TestImageLoader { - public: - explicit TestImageLoader(const Extension* extension) - : extension_(extension), - waiting_(false), - image_loaded_(false) { - } - virtual ~TestImageLoader() {} - - void OnImageLoaded(const gfx::Image& image) { - image_ = image; - image_loaded_ = true; - if (waiting_) - base::MessageLoop::current()->Quit(); - } - - SkBitmap LoadBitmap(const std::string& path, - int size) { - image_loaded_ = false; - - image_loader_.LoadImageAsync( - extension_, extension_->GetResource(path), gfx::Size(size, size), - base::Bind(&TestImageLoader::OnImageLoaded, - base::Unretained(this))); - - // If |image_| still hasn't been loaded (i.e. it is being loaded - // asynchronously), wait for it. - if (!image_loaded_) { - waiting_ = true; - base::MessageLoop::current()->Run(); - waiting_ = false; - } - - EXPECT_TRUE(image_loaded_); - - return image_.IsEmpty() ? SkBitmap() : *image_.ToSkBitmap(); - } - - private: - const Extension* extension_; - bool waiting_; - bool image_loaded_; - gfx::Image image_; - ImageLoader image_loader_; - - DISALLOW_COPY_AND_ASSIGN(TestImageLoader); -}; - class ExtensionIconImageTest : public ExtensionsTest, public IconImage::Observer { public: @@ -184,15 +136,6 @@ class ExtensionIconImageTest : public ExtensionsTest, return gfx::ImageSkia(gfx::ImageSkiaRep(gfx::Size(16, 16), 1.0f)); } - // Loads an image to be used in test from the extension. - // The image will be loaded from the relative path |path|. - SkBitmap GetTestBitmap(const Extension* extension, - const std::string& path, - int size) { - TestImageLoader image_loader(extension); - return image_loader.LoadBitmap(path, size); - } - private: int image_loaded_count_; bool quit_in_image_loaded_; @@ -220,13 +163,14 @@ TEST_F(ExtensionIconImageTest, Basic) { // Load images we expect to find as representations in icon_image, so we // can later use them to validate icon_image. - SkBitmap bitmap_16 = GetTestBitmap(extension.get(), "16.png", 16); + SkBitmap bitmap_16 = + TestImageLoader::LoadAndGetExtensionBitmap(extension.get(), "16.png", 16); ASSERT_FALSE(bitmap_16.empty()); // There is no image of size 32 defined in the extension manifest, so we // should expect manifest image of size 48 resized to size 32. SkBitmap bitmap_48_resized_to_32 = - GetTestBitmap(extension.get(), "48.png", 32); + TestImageLoader::LoadAndGetExtensionBitmap(extension.get(), "48.png", 32); ASSERT_FALSE(bitmap_48_resized_to_32.empty()); IconImage image(browser_context(), @@ -293,7 +237,8 @@ TEST_F(ExtensionIconImageTest, FallbackToSmallerWhenNoBigger) { // Load images we expect to find as representations in icon_image, so we // can later use them to validate icon_image. - SkBitmap bitmap_48 = GetTestBitmap(extension.get(), "48.png", 48); + SkBitmap bitmap_48 = + TestImageLoader::LoadAndGetExtensionBitmap(extension.get(), "48.png", 48); ASSERT_FALSE(bitmap_48.empty()); IconImage image(browser_context(), @@ -330,7 +275,8 @@ TEST_F(ExtensionIconImageTest, FallbackToBigger) { // Load images we expect to find as representations in icon_image, so we // can later use them to validate icon_image. - SkBitmap bitmap_24 = GetTestBitmap(extension.get(), "24.png", 24); + SkBitmap bitmap_24 = + TestImageLoader::LoadAndGetExtensionBitmap(extension.get(), "24.png", 24); ASSERT_FALSE(bitmap_24.empty()); IconImage image(browser_context(), @@ -522,7 +468,8 @@ TEST_F(ExtensionIconImageTest, IconImageDestruction) { // Load images we expect to find as representations in icon_image, so we // can later use them to validate icon_image. - SkBitmap bitmap_16 = GetTestBitmap(extension.get(), "16.png", 16); + SkBitmap bitmap_16 = + TestImageLoader::LoadAndGetExtensionBitmap(extension.get(), "16.png", 16); ASSERT_FALSE(bitmap_16.empty()); scoped_ptr image( diff --git a/extensions/browser/test_image_loader.cc b/extensions/browser/test_image_loader.cc new file mode 100644 index 0000000000000..af6ce25cfb15a --- /dev/null +++ b/extensions/browser/test_image_loader.cc @@ -0,0 +1,58 @@ +// 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 "extensions/browser/test_image_loader.h" + +#include "base/bind.h" +#include "extensions/browser/image_loader.h" +#include "extensions/common/extension.h" + +namespace extensions { + +TestImageLoader::TestImageLoader() : waiting_(false), image_loaded_(false) {} + +TestImageLoader::~TestImageLoader() {} + +// static +SkBitmap TestImageLoader::LoadAndGetExtensionBitmap( + const Extension* extension, + const std::string& image_path, + int size) { + TestImageLoader image_loader; + return image_loader.LoadAndGetBitmap(extension, image_path, size); +} + +void TestImageLoader::OnImageLoaded(const gfx::Image& image) { + image_ = image; + image_loaded_ = true; + if (waiting_) + loader_message_loop_quit_.Run(); +} + +SkBitmap TestImageLoader::LoadAndGetBitmap(const Extension* extension, + const std::string& path, + int size) { + image_loaded_ = false; + + ImageLoader image_loader; + image_loader.LoadImageAsync( + extension, extension->GetResource(path), gfx::Size(size, size), + base::Bind(&TestImageLoader::OnImageLoaded, base::Unretained(this))); + + // If |image_| still hasn't been loaded (i.e. it is being loaded + // asynchronously), wait for it. + if (!image_loaded_) { + waiting_ = true; + base::RunLoop run_loop; + loader_message_loop_quit_ = run_loop.QuitClosure(); + run_loop.Run(); + waiting_ = false; + } + + DCHECK(image_loaded_); + + return image_.IsEmpty() ? SkBitmap() : *image_.ToSkBitmap(); +} + +} // namespace extensions diff --git a/extensions/browser/test_image_loader.h b/extensions/browser/test_image_loader.h new file mode 100644 index 0000000000000..89003286514ed --- /dev/null +++ b/extensions/browser/test_image_loader.h @@ -0,0 +1,44 @@ +// 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. + +#ifndef EXTENSIONS_BROWSER_TEST_IMAGE_LOADER_H_ +#define EXTENSIONS_BROWSER_TEST_IMAGE_LOADER_H_ + +#include "base/run_loop.h" +#include "ui/gfx/image/image.h" + +namespace extensions { + +class Extension; + +// Helper class for synchronously loading an extension image resource. +class TestImageLoader { + public: + TestImageLoader(); + ~TestImageLoader(); + + // Loads an image to be used in test from |extension|. + // The image will be loaded from the relative path |image_path|. + static SkBitmap LoadAndGetExtensionBitmap(const Extension* extension, + const std::string& image_path, + int size); + + private: + void OnImageLoaded(const gfx::Image& image); + + SkBitmap LoadAndGetBitmap(const Extension* extension, + const std::string& path, + int size); + + gfx::Image image_; + base::Closure loader_message_loop_quit_; + bool waiting_; + bool image_loaded_; + + DISALLOW_COPY_AND_ASSIGN(TestImageLoader); +}; + +} // namespace extensions + +#endif // EXTENSIONS_BROWSER_TEST_IMAGE_LOADER_H_ diff --git a/extensions/extensions.gypi b/extensions/extensions.gypi index 2575d85e8bd4f..e1d14f3df7c43 100644 --- a/extensions/extensions.gypi +++ b/extensions/extensions.gypi @@ -1045,6 +1045,8 @@ 'browser/test_extension_registry_observer.h', 'browser/test_extensions_browser_client.cc', 'browser/test_extensions_browser_client.h', + 'browser/test_image_loader.cc', + 'browser/test_image_loader.h', 'browser/test_management_policy.cc', 'browser/test_management_policy.h', 'browser/test_runtime_api_delegate.cc',