From 977abc645872f3638c3710128c08a964b734a5f6 Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Thu, 16 Feb 2017 22:19:19 +0300 Subject: [PATCH 1/2] Update icon loading API implementation --- atom/browser/api/atom_api_app.cc | 11 +- atom/browser/api/atom_api_app.h | 5 +- chromium_src/chrome/browser/icon_loader.cc | 48 +++--- chromium_src/chrome/browser/icon_loader.h | 75 ++++----- .../chrome/browser/icon_loader_auralinux.cc | 16 +- .../chrome/browser/icon_loader_mac.mm | 16 +- .../chrome/browser/icon_loader_win.cc | 35 ++--- chromium_src/chrome/browser/icon_manager.cc | 143 +++++++----------- chromium_src/chrome/browser/icon_manager.h | 70 ++++----- 9 files changed, 176 insertions(+), 243 deletions(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index 904a63a5d09f..b8cc49f57621 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -890,15 +890,18 @@ void App::GetFileIcon(const base::FilePath& path, return; } - IconManager* icon_manager = IconManager::GetInstance(); - gfx::Image* icon = icon_manager->LookupIconFromFilepath(normalized_path, + if (!icon_manager_.get()) { + icon_manager_.reset(new IconManager()); + } + + gfx::Image* icon = icon_manager_->LookupIconFromFilepath(normalized_path, icon_size); if (icon) { callback.Run(v8::Null(isolate()), *icon); } else { - icon_manager->LoadIcon(normalized_path, icon_size, + icon_manager_->LoadIcon(normalized_path, icon_size, base::Bind(&OnIconDataAvailable, isolate(), - callback)); + callback), &cancelable_task_tracker_); } } diff --git a/atom/browser/api/atom_api_app.h b/atom/browser/api/atom_api_app.h index 1d8e5804e5d2..0f7b138cb98c 100644 --- a/atom/browser/api/atom_api_app.h +++ b/atom/browser/api/atom_api_app.h @@ -13,7 +13,8 @@ #include "atom/browser/browser.h" #include "atom/browser/browser_observer.h" #include "atom/common/native_mate_converters/callback.h" -#include "chrome/browser/icon_loader.h" +#include "base/task/cancelable_task_tracker.h" +#include "chrome/browser/icon_manager.h" #include "chrome/browser/process_singleton.h" #include "content/public/browser/gpu_data_manager_observer.h" #include "native_mate/handle.h" @@ -129,6 +130,8 @@ class App : public AtomBrowserClient::Delegate, void DisableHardwareAcceleration(mate::Arguments* args); bool IsAccessibilitySupportEnabled(); Browser::LoginItemSettings GetLoginItemSettings(mate::Arguments* args); + base::CancelableTaskTracker cancelable_task_tracker_; + std::unique_ptr icon_manager_; #if defined(USE_NSS_CERTS) void ImportCertificate(const base::DictionaryValue& options, const net::CompletionCallback& callback); diff --git a/chromium_src/chrome/browser/icon_loader.cc b/chromium_src/chrome/browser/icon_loader.cc index afc5d8016ea0..a0cca6379d4c 100644 --- a/chromium_src/chrome/browser/icon_loader.cc +++ b/chromium_src/chrome/browser/icon_loader.cc @@ -3,46 +3,40 @@ // found in the LICENSE file. #include "chrome/browser/icon_loader.h" - #include "base/bind.h" #include "base/threading/thread_task_runner_handle.h" #include "content/public/browser/browser_thread.h" using content::BrowserThread; -IconLoader::IconLoader(const base::FilePath& file_path, - IconSize size, - Delegate* delegate) - : target_task_runner_(NULL), - file_path_(file_path), - icon_size_(size), - delegate_(delegate) {} - -IconLoader::~IconLoader() {} +// static +IconLoader* IconLoader::Create(const base::FilePath& file_path, + IconSize size, + IconLoadedCallback callback) { + return new IconLoader(file_path, size, callback); +} void IconLoader::Start() { target_task_runner_ = base::ThreadTaskRunnerHandle::Get(); - - BrowserThread::PostTaskAndReply(BrowserThread::FILE, FROM_HERE, - base::Bind(&IconLoader::ReadGroup, this), - base::Bind(&IconLoader::OnReadGroup, this)); + BrowserThread::PostTaskAndReply( + BrowserThread::FILE, FROM_HERE, + base::Bind(&IconLoader::ReadGroup, base::Unretained(this)), + base::Bind(&IconLoader::OnReadGroup, base::Unretained(this))); } +IconLoader::IconLoader(const base::FilePath& file_path, + IconSize size, + IconLoadedCallback callback) + : file_path_(file_path), icon_size_(size), callback_(callback) {} + +IconLoader::~IconLoader() {} + void IconLoader::ReadGroup() { - group_ = ReadGroupIDFromFilepath(file_path_); + group_ = GroupForFilepath(file_path_); } void IconLoader::OnReadGroup() { - if (IsIconMutableFromFilepath(file_path_) || - !delegate_->OnGroupLoaded(this, group_)) { - BrowserThread::PostTask(ReadIconThreadID(), FROM_HERE, - base::Bind(&IconLoader::ReadIcon, this)); - } -} - -void IconLoader::NotifyDelegate() { - // If the delegate takes ownership of the Image, release it from the scoped - // pointer. - if (delegate_->OnImageLoaded(this, image_.get(), group_)) - ignore_result(image_.release()); // Can't ignore return value. + BrowserThread::PostTask( + ReadIconThreadID(), FROM_HERE, + base::Bind(&IconLoader::ReadIcon, base::Unretained(this))); } diff --git a/chromium_src/chrome/browser/icon_loader.h b/chromium_src/chrome/browser/icon_loader.h index 33301acd8295..50d9ef45410d 100644 --- a/chromium_src/chrome/browser/icon_loader.h +++ b/chromium_src/chrome/browser/icon_loader.h @@ -8,31 +8,30 @@ #include #include +#include "base/callback.h" #include "base/files/file_path.h" #include "base/macros.h" -#include "base/memory/ref_counted.h" #include "base/single_thread_task_runner.h" #include "build/build_config.h" #include "content/public/browser/browser_thread.h" #include "ui/gfx/image/image.h" -#if defined(OS_WIN) -// On Windows, we group files by their extension, with several exceptions: -// .dll, .exe, .ico. See IconManager.h for explanation. -typedef std::wstring IconGroupID; -#elif defined(OS_POSIX) -// On POSIX, we group files by MIME type. -typedef std::string IconGroupID; -#endif - //////////////////////////////////////////////////////////////////////////////// // // A facility to read a file containing an icon asynchronously in the IO // thread. Returns the icon in the form of an ImageSkia. // //////////////////////////////////////////////////////////////////////////////// -class IconLoader : public base::RefCountedThreadSafe { +class IconLoader { public: + // An IconGroup is a class of files that all share the same icon. For all + // platforms but Windows, and for most files on Windows, it is the file type + // (e.g. all .mp3 files share an icon, all .html files share an icon). On + // Windows, for certain file types (.exe, .dll, etc), each file of that type + // is assumed to have a unique icon. In that case, each of those files is a + // group to itself. + using IconGroup = base::FilePath::StringType; + enum IconSize { SMALL = 0, // 16x16 NORMAL, // 32x32 @@ -40,42 +39,32 @@ class IconLoader : public base::RefCountedThreadSafe { ALL, // All sizes available }; - class Delegate { - public: - // Invoked when an icon group has been read, but before the icon data - // is read. If the icon is already cached, this method should call and - // return the results of OnImageLoaded with the cached image. - virtual bool OnGroupLoaded(IconLoader* source, - const IconGroupID& group) = 0; - // Invoked when an icon has been read. |source| is the IconLoader. If the - // icon has been successfully loaded, result is non-null. This method must - // return true if it is taking ownership of the returned image. - virtual bool OnImageLoaded(IconLoader* source, - gfx::Image* result, - const IconGroupID& group) = 0; + // The callback invoked when an icon has been read. The parameters are: + // - The icon that was loaded, or null if there was a failure to load it. + // - The determined group from the original requested path. + using IconLoadedCallback = + base::Callback, const IconGroup&)>; - protected: - virtual ~Delegate() {} - }; + // Creates an IconLoader, which owns itself. If the IconLoader might outlive + // the caller, be sure to use a weak pointer in the |callback|. + static IconLoader* Create(const base::FilePath& file_path, + IconSize size, + IconLoadedCallback callback); - IconLoader(const base::FilePath& file_path, - IconSize size, - Delegate* delegate); - - // Start reading the icon on the file thread. + // Starts the process of reading the icon. When the reading of the icon is + // complete, the IconLoadedCallback callback will be fulfilled, and the + // IconLoader will delete itself. void Start(); private: - friend class base::RefCountedThreadSafe; + IconLoader(const base::FilePath& file_path, + IconSize size, + IconLoadedCallback callback); - virtual ~IconLoader(); + ~IconLoader(); - // Get the identifying string for the given file. The implementation - // is in icon_loader_[platform].cc. - static IconGroupID ReadGroupIDFromFilepath(const base::FilePath& path); - - // Some icons (exe's on windows) can change as they're loaded. - static bool IsIconMutableFromFilepath(const base::FilePath& path); + // Given a file path, get the group for the given file. + static IconGroup GroupForFilepath(const base::FilePath& file_path); // The thread ReadIcon() should be called on. static content::BrowserThread::ID ReadIconThreadID(); @@ -84,20 +73,18 @@ class IconLoader : public base::RefCountedThreadSafe { void OnReadGroup(); void ReadIcon(); - void NotifyDelegate(); - // The task runner object of the thread in which we notify the delegate. scoped_refptr target_task_runner_; base::FilePath file_path_; - IconGroupID group_; + IconGroup group_; IconSize icon_size_; std::unique_ptr image_; - Delegate* delegate_; + IconLoadedCallback callback_; DISALLOW_COPY_AND_ASSIGN(IconLoader); }; diff --git a/chromium_src/chrome/browser/icon_loader_auralinux.cc b/chromium_src/chrome/browser/icon_loader_auralinux.cc index 2a1b0d868f47..449d05771d76 100644 --- a/chromium_src/chrome/browser/icon_loader_auralinux.cc +++ b/chromium_src/chrome/browser/icon_loader_auralinux.cc @@ -10,14 +10,9 @@ #include "ui/views/linux_ui/linux_ui.h" // static -IconGroupID IconLoader::ReadGroupIDFromFilepath( - const base::FilePath& filepath) { - return base::nix::GetFileMimeType(filepath); -} - -// static -bool IconLoader::IsIconMutableFromFilepath(const base::FilePath&) { - return false; +IconLoader::IconGroup IconLoader::GroupForFilepath( + const base::FilePath& file_path) { + return base::nix::GetFileMimeType(file_path); } // static @@ -50,6 +45,7 @@ void IconLoader::ReadIcon() { image_.reset(new gfx::Image(image)); } - target_task_runner_->PostTask(FROM_HERE, - base::Bind(&IconLoader::NotifyDelegate, this)); + target_task_runner_->PostTask( + FROM_HERE, base::Bind(callback_, base::Passed(&image_), group_)); + delete this; } diff --git a/chromium_src/chrome/browser/icon_loader_mac.mm b/chromium_src/chrome/browser/icon_loader_mac.mm index a9dd42566b9e..dd94e9fe7eab 100644 --- a/chromium_src/chrome/browser/icon_loader_mac.mm +++ b/chromium_src/chrome/browser/icon_loader_mac.mm @@ -15,14 +15,9 @@ #include "ui/gfx/image/image_skia_util_mac.h" // static -IconGroupID IconLoader::ReadGroupIDFromFilepath( - const base::FilePath& filepath) { - return filepath.Extension(); -} - -// static -bool IconLoader::IsIconMutableFromFilepath(const base::FilePath&) { - return false; +IconLoader::IconGroup IconLoader::GroupForFilepath( + const base::FilePath& file_path) { + return file_path.Extension(); } // static @@ -57,6 +52,7 @@ void IconLoader::ReadIcon() { } } - target_task_runner_->PostTask(FROM_HERE, - base::Bind(&IconLoader::NotifyDelegate, this)); + target_task_runner_->PostTask( + FROM_HERE, base::Bind(callback_, base::Passed(&image_), group_)); + delete this; } diff --git a/chromium_src/chrome/browser/icon_loader_win.cc b/chromium_src/chrome/browser/icon_loader_win.cc index 079c1f4828d2..279c819cc492 100644 --- a/chromium_src/chrome/browser/icon_loader_win.cc +++ b/chromium_src/chrome/browser/icon_loader_win.cc @@ -17,18 +17,15 @@ #include "ui/gfx/image/image_skia.h" // static -IconGroupID IconLoader::ReadGroupIDFromFilepath( - const base::FilePath& filepath) { - if (!IsIconMutableFromFilepath(filepath)) - return filepath.Extension(); - return filepath.value(); -} +IconLoader::IconGroup IconLoader::GroupForFilepath( + const base::FilePath& file_path) { + if (file_path.MatchesExtension(L".exe") || + file_path.MatchesExtension(L".dll") || + file_path.MatchesExtension(L".ico")) { + return file_path.value(); + } -// static -bool IconLoader::IsIconMutableFromFilepath(const base::FilePath& filepath) { - return filepath.MatchesExtension(L".exe") || - filepath.MatchesExtension(L".dll") || - filepath.MatchesExtension(L".ico"); + return file_path.Extension(); } // static @@ -54,22 +51,22 @@ void IconLoader::ReadIcon() { image_.reset(); - SHFILEINFO file_info = {0}; + SHFILEINFO file_info = { 0 }; if (SHGetFileInfo(group_.c_str(), FILE_ATTRIBUTE_NORMAL, &file_info, - sizeof(SHFILEINFO), - SHGFI_ICON | size | SHGFI_USEFILEATTRIBUTES)) { + sizeof(SHFILEINFO), + SHGFI_ICON | size | SHGFI_USEFILEATTRIBUTES)) { std::unique_ptr bitmap( IconUtil::CreateSkBitmapFromHICON(file_info.hIcon)); if (bitmap.get()) { - gfx::ImageSkia image_skia( - gfx::ImageSkiaRep(*bitmap, display::win::GetDPIScale())); + gfx::ImageSkia image_skia(gfx::ImageSkiaRep(*bitmap, + display::win::GetDPIScale())); image_skia.MakeThreadSafe(); image_.reset(new gfx::Image(image_skia)); DestroyIcon(file_info.hIcon); } } - // Always notify the delegate, regardless of success. - target_task_runner_->PostTask(FROM_HERE, - base::Bind(&IconLoader::NotifyDelegate, this)); + target_task_runner_->PostTask( + FROM_HERE, base::Bind(callback_, base::Passed(&image_), group_)); + delete this; } diff --git a/chromium_src/chrome/browser/icon_manager.cc b/chromium_src/chrome/browser/icon_manager.cc index 95073fcdd755..7b20ef323a79 100644 --- a/chromium_src/chrome/browser/icon_manager.cc +++ b/chromium_src/chrome/browser/icon_manager.cc @@ -8,121 +8,86 @@ #include #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" -struct IconManager::ClientRequest { - IconRequestCallback callback; - base::FilePath file_path; - IconLoader::IconSize size; -}; +namespace { -// static -IconManager* IconManager::GetInstance() { - return base::Singleton::get(); +void RunCallbackIfNotCanceled( + const base::CancelableTaskTracker::IsCanceledCallback& is_canceled, + const IconManager::IconRequestCallback& callback, + gfx::Image* image) { + if (is_canceled.Run()) + return; + callback.Run(image); } -IconManager::IconManager() {} +} // namespace + +IconManager::IconManager() : weak_factory_(this) {} IconManager::~IconManager() { - 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; + CacheKey key(group_it->second, size); + auto icon_it = icon_cache_.find(key); + if (icon_it == icon_cache_.end()) + return nullptr; + + return icon_it->second.get(); } -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; +base::CancelableTaskTracker::TaskId IconManager::LoadIcon( + const base::FilePath& file_path, + IconLoader::IconSize size, + const IconRequestCallback& callback, + base::CancelableTaskTracker* tracker) { + base::CancelableTaskTracker::IsCanceledCallback is_canceled; + base::CancelableTaskTracker::TaskId id = + tracker->NewTrackedTaskId(&is_canceled); + IconRequestCallback callback_runner = base::Bind( + &RunCallbackIfNotCanceled, is_canceled, callback); - return nullptr; -} - -void IconManager::LoadIcon(const base::FilePath& file_name, - IconLoader::IconSize size, - const IconRequestCallback& callback) { - IconLoader* loader = new IconLoader(file_name, size, this); - loader->AddRef(); + IconLoader* loader = IconLoader::Create( + file_path, size, + base::Bind(&IconManager::OnIconLoaded, weak_factory_.GetWeakPtr(), + callback_runner, file_path, size)); loader->Start(); - ClientRequest client_request = {callback, file_name, size}; - requests_[loader] = client_request; + 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; +void IconManager::OnIconLoaded(IconRequestCallback callback, + base::FilePath file_path, + IconLoader::IconSize size, + std::unique_ptr result, + const IconLoader::IconGroup& group) { + // 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, size); + if (result) { + callback.Run(result.get()); + icon_cache_[key] = std::move(result); + } else { + callback.Run(nullptr); + icon_cache_.erase(key); } - gfx::Image* result = LookupIconFromGroup(group, rit->second.size); - if (!result) { - return false; - } - - return OnImageLoaded(loader, result, group); + group_cache_[file_path] = group; } -bool IconManager::OnImageLoaded(IconLoader* loader, - gfx::Image* result, - const IconGroupID& group) { - ClientRequests::iterator rit = requests_.find(loader); - - // 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; - - // 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. - 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; - } - - 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. -} - -IconManager::CacheKey::CacheKey(const IconGroupID& group, +IconManager::CacheKey::CacheKey(const IconLoader::IconGroup& group, IconLoader::IconSize size) : group(group), size(size) {} -bool IconManager::CacheKey::operator<(const CacheKey& other) const { +bool IconManager::CacheKey::operator<(const CacheKey &other) const { return std::tie(group, size) < std::tie(other.group, other.size); } diff --git a/chromium_src/chrome/browser/icon_manager.h b/chromium_src/chrome/browser/icon_manager.h index dd6de8252756..5cda41d469b3 100644 --- a/chromium_src/chrome/browser/icon_manager.h +++ b/chromium_src/chrome/browser/icon_manager.h @@ -34,7 +34,7 @@ // 2. An asynchronous icon load from a file on the file thread: // IconManager::LoadIcon() // -// When using the second (asychronous) method, callers must supply a callback +// When using the second (asynchronous) method, callers must supply a callback // which will be run once the icon has been extracted. The icon manager will // cache the results of the icon extraction so that subsequent lookups will be // fast. @@ -46,25 +46,29 @@ #define CHROME_BROWSER_ICON_MANAGER_H_ #include +#include #include "base/files/file_path.h" #include "base/macros.h" -#include "base/memory/singleton.h" +#include "base/memory/weak_ptr.h" +#include "base/task/cancelable_task_tracker.h" #include "chrome/browser/icon_loader.h" #include "ui/gfx/image/image.h" -class IconManager : public IconLoader::Delegate { +class IconManager { public: - static IconManager* GetInstance(); + IconManager(); + ~IconManager(); + // Synchronous call to examine the internal caches for the icon. Returns the - // icon if we have already loaded it, NULL if we don't have it and must load - // it via 'LoadIcon'. The returned bitmap is owned by the IconManager and must - // not be free'd by the caller. If the caller needs to modify the icon, it - // must make a copy and modify the copy. - gfx::Image* LookupIconFromFilepath(const base::FilePath& file_name, + // icon if we have already loaded it, or null if we don't have it and must + // load it via LoadIcon(). The returned bitmap is owned by the IconManager and + // must not be free'd by the caller. If the caller needs to modify the icon, + // it must make a copy and modify the copy. + gfx::Image* LookupIconFromFilepath(const base::FilePath& file_path, IconLoader::IconSize size); - typedef base::Callback IconRequestCallback; + using IconRequestCallback = base::Callback; // Asynchronous call to lookup and return the icon associated with file. The // work is done on the file thread, with the callbacks running on the thread @@ -74,47 +78,35 @@ class IconManager : public IconLoader::Delegate { // 1. This does *not* check the cache. // 2. The returned bitmap pointer is *not* owned by callback. So callback // should never keep it or delete it. - // 3. The gfx::Image pointer passed to the callback may be NULL if decoding + // 3. The gfx::Image pointer passed to the callback will be null if decoding // failed. - void LoadIcon(const base::FilePath& file_name, - IconLoader::IconSize size, - const IconRequestCallback& callback); - - // IconLoader::Delegate interface. - bool OnGroupLoaded(IconLoader* loader, const IconGroupID& group) override; - bool OnImageLoaded(IconLoader* loader, - gfx::Image* result, - const IconGroupID& group) override; + base::CancelableTaskTracker::TaskId LoadIcon( + const base::FilePath& file_name, + IconLoader::IconSize size, + const IconRequestCallback& callback, + base::CancelableTaskTracker* tracker); private: - friend struct base::DefaultSingletonTraits; - - IconManager(); - ~IconManager() override; + void OnIconLoaded(IconRequestCallback callback, + base::FilePath file_path, + IconLoader::IconSize size, + std::unique_ptr result, + const IconLoader::IconGroup& group); struct CacheKey { - CacheKey(const IconGroupID& group, IconLoader::IconSize size); + CacheKey(const IconLoader::IconGroup& group, IconLoader::IconSize size); // Used as a key in the map below, so we need this comparator. - bool operator<(const CacheKey& other) const; + bool operator<(const CacheKey &other) const; - IconGroupID group; + IconLoader::IconGroup group; IconLoader::IconSize size; }; - gfx::Image* LookupIconFromGroup(const IconGroupID& group, - IconLoader::IconSize size); + std::map group_cache_; + std::map> icon_cache_; - typedef std::map IconMap; - IconMap icon_cache_; - - typedef std::map GroupMap; - GroupMap group_cache_; - - // Asynchronous requests that have not yet been completed. - struct ClientRequest; - typedef std::map ClientRequests; - ClientRequests requests_; + base::WeakPtrFactory weak_factory_; DISALLOW_COPY_AND_ASSIGN(IconManager); }; From 5687f8b3b712d296e3fcd2f71a405383e33eb115 Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Fri, 17 Feb 2017 14:03:46 +0530 Subject: [PATCH 2/2] Destroy icon manager after file thread is destroyed --- atom/browser/api/atom_api_app.cc | 17 ++++++++--------- atom/browser/api/atom_api_app.h | 5 +++-- chromium_src/chrome/browser/browser_process.cc | 10 +++++++++- chromium_src/chrome/browser/browser_process.h | 4 ++++ 4 files changed, 24 insertions(+), 12 deletions(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index b8cc49f57621..56a11daf69bd 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -30,6 +30,7 @@ #include "base/path_service.h" #include "base/strings/string_util.h" #include "brightray/browser/brightray_paths.h" +#include "chrome/browser/browser_process.h" #include "chrome/browser/icon_manager.h" #include "chrome/common/chrome_paths.h" #include "content/public/browser/browser_accessibility_state.h" @@ -890,18 +891,16 @@ void App::GetFileIcon(const base::FilePath& path, return; } - if (!icon_manager_.get()) { - icon_manager_.reset(new IconManager()); - } - - gfx::Image* icon = icon_manager_->LookupIconFromFilepath(normalized_path, - icon_size); + auto icon_manager = g_browser_process->GetIconManager(); + gfx::Image* icon = + icon_manager->LookupIconFromFilepath(normalized_path, icon_size); if (icon) { callback.Run(v8::Null(isolate()), *icon); } else { - icon_manager_->LoadIcon(normalized_path, icon_size, - base::Bind(&OnIconDataAvailable, isolate(), - callback), &cancelable_task_tracker_); + icon_manager->LoadIcon( + normalized_path, icon_size, + base::Bind(&OnIconDataAvailable, isolate(), callback), + &cancelable_task_tracker_); } } diff --git a/atom/browser/api/atom_api_app.h b/atom/browser/api/atom_api_app.h index 0f7b138cb98c..8b276f334d5c 100644 --- a/atom/browser/api/atom_api_app.h +++ b/atom/browser/api/atom_api_app.h @@ -130,8 +130,6 @@ class App : public AtomBrowserClient::Delegate, void DisableHardwareAcceleration(mate::Arguments* args); bool IsAccessibilitySupportEnabled(); Browser::LoginItemSettings GetLoginItemSettings(mate::Arguments* args); - base::CancelableTaskTracker cancelable_task_tracker_; - std::unique_ptr icon_manager_; #if defined(USE_NSS_CERTS) void ImportCertificate(const base::DictionaryValue& options, const net::CompletionCallback& callback); @@ -153,6 +151,9 @@ class App : public AtomBrowserClient::Delegate, std::unique_ptr certificate_manager_model_; #endif + // Tracks tasks requesting file icons. + base::CancelableTaskTracker cancelable_task_tracker_; + DISALLOW_COPY_AND_ASSIGN(App); }; diff --git a/chromium_src/chrome/browser/browser_process.cc b/chromium_src/chrome/browser/browser_process.cc index a38d55f87159..d37478396ef8 100644 --- a/chromium_src/chrome/browser/browser_process.cc +++ b/chromium_src/chrome/browser/browser_process.cc @@ -4,13 +4,15 @@ #include "chrome/browser/browser_process.h" +#include "chrome/browser/icon_manager.h" #include "chrome/browser/printing/print_job_manager.h" #include "ui/base/l10n/l10n_util.h" BrowserProcess* g_browser_process = NULL; BrowserProcess::BrowserProcess() - : print_job_manager_(new printing::PrintJobManager) { + : print_job_manager_(new printing::PrintJobManager), + icon_manager_(new IconManager) { g_browser_process = this; } @@ -22,6 +24,12 @@ std::string BrowserProcess::GetApplicationLocale() { return l10n_util::GetApplicationLocale(""); } +IconManager* BrowserProcess::GetIconManager() { + if (!icon_manager_.get()) + icon_manager_.reset(new IconManager); + return icon_manager_.get(); +} + printing::PrintJobManager* BrowserProcess::print_job_manager() { return print_job_manager_.get(); } diff --git a/chromium_src/chrome/browser/browser_process.h b/chromium_src/chrome/browser/browser_process.h index 1459ca31a60a..1c1156f45244 100644 --- a/chromium_src/chrome/browser/browser_process.h +++ b/chromium_src/chrome/browser/browser_process.h @@ -15,6 +15,8 @@ #include "base/macros.h" +class IconManager; + namespace printing { class PrintJobManager; } @@ -27,11 +29,13 @@ class BrowserProcess { ~BrowserProcess(); std::string GetApplicationLocale(); + IconManager* GetIconManager(); printing::PrintJobManager* print_job_manager(); private: std::unique_ptr print_job_manager_; + std::unique_ptr icon_manager_; DISALLOW_COPY_AND_ASSIGN(BrowserProcess); };