Index: chrome/browser/icon_manager.cc |
diff --git a/chrome/browser/icon_manager.cc b/chrome/browser/icon_manager.cc |
index 6596ab311234c4660a1a7b0e7a54ee8becf43d26..1796f417fadcd22cc2d0d79625b8daa2ceb88b63 100644 |
--- a/chrome/browser/icon_manager.cc |
+++ b/chrome/browser/icon_manager.cc |
@@ -8,7 +8,6 @@ |
#include <tuple> |
#include "base/bind.h" |
-#include "base/stl_util.h" |
#include "base/task_runner.h" |
#include "third_party/skia/include/core/SkBitmap.h" |
#include "third_party/skia/include/core/SkCanvas.h" |
@@ -36,33 +35,28 @@ IconManager::IconManager() { |
} |
IconManager::~IconManager() { |
sky
2016/12/15 22:36:05
In reviewing this code it seems like IconManager i
Avi (use Gerrit)
2016/12/15 23:01:10
IconManager is the only user of IconLoader. Why is
|
- base::STLDeleteValues(&icon_cache_); |
} |
-gfx::Image* IconManager::LookupIconFromFilepath(const base::FilePath& file_name, |
+gfx::Image* IconManager::LookupIconFromFilepath(const base::FilePath& file_path, |
IconLoader::IconSize size) { |
- GroupMap::iterator it = group_cache_.find(file_name); |
- if (it != group_cache_.end()) |
- return LookupIconFromGroup(it->second, size); |
+ auto group_it = group_cache_.find(file_path); |
+ if (group_it == group_cache_.end()) |
+ return nullptr; |
- return NULL; |
-} |
- |
-gfx::Image* IconManager::LookupIconFromGroup(const IconGroupID& group, |
- IconLoader::IconSize size) { |
- IconMap::iterator it = icon_cache_.find(CacheKey(group, size)); |
- if (it != icon_cache_.end()) |
- return it->second; |
+ CacheKey key(group_it->second, size); |
+ auto icon_it = icon_cache_.find(key); |
+ if (icon_it == icon_cache_.end()) |
+ return nullptr; |
- return NULL; |
+ return icon_it->second.get(); |
} |
base::CancelableTaskTracker::TaskId IconManager::LoadIcon( |
- const base::FilePath& file_name, |
+ const base::FilePath& file_path, |
IconLoader::IconSize size, |
const IconRequestCallback& callback, |
base::CancelableTaskTracker* tracker) { |
- IconLoader* loader = new IconLoader(file_name, size, this); |
+ IconLoader* loader = new IconLoader(file_path, size, this); |
loader->AddRef(); |
sky
2016/12/15 22:36:05
This code is equally error prone. Seems like the m
Avi (use Gerrit)
2016/12/15 23:01:10
Will address in followup.
|
loader->Start(); |
@@ -72,76 +66,43 @@ base::CancelableTaskTracker::TaskId IconManager::LoadIcon( |
IconRequestCallback callback_runner = base::Bind( |
&RunCallbackIfNotCanceled, is_canceled, callback); |
- ClientRequest client_request = { callback_runner, file_name, size }; |
- requests_[loader] = client_request; |
+ requests_[loader] = {callback_runner, file_path, size}; |
return id; |
} |
// IconLoader::Delegate implementation ----------------------------------------- |
-bool IconManager::OnGroupLoaded(IconLoader* loader, |
- const IconGroupID& group) { |
- ClientRequests::iterator rit = requests_.find(loader); |
- if (rit == requests_.end()) { |
- NOTREACHED(); |
- return false; |
- } |
- |
- gfx::Image* result = LookupIconFromGroup(group, rit->second.size); |
- if (!result) { |
- return false; |
- } |
- |
- return OnImageLoaded(loader, result, group); |
-} |
- |
-bool IconManager::OnImageLoaded( |
- IconLoader* loader, gfx::Image* result, const IconGroupID& group) { |
- ClientRequests::iterator rit = requests_.find(loader); |
+void IconManager::OnImageLoaded(IconLoader* loader, |
+ std::unique_ptr<gfx::Image> result, |
+ const IconLoader::IconGroup& group) { |
+ auto request_it = requests_.find(loader); |
+ DCHECK(request_it != requests_.end()); |
// Balances the AddRef() in LoadIcon(). |
loader->Release(); |
- // Look up our client state. |
- if (rit == requests_.end()) { |
- NOTREACHED(); |
- return false; // Return false to indicate result should be deleted. |
- } |
- |
- const ClientRequest& client_request = rit->second; |
+ const ClientRequest& client_request = request_it->second; |
- // Cache the bitmap. Watch out: |result| may be NULL to indicate a current |
- // failure. We assume that if we have an entry in |icon_cache_| |
- // it must not be NULL. |
+ // Cache the bitmap. Watch out: |result| may be null, which indicates a |
+ // failure. We assume that if we have an entry in |icon_cache_| it must not be |
+ // null. |
CacheKey key(group, client_request.size); |
- IconMap::iterator it = icon_cache_.find(key); |
- if (it != icon_cache_.end()) { |
- if (!result) { |
- delete it->second; |
- icon_cache_.erase(it); |
- } else if (result != it->second) { |
- it->second->SwapRepresentations(result); |
- delete result; |
- result = it->second; |
- } |
- } else if (result) { |
- icon_cache_[key] = result; |
+ if (result) { |
+ client_request.callback.Run(result.get()); |
+ icon_cache_[key] = std::move(result); |
+ } else { |
+ client_request.callback.Run(nullptr); |
+ icon_cache_.erase(key); |
} |
group_cache_[client_request.file_path] = group; |
- // Inform our client that the request has completed. |
- client_request.callback.Run(result); |
- requests_.erase(rit); |
- |
- return true; // Indicates we took ownership of result. |
+ requests_.erase(request_it); |
} |
-IconManager::CacheKey::CacheKey(const IconGroupID& group, |
+IconManager::CacheKey::CacheKey(const IconLoader::IconGroup& group, |
IconLoader::IconSize size) |
- : group(group), |
- size(size) { |
-} |
+ : group(group), size(size) {} |
bool IconManager::CacheKey::operator<(const CacheKey &other) const { |
return std::tie(group, size) < std::tie(other.group, other.size); |