Skip to content

Commit

Permalink
Make LauncherSearchIconImageLoader ref counted.
Browse files Browse the repository at this point in the history 
Get rid of deprecated linked_ptr usage. Use RefCounted because the
object has shared ownership.

BUG=556939

Change-Id: Iab75d2a4cf834227764d4ac1888e3efa690b2ce2
Reviewed-on: https://chromium-review.googlesource.com/1053992
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Yuki Awano <yawano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559422}
  • Loading branch information
@leizleiz
leizleiz authored and Commit Bot committed May 17, 2018
1 parent b7f4df2 commit 7e512a5
Show file tree
Hide file tree
Showing 6 changed files with 70 additions and 67 deletions.
12 changes: 10 additions & 2 deletions chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,11 @@
#include <stdint.h>

#include <memory>
#include <set>
#include <string>

#include "base/macros.h"
#include "base/memory/ref_counted.h"
#include "chrome/browser/chromeos/launcher_search_provider/error_reporter.h"
#include "chrome/browser/profiles/profile.h"
#include "extensions/common/extension.h"
Expand All @@ -20,7 +23,8 @@
namespace app_list {

// Loads icons of launcher search results.
class LauncherSearchIconImageLoader {
class LauncherSearchIconImageLoader
: public base::RefCounted<LauncherSearchIconImageLoader> {
public:
class Observer {
public:
Expand All @@ -43,7 +47,6 @@ class LauncherSearchIconImageLoader {
const int icon_dimension,
std::unique_ptr<chromeos::launcher_search_provider::ErrorReporter>
error_reporter);
virtual ~LauncherSearchIconImageLoader();

// Load resources caller must call this function to generate icon image.
void LoadResources();
Expand All @@ -62,6 +65,9 @@ class LauncherSearchIconImageLoader {
const gfx::ImageSkia& GetBadgeIconImage() const;

protected:
// Ref counted class.
virtual ~LauncherSearchIconImageLoader();

// Loads |extension| icon and returns it as sync if possible. When it loads
// icon as async, it calls OnExtensionIconImageChanged.
virtual const gfx::ImageSkia& LoadExtensionIcon() = 0;
Expand All @@ -83,6 +89,8 @@ class LauncherSearchIconImageLoader {
const gfx::Size icon_size_;

private:
friend class base::RefCounted<LauncherSearchIconImageLoader>;

// Notifies to observers.
void NotifyObserversIconImageChange();
void NotifyObserversBadgeIconImageChange();
Expand Down
9 changes: 5 additions & 4 deletions chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
#include "chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader_impl.h"

#include <utility>
#include <vector>

#include "base/memory/ptr_util.h"
#include "chrome/browser/extensions/extension_util.h"
#include "extensions/browser/image_loader.h"
#include "extensions/common/file_util.h"
Expand All @@ -24,14 +26,13 @@ LauncherSearchIconImageLoaderImpl::LauncherSearchIconImageLoaderImpl(
profile,
extension,
icon_dimension,
std::move(error_reporter)),
weak_ptr_factory_(this) {}
std::move(error_reporter)) {}

LauncherSearchIconImageLoaderImpl::~LauncherSearchIconImageLoaderImpl() =
default;

const gfx::ImageSkia& LauncherSearchIconImageLoaderImpl::LoadExtensionIcon() {
extension_icon_image_.reset(new extensions::IconImage(
extension_icon_image_ = base::WrapUnique(new extensions::IconImage(
profile_, extension_, extensions::IconsInfo::GetIcons(extension_),
icon_size_.width(), extensions::util::GetDefaultExtensionIcon(), this));

Expand All @@ -53,7 +54,7 @@ void LauncherSearchIconImageLoaderImpl::LoadIconResourceFromExtension() {
extensions::ImageLoader::Get(profile_)->LoadImagesAsync(
extension_, info_list,
base::Bind(&LauncherSearchIconImageLoaderImpl::OnCustomIconImageLoaded,
weak_ptr_factory_.GetWeakPtr()));
this));
}

void LauncherSearchIconImageLoaderImpl::OnExtensionIconImageChanged(
Expand Down
8 changes: 4 additions & 4 deletions chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#include <memory>

#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h"
#include "extensions/browser/extension_icon_image.h"

Expand All @@ -26,7 +25,7 @@ class LauncherSearchIconImageLoaderImpl : public LauncherSearchIconImageLoader,
const int icon_dimension,
std::unique_ptr<chromeos::launcher_search_provider::ErrorReporter>
error_reporter);
~LauncherSearchIconImageLoaderImpl() override;

const gfx::ImageSkia& LoadExtensionIcon() override;
void LoadIconResourceFromExtension() override;

Expand All @@ -37,9 +36,10 @@ class LauncherSearchIconImageLoaderImpl : public LauncherSearchIconImageLoader,
void OnCustomIconImageLoaded(const gfx::Image& image);

private:
std::unique_ptr<extensions::IconImage> extension_icon_image_;
// Ref counted class.
~LauncherSearchIconImageLoaderImpl() override;

base::WeakPtrFactory<LauncherSearchIconImageLoaderImpl> weak_ptr_factory_;
std::unique_ptr<extensions::IconImage> extension_icon_image_;

DISALLOW_COPY_AND_ASSIGN(LauncherSearchIconImageLoaderImpl);
};
Expand Down
91 changes: 43 additions & 48 deletions .../browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@

#include "chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h"

#include <utility>

#include "base/macros.h"
#include "base/memory/linked_ptr.h"
#include "base/memory/scoped_refptr.h"
#include "chrome/browser/chromeos/launcher_search_provider/error_reporter.h"
#include "extensions/common/manifest_constants.h"
#include "testing/gtest/include/gtest/gtest.h"
Expand Down Expand Up @@ -78,32 +80,25 @@ class LauncherSearchIconImageLoaderTestImpl
}

private:
// Ref counted class.
~LauncherSearchIconImageLoaderTestImpl() override = default;

gfx::ImageSkia extension_icon_;
bool is_load_extension_icon_resource_called_ = false;
};

// A fake error reporter to test error message.
class FakeErrorReporter : public ErrorReporter {
public:
FakeErrorReporter() : ErrorReporter(nullptr) {
last_message_.reset(new std::string());
}
explicit FakeErrorReporter(const linked_ptr<std::string>& last_message)
: ErrorReporter(nullptr), last_message_(last_message) {}
FakeErrorReporter() : ErrorReporter(nullptr) {}
~FakeErrorReporter() override {}
void Warn(const std::string& message) override {
last_message_->clear();
last_message_->append(message);
}

const std::string& GetLastWarningMessage() { return *last_message_.get(); }
void Warn(const std::string& message) override { last_message_ = message; }

std::unique_ptr<ErrorReporter> Duplicate() override {
return std::make_unique<FakeErrorReporter>(last_message_);
}
const std::string& GetLastWarningMessage() const { return last_message_; }

private:
linked_ptr<std::string> last_message_;
std::string last_message_;

DISALLOW_COPY_AND_ASSIGN(FakeErrorReporter);
};
Expand Down Expand Up @@ -145,72 +140,72 @@ class LauncherSearchIconImageLoaderTest : public testing::Test {

TEST_F(LauncherSearchIconImageLoaderTest, WithoutCustomIconSuccessCase) {
GURL icon_url; // No custom icon.
LauncherSearchIconImageLoaderTestImpl impl(icon_url, nullptr, nullptr, 32,
GetFakeErrorReporter());
impl.LoadResources();
auto impl = base::MakeRefCounted<LauncherSearchIconImageLoaderTestImpl>(
icon_url, nullptr, nullptr, 32, GetFakeErrorReporter());
impl->LoadResources();

// Assert that extension icon image is set to icon image and badge icon image
// is null.
gfx::Size icon_size(32, 32);
gfx::ImageSkia expected_image(
std::make_unique<FillColorImageSource>(icon_size, SK_ColorBLACK),
icon_size);
ASSERT_TRUE(IsEqual(expected_image, impl.GetIconImage()));
ASSERT_TRUE(IsEqual(expected_image, impl->GetIconImage()));

ASSERT_TRUE(impl.GetBadgeIconImage().isNull());
ASSERT_TRUE(impl->GetBadgeIconImage().isNull());
}

TEST_F(LauncherSearchIconImageLoaderTest, ExtensionIconAsyncLoadSuccessCase) {
GURL icon_url; // No custom icon.
LauncherSearchIconImageLoaderTestImpl impl(icon_url, nullptr, nullptr, 32,
GetFakeErrorReporter());
impl.LoadResources();
auto impl = base::MakeRefCounted<LauncherSearchIconImageLoaderTestImpl>(
icon_url, nullptr, nullptr, 32, GetFakeErrorReporter());
impl->LoadResources();

// Extension icon is loaded as async.
gfx::Size icon_size(32, 32);
gfx::ImageSkia extension_icon(
std::make_unique<FillColorImageSource>(icon_size, SK_ColorGREEN),
icon_size);
impl.LoadExtensionIconAsync(extension_icon);
impl->LoadExtensionIconAsync(extension_icon);

// Assert that the asynchronously loaded image is set to icon image and badge
// icon image is null.
gfx::ImageSkia expected_image(
std::make_unique<FillColorImageSource>(icon_size, SK_ColorGREEN),
icon_size);
ASSERT_TRUE(IsEqual(expected_image, impl.GetIconImage()));
ASSERT_TRUE(IsEqual(expected_image, impl->GetIconImage()));

ASSERT_TRUE(impl.GetBadgeIconImage().isNull());
ASSERT_TRUE(impl->GetBadgeIconImage().isNull());
}

TEST_F(LauncherSearchIconImageLoaderTest, WithCustomIconSuccessCase) {
GURL icon_url(kTestCustomIconURL);
LauncherSearchIconImageLoaderTestImpl impl(
auto impl = base::MakeRefCounted<LauncherSearchIconImageLoaderTestImpl>(
icon_url, nullptr, extension_.get(), 32, GetFakeErrorReporter());
ASSERT_FALSE(impl.IsLoadExtensionIconResourceCalled());
impl.LoadResources();
ASSERT_FALSE(impl->IsLoadExtensionIconResourceCalled());
impl->LoadResources();

// Assert that LoadExtensionIconResource is called.
ASSERT_TRUE(impl.IsLoadExtensionIconResourceCalled());
ASSERT_TRUE(impl->IsLoadExtensionIconResourceCalled());

// Load custom icon as async.
gfx::Size icon_size(32, 32);
gfx::ImageSkia custom_icon(
std::make_unique<FillColorImageSource>(icon_size, SK_ColorGREEN),
icon_size);
impl.CallOnCustomIconLoaded(custom_icon);
impl->CallOnCustomIconLoaded(custom_icon);

// Assert that custom icon image is set to icon image and extension icon image
// is set to badge icon image.
gfx::ImageSkia expected_image(
std::make_unique<FillColorImageSource>(icon_size, SK_ColorGREEN),
icon_size);
ASSERT_TRUE(IsEqual(expected_image, impl.GetIconImage()));
ASSERT_TRUE(IsEqual(expected_image, impl->GetIconImage()));

gfx::ImageSkia expected_badge_icon_image(
std::make_unique<FillColorImageSource>(icon_size, SK_ColorBLACK),
icon_size);
ASSERT_TRUE(IsEqual(expected_badge_icon_image, impl.GetBadgeIconImage()));
ASSERT_TRUE(IsEqual(expected_badge_icon_image, impl->GetBadgeIconImage()));
}

TEST_F(LauncherSearchIconImageLoaderTest, InvalidCustomIconUrl) {
Expand All @@ -224,53 +219,53 @@ TEST_F(LauncherSearchIconImageLoaderTest, InvalidCustomIconUrl) {

std::unique_ptr<FakeErrorReporter> fake_error_reporter =
GetFakeErrorReporter();
FakeErrorReporter* fake_error_reporter_ptr = fake_error_reporter.get();
GURL icon_url(invalid_url);
LauncherSearchIconImageLoaderTestImpl impl(icon_url, nullptr,
extension_.get(), 32,
fake_error_reporter->Duplicate());
impl.LoadResources();
auto impl = base::MakeRefCounted<LauncherSearchIconImageLoaderTestImpl>(
icon_url, nullptr, extension_.get(), 32, std::move(fake_error_reporter));
impl->LoadResources();

// Warning message should be provided.
ASSERT_EQ(
"[chrome.launcherSearchProvider.setSearchResults] Invalid icon URL: "
"chrome-extension://foo2/bar/"
"901234567890123456789012345678901234567890123456789012345678901234567..."
". Must have a valid URL within chrome-extension://foo.",
fake_error_reporter->GetLastWarningMessage());
fake_error_reporter_ptr->GetLastWarningMessage());

// LoadExtensionIconResource should not be called.
ASSERT_FALSE(impl.IsLoadExtensionIconResourceCalled());
ASSERT_FALSE(impl->IsLoadExtensionIconResourceCalled());
}

TEST_F(LauncherSearchIconImageLoaderTest, FailedToLoadCustomIcon) {
std::unique_ptr<FakeErrorReporter> fake_error_reporter =
GetFakeErrorReporter();
FakeErrorReporter* fake_error_reporter_ptr = fake_error_reporter.get();
GURL icon_url(kTestCustomIconURL);
LauncherSearchIconImageLoaderTestImpl impl(icon_url, nullptr,
extension_.get(), 32,
fake_error_reporter->Duplicate());
impl.LoadResources();
ASSERT_TRUE(impl.IsLoadExtensionIconResourceCalled());
auto impl = base::MakeRefCounted<LauncherSearchIconImageLoaderTestImpl>(
icon_url, nullptr, extension_.get(), 32, std::move(fake_error_reporter));
impl->LoadResources();
ASSERT_TRUE(impl->IsLoadExtensionIconResourceCalled());

// Fails to load custom icon by passing an empty image.
gfx::ImageSkia custom_icon;
impl.CallOnCustomIconLoaded(custom_icon);
impl->CallOnCustomIconLoaded(custom_icon);

// Warning message should be shown.
ASSERT_EQ(
"[chrome.launcherSearchProvider.setSearchResults] Failed to load icon "
"URL: chrome-extension://foo/bar",
fake_error_reporter->GetLastWarningMessage());
fake_error_reporter_ptr->GetLastWarningMessage());

// Assert that extension icon image is set to icon image and badge icon image
// is null.
gfx::Size icon_size(32, 32);
gfx::ImageSkia expected_image(
std::make_unique<FillColorImageSource>(icon_size, SK_ColorBLACK),
icon_size);
ASSERT_TRUE(IsEqual(expected_image, impl.GetIconImage()));
ASSERT_TRUE(IsEqual(expected_image, impl->GetIconImage()));

ASSERT_TRUE(impl.GetBadgeIconImage().isNull());
ASSERT_TRUE(impl->GetBadgeIconImage().isNull());
}

} // namespace app_list
11 changes: 5 additions & 6 deletions chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,17 +39,16 @@ LauncherSearchResult::LauncherSearchResult(
DCHECK_LE(discrete_value_relevance,
chromeos::launcher_search_provider::kMaxSearchResultScore);

icon_image_loader_.reset(new LauncherSearchIconImageLoaderImpl(
icon_image_loader_ = base::MakeRefCounted<LauncherSearchIconImageLoaderImpl>(
icon_url, profile, extension, GetPreferredIconDimension(display_type()),
std::move(error_reporter)));
std::move(error_reporter));
icon_image_loader_->LoadResources();

Initialize();
}

LauncherSearchResult::~LauncherSearchResult() {
if (icon_image_loader_ != nullptr)
icon_image_loader_->RemoveObserver(this);
icon_image_loader_->RemoveObserver(this);
}

std::unique_ptr<LauncherSearchResult> LauncherSearchResult::Duplicate() const {
Expand Down Expand Up @@ -85,13 +84,13 @@ LauncherSearchResult::LauncherSearchResult(
const int discrete_value_relevance,
Profile* profile,
const extensions::Extension* extension,
const linked_ptr<LauncherSearchIconImageLoader>& icon_image_loader)
const scoped_refptr<LauncherSearchIconImageLoader>& icon_image_loader)
: item_id_(item_id),
discrete_value_relevance_(discrete_value_relevance),
profile_(profile),
extension_(extension),
icon_image_loader_(icon_image_loader) {
DCHECK(icon_image_loader_ != nullptr);
DCHECK(icon_image_loader_);
Initialize();
}

Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
#include <string>

#include "base/macros.h"
#include "base/memory/linked_ptr.h"
#include "base/memory/scoped_refptr.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/app_list/search/chrome_search_result.h"
#include "chrome/browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader.h"
Expand Down Expand Up @@ -44,7 +44,7 @@ class LauncherSearchResult : public ChromeSearchResult,
const int discrete_value_relevance,
Profile* profile,
const extensions::Extension* extension,
const linked_ptr<LauncherSearchIconImageLoader>& icon_image_loader);
const scoped_refptr<LauncherSearchIconImageLoader>& icon_image_loader);
void Initialize();

// Returns search result ID. The search result ID is comprised of the
Expand All @@ -57,7 +57,7 @@ class LauncherSearchResult : public ChromeSearchResult,
const int discrete_value_relevance_;
Profile* profile_;
const extensions::Extension* extension_;
linked_ptr<LauncherSearchIconImageLoader> icon_image_loader_;
scoped_refptr<LauncherSearchIconImageLoader> icon_image_loader_;

DISALLOW_COPY_AND_ASSIGN(LauncherSearchResult);
};
Expand Down

0 comments on commit 7e512a5

Please sign in to comment.