Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Commit

Permalink
Task manager's apps and extensions should show the correct favicon
Browse files Browse the repository at this point in the history
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.

[email protected],[email protected]
BUG=525293
TEST=browser_tests --gtest_filter=ExtensionTagsTest.*

Review URL: https://codereview.chromium.org/1302423005

Cr-Commit-Position: refs/heads/master@{#346155}
(cherry picked from commit d91a619)

Review URL: https://codereview.chromium.org/1303103005 .

Cr-Commit-Position: refs/branch-heads/2490@{#172}
Cr-Branched-From: 7790a35-refs/heads/master@{#344925}
  • Loading branch information
Ahmed Fakhry committed Sep 4, 2015
1 parent e1de7a5 commit 28aae55
Show file tree
Hide file tree
Showing 7 changed files with 180 additions and 72 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -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
Expand All @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -40,6 +42,7 @@ ExtensionTask::ExtensionTask(content::WebContents* web_contents,
GetDefaultIcon(),
web_contents,
web_contents->GetRenderProcessHost()) {
LoadExtensionIcon(extension);
}

ExtensionTask::~ExtensionTask() {
Expand All @@ -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,
Expand All @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
Expand All @@ -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|.
Expand All @@ -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<extensions::IconImage> extension_icon_;

DISALLOW_COPY_AND_ASSIGN(ExtensionTask);
};

Expand Down
73 changes: 10 additions & 63 deletions extensions/browser/extension_icon_image_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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_;
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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<IconImage> image(
Expand Down
58 changes: 58 additions & 0 deletions extensions/browser/test_image_loader.cc
Original file line number Diff line number Diff line change
@@ -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
44 changes: 44 additions & 0 deletions extensions/browser/test_image_loader.h
Original file line number Diff line number Diff line change
@@ -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_
Loading

0 comments on commit 28aae55

Please sign in to comment.