From be5b90717961ba0bbd7dac0762b791a2b7c1b2af Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Sat, 29 Oct 2016 14:02:33 +0300 Subject: [PATCH 01/32] WIP --- atom/common/api/atom_api_native_image.cc | 8 ++++++++ atom/common/api/atom_api_native_image.h | 4 ++++ 2 files changed, 12 insertions(+) diff --git a/atom/common/api/atom_api_native_image.cc b/atom/common/api/atom_api_native_image.cc index 0c174941db..30d4b124a1 100644 --- a/atom/common/api/atom_api_native_image.cc +++ b/atom/common/api/atom_api_native_image.cc @@ -301,6 +301,12 @@ gfx::Size NativeImage::GetSize() { return image_.Size(); } +void NativeImage::CreateFromFileIcon(v8::Isolate* isolate, + const base::FilePath& path, + const IconLoadedCallback& callback) { + callback.Run(CreateEmpty(isolate)); +} + float NativeImage::GetAspectRatio() { gfx::Size size = GetSize(); if (size.IsEmpty()) @@ -514,6 +520,8 @@ void Initialize(v8::Local exports, v8::Local unused, mate::Dictionary dict(context->GetIsolate(), exports); dict.SetMethod("createEmpty", &atom::api::NativeImage::CreateEmpty); dict.SetMethod("createFromPath", &atom::api::NativeImage::CreateFromPath); + dict.SetMethod("createFromFileIcon", + &atom::api::NativeImage::CreateFromFileIcon); dict.SetMethod("createFromBuffer", &atom::api::NativeImage::CreateFromBuffer); dict.SetMethod("createFromDataURL", &atom::api::NativeImage::CreateFromDataURL); diff --git a/atom/common/api/atom_api_native_image.h b/atom/common/api/atom_api_native_image.h index ee1b5f5d4b..251776b893 100644 --- a/atom/common/api/atom_api_native_image.h +++ b/atom/common/api/atom_api_native_image.h @@ -38,6 +38,7 @@ namespace atom { namespace api { class NativeImage : public mate::Wrappable { +using IconLoadedCallback = base::Callback)>; public: static mate::Handle CreateEmpty(v8::Isolate* isolate); static mate::Handle Create( @@ -52,6 +53,9 @@ class NativeImage : public mate::Wrappable { mate::Arguments* args, v8::Local buffer); static mate::Handle CreateFromDataURL( v8::Isolate* isolate, const GURL& url); + static void CreateFromFileIcon(v8::Isolate* isolate, + const base::FilePath& path, + const IconLoadedCallback& callback); static void BuildPrototype(v8::Isolate* isolate, v8::Local prototype); From 1d24a3a1751ea3c5020a636e9c6020eb2376dabe Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Sat, 29 Oct 2016 14:38:23 +0300 Subject: [PATCH 02/32] Add callback converters --- atom/common/api/atom_api_native_image.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/atom/common/api/atom_api_native_image.cc b/atom/common/api/atom_api_native_image.cc index 30d4b124a1..c9e11f7619 100644 --- a/atom/common/api/atom_api_native_image.cc +++ b/atom/common/api/atom_api_native_image.cc @@ -12,6 +12,7 @@ #include "atom/common/native_mate_converters/gfx_converter.h" #include "atom/common/native_mate_converters/gurl_converter.h" #include "atom/common/native_mate_converters/value_converter.h" +#include "atom/common/native_mate_converters/callback.h" #include "base/base64.h" #include "base/files/file_util.h" #include "base/strings/pattern.h" From 8e4ed664d9be988f73bce78ef36b87bb9f07e944 Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Sat, 29 Oct 2016 15:35:50 +0300 Subject: [PATCH 03/32] Add icon fetching sources --- atom/common/api/atom_api_native_image.cc | 3 +- chromium_src/chrome/browser/icon_loader.cc | 49 ++++++ chromium_src/chrome/browser/icon_loader.h | 105 +++++++++++ chromium_src/chrome/browser/icon_manager.cc | 148 ++++++++++++++++ chromium_src/chrome/browser/icon_manager.h | 121 +++++++++++++ .../browser/ui/webui/fileicon_source.cc | 164 ++++++++++++++++++ .../chrome/browser/ui/webui/fileicon_source.h | 73 ++++++++ filenames.gypi | 6 + 8 files changed, 668 insertions(+), 1 deletion(-) create mode 100644 chromium_src/chrome/browser/icon_loader.cc create mode 100644 chromium_src/chrome/browser/icon_loader.h create mode 100644 chromium_src/chrome/browser/icon_manager.cc create mode 100644 chromium_src/chrome/browser/icon_manager.h create mode 100644 chromium_src/chrome/browser/ui/webui/fileicon_source.cc create mode 100644 chromium_src/chrome/browser/ui/webui/fileicon_source.h diff --git a/atom/common/api/atom_api_native_image.cc b/atom/common/api/atom_api_native_image.cc index c9e11f7619..6fd34153d3 100644 --- a/atom/common/api/atom_api_native_image.cc +++ b/atom/common/api/atom_api_native_image.cc @@ -8,15 +8,16 @@ #include #include "atom/common/asar/asar_util.h" +#include "atom/common/native_mate_converters/callback.h" #include "atom/common/native_mate_converters/file_path_converter.h" #include "atom/common/native_mate_converters/gfx_converter.h" #include "atom/common/native_mate_converters/gurl_converter.h" #include "atom/common/native_mate_converters/value_converter.h" -#include "atom/common/native_mate_converters/callback.h" #include "base/base64.h" #include "base/files/file_util.h" #include "base/strings/pattern.h" #include "base/strings/string_util.h" +#include "chrome/browser/ui/webui/fileicon_source.h" #include "native_mate/dictionary.h" #include "native_mate/object_template_builder.h" #include "net/base/data_url.h" diff --git a/chromium_src/chrome/browser/icon_loader.cc b/chromium_src/chrome/browser/icon_loader.cc new file mode 100644 index 0000000000..979ee5772c --- /dev/null +++ b/chromium_src/chrome/browser/icon_loader.cc @@ -0,0 +1,49 @@ +// Copyright (c) 2012 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 "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() { +} + +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)); +} + +void IconLoader::ReadGroup() { + group_ = ReadGroupIDFromFilepath(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. +} diff --git a/chromium_src/chrome/browser/icon_loader.h b/chromium_src/chrome/browser/icon_loader.h new file mode 100644 index 0000000000..33301acd82 --- /dev/null +++ b/chromium_src/chrome/browser/icon_loader.h @@ -0,0 +1,105 @@ +// Copyright (c) 2012 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 CHROME_BROWSER_ICON_LOADER_H_ +#define CHROME_BROWSER_ICON_LOADER_H_ + +#include +#include + +#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 { + public: + enum IconSize { + SMALL = 0, // 16x16 + NORMAL, // 32x32 + LARGE, // Windows: 32x32, Linux: 48x48, Mac: Unsupported + 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; + + protected: + virtual ~Delegate() {} + }; + + IconLoader(const base::FilePath& file_path, + IconSize size, + Delegate* delegate); + + // Start reading the icon on the file thread. + void Start(); + + private: + friend class base::RefCountedThreadSafe; + + virtual ~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); + + // The thread ReadIcon() should be called on. + static content::BrowserThread::ID ReadIconThreadID(); + + void ReadGroup(); + 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_; + + IconSize icon_size_; + + std::unique_ptr image_; + + Delegate* delegate_; + + DISALLOW_COPY_AND_ASSIGN(IconLoader); +}; + +#endif // CHROME_BROWSER_ICON_LOADER_H_ diff --git a/chromium_src/chrome/browser/icon_manager.cc b/chromium_src/chrome/browser/icon_manager.cc new file mode 100644 index 0000000000..6596ab3112 --- /dev/null +++ b/chromium_src/chrome/browser/icon_manager.cc @@ -0,0 +1,148 @@ +// Copyright (c) 2011 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 "chrome/browser/icon_manager.h" + +#include +#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" + +namespace { + +void RunCallbackIfNotCanceled( + const base::CancelableTaskTracker::IsCanceledCallback& is_canceled, + const IconManager::IconRequestCallback& callback, + gfx::Image* image) { + if (is_canceled.Run()) + return; + callback.Run(image); +} + +} // namespace + +struct IconManager::ClientRequest { + IconRequestCallback callback; + base::FilePath file_path; + IconLoader::IconSize size; +}; + +IconManager::IconManager() { +} + +IconManager::~IconManager() { + base::STLDeleteValues(&icon_cache_); +} + +gfx::Image* IconManager::LookupIconFromFilepath(const base::FilePath& file_name, + IconLoader::IconSize size) { + GroupMap::iterator it = group_cache_.find(file_name); + if (it != group_cache_.end()) + return LookupIconFromGroup(it->second, size); + + 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; + + return NULL; +} + +base::CancelableTaskTracker::TaskId IconManager::LoadIcon( + const base::FilePath& file_name, + IconLoader::IconSize size, + const IconRequestCallback& callback, + base::CancelableTaskTracker* tracker) { + IconLoader* loader = new IconLoader(file_name, size, this); + loader->AddRef(); + loader->Start(); + + base::CancelableTaskTracker::IsCanceledCallback is_canceled; + base::CancelableTaskTracker::TaskId id = + tracker->NewTrackedTaskId(&is_canceled); + IconRequestCallback callback_runner = base::Bind( + &RunCallbackIfNotCanceled, is_canceled, callback); + + ClientRequest client_request = { callback_runner, 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; + } + + 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); + + // 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, + IconLoader::IconSize size) + : group(group), + size(size) { +} + +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 new file mode 100644 index 0000000000..0b5ec07d41 --- /dev/null +++ b/chromium_src/chrome/browser/icon_manager.h @@ -0,0 +1,121 @@ +// Copyright (c) 2012 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. +// +// Class for finding and caching Windows explorer icons. The IconManager +// lives on the UI thread but performs icon extraction work on the file thread +// to avoid blocking the UI thread with potentially expensive COM and disk +// operations. +// +// Terminology +// +// Windows files have icons associated with them that can be of two types: +// 1. "Per class": the icon used for this file is used for all files with the +// same file extension or class. Examples are PDF or MP3 files, which use +// the same icon for all files of that type. +// 2. "Per instance": the icon used for this file is embedded in the file +// itself and is unique. Executable files are typically "per instance". +// +// Files that end in the following extensions are considered "per instance": +// .exe +// .dll +// .ico +// The IconManager will do explicit icon loads on the full path of these files +// and cache the results per file. All other file types will be looked up by +// file extension and the results will be cached per extension. That way, all +// .mp3 files will share one icon, but all .exe files will have their own icon. +// +// POSIX files don't have associated icons. We query the OS by the file's +// mime type. +// +// The IconManager can be queried in two ways: +// 1. A quick, synchronous check of its caches which does not touch the disk: +// IconManager::LookupIcon() +// 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 +// 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. +// +// Icon bitmaps returned should be treated as const since they may be referenced +// by other clients. Make a copy of the icon if you need to modify it. + +#ifndef CHROME_BROWSER_ICON_MANAGER_H_ +#define CHROME_BROWSER_ICON_MANAGER_H_ + +#include + +#include "base/files/file_path.h" +#include "base/macros.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 { + public: + IconManager(); + ~IconManager() override; + + // 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, + IconLoader::IconSize size); + + typedef base::Callback IconRequestCallback; + + // 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 + // this function is called. + // + // Note: + // 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 + // failed. + base::CancelableTaskTracker::TaskId LoadIcon( + const base::FilePath& file_name, + IconLoader::IconSize size, + const IconRequestCallback& callback, + base::CancelableTaskTracker* tracker); + + // IconLoader::Delegate interface. + bool OnGroupLoaded(IconLoader* loader, const IconGroupID& group) override; + bool OnImageLoaded(IconLoader* loader, + gfx::Image* result, + const IconGroupID& group) override; + + private: + struct CacheKey { + CacheKey(const IconGroupID& group, IconLoader::IconSize size); + + // Used as a key in the map below, so we need this comparator. + bool operator<(const CacheKey &other) const; + + IconGroupID group; + IconLoader::IconSize size; + }; + + gfx::Image* LookupIconFromGroup(const IconGroupID& group, + IconLoader::IconSize size); + + 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_; + + DISALLOW_COPY_AND_ASSIGN(IconManager); +}; + +#endif // CHROME_BROWSER_ICON_MANAGER_H_ diff --git a/chromium_src/chrome/browser/ui/webui/fileicon_source.cc b/chromium_src/chrome/browser/ui/webui/fileicon_source.cc new file mode 100644 index 0000000000..8659346b58 --- /dev/null +++ b/chromium_src/chrome/browser/ui/webui/fileicon_source.cc @@ -0,0 +1,164 @@ +// Copyright (c) 2012 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 "chrome/browser/ui/webui/fileicon_source.h" + +#include "base/bind.h" +#include "base/callback.h" +#include "base/files/file_path.h" +#include "base/memory/ref_counted_memory.h" +#include "base/message_loop/message_loop.h" +#include "base/strings/string_split.h" +#include "base/strings/utf_string_conversions.h" +#include "chrome/browser/browser_process.h" +#include "net/base/escape.h" +#include "third_party/skia/include/core/SkBitmap.h" +#include "ui/base/webui/web_ui_util.h" +#include "ui/gfx/codec/png_codec.h" +#include "ui/gfx/image/image.h" +#include "ui/gfx/image/image_skia.h" +#include "url/gurl.h" + +namespace { + +typedef std::map QueryIconSizeMap; + +// The path used in internal URLs to file icon data. +const char kFileIconPath[] = "fileicon"; + +// URL parameter specifying icon size. +const char kIconSize[] = "iconsize"; + +// URL parameter specifying scale factor. +const char kScaleFactor[] = "scale"; + +// Assuming the url is of the form '/path?query', convert the path portion into +// a FilePath and return the resulting |file_path| and |query|. The path +// portion may have been encoded using encodeURIComponent(). +void GetFilePathAndQuery(const std::string& url, + base::FilePath* file_path, + std::string* query) { + // We receive the url with chrome://fileicon/ stripped but GURL expects it. + const GURL gurl("chrome://fileicon/" + url); + std::string path = net::UnescapeURLComponent( + gurl.path().substr(1), + net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS | + net::UnescapeRule::PATH_SEPARATORS | net::UnescapeRule::SPACES); + + *file_path = base::FilePath::FromUTF8Unsafe(path); + *file_path = file_path->NormalizePathSeparators(); + query->assign(gurl.query()); +} + +IconLoader::IconSize SizeStringToIconSize(const std::string& size_string) { + if (size_string == "small") return IconLoader::SMALL; + if (size_string == "large") return IconLoader::LARGE; + // We default to NORMAL if we don't recognize the size_string. Including + // size_string=="normal". + return IconLoader::NORMAL; +} + +// Simple parser for data on the query. +void ParseQueryParams(const std::string& query, + float* scale_factor, + IconLoader::IconSize* icon_size) { + base::StringPairs parameters; + if (icon_size) + *icon_size = IconLoader::NORMAL; + if (scale_factor) + *scale_factor = 1.0f; + base::SplitStringIntoKeyValuePairs(query, '=', '&', ¶meters); + for (base::StringPairs::const_iterator iter = parameters.begin(); + iter != parameters.end(); ++iter) { + if (icon_size && iter->first == kIconSize) + *icon_size = SizeStringToIconSize(iter->second); + else if (scale_factor && iter->first == kScaleFactor) + webui::ParseScaleFactor(iter->second, scale_factor); + } +} + +} // namespace + +FileIconSource::IconRequestDetails::IconRequestDetails() : scale_factor(1.0f) { +} + +FileIconSource::IconRequestDetails::IconRequestDetails( + const IconRequestDetails& other) = default; + +FileIconSource::IconRequestDetails::~IconRequestDetails() { +} + +FileIconSource::FileIconSource() {} + +FileIconSource::~FileIconSource() {} + +void FileIconSource::FetchFileIcon( + const base::FilePath& path, + float scale_factor, + IconLoader::IconSize icon_size, + const content::URLDataSource::GotDataCallback& callback) { + IconManager* im = g_browser_process->icon_manager(); + gfx::Image* icon = im->LookupIconFromFilepath(path, icon_size); + + if (icon) { + scoped_refptr icon_data(new base::RefCountedBytes); + gfx::PNGCodec::EncodeBGRASkBitmap( + icon->ToImageSkia()->GetRepresentation(scale_factor).sk_bitmap(), + false, + &icon_data->data()); + + callback.Run(icon_data.get()); + } else { + // Attach the ChromeURLDataManager request ID to the history request. + IconRequestDetails details; + details.callback = callback; + details.scale_factor = scale_factor; + + // Icon was not in cache, go fetch it slowly. + im->LoadIcon(path, + icon_size, + base::Bind(&FileIconSource::OnFileIconDataAvailable, + base::Unretained(this), details), + &cancelable_task_tracker_); + } +} + +std::string FileIconSource::GetSource() const { + return kFileIconPath; +} + +// void FileIconSource::StartDataRequest( +// const std::string& url_path, +// const content::ResourceRequestInfo::WebContentsGetter& wc_getter, +// const content::URLDataSource::GotDataCallback& callback) { +// std::string query; +// base::FilePath file_path; +// IconLoader::IconSize icon_size; +// float scale_factor = 1.0f; +// GetFilePathAndQuery(url_path, &file_path, &query); +// ParseQueryParams(query, &scale_factor, &icon_size); +// FetchFileIcon(file_path, scale_factor, icon_size, callback); +// } + +std::string FileIconSource::GetMimeType(const std::string&) const { + // Rely on image decoder inferring the correct type. + return std::string(); +} + +void FileIconSource::OnFileIconDataAvailable(const IconRequestDetails& details, + gfx::Image* icon) { + if (icon) { + scoped_refptr icon_data(new base::RefCountedBytes); + gfx::PNGCodec::EncodeBGRASkBitmap( + icon->ToImageSkia()->GetRepresentation( + details.scale_factor).sk_bitmap(), + false, + &icon_data->data()); + + details.callback.Run(icon_data.get()); + } else { + // TODO(glen): send a dummy icon. + details.callback.Run(NULL); + } +} diff --git a/chromium_src/chrome/browser/ui/webui/fileicon_source.h b/chromium_src/chrome/browser/ui/webui/fileicon_source.h new file mode 100644 index 0000000000..7fd6671b60 --- /dev/null +++ b/chromium_src/chrome/browser/ui/webui/fileicon_source.h @@ -0,0 +1,73 @@ +// Copyright (c) 2012 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 CHROME_BROWSER_UI_WEBUI_FILEICON_SOURCE_H_ +#define CHROME_BROWSER_UI_WEBUI_FILEICON_SOURCE_H_ + +#include + +#include "base/files/file_path.h" +#include "base/macros.h" +#include "base/task/cancelable_task_tracker.h" +#include "chrome/browser/icon_manager.h" +#include "content/public/browser/url_data_source.h" + +namespace gfx { +class Image; +} + +namespace content { +class ResourceRequestInfo; +} + +// FileIconSource is the gateway between network-level chrome: +// requests for favicons and the history backend that serves these. +class FileIconSource : public content::URLDataSource { + public: + FileIconSource(); + + // content::URLDataSource implementation. + std::string GetSource() const override; + // void StartDataRequest( + // const std::string& path, + // const content::ResourceRequestInfo::WebContentsGetter& wc_getter, + // const content::URLDataSource::GotDataCallback& callback) override; + std::string GetMimeType(const std::string&) const override; + + protected: + ~FileIconSource() override; + + // Once the |path| and |icon_size| has been determined from the request, this + // function is called to perform the actual fetch. Declared as virtual for + // testing. + virtual void FetchFileIcon( + const base::FilePath& path, + float scale_factor, + IconLoader::IconSize icon_size, + const content::URLDataSource::GotDataCallback& callback); + + private: + // Contains the necessary information for completing an icon fetch request. + struct IconRequestDetails { + IconRequestDetails(); + IconRequestDetails(const IconRequestDetails& other); + ~IconRequestDetails(); + + // The callback to run with the response. + content::URLDataSource::GotDataCallback callback; + + // The requested scale factor to respond with. + float scale_factor; + }; + + // Called when favicon data is available from the history backend. + void OnFileIconDataAvailable(const IconRequestDetails& details, + gfx::Image* icon); + + // Tracks tasks requesting file icons. + base::CancelableTaskTracker cancelable_task_tracker_; + + DISALLOW_COPY_AND_ASSIGN(FileIconSource); +}; +#endif // CHROME_BROWSER_UI_WEBUI_FILEICON_SOURCE_H_ diff --git a/filenames.gypi b/filenames.gypi index 636cbb6063..65e74b5ee6 100644 --- a/filenames.gypi +++ b/filenames.gypi @@ -475,6 +475,10 @@ 'chromium_src/chrome/browser/chrome_process_finder_win.cc', 'chromium_src/chrome/browser/chrome_process_finder_win.h', 'chromium_src/chrome/browser/chrome_notification_types.h', + 'chromium_src/chrome/browser/icon_loader.cc', + 'chromium_src/chrome/browser/icon_loader.h', + 'chromium_src/chrome/browser/icon_manager.cc', + 'chromium_src/chrome/browser/icon_manager.h', 'chromium_src/chrome/browser/extensions/global_shortcut_listener.cc', 'chromium_src/chrome/browser/extensions/global_shortcut_listener.h', 'chromium_src/chrome/browser/extensions/global_shortcut_listener_mac.mm', @@ -543,6 +547,8 @@ 'chromium_src/chrome/browser/ui/views/color_chooser_aura.h', 'chromium_src/chrome/browser/ui/views/frame/global_menu_bar_registrar_x11.cc', 'chromium_src/chrome/browser/ui/views/frame/global_menu_bar_registrar_x11.h', + 'chromium_src/chrome/browser/ui/webui/fileicon_source.cc', + 'chromium_src/chrome/browser/ui/webui/fileicon_source.h', 'chromium_src/chrome/common/chrome_constants.cc', 'chromium_src/chrome/common/chrome_constants.h', 'chromium_src/chrome/common/chrome_paths.cc', From d118fed5c2de11c185dcff122f6e00c5111bc8c9 Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Sat, 29 Oct 2016 20:42:27 +0300 Subject: [PATCH 04/32] Try my own class --- atom/common/api/atom_api_native_image.cc | 13 +- atom/common/api/atom_api_native_image.h | 5 +- atom/common/fileicon_fetcher.cc | 76 ++++++++ atom/common/fileicon_fetcher.h | 48 +++++ .../browser/ui/webui/fileicon_source.cc | 164 ------------------ .../chrome/browser/ui/webui/fileicon_source.h | 73 -------- filenames.gypi | 4 +- 7 files changed, 141 insertions(+), 242 deletions(-) create mode 100644 atom/common/fileicon_fetcher.cc create mode 100644 atom/common/fileicon_fetcher.h delete mode 100644 chromium_src/chrome/browser/ui/webui/fileicon_source.cc delete mode 100644 chromium_src/chrome/browser/ui/webui/fileicon_source.h diff --git a/atom/common/api/atom_api_native_image.cc b/atom/common/api/atom_api_native_image.cc index 6fd34153d3..3f6f830e14 100644 --- a/atom/common/api/atom_api_native_image.cc +++ b/atom/common/api/atom_api_native_image.cc @@ -17,7 +17,7 @@ #include "base/files/file_util.h" #include "base/strings/pattern.h" #include "base/strings/string_util.h" -#include "chrome/browser/ui/webui/fileicon_source.h" +#include "atom/common/fileicon_fetcher.h" #include "native_mate/dictionary.h" #include "native_mate/object_template_builder.h" #include "net/base/data_url.h" @@ -306,7 +306,16 @@ gfx::Size NativeImage::GetSize() { void NativeImage::CreateFromFileIcon(v8::Isolate* isolate, const base::FilePath& path, const IconLoadedCallback& callback) { - callback.Run(CreateEmpty(isolate)); + IconLoader::IconSize icon_size = IconLoader::IconSize::NORMAL; + float scale_factor = 1.0f; + auto onready = base::Bind(&NativeImage::OnIconLoaded, isolate, callback); + FileIconFetcher::FetchFileIcon(path, scale_factor, icon_size, onready); +} + +void NativeImage::OnIconLoaded(v8::Isolate* isolate, + const IconLoadedCallback& callback, + gfx::Image& image) { + callback.Run(Create(isolate, image)); } float NativeImage::GetAspectRatio() { diff --git a/atom/common/api/atom_api_native_image.h b/atom/common/api/atom_api_native_image.h index 251776b893..d0474835eb 100644 --- a/atom/common/api/atom_api_native_image.h +++ b/atom/common/api/atom_api_native_image.h @@ -38,7 +38,7 @@ namespace atom { namespace api { class NativeImage : public mate::Wrappable { -using IconLoadedCallback = base::Callback)>; + using IconLoadedCallback = base::Callback)>; public: static mate::Handle CreateEmpty(v8::Isolate* isolate); static mate::Handle Create( @@ -90,6 +90,9 @@ using IconLoadedCallback = base::Callback)>; gfx::Size GetSize(); float GetAspectRatio(); + void OnIconLoaded(v8::Isolate* isolate, + const IconLoadedCallback& callback, + gfx::Image& image); // Mark the image as template image. void SetTemplateImage(bool setAsTemplate); // Determine if the image is a template image. diff --git a/atom/common/fileicon_fetcher.cc b/atom/common/fileicon_fetcher.cc new file mode 100644 index 0000000000..43a864a086 --- /dev/null +++ b/atom/common/fileicon_fetcher.cc @@ -0,0 +1,76 @@ +// Copyright (c) 2016 GitHub, Inc. +// Use of this source code is governed by the MIT license that can be +// found in the LICENSE file. + +#include "chrome/browser/ui/webui/fileicon_source.h" + +#include "base/bind.h" +#include "base/callback.h" +#include "base/files/file_path.h" +#include "base/memory/ref_counted_memory.h" +#include "base/message_loop/message_loop.h" +#include "base/strings/string_split.h" +#include "base/strings/utf_string_conversions.h" +#include "chrome/browser/browser_process.h" +#include "net/base/escape.h" +#include "third_party/skia/include/core/SkBitmap.h" +#include "ui/base/webui/web_ui_util.h" +#include "ui/gfx/codec/png_codec.h" +#include "ui/gfx/image/image.h" +#include "ui/gfx/image/image_skia.h" +#include "url/gurl.h" + +void FileIconFetcher::FetchFileIcon(const base::FilePath& path, + float scale_factor, + IconLoader::IconSize icon_size, + const IconFetchedCallback& callback) { + IconManager* im = g_browser_process->icon_manager(); + gfx::Image* icon = im->LookupIconFromFilepath(path, icon_size); + + if (icon) { + scoped_refptr icon_data(new base::RefCountedBytes); + gfx::PNGCodec::EncodeBGRASkBitmap( + icon->ToImageSkia()->GetRepresentation(scale_factor).sk_bitmap(), + false, + &icon_data->data()); + + callback.Run(icon_data.get()); + } else { + // Attach the ChromeURLDataManager request ID to the history request. + IconRequestDetails details; + details.callback = callback; + details.scale_factor = scale_factor; + + // Icon was not in cache, go fetch it slowly. + im->LoadIcon(path, + icon_size, + base::Bind(&FileIconFetcher::OnFileIconDataAvailable, details), + &cancelable_task_tracker_); + } +} + +void FileIconFetcher::OnFileIconDataAvailable(const IconRequestDetails& details, + gfx::Image* icon) { + if (icon) { + scoped_refptr icon_data(new base::RefCountedBytes); + gfx::PNGCodec::EncodeBGRASkBitmap( + icon->ToImageSkia()->GetRepresentation( + details.scale_factor).sk_bitmap(), + false, + &icon_data->data()); + + details.callback.Run(icon_data.get()); + } else { + details.callback.Run(NULL); + } +} + + +FileIconFetcher::IconRequestDetails::IconRequestDetails() : scale_factor(1.0f) { +} + +FileIconFetcher::IconRequestDetails::IconRequestDetails( + const IconRequestDetails& other) = default; + +FileIconFetcher::IconRequestDetails::~IconRequestDetails() { +} diff --git a/atom/common/fileicon_fetcher.h b/atom/common/fileicon_fetcher.h new file mode 100644 index 0000000000..217058d6a9 --- /dev/null +++ b/atom/common/fileicon_fetcher.h @@ -0,0 +1,48 @@ +// Copyright (c) 2016 GitHub, Inc. +// Use of this source code is governed by the MIT license that can be +// found in the LICENSE file. + +#ifndef ATOM_COMMON_API_FILEICON_FETCHER_H_ +#define ATOM_COMMON_API_FILEICON_FETCHER_H_ + +#include + +#include "base/files/file_path.h" +#include "base/macros.h" +#include "base/task/cancelable_task_tracker.h" +#include "chrome/browser/icon_manager.h" +#include "content/public/browser/url_data_source.h" + +namespace gfx { +class Image; +} + +class FileIconFetcher { + using IconFetchedCallback = base::Callback; + public: + static void FetchFileIcon(const base::FilePath& path, + float scale_factor, + IconLoader::IconSize icon_size, + const IconFetchedCallback& callback); + + private: + struct IconRequestDetails { + IconRequestDetails(); + IconRequestDetails(const IconRequestDetails& other); + ~IconRequestDetails(); + + // The callback to run with the response. + IconFetchedCallback callback; + + // The requested scale factor to respond with. + float scale_factor; + }; + // Called when favicon data is available from the history backend. + static void OnFileIconDataAvailable(const IconRequestDetails& details, + gfx::Image* icon); + + // Tracks tasks requesting file icons. + base::CancelableTaskTracker cancelable_task_tracker_; +}; + +#endif // ATOM_COMMON_API_FILEICON_FETCHER_H_ diff --git a/chromium_src/chrome/browser/ui/webui/fileicon_source.cc b/chromium_src/chrome/browser/ui/webui/fileicon_source.cc deleted file mode 100644 index 8659346b58..0000000000 --- a/chromium_src/chrome/browser/ui/webui/fileicon_source.cc +++ /dev/null @@ -1,164 +0,0 @@ -// Copyright (c) 2012 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 "chrome/browser/ui/webui/fileicon_source.h" - -#include "base/bind.h" -#include "base/callback.h" -#include "base/files/file_path.h" -#include "base/memory/ref_counted_memory.h" -#include "base/message_loop/message_loop.h" -#include "base/strings/string_split.h" -#include "base/strings/utf_string_conversions.h" -#include "chrome/browser/browser_process.h" -#include "net/base/escape.h" -#include "third_party/skia/include/core/SkBitmap.h" -#include "ui/base/webui/web_ui_util.h" -#include "ui/gfx/codec/png_codec.h" -#include "ui/gfx/image/image.h" -#include "ui/gfx/image/image_skia.h" -#include "url/gurl.h" - -namespace { - -typedef std::map QueryIconSizeMap; - -// The path used in internal URLs to file icon data. -const char kFileIconPath[] = "fileicon"; - -// URL parameter specifying icon size. -const char kIconSize[] = "iconsize"; - -// URL parameter specifying scale factor. -const char kScaleFactor[] = "scale"; - -// Assuming the url is of the form '/path?query', convert the path portion into -// a FilePath and return the resulting |file_path| and |query|. The path -// portion may have been encoded using encodeURIComponent(). -void GetFilePathAndQuery(const std::string& url, - base::FilePath* file_path, - std::string* query) { - // We receive the url with chrome://fileicon/ stripped but GURL expects it. - const GURL gurl("chrome://fileicon/" + url); - std::string path = net::UnescapeURLComponent( - gurl.path().substr(1), - net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS | - net::UnescapeRule::PATH_SEPARATORS | net::UnescapeRule::SPACES); - - *file_path = base::FilePath::FromUTF8Unsafe(path); - *file_path = file_path->NormalizePathSeparators(); - query->assign(gurl.query()); -} - -IconLoader::IconSize SizeStringToIconSize(const std::string& size_string) { - if (size_string == "small") return IconLoader::SMALL; - if (size_string == "large") return IconLoader::LARGE; - // We default to NORMAL if we don't recognize the size_string. Including - // size_string=="normal". - return IconLoader::NORMAL; -} - -// Simple parser for data on the query. -void ParseQueryParams(const std::string& query, - float* scale_factor, - IconLoader::IconSize* icon_size) { - base::StringPairs parameters; - if (icon_size) - *icon_size = IconLoader::NORMAL; - if (scale_factor) - *scale_factor = 1.0f; - base::SplitStringIntoKeyValuePairs(query, '=', '&', ¶meters); - for (base::StringPairs::const_iterator iter = parameters.begin(); - iter != parameters.end(); ++iter) { - if (icon_size && iter->first == kIconSize) - *icon_size = SizeStringToIconSize(iter->second); - else if (scale_factor && iter->first == kScaleFactor) - webui::ParseScaleFactor(iter->second, scale_factor); - } -} - -} // namespace - -FileIconSource::IconRequestDetails::IconRequestDetails() : scale_factor(1.0f) { -} - -FileIconSource::IconRequestDetails::IconRequestDetails( - const IconRequestDetails& other) = default; - -FileIconSource::IconRequestDetails::~IconRequestDetails() { -} - -FileIconSource::FileIconSource() {} - -FileIconSource::~FileIconSource() {} - -void FileIconSource::FetchFileIcon( - const base::FilePath& path, - float scale_factor, - IconLoader::IconSize icon_size, - const content::URLDataSource::GotDataCallback& callback) { - IconManager* im = g_browser_process->icon_manager(); - gfx::Image* icon = im->LookupIconFromFilepath(path, icon_size); - - if (icon) { - scoped_refptr icon_data(new base::RefCountedBytes); - gfx::PNGCodec::EncodeBGRASkBitmap( - icon->ToImageSkia()->GetRepresentation(scale_factor).sk_bitmap(), - false, - &icon_data->data()); - - callback.Run(icon_data.get()); - } else { - // Attach the ChromeURLDataManager request ID to the history request. - IconRequestDetails details; - details.callback = callback; - details.scale_factor = scale_factor; - - // Icon was not in cache, go fetch it slowly. - im->LoadIcon(path, - icon_size, - base::Bind(&FileIconSource::OnFileIconDataAvailable, - base::Unretained(this), details), - &cancelable_task_tracker_); - } -} - -std::string FileIconSource::GetSource() const { - return kFileIconPath; -} - -// void FileIconSource::StartDataRequest( -// const std::string& url_path, -// const content::ResourceRequestInfo::WebContentsGetter& wc_getter, -// const content::URLDataSource::GotDataCallback& callback) { -// std::string query; -// base::FilePath file_path; -// IconLoader::IconSize icon_size; -// float scale_factor = 1.0f; -// GetFilePathAndQuery(url_path, &file_path, &query); -// ParseQueryParams(query, &scale_factor, &icon_size); -// FetchFileIcon(file_path, scale_factor, icon_size, callback); -// } - -std::string FileIconSource::GetMimeType(const std::string&) const { - // Rely on image decoder inferring the correct type. - return std::string(); -} - -void FileIconSource::OnFileIconDataAvailable(const IconRequestDetails& details, - gfx::Image* icon) { - if (icon) { - scoped_refptr icon_data(new base::RefCountedBytes); - gfx::PNGCodec::EncodeBGRASkBitmap( - icon->ToImageSkia()->GetRepresentation( - details.scale_factor).sk_bitmap(), - false, - &icon_data->data()); - - details.callback.Run(icon_data.get()); - } else { - // TODO(glen): send a dummy icon. - details.callback.Run(NULL); - } -} diff --git a/chromium_src/chrome/browser/ui/webui/fileicon_source.h b/chromium_src/chrome/browser/ui/webui/fileicon_source.h deleted file mode 100644 index 7fd6671b60..0000000000 --- a/chromium_src/chrome/browser/ui/webui/fileicon_source.h +++ /dev/null @@ -1,73 +0,0 @@ -// Copyright (c) 2012 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 CHROME_BROWSER_UI_WEBUI_FILEICON_SOURCE_H_ -#define CHROME_BROWSER_UI_WEBUI_FILEICON_SOURCE_H_ - -#include - -#include "base/files/file_path.h" -#include "base/macros.h" -#include "base/task/cancelable_task_tracker.h" -#include "chrome/browser/icon_manager.h" -#include "content/public/browser/url_data_source.h" - -namespace gfx { -class Image; -} - -namespace content { -class ResourceRequestInfo; -} - -// FileIconSource is the gateway between network-level chrome: -// requests for favicons and the history backend that serves these. -class FileIconSource : public content::URLDataSource { - public: - FileIconSource(); - - // content::URLDataSource implementation. - std::string GetSource() const override; - // void StartDataRequest( - // const std::string& path, - // const content::ResourceRequestInfo::WebContentsGetter& wc_getter, - // const content::URLDataSource::GotDataCallback& callback) override; - std::string GetMimeType(const std::string&) const override; - - protected: - ~FileIconSource() override; - - // Once the |path| and |icon_size| has been determined from the request, this - // function is called to perform the actual fetch. Declared as virtual for - // testing. - virtual void FetchFileIcon( - const base::FilePath& path, - float scale_factor, - IconLoader::IconSize icon_size, - const content::URLDataSource::GotDataCallback& callback); - - private: - // Contains the necessary information for completing an icon fetch request. - struct IconRequestDetails { - IconRequestDetails(); - IconRequestDetails(const IconRequestDetails& other); - ~IconRequestDetails(); - - // The callback to run with the response. - content::URLDataSource::GotDataCallback callback; - - // The requested scale factor to respond with. - float scale_factor; - }; - - // Called when favicon data is available from the history backend. - void OnFileIconDataAvailable(const IconRequestDetails& details, - gfx::Image* icon); - - // Tracks tasks requesting file icons. - base::CancelableTaskTracker cancelable_task_tracker_; - - DISALLOW_COPY_AND_ASSIGN(FileIconSource); -}; -#endif // CHROME_BROWSER_UI_WEBUI_FILEICON_SOURCE_H_ diff --git a/filenames.gypi b/filenames.gypi index 65e74b5ee6..232fd66666 100644 --- a/filenames.gypi +++ b/filenames.gypi @@ -404,6 +404,8 @@ 'atom/common/crash_reporter/win/crash_service_main.h', 'atom/common/draggable_region.cc', 'atom/common/draggable_region.h', + 'atom/common/fileicon_fetcher.cc', + 'atom/common/fileicon_fetcher.h', 'atom/common/google_api_key.h', 'atom/common/key_weak_map.h', 'atom/common/keyboard_util.cc', @@ -547,8 +549,6 @@ 'chromium_src/chrome/browser/ui/views/color_chooser_aura.h', 'chromium_src/chrome/browser/ui/views/frame/global_menu_bar_registrar_x11.cc', 'chromium_src/chrome/browser/ui/views/frame/global_menu_bar_registrar_x11.h', - 'chromium_src/chrome/browser/ui/webui/fileicon_source.cc', - 'chromium_src/chrome/browser/ui/webui/fileicon_source.h', 'chromium_src/chrome/common/chrome_constants.cc', 'chromium_src/chrome/common/chrome_constants.h', 'chromium_src/chrome/common/chrome_paths.cc', From eb889b9b868af5d3b82b920509b673561538699b Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Sun, 30 Oct 2016 14:56:22 +0300 Subject: [PATCH 05/32] Get it compiling, linking till fails though --- atom/common/api/atom_api_native_image.cc | 4 +++- atom/common/api/atom_api_native_image.h | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/atom/common/api/atom_api_native_image.cc b/atom/common/api/atom_api_native_image.cc index 3f6f830e14..33c38eac00 100644 --- a/atom/common/api/atom_api_native_image.cc +++ b/atom/common/api/atom_api_native_image.cc @@ -308,7 +308,9 @@ void NativeImage::CreateFromFileIcon(v8::Isolate* isolate, const IconLoadedCallback& callback) { IconLoader::IconSize icon_size = IconLoader::IconSize::NORMAL; float scale_factor = 1.0f; - auto onready = base::Bind(&NativeImage::OnIconLoaded, isolate, callback); + auto onready = base::Bind(&NativeImage::OnIconLoaded, + base::Unretained(isolate), + callback); FileIconFetcher::FetchFileIcon(path, scale_factor, icon_size, onready); } diff --git a/atom/common/api/atom_api_native_image.h b/atom/common/api/atom_api_native_image.h index d0474835eb..2b8cbf7a2e 100644 --- a/atom/common/api/atom_api_native_image.h +++ b/atom/common/api/atom_api_native_image.h @@ -90,7 +90,7 @@ class NativeImage : public mate::Wrappable { gfx::Size GetSize(); float GetAspectRatio(); - void OnIconLoaded(v8::Isolate* isolate, + static void OnIconLoaded(v8::Isolate* isolate, const IconLoadedCallback& callback, gfx::Image& image); // Mark the image as template image. From 602aba872327df664b53b506c70fcba7107977bd Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Sun, 30 Oct 2016 15:31:30 +0300 Subject: [PATCH 06/32] Include proper header --- atom/common/fileicon_fetcher.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atom/common/fileicon_fetcher.cc b/atom/common/fileicon_fetcher.cc index 43a864a086..a0b3b73808 100644 --- a/atom/common/fileicon_fetcher.cc +++ b/atom/common/fileicon_fetcher.cc @@ -2,7 +2,7 @@ // Use of this source code is governed by the MIT license that can be // found in the LICENSE file. -#include "chrome/browser/ui/webui/fileicon_source.h" +#include "atom/common/fileicon_fetcher.h" #include "base/bind.h" #include "base/callback.h" From 1b3cd87fc965acf0c41e74f25a78faa48221dc95 Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Sun, 30 Oct 2016 17:10:09 +0300 Subject: [PATCH 07/32] Add icon manager to browser process --- atom/common/fileicon_fetcher.h | 2 +- chromium_src/chrome/browser/browser_process.cc | 7 +++++++ chromium_src/chrome/browser/browser_process.h | 4 ++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/atom/common/fileicon_fetcher.h b/atom/common/fileicon_fetcher.h index 217058d6a9..f8b0024126 100644 --- a/atom/common/fileicon_fetcher.h +++ b/atom/common/fileicon_fetcher.h @@ -42,7 +42,7 @@ class FileIconFetcher { gfx::Image* icon); // Tracks tasks requesting file icons. - base::CancelableTaskTracker cancelable_task_tracker_; + static base::CancelableTaskTracker cancelable_task_tracker_; }; #endif // ATOM_COMMON_API_FILEICON_FETCHER_H_ diff --git a/chromium_src/chrome/browser/browser_process.cc b/chromium_src/chrome/browser/browser_process.cc index a38d55f871..bed32469a6 100644 --- a/chromium_src/chrome/browser/browser_process.cc +++ b/chromium_src/chrome/browser/browser_process.cc @@ -5,6 +5,7 @@ #include "chrome/browser/browser_process.h" #include "chrome/browser/printing/print_job_manager.h" +#include "chrome/browser/icon_manager.h" #include "ui/base/l10n/l10n_util.h" BrowserProcess* g_browser_process = NULL; @@ -12,6 +13,8 @@ BrowserProcess* g_browser_process = NULL; BrowserProcess::BrowserProcess() : print_job_manager_(new printing::PrintJobManager) { g_browser_process = this; + + icon_manager_.reset(new IconManager); } BrowserProcess::~BrowserProcess() { @@ -25,3 +28,7 @@ std::string BrowserProcess::GetApplicationLocale() { printing::PrintJobManager* BrowserProcess::print_job_manager() { return print_job_manager_.get(); } + +IconManager* BrowserProcess::icon_manager() { + return icon_manager_.get(); +} diff --git a/chromium_src/chrome/browser/browser_process.h b/chromium_src/chrome/browser/browser_process.h index 1459ca31a6..9f28045c38 100644 --- a/chromium_src/chrome/browser/browser_process.h +++ b/chromium_src/chrome/browser/browser_process.h @@ -19,6 +19,8 @@ namespace printing { class PrintJobManager; } +class IconManager; + // NOT THREAD SAFE, call only from the main thread. // These functions shouldn't return NULL unless otherwise noted. class BrowserProcess { @@ -29,9 +31,11 @@ class BrowserProcess { std::string GetApplicationLocale(); printing::PrintJobManager* print_job_manager(); + IconManager* icon_manager(); private: std::unique_ptr print_job_manager_; + std::unique_ptr icon_manager_; DISALLOW_COPY_AND_ASSIGN(BrowserProcess); }; From b25b14164285405612b0c858c3596b6e7e9580d8 Mon Sep 17 00:00:00 2001 From: Robo Date: Thu, 3 Nov 2016 00:27:16 +0530 Subject: [PATCH 08/32] create iconmanager as singleton class and cleanup code (#1) * create iconmanager as singleton class and cleanup code --- atom/browser/api/atom_api_app.cc | 42 +++++++++- atom/browser/api/atom_api_app.h | 6 ++ atom/common/api/atom_api_native_image.cc | 20 ----- atom/common/api/atom_api_native_image.h | 7 -- atom/common/fileicon_fetcher.cc | 76 ------------------- atom/common/fileicon_fetcher.h | 48 ------------ .../chrome/browser/browser_process.cc | 7 -- chromium_src/chrome/browser/browser_process.h | 2 - chromium_src/chrome/browser/icon_loader.cc | 9 +-- .../chrome/browser/icon_loader_auralinux.cc | 55 ++++++++++++++ .../chrome/browser/icon_loader_mac.mm | 62 +++++++++++++++ .../chrome/browser/icon_loader_win.cc | 75 ++++++++++++++++++ chromium_src/chrome/browser/icon_manager.cc | 54 +++++-------- chromium_src/chrome/browser/icon_manager.h | 21 ++--- filenames.gypi | 7 +- 15 files changed, 275 insertions(+), 216 deletions(-) delete mode 100644 atom/common/fileicon_fetcher.cc delete mode 100644 atom/common/fileicon_fetcher.h create mode 100644 chromium_src/chrome/browser/icon_loader_auralinux.cc create mode 100644 chromium_src/chrome/browser/icon_loader_mac.mm create mode 100644 chromium_src/chrome/browser/icon_loader_win.cc diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index caaee9d25c..5f878aace1 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/icon_manager.h" #include "chrome/common/chrome_paths.h" #include "content/public/browser/browser_accessibility_state.h" #include "content/public/browser/client_certificate_delegate.h" @@ -314,6 +315,26 @@ struct Converter { } }; +template <> +struct Converter { + static bool FromV8(v8::Isolate* isolate, + v8::Local val, + IconLoader::IconSize* out) { + std::string icon_size_string; + if (!ConvertFromV8(isolate, val, &icon_size_string)) + return false; + if (icon_size_string == "small") + *out = IconLoader::IconSize::SMALL; + else if (icon_size_string == "normal") + *out = IconLoader::IconSize::NORMAL; + else if (icon_size_string == "large") + *out = IconLoader::IconSize::LARGE; + else + return false; + return true; + } +}; + template<> struct Converter { static bool FromV8(v8::Isolate* isolate, v8::Local val, @@ -462,6 +483,11 @@ int ImportIntoCertStore( } #endif +void OnIconDataAvailable(const App::FileIconCallback& callback, + gfx::Image* icon) { + callback.Run(icon ? *icon : gfx::Image()); +} + } // namespace App::App(v8::Isolate* isolate) { @@ -841,6 +867,19 @@ JumpListResult App::SetJumpList(v8::Local val, } #endif // defined(OS_WIN) +void App::GetFileIcon(const base::FilePath& path, + IconLoader::IconSize icon_size, + const FileIconCallback& callback) { + IconManager* icon_manager = IconManager::GetInstance(); + gfx::Image* icon = icon_manager->LookupIconFromFilepath(path, icon_size); + if (icon) { + callback.Run(*icon); + } else { + icon_manager->LoadIcon(path, icon_size, + base::Bind(&OnIconDataAvailable, callback)); + } +} + // static mate::Handle App::Create(v8::Isolate* isolate) { return mate::CreateHandle(isolate, new App(isolate)); @@ -909,7 +948,8 @@ void App::BuildPrototype( .SetMethod("isAccessibilitySupportEnabled", &App::IsAccessibilitySupportEnabled) .SetMethod("disableHardwareAcceleration", - &App::DisableHardwareAcceleration); + &App::DisableHardwareAcceleration) + .SetMethod("getFileIcon", &App::GetFileIcon); } } // namespace api diff --git a/atom/browser/api/atom_api_app.h b/atom/browser/api/atom_api_app.h index c7e62927c5..b42b201fff 100644 --- a/atom/browser/api/atom_api_app.h +++ b/atom/browser/api/atom_api_app.h @@ -13,6 +13,7 @@ #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 "chrome/browser/process_singleton.h" #include "content/public/browser/gpu_data_manager_observer.h" #include "native_mate/handle.h" @@ -43,6 +44,8 @@ class App : public AtomBrowserClient::Delegate, public BrowserObserver, public content::GpuDataManagerObserver { public: + using FileIconCallback = base::Callback; + static mate::Handle Create(v8::Isolate* isolate); static void BuildPrototype(v8::Isolate* isolate, @@ -129,6 +132,9 @@ class App : public AtomBrowserClient::Delegate, void ImportCertificate(const base::DictionaryValue& options, const net::CompletionCallback& callback); #endif + void GetFileIcon(const base::FilePath& path, + IconLoader::IconSize icon_size, + const FileIconCallback& callback); #if defined(OS_WIN) // Get the current Jump List settings. diff --git a/atom/common/api/atom_api_native_image.cc b/atom/common/api/atom_api_native_image.cc index 33c38eac00..1d4e20e3cf 100644 --- a/atom/common/api/atom_api_native_image.cc +++ b/atom/common/api/atom_api_native_image.cc @@ -17,7 +17,6 @@ #include "base/files/file_util.h" #include "base/strings/pattern.h" #include "base/strings/string_util.h" -#include "atom/common/fileicon_fetcher.h" #include "native_mate/dictionary.h" #include "native_mate/object_template_builder.h" #include "net/base/data_url.h" @@ -303,23 +302,6 @@ gfx::Size NativeImage::GetSize() { return image_.Size(); } -void NativeImage::CreateFromFileIcon(v8::Isolate* isolate, - const base::FilePath& path, - const IconLoadedCallback& callback) { - IconLoader::IconSize icon_size = IconLoader::IconSize::NORMAL; - float scale_factor = 1.0f; - auto onready = base::Bind(&NativeImage::OnIconLoaded, - base::Unretained(isolate), - callback); - FileIconFetcher::FetchFileIcon(path, scale_factor, icon_size, onready); -} - -void NativeImage::OnIconLoaded(v8::Isolate* isolate, - const IconLoadedCallback& callback, - gfx::Image& image) { - callback.Run(Create(isolate, image)); -} - float NativeImage::GetAspectRatio() { gfx::Size size = GetSize(); if (size.IsEmpty()) @@ -533,8 +515,6 @@ void Initialize(v8::Local exports, v8::Local unused, mate::Dictionary dict(context->GetIsolate(), exports); dict.SetMethod("createEmpty", &atom::api::NativeImage::CreateEmpty); dict.SetMethod("createFromPath", &atom::api::NativeImage::CreateFromPath); - dict.SetMethod("createFromFileIcon", - &atom::api::NativeImage::CreateFromFileIcon); dict.SetMethod("createFromBuffer", &atom::api::NativeImage::CreateFromBuffer); dict.SetMethod("createFromDataURL", &atom::api::NativeImage::CreateFromDataURL); diff --git a/atom/common/api/atom_api_native_image.h b/atom/common/api/atom_api_native_image.h index 2b8cbf7a2e..ee1b5f5d4b 100644 --- a/atom/common/api/atom_api_native_image.h +++ b/atom/common/api/atom_api_native_image.h @@ -38,7 +38,6 @@ namespace atom { namespace api { class NativeImage : public mate::Wrappable { - using IconLoadedCallback = base::Callback)>; public: static mate::Handle CreateEmpty(v8::Isolate* isolate); static mate::Handle Create( @@ -53,9 +52,6 @@ class NativeImage : public mate::Wrappable { mate::Arguments* args, v8::Local buffer); static mate::Handle CreateFromDataURL( v8::Isolate* isolate, const GURL& url); - static void CreateFromFileIcon(v8::Isolate* isolate, - const base::FilePath& path, - const IconLoadedCallback& callback); static void BuildPrototype(v8::Isolate* isolate, v8::Local prototype); @@ -90,9 +86,6 @@ class NativeImage : public mate::Wrappable { gfx::Size GetSize(); float GetAspectRatio(); - static void OnIconLoaded(v8::Isolate* isolate, - const IconLoadedCallback& callback, - gfx::Image& image); // Mark the image as template image. void SetTemplateImage(bool setAsTemplate); // Determine if the image is a template image. diff --git a/atom/common/fileicon_fetcher.cc b/atom/common/fileicon_fetcher.cc deleted file mode 100644 index a0b3b73808..0000000000 --- a/atom/common/fileicon_fetcher.cc +++ /dev/null @@ -1,76 +0,0 @@ -// Copyright (c) 2016 GitHub, Inc. -// Use of this source code is governed by the MIT license that can be -// found in the LICENSE file. - -#include "atom/common/fileicon_fetcher.h" - -#include "base/bind.h" -#include "base/callback.h" -#include "base/files/file_path.h" -#include "base/memory/ref_counted_memory.h" -#include "base/message_loop/message_loop.h" -#include "base/strings/string_split.h" -#include "base/strings/utf_string_conversions.h" -#include "chrome/browser/browser_process.h" -#include "net/base/escape.h" -#include "third_party/skia/include/core/SkBitmap.h" -#include "ui/base/webui/web_ui_util.h" -#include "ui/gfx/codec/png_codec.h" -#include "ui/gfx/image/image.h" -#include "ui/gfx/image/image_skia.h" -#include "url/gurl.h" - -void FileIconFetcher::FetchFileIcon(const base::FilePath& path, - float scale_factor, - IconLoader::IconSize icon_size, - const IconFetchedCallback& callback) { - IconManager* im = g_browser_process->icon_manager(); - gfx::Image* icon = im->LookupIconFromFilepath(path, icon_size); - - if (icon) { - scoped_refptr icon_data(new base::RefCountedBytes); - gfx::PNGCodec::EncodeBGRASkBitmap( - icon->ToImageSkia()->GetRepresentation(scale_factor).sk_bitmap(), - false, - &icon_data->data()); - - callback.Run(icon_data.get()); - } else { - // Attach the ChromeURLDataManager request ID to the history request. - IconRequestDetails details; - details.callback = callback; - details.scale_factor = scale_factor; - - // Icon was not in cache, go fetch it slowly. - im->LoadIcon(path, - icon_size, - base::Bind(&FileIconFetcher::OnFileIconDataAvailable, details), - &cancelable_task_tracker_); - } -} - -void FileIconFetcher::OnFileIconDataAvailable(const IconRequestDetails& details, - gfx::Image* icon) { - if (icon) { - scoped_refptr icon_data(new base::RefCountedBytes); - gfx::PNGCodec::EncodeBGRASkBitmap( - icon->ToImageSkia()->GetRepresentation( - details.scale_factor).sk_bitmap(), - false, - &icon_data->data()); - - details.callback.Run(icon_data.get()); - } else { - details.callback.Run(NULL); - } -} - - -FileIconFetcher::IconRequestDetails::IconRequestDetails() : scale_factor(1.0f) { -} - -FileIconFetcher::IconRequestDetails::IconRequestDetails( - const IconRequestDetails& other) = default; - -FileIconFetcher::IconRequestDetails::~IconRequestDetails() { -} diff --git a/atom/common/fileicon_fetcher.h b/atom/common/fileicon_fetcher.h deleted file mode 100644 index f8b0024126..0000000000 --- a/atom/common/fileicon_fetcher.h +++ /dev/null @@ -1,48 +0,0 @@ -// Copyright (c) 2016 GitHub, Inc. -// Use of this source code is governed by the MIT license that can be -// found in the LICENSE file. - -#ifndef ATOM_COMMON_API_FILEICON_FETCHER_H_ -#define ATOM_COMMON_API_FILEICON_FETCHER_H_ - -#include - -#include "base/files/file_path.h" -#include "base/macros.h" -#include "base/task/cancelable_task_tracker.h" -#include "chrome/browser/icon_manager.h" -#include "content/public/browser/url_data_source.h" - -namespace gfx { -class Image; -} - -class FileIconFetcher { - using IconFetchedCallback = base::Callback; - public: - static void FetchFileIcon(const base::FilePath& path, - float scale_factor, - IconLoader::IconSize icon_size, - const IconFetchedCallback& callback); - - private: - struct IconRequestDetails { - IconRequestDetails(); - IconRequestDetails(const IconRequestDetails& other); - ~IconRequestDetails(); - - // The callback to run with the response. - IconFetchedCallback callback; - - // The requested scale factor to respond with. - float scale_factor; - }; - // Called when favicon data is available from the history backend. - static void OnFileIconDataAvailable(const IconRequestDetails& details, - gfx::Image* icon); - - // Tracks tasks requesting file icons. - static base::CancelableTaskTracker cancelable_task_tracker_; -}; - -#endif // ATOM_COMMON_API_FILEICON_FETCHER_H_ diff --git a/chromium_src/chrome/browser/browser_process.cc b/chromium_src/chrome/browser/browser_process.cc index bed32469a6..a38d55f871 100644 --- a/chromium_src/chrome/browser/browser_process.cc +++ b/chromium_src/chrome/browser/browser_process.cc @@ -5,7 +5,6 @@ #include "chrome/browser/browser_process.h" #include "chrome/browser/printing/print_job_manager.h" -#include "chrome/browser/icon_manager.h" #include "ui/base/l10n/l10n_util.h" BrowserProcess* g_browser_process = NULL; @@ -13,8 +12,6 @@ BrowserProcess* g_browser_process = NULL; BrowserProcess::BrowserProcess() : print_job_manager_(new printing::PrintJobManager) { g_browser_process = this; - - icon_manager_.reset(new IconManager); } BrowserProcess::~BrowserProcess() { @@ -28,7 +25,3 @@ std::string BrowserProcess::GetApplicationLocale() { printing::PrintJobManager* BrowserProcess::print_job_manager() { return print_job_manager_.get(); } - -IconManager* BrowserProcess::icon_manager() { - return icon_manager_.get(); -} diff --git a/chromium_src/chrome/browser/browser_process.h b/chromium_src/chrome/browser/browser_process.h index 9f28045c38..a21371efb2 100644 --- a/chromium_src/chrome/browser/browser_process.h +++ b/chromium_src/chrome/browser/browser_process.h @@ -31,11 +31,9 @@ class BrowserProcess { std::string GetApplicationLocale(); printing::PrintJobManager* print_job_manager(); - IconManager* icon_manager(); private: std::unique_ptr print_job_manager_; - std::unique_ptr icon_manager_; DISALLOW_COPY_AND_ASSIGN(BrowserProcess); }; diff --git a/chromium_src/chrome/browser/icon_loader.cc b/chromium_src/chrome/browser/icon_loader.cc index 979ee5772c..afc5d8016e 100644 --- a/chromium_src/chrome/browser/icon_loader.cc +++ b/chromium_src/chrome/browser/icon_loader.cc @@ -18,15 +18,14 @@ IconLoader::IconLoader(const base::FilePath& file_path, icon_size_(size), delegate_(delegate) {} -IconLoader::~IconLoader() { -} +IconLoader::~IconLoader() {} 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)); + base::Bind(&IconLoader::ReadGroup, this), + base::Bind(&IconLoader::OnReadGroup, this)); } void IconLoader::ReadGroup() { @@ -37,7 +36,7 @@ void IconLoader::OnReadGroup() { if (IsIconMutableFromFilepath(file_path_) || !delegate_->OnGroupLoaded(this, group_)) { BrowserThread::PostTask(ReadIconThreadID(), FROM_HERE, - base::Bind(&IconLoader::ReadIcon, this)); + base::Bind(&IconLoader::ReadIcon, this)); } } diff --git a/chromium_src/chrome/browser/icon_loader_auralinux.cc b/chromium_src/chrome/browser/icon_loader_auralinux.cc new file mode 100644 index 0000000000..2a1b0d868f --- /dev/null +++ b/chromium_src/chrome/browser/icon_loader_auralinux.cc @@ -0,0 +1,55 @@ +// Copyright 2013 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 "chrome/browser/icon_loader.h" + +#include "base/bind.h" +#include "base/message_loop/message_loop.h" +#include "base/nix/mime_util_xdg.h" +#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; +} + +// static +content::BrowserThread::ID IconLoader::ReadIconThreadID() { + // ReadIcon() calls into views::LinuxUI and GTK2 code, so it must be on the UI + // thread. + return content::BrowserThread::UI; +} + +void IconLoader::ReadIcon() { + int size_pixels = 0; + switch (icon_size_) { + case IconLoader::SMALL: + size_pixels = 16; + break; + case IconLoader::NORMAL: + size_pixels = 32; + break; + case IconLoader::LARGE: + size_pixels = 48; + break; + default: + NOTREACHED(); + } + + views::LinuxUI* ui = views::LinuxUI::instance(); + if (ui) { + gfx::Image image = ui->GetIconForContentType(group_, size_pixels); + if (!image.IsEmpty()) + image_.reset(new gfx::Image(image)); + } + + target_task_runner_->PostTask(FROM_HERE, + base::Bind(&IconLoader::NotifyDelegate, this)); +} diff --git a/chromium_src/chrome/browser/icon_loader_mac.mm b/chromium_src/chrome/browser/icon_loader_mac.mm new file mode 100644 index 0000000000..a9dd42566b --- /dev/null +++ b/chromium_src/chrome/browser/icon_loader_mac.mm @@ -0,0 +1,62 @@ +// Copyright (c) 2012 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 "chrome/browser/icon_loader.h" + +#import + +#include "base/bind.h" +#include "base/files/file_path.h" +#include "base/message_loop/message_loop.h" +#include "base/strings/sys_string_conversions.h" +#include "base/threading/thread.h" +#include "ui/gfx/image/image_skia.h" +#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; +} + +// static +content::BrowserThread::ID IconLoader::ReadIconThreadID() { + return content::BrowserThread::FILE; +} + +void IconLoader::ReadIcon() { + NSString* group = base::SysUTF8ToNSString(group_); + NSWorkspace* workspace = [NSWorkspace sharedWorkspace]; + NSImage* icon = [workspace iconForFileType:group]; + + if (icon_size_ == ALL) { + // The NSImage already has all sizes. + image_.reset(new gfx::Image([icon retain])); + } else { + NSSize size = NSZeroSize; + switch (icon_size_) { + case IconLoader::SMALL: + size = NSMakeSize(16, 16); + break; + case IconLoader::NORMAL: + size = NSMakeSize(32, 32); + break; + default: + NOTREACHED(); + } + gfx::ImageSkia image_skia(gfx::ImageSkiaFromResizedNSImage(icon, size)); + if (!image_skia.isNull()) { + image_skia.MakeThreadSafe(); + image_.reset(new gfx::Image(image_skia)); + } + } + + target_task_runner_->PostTask(FROM_HERE, + base::Bind(&IconLoader::NotifyDelegate, this)); +} diff --git a/chromium_src/chrome/browser/icon_loader_win.cc b/chromium_src/chrome/browser/icon_loader_win.cc new file mode 100644 index 0000000000..079c1f4828 --- /dev/null +++ b/chromium_src/chrome/browser/icon_loader_win.cc @@ -0,0 +1,75 @@ +// Copyright (c) 2012 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 "chrome/browser/icon_loader.h" + +#include +#include + +#include "base/bind.h" +#include "base/message_loop/message_loop.h" +#include "base/threading/thread.h" +#include "third_party/skia/include/core/SkBitmap.h" +#include "ui/display/win/dpi.h" +#include "ui/gfx/geometry/size.h" +#include "ui/gfx/icon_util.h" +#include "ui/gfx/image/image_skia.h" + +// static +IconGroupID IconLoader::ReadGroupIDFromFilepath( + const base::FilePath& filepath) { + if (!IsIconMutableFromFilepath(filepath)) + return filepath.Extension(); + return filepath.value(); +} + +// static +bool IconLoader::IsIconMutableFromFilepath(const base::FilePath& filepath) { + return filepath.MatchesExtension(L".exe") || + filepath.MatchesExtension(L".dll") || + filepath.MatchesExtension(L".ico"); +} + +// static +content::BrowserThread::ID IconLoader::ReadIconThreadID() { + return content::BrowserThread::FILE; +} + +void IconLoader::ReadIcon() { + int size = 0; + switch (icon_size_) { + case IconLoader::SMALL: + size = SHGFI_SMALLICON; + break; + case IconLoader::NORMAL: + size = 0; + break; + case IconLoader::LARGE: + size = SHGFI_LARGEICON; + break; + default: + NOTREACHED(); + } + + image_.reset(); + + SHFILEINFO file_info = {0}; + if (SHGetFileInfo(group_.c_str(), FILE_ATTRIBUTE_NORMAL, &file_info, + 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())); + 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)); +} diff --git a/chromium_src/chrome/browser/icon_manager.cc b/chromium_src/chrome/browser/icon_manager.cc index 6596ab3112..17e362c534 100644 --- a/chromium_src/chrome/browser/icon_manager.cc +++ b/chromium_src/chrome/browser/icon_manager.cc @@ -13,30 +13,21 @@ #include "third_party/skia/include/core/SkBitmap.h" #include "third_party/skia/include/core/SkCanvas.h" -namespace { - -void RunCallbackIfNotCanceled( - const base::CancelableTaskTracker::IsCanceledCallback& is_canceled, - const IconManager::IconRequestCallback& callback, - gfx::Image* image) { - if (is_canceled.Run()) - return; - callback.Run(image); -} - -} // namespace - struct IconManager::ClientRequest { IconRequestCallback callback; base::FilePath file_path; IconLoader::IconSize size; }; -IconManager::IconManager() { +// static +IconManager* IconManager::GetInstance() { + return base::Singleton::get(); } +IconManager::IconManager() {} + IconManager::~IconManager() { - base::STLDeleteValues(&icon_cache_); + STLDeleteValues(&icon_cache_); } gfx::Image* IconManager::LookupIconFromFilepath(const base::FilePath& file_name, @@ -54,33 +45,23 @@ gfx::Image* IconManager::LookupIconFromGroup(const IconGroupID& group, if (it != icon_cache_.end()) return it->second; - return NULL; + return nullptr; } -base::CancelableTaskTracker::TaskId IconManager::LoadIcon( - const base::FilePath& file_name, - IconLoader::IconSize size, - const IconRequestCallback& callback, - base::CancelableTaskTracker* tracker) { +void IconManager::LoadIcon(const base::FilePath& file_name, + IconLoader::IconSize size, + const IconRequestCallback& callback) { IconLoader* loader = new IconLoader(file_name, size, this); loader->AddRef(); loader->Start(); - base::CancelableTaskTracker::IsCanceledCallback is_canceled; - base::CancelableTaskTracker::TaskId id = - tracker->NewTrackedTaskId(&is_canceled); - IconRequestCallback callback_runner = base::Bind( - &RunCallbackIfNotCanceled, is_canceled, callback); - - ClientRequest client_request = { callback_runner, file_name, size }; + ClientRequest client_request = {callback, file_name, size}; requests_[loader] = client_request; - return id; } // IconLoader::Delegate implementation ----------------------------------------- -bool IconManager::OnGroupLoaded(IconLoader* loader, - const IconGroupID& group) { +bool IconManager::OnGroupLoaded(IconLoader* loader, const IconGroupID& group) { ClientRequests::iterator rit = requests_.find(loader); if (rit == requests_.end()) { NOTREACHED(); @@ -95,8 +76,9 @@ bool IconManager::OnGroupLoaded(IconLoader* loader, return OnImageLoaded(loader, result, group); } -bool IconManager::OnImageLoaded( - IconLoader* loader, gfx::Image* result, const IconGroupID& group) { +bool IconManager::OnImageLoaded(IconLoader* loader, + gfx::Image* result, + const IconGroupID& group) { ClientRequests::iterator rit = requests_.find(loader); // Balances the AddRef() in LoadIcon(). @@ -139,10 +121,8 @@ bool IconManager::OnImageLoaded( IconManager::CacheKey::CacheKey(const IconGroupID& group, IconLoader::IconSize size) - : group(group), - size(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 0b5ec07d41..dd6de82527 100644 --- a/chromium_src/chrome/browser/icon_manager.h +++ b/chromium_src/chrome/browser/icon_manager.h @@ -49,15 +49,13 @@ #include "base/files/file_path.h" #include "base/macros.h" -#include "base/task/cancelable_task_tracker.h" +#include "base/memory/singleton.h" #include "chrome/browser/icon_loader.h" #include "ui/gfx/image/image.h" class IconManager : public IconLoader::Delegate { public: - IconManager(); - ~IconManager() override; - + static IconManager* GetInstance(); // 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 @@ -78,11 +76,9 @@ class IconManager : public IconLoader::Delegate { // should never keep it or delete it. // 3. The gfx::Image pointer passed to the callback may be NULL if decoding // failed. - base::CancelableTaskTracker::TaskId LoadIcon( - const base::FilePath& file_name, - IconLoader::IconSize size, - const IconRequestCallback& callback, - base::CancelableTaskTracker* tracker); + void LoadIcon(const base::FilePath& file_name, + IconLoader::IconSize size, + const IconRequestCallback& callback); // IconLoader::Delegate interface. bool OnGroupLoaded(IconLoader* loader, const IconGroupID& group) override; @@ -91,11 +87,16 @@ class IconManager : public IconLoader::Delegate { const IconGroupID& group) override; private: + friend struct base::DefaultSingletonTraits; + + IconManager(); + ~IconManager() override; + struct CacheKey { CacheKey(const IconGroupID& 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::IconSize size; diff --git a/filenames.gypi b/filenames.gypi index 232fd66666..74cef968cd 100644 --- a/filenames.gypi +++ b/filenames.gypi @@ -404,8 +404,6 @@ 'atom/common/crash_reporter/win/crash_service_main.h', 'atom/common/draggable_region.cc', 'atom/common/draggable_region.h', - 'atom/common/fileicon_fetcher.cc', - 'atom/common/fileicon_fetcher.h', 'atom/common/google_api_key.h', 'atom/common/key_weak_map.h', 'atom/common/keyboard_util.cc', @@ -476,11 +474,14 @@ 'chromium_src/chrome/browser/browser_process.h', 'chromium_src/chrome/browser/chrome_process_finder_win.cc', 'chromium_src/chrome/browser/chrome_process_finder_win.h', - 'chromium_src/chrome/browser/chrome_notification_types.h', + 'chromium_src/chrome/browser/icon_loader_auralinux.cc', + 'chromium_src/chrome/browser/icon_loader_mac.mm', + 'chromium_src/chrome/browser/icon_loader_win.cc', 'chromium_src/chrome/browser/icon_loader.cc', 'chromium_src/chrome/browser/icon_loader.h', 'chromium_src/chrome/browser/icon_manager.cc', 'chromium_src/chrome/browser/icon_manager.h', + 'chromium_src/chrome/browser/chrome_notification_types.h', 'chromium_src/chrome/browser/extensions/global_shortcut_listener.cc', 'chromium_src/chrome/browser/extensions/global_shortcut_listener.h', 'chromium_src/chrome/browser/extensions/global_shortcut_listener_mac.mm', From ff3aaa55f7cb303443cb511f70518b06708460ce Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Thu, 3 Nov 2016 10:47:03 +0530 Subject: [PATCH 09/32] define icon loader for liunx as separate target --- electron.gyp | 1 + filenames.gypi | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/electron.gyp b/electron.gyp index adf38cfbe5..a657d2d1c5 100644 --- a/electron.gyp +++ b/electron.gyp @@ -328,6 +328,7 @@ }], # OS=="mac" and mas_build==1 ['OS=="linux"', { 'sources': [ + '<@(lib_sources_linux)', '<@(lib_sources_nss)', ], 'link_settings': { diff --git a/filenames.gypi b/filenames.gypi index 74cef968cd..5b16beaa91 100644 --- a/filenames.gypi +++ b/filenames.gypi @@ -474,7 +474,6 @@ 'chromium_src/chrome/browser/browser_process.h', 'chromium_src/chrome/browser/chrome_process_finder_win.cc', 'chromium_src/chrome/browser/chrome_process_finder_win.h', - 'chromium_src/chrome/browser/icon_loader_auralinux.cc', 'chromium_src/chrome/browser/icon_loader_mac.mm', 'chromium_src/chrome/browser/icon_loader_win.cc', 'chromium_src/chrome/browser/icon_loader.cc', @@ -608,6 +607,9 @@ '<@(native_mate_files)', '<(SHARED_INTERMEDIATE_DIR)/atom_natives.h', ], + 'lib_sources_linux': [ + 'chromium_src/chrome/browser/icon_loader_auralinux.cc', + ], 'lib_sources_nss': [ 'chromium_src/chrome/browser/certificate_manager_model.cc', 'chromium_src/chrome/browser/certificate_manager_model.h', From bec671bac83c23dcf6b74bc6de582aa363c8b900 Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Thu, 3 Nov 2016 21:04:56 +0300 Subject: [PATCH 10/32] Make size optional --- atom/browser/api/atom_api_app.cc | 15 +++++++++++++-- atom/browser/api/atom_api_app.h | 3 +-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index 5f878aace1..f5d9201f84 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -868,8 +868,19 @@ JumpListResult App::SetJumpList(v8::Local val, #endif // defined(OS_WIN) void App::GetFileIcon(const base::FilePath& path, - IconLoader::IconSize icon_size, - const FileIconCallback& callback) { + mate::Arguments* args) { + IconLoader::IconSize icon_size; + FileIconCallback callback; + + if (!args->GetNext(&icon_size)) { + icon_size = IconLoader::IconSize::NORMAL; + } + + if (!args->GetNext(&callback)) { + args->ThrowError(); + return; + } + IconManager* icon_manager = IconManager::GetInstance(); gfx::Image* icon = icon_manager->LookupIconFromFilepath(path, icon_size); if (icon) { diff --git a/atom/browser/api/atom_api_app.h b/atom/browser/api/atom_api_app.h index b42b201fff..7d2f6f752b 100644 --- a/atom/browser/api/atom_api_app.h +++ b/atom/browser/api/atom_api_app.h @@ -133,8 +133,7 @@ class App : public AtomBrowserClient::Delegate, const net::CompletionCallback& callback); #endif void GetFileIcon(const base::FilePath& path, - IconLoader::IconSize icon_size, - const FileIconCallback& callback); + mate::Arguments* args); #if defined(OS_WIN) // Get the current Jump List settings. From 05cb26a1743fbe11d2218c13764c4fc3658d729c Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Thu, 3 Nov 2016 21:23:40 +0300 Subject: [PATCH 11/32] Use object for options --- atom/browser/api/atom_api_app.cc | 36 +++++++++++++------------------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index f5d9201f84..c6b50a48e9 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -315,26 +315,6 @@ struct Converter { } }; -template <> -struct Converter { - static bool FromV8(v8::Isolate* isolate, - v8::Local val, - IconLoader::IconSize* out) { - std::string icon_size_string; - if (!ConvertFromV8(isolate, val, &icon_size_string)) - return false; - if (icon_size_string == "small") - *out = IconLoader::IconSize::SMALL; - else if (icon_size_string == "normal") - *out = IconLoader::IconSize::NORMAL; - else if (icon_size_string == "large") - *out = IconLoader::IconSize::LARGE; - else - return false; - return true; - } -}; - template<> struct Converter { static bool FromV8(v8::Isolate* isolate, v8::Local val, @@ -356,6 +336,15 @@ namespace api { namespace { +IconLoader::IconSize GetIconSizeByString(std::string size) { + if (size == "small") { + return IconLoader::IconSize::SMALL; + } else if (size == "large") { + return IconLoader::IconSize::LARGE; + } + return IconLoader::IconSize::NORMAL; +}; + // Return the path constant from string. int GetPathConstant(const std::string& name) { if (name == "appData") @@ -869,11 +858,16 @@ JumpListResult App::SetJumpList(v8::Local val, void App::GetFileIcon(const base::FilePath& path, mate::Arguments* args) { + base::DictionaryValue options; IconLoader::IconSize icon_size; FileIconCallback callback; - if (!args->GetNext(&icon_size)) { + if (!args->GetNext(&options)) { icon_size = IconLoader::IconSize::NORMAL; + } else { + std::string icon_size_string; + options.GetString("size", &icon_size_string); + icon_size = GetIconSizeByString(icon_size_string); } if (!args->GetNext(&callback)) { From fe99b255c425591dc5fc5729eb2f9f684320e205 Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Thu, 3 Nov 2016 21:38:57 +0300 Subject: [PATCH 12/32] Add docs --- docs/api/app.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/api/app.md b/docs/api/app.md index 83e071618d..8398f4c0a7 100644 --- a/docs/api/app.md +++ b/docs/api/app.md @@ -401,6 +401,14 @@ You can request the following paths by the name: * `videos` Directory for a user's videos. * `pepperFlashSystemPlugin` Full path to the system version of the Pepper Flash plugin. +### `app.getFileIcon(path, [options], callback)` + +* `path` String +* `options` Object (optional) + * `size` String - Defaults to `normal`. Can be `small`, `normal`, `large` +* `callback` Function + * `Icon` [NativeImage](native-image.md) + ### `app.setPath(name, path)` * `name` String From 3d47c9b71df7df1b65c330f2110e059a87b4237e Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Thu, 3 Nov 2016 21:43:53 +0300 Subject: [PATCH 13/32] Fix lint --- atom/browser/api/atom_api_app.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index c6b50a48e9..4b59914993 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -343,7 +343,7 @@ IconLoader::IconSize GetIconSizeByString(std::string size) { return IconLoader::IconSize::LARGE; } return IconLoader::IconSize::NORMAL; -}; +} // Return the path constant from string. int GetPathConstant(const std::string& name) { From 2e85ff1f57b9d4cca28a89f3c955c5c34d6ac55c Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Fri, 4 Nov 2016 12:29:18 +0530 Subject: [PATCH 14/32] Fix code style --- atom/browser/api/atom_api_app.cc | 6 +++--- atom/common/api/atom_api_native_image.cc | 1 - chromium_src/chrome/browser/browser_process.h | 2 -- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index 4b59914993..cabe751a04 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -336,7 +336,7 @@ namespace api { namespace { -IconLoader::IconSize GetIconSizeByString(std::string size) { +IconLoader::IconSize GetIconSizeByString(const std::string& size) { if (size == "small") { return IconLoader::IconSize::SMALL; } else if (size == "large") { @@ -858,7 +858,7 @@ JumpListResult App::SetJumpList(v8::Local val, void App::GetFileIcon(const base::FilePath& path, mate::Arguments* args) { - base::DictionaryValue options; + mate::Dictionary options; IconLoader::IconSize icon_size; FileIconCallback callback; @@ -866,7 +866,7 @@ void App::GetFileIcon(const base::FilePath& path, icon_size = IconLoader::IconSize::NORMAL; } else { std::string icon_size_string; - options.GetString("size", &icon_size_string); + options.Get("size", &icon_size_string); icon_size = GetIconSizeByString(icon_size_string); } diff --git a/atom/common/api/atom_api_native_image.cc b/atom/common/api/atom_api_native_image.cc index 1d4e20e3cf..0c174941db 100644 --- a/atom/common/api/atom_api_native_image.cc +++ b/atom/common/api/atom_api_native_image.cc @@ -8,7 +8,6 @@ #include #include "atom/common/asar/asar_util.h" -#include "atom/common/native_mate_converters/callback.h" #include "atom/common/native_mate_converters/file_path_converter.h" #include "atom/common/native_mate_converters/gfx_converter.h" #include "atom/common/native_mate_converters/gurl_converter.h" diff --git a/chromium_src/chrome/browser/browser_process.h b/chromium_src/chrome/browser/browser_process.h index a21371efb2..1459ca31a6 100644 --- a/chromium_src/chrome/browser/browser_process.h +++ b/chromium_src/chrome/browser/browser_process.h @@ -19,8 +19,6 @@ namespace printing { class PrintJobManager; } -class IconManager; - // NOT THREAD SAFE, call only from the main thread. // These functions shouldn't return NULL unless otherwise noted. class BrowserProcess { From 1b4ee6e0d87c5a881083966510ab62fd9beead58 Mon Sep 17 00:00:00 2001 From: Yury Date: Sat, 5 Nov 2016 21:23:49 +0300 Subject: [PATCH 15/32] Image from icon node-style callback (#2) * Try implementing node-style callbacks * Add locker and handle scope --- atom/browser/api/atom_api_app.cc | 16 ++++++++++++++-- atom/browser/api/atom_api_app.h | 3 ++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index cabe751a04..7358960685 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -474,7 +474,16 @@ int ImportIntoCertStore( void OnIconDataAvailable(const App::FileIconCallback& callback, gfx::Image* icon) { - callback.Run(icon ? *icon : gfx::Image()); + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + if (icon && !icon->IsEmpty()) { + v8::Locker locker(isolate); + v8::HandleScope handle_scope(isolate); + callback.Run(v8::Null(isolate), *icon); + } else { + v8::Local error_message = + v8::String::NewFromUtf8(isolate, "Failed to get file icon."); + callback.Run(v8::Exception::Error(error_message), gfx::Image()); + } } } // namespace @@ -878,7 +887,10 @@ void App::GetFileIcon(const base::FilePath& path, IconManager* icon_manager = IconManager::GetInstance(); gfx::Image* icon = icon_manager->LookupIconFromFilepath(path, icon_size); if (icon) { - callback.Run(*icon); + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + v8::Locker locker(isolate); + v8::HandleScope handle_scope(isolate); + callback.Run(v8::Null(isolate), *icon); } else { icon_manager->LoadIcon(path, icon_size, base::Bind(&OnIconDataAvailable, callback)); diff --git a/atom/browser/api/atom_api_app.h b/atom/browser/api/atom_api_app.h index 7d2f6f752b..b89761ede5 100644 --- a/atom/browser/api/atom_api_app.h +++ b/atom/browser/api/atom_api_app.h @@ -12,6 +12,7 @@ #include "atom/browser/atom_browser_client.h" #include "atom/browser/browser.h" #include "atom/browser/browser_observer.h" +#include "atom/common/api/atom_api_native_image.h" #include "atom/common/native_mate_converters/callback.h" #include "chrome/browser/icon_loader.h" #include "chrome/browser/process_singleton.h" @@ -44,7 +45,7 @@ class App : public AtomBrowserClient::Delegate, public BrowserObserver, public content::GpuDataManagerObserver { public: - using FileIconCallback = base::Callback; + using FileIconCallback = base::Callback, const gfx::Image&)>; static mate::Handle Create(v8::Isolate* isolate); From 5794138ed43d76f25291428bf25f37dd6b12ed7b Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Sat, 5 Nov 2016 21:55:23 +0300 Subject: [PATCH 16/32] Normalize path --- atom/browser/api/atom_api_app.cc | 7 +++++-- atom/browser/api/atom_api_app.h | 3 ++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index 7358960685..9beff98805 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -871,6 +871,8 @@ void App::GetFileIcon(const base::FilePath& path, IconLoader::IconSize icon_size; FileIconCallback callback; + base::FilePath normalized_path = path.NormalizePathSeparators(); + if (!args->GetNext(&options)) { icon_size = IconLoader::IconSize::NORMAL; } else { @@ -885,14 +887,15 @@ void App::GetFileIcon(const base::FilePath& path, } IconManager* icon_manager = IconManager::GetInstance(); - gfx::Image* icon = icon_manager->LookupIconFromFilepath(path, icon_size); + gfx::Image* icon = icon_manager->LookupIconFromFilepath(normalized_path, + icon_size); if (icon) { v8::Isolate* isolate = v8::Isolate::GetCurrent(); v8::Locker locker(isolate); v8::HandleScope handle_scope(isolate); callback.Run(v8::Null(isolate), *icon); } else { - icon_manager->LoadIcon(path, icon_size, + icon_manager->LoadIcon(normalized_path, icon_size, base::Bind(&OnIconDataAvailable, callback)); } } diff --git a/atom/browser/api/atom_api_app.h b/atom/browser/api/atom_api_app.h index b89761ede5..b051e4e5fa 100644 --- a/atom/browser/api/atom_api_app.h +++ b/atom/browser/api/atom_api_app.h @@ -45,7 +45,8 @@ class App : public AtomBrowserClient::Delegate, public BrowserObserver, public content::GpuDataManagerObserver { public: - using FileIconCallback = base::Callback, const gfx::Image&)>; + using FileIconCallback = base::Callback, + const gfx::Image&)>; static mate::Handle Create(v8::Isolate* isolate); From 11ef2c539b73250ebfdadd48d580532377756d23 Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Sat, 5 Nov 2016 22:04:38 +0300 Subject: [PATCH 17/32] Update docs --- docs/api/app.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/api/app.md b/docs/api/app.md index 8398f4c0a7..8acccb126f 100644 --- a/docs/api/app.md +++ b/docs/api/app.md @@ -407,7 +407,8 @@ You can request the following paths by the name: * `options` Object (optional) * `size` String - Defaults to `normal`. Can be `small`, `normal`, `large` * `callback` Function - * `Icon` [NativeImage](native-image.md) + * `error` `Error` + * `icon` [NativeImage](native-image.md) ### `app.setPath(name, path)` From c2bf5bb986d1af57448b659a2ba9cb21211dbffd Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Sun, 6 Nov 2016 12:58:01 +0300 Subject: [PATCH 18/32] Put locker and handle scope to the top of the function. Remove unneeded header --- atom/browser/api/atom_api_app.cc | 12 +++++++----- atom/browser/api/atom_api_app.h | 1 - 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index 9beff98805..7bb797468a 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -475,9 +475,10 @@ int ImportIntoCertStore( void OnIconDataAvailable(const App::FileIconCallback& callback, gfx::Image* icon) { v8::Isolate* isolate = v8::Isolate::GetCurrent(); + v8::Locker locker(isolate); + v8::HandleScope handle_scope(isolate); + if (icon && !icon->IsEmpty()) { - v8::Locker locker(isolate); - v8::HandleScope handle_scope(isolate); callback.Run(v8::Null(isolate), *icon); } else { v8::Local error_message = @@ -871,6 +872,10 @@ void App::GetFileIcon(const base::FilePath& path, IconLoader::IconSize icon_size; FileIconCallback callback; + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + v8::Locker locker(isolate); + v8::HandleScope handle_scope(isolate); + base::FilePath normalized_path = path.NormalizePathSeparators(); if (!args->GetNext(&options)) { @@ -890,9 +895,6 @@ void App::GetFileIcon(const base::FilePath& path, gfx::Image* icon = icon_manager->LookupIconFromFilepath(normalized_path, icon_size); if (icon) { - v8::Isolate* isolate = v8::Isolate::GetCurrent(); - v8::Locker locker(isolate); - v8::HandleScope handle_scope(isolate); callback.Run(v8::Null(isolate), *icon); } else { icon_manager->LoadIcon(normalized_path, icon_size, diff --git a/atom/browser/api/atom_api_app.h b/atom/browser/api/atom_api_app.h index b051e4e5fa..1d8e5804e5 100644 --- a/atom/browser/api/atom_api_app.h +++ b/atom/browser/api/atom_api_app.h @@ -12,7 +12,6 @@ #include "atom/browser/atom_browser_client.h" #include "atom/browser/browser.h" #include "atom/browser/browser_observer.h" -#include "atom/common/api/atom_api_native_image.h" #include "atom/common/native_mate_converters/callback.h" #include "chrome/browser/icon_loader.h" #include "chrome/browser/process_singleton.h" From 00748889f93455ac5f1401844da241dab01f0b58 Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Sun, 6 Nov 2016 13:59:17 +0300 Subject: [PATCH 19/32] Add tests --- spec/api-app-spec.js | 58 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/spec/api-app-spec.js b/spec/api-app-spec.js index 085522a0cc..1dc9fae100 100644 --- a/spec/api-app-spec.js +++ b/spec/api-app-spec.js @@ -456,4 +456,62 @@ describe('app module', function () { assert.equal(app.isDefaultProtocolClient(protocol), false) }) }) + + describe('getFileIcon() API', function() { + const iconPath = path.join(process.cwd(), 'fixtures/assets/icon.ico') + const sizes = { + small: 16, + normal: 32, + large: 48 + }; + + it('fetches non-empty icon', function(done) { + app.getFileIcon(iconPath, function(err, icon) { + assert.equal(err, null) + assert.equal(icon.isEmpty(), false) + done() + }) + }) + + it('fetches normal size by default', function(done) { + app.getFileIcon(iconPath, function(err, icon) { + const size = icon.getSize() + assert.equal(size.height, sizes.normal) + assert.equal(size.width, sizes.normal) + done() + }) + }) + + describe('size option', function() { + it('fetches small icons', function(done) { + app.getFileIcon(iconPath, { size: 'small' }, function(err, icon) { + const size = icon.getSize() + assert.equal(size.height, sizes.small) + assert.equal(size.width, sizes.small) + done() + }) + }) + + it('fetches normal icons', function(done) { + app.getFileIcon(iconPath, { size: 'normal' }, function(err, icon) { + const size = icon.getSize() + assert.equal(size.height, sizes.normal) + assert.equal(size.width, sizes.normal) + done() + }) + }) + + it('fetches large icons', function(done) { + if (process.platform === 'darwin') { + done() // macOS does not support large icons + } + app.getFileIcon(iconPath, { size: 'normal' }, function(err, icon) { + const size = icon.getSize() + assert.equal(size.height, sizes.normal) + assert.equal(size.width, sizes.normal) + done() + }) + }) + }) + }) }) From 1aa4fcae08b9737187df9e0cdcc2c583bd7b80f1 Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Sun, 6 Nov 2016 14:03:16 +0300 Subject: [PATCH 20/32] Lint tests --- spec/api-app-spec.js | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/spec/api-app-spec.js b/spec/api-app-spec.js index 1dc9fae100..9212e9711b 100644 --- a/spec/api-app-spec.js +++ b/spec/api-app-spec.js @@ -457,56 +457,60 @@ describe('app module', function () { }) }) - describe('getFileIcon() API', function() { + describe('getFileIcon() API', function () { const iconPath = path.join(process.cwd(), 'fixtures/assets/icon.ico') const sizes = { small: 16, normal: 32, large: 48 - }; + } - it('fetches non-empty icon', function(done) { - app.getFileIcon(iconPath, function(err, icon) { + it('fetches non-empty icon', function (done) { + app.getFileIcon(iconPath, function (err, icon) { assert.equal(err, null) assert.equal(icon.isEmpty(), false) done() }) }) - it('fetches normal size by default', function(done) { - app.getFileIcon(iconPath, function(err, icon) { + it('fetches normal size by default', function (done) { + app.getFileIcon(iconPath, function (err, icon) { const size = icon.getSize() + assert.equal(err, null) assert.equal(size.height, sizes.normal) assert.equal(size.width, sizes.normal) done() }) }) - describe('size option', function() { - it('fetches small icons', function(done) { - app.getFileIcon(iconPath, { size: 'small' }, function(err, icon) { + describe('size option', function () { + it('fetches small icons', function (done) { + app.getFileIcon(iconPath, { size: 'small' }, function (err, icon) { const size = icon.getSize() + assert.equal(err, null) assert.equal(size.height, sizes.small) assert.equal(size.width, sizes.small) done() }) }) - it('fetches normal icons', function(done) { - app.getFileIcon(iconPath, { size: 'normal' }, function(err, icon) { + it('fetches normal icons', function (done) { + app.getFileIcon(iconPath, { size: 'normal' }, function (err, icon) { const size = icon.getSize() + assert.equal(err, null) assert.equal(size.height, sizes.normal) assert.equal(size.width, sizes.normal) done() }) }) - it('fetches large icons', function(done) { + it('fetches large icons', function (done) { if (process.platform === 'darwin') { done() // macOS does not support large icons } - app.getFileIcon(iconPath, { size: 'normal' }, function(err, icon) { + app.getFileIcon(iconPath, { size: 'normal' }, function (err, icon) { const size = icon.getSize() + assert.equal(err, null) assert.equal(size.height, sizes.normal) assert.equal(size.width, sizes.normal) done() From c36cdb858025836938702e85b1dbcc8525b07cd5 Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Sun, 6 Nov 2016 14:29:45 +0300 Subject: [PATCH 21/32] Properly skip large size test on macOS --- spec/api-app-spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/api-app-spec.js b/spec/api-app-spec.js index 9212e9711b..2508efe388 100644 --- a/spec/api-app-spec.js +++ b/spec/api-app-spec.js @@ -506,7 +506,7 @@ describe('app module', function () { it('fetches large icons', function (done) { if (process.platform === 'darwin') { - done() // macOS does not support large icons + return this.skip() // macOS does not support large icons } app.getFileIcon(iconPath, { size: 'normal' }, function (err, icon) { const size = icon.getSize() From 29452364f30a06d762c633f18f2f849b00cc1dd6 Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Sun, 6 Nov 2016 15:21:21 +0300 Subject: [PATCH 22/32] Use isolate() method to get isolate --- atom/browser/api/atom_api_app.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index 7bb797468a..47ce814c6e 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -472,9 +472,9 @@ int ImportIntoCertStore( } #endif -void OnIconDataAvailable(const App::FileIconCallback& callback, +void OnIconDataAvailable(v8::Isolate* isolate, + const App::FileIconCallback& callback, gfx::Image* icon) { - v8::Isolate* isolate = v8::Isolate::GetCurrent(); v8::Locker locker(isolate); v8::HandleScope handle_scope(isolate); @@ -872,9 +872,8 @@ void App::GetFileIcon(const base::FilePath& path, IconLoader::IconSize icon_size; FileIconCallback callback; - v8::Isolate* isolate = v8::Isolate::GetCurrent(); - v8::Locker locker(isolate); - v8::HandleScope handle_scope(isolate); + v8::Locker locker(isolate()); + v8::HandleScope handle_scope(isolate()); base::FilePath normalized_path = path.NormalizePathSeparators(); @@ -895,10 +894,11 @@ void App::GetFileIcon(const base::FilePath& path, gfx::Image* icon = icon_manager->LookupIconFromFilepath(normalized_path, icon_size); if (icon) { - callback.Run(v8::Null(isolate), *icon); + callback.Run(v8::Null(isolate()), *icon); } else { icon_manager->LoadIcon(normalized_path, icon_size, - base::Bind(&OnIconDataAvailable, callback)); + base::Bind(&OnIconDataAvailable, isolate(), + callback)); } } From 2b60df3d8bbb5b5ac1b8df2efc5b5ab8e225b319 Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Sun, 6 Nov 2016 17:10:33 +0300 Subject: [PATCH 23/32] Fix some feedback --- docs/api/app.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/api/app.md b/docs/api/app.md index 8acccb126f..5bd12f361d 100644 --- a/docs/api/app.md +++ b/docs/api/app.md @@ -405,7 +405,7 @@ You can request the following paths by the name: * `path` String * `options` Object (optional) - * `size` String - Defaults to `normal`. Can be `small`, `normal`, `large` + * `size` String - Can be `small`, `normal`, `large`. Except for `large` on _macOS_ * `callback` Function * `error` `Error` * `icon` [NativeImage](native-image.md) From bcf0964c618637b24e54ecfe87d178fd1bc79a41 Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Thu, 10 Nov 2016 21:34:30 +0300 Subject: [PATCH 24/32] Fix more review --- atom/browser/api/atom_api_app.cc | 2 +- docs/api/app.md | 8 +++++++- spec/api-app-spec.js | 8 ++++---- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index 47ce814c6e..904a63a5d0 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -886,7 +886,7 @@ void App::GetFileIcon(const base::FilePath& path, } if (!args->GetNext(&callback)) { - args->ThrowError(); + args->ThrowError("Missing required callback function"); return; } diff --git a/docs/api/app.md b/docs/api/app.md index 5bd12f361d..e825add47e 100644 --- a/docs/api/app.md +++ b/docs/api/app.md @@ -405,11 +405,17 @@ You can request the following paths by the name: * `path` String * `options` Object (optional) - * `size` String - Can be `small`, `normal`, `large`. Except for `large` on _macOS_ + * `size` String - Can be `small`, `normal`, `large`. The `large` size is not supported on _macOS_ * `callback` Function * `error` `Error` * `icon` [NativeImage](native-image.md) +Fetches associated icon for using OS rules for handling icons. +On _Windows_, there a 2 kinds of icons: +- icons associated with certain file extension - `.mp3`, `.png`, etc. +- icons inside file itself, like `.exe`, `.dll`, `.ico`. +On _Linux_ and _macOS_, icons depend on application associated with file mime type. + ### `app.setPath(name, path)` * `name` String diff --git a/spec/api-app-spec.js b/spec/api-app-spec.js index 2508efe388..f158cc851a 100644 --- a/spec/api-app-spec.js +++ b/spec/api-app-spec.js @@ -458,7 +458,7 @@ describe('app module', function () { }) describe('getFileIcon() API', function () { - const iconPath = path.join(process.cwd(), 'fixtures/assets/icon.ico') + const iconPath = path.join(__dirname, 'fixtures/assets/icon.ico') const sizes = { small: 16, normal: 32, @@ -484,7 +484,7 @@ describe('app module', function () { }) describe('size option', function () { - it('fetches small icons', function (done) { + it('fetches small icon', function (done) { app.getFileIcon(iconPath, { size: 'small' }, function (err, icon) { const size = icon.getSize() assert.equal(err, null) @@ -494,7 +494,7 @@ describe('app module', function () { }) }) - it('fetches normal icons', function (done) { + it('fetches normal icon', function (done) { app.getFileIcon(iconPath, { size: 'normal' }, function (err, icon) { const size = icon.getSize() assert.equal(err, null) @@ -504,7 +504,7 @@ describe('app module', function () { }) }) - it('fetches large icons', function (done) { + it('fetches large icon', function (done) { if (process.platform === 'darwin') { return this.skip() // macOS does not support large icons } From c810e64fdafa0f4de4bd5864bda337f5f3ea2904 Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Thu, 10 Nov 2016 22:00:58 +0300 Subject: [PATCH 25/32] Fix doc lint --- docs/api/app.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/api/app.md b/docs/api/app.md index e825add47e..5285800551 100644 --- a/docs/api/app.md +++ b/docs/api/app.md @@ -407,7 +407,7 @@ You can request the following paths by the name: * `options` Object (optional) * `size` String - Can be `small`, `normal`, `large`. The `large` size is not supported on _macOS_ * `callback` Function - * `error` `Error` + * `error` Error * `icon` [NativeImage](native-image.md) Fetches associated icon for using OS rules for handling icons. From a9dae243b4df683c6cd07e0ee38364200e4a47c1 Mon Sep 17 00:00:00 2001 From: Yury Solovyov Date: Fri, 11 Nov 2016 09:32:41 +0300 Subject: [PATCH 26/32] Fix large icon spec --- spec/api-app-spec.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/api-app-spec.js b/spec/api-app-spec.js index f158cc851a..bf5e767d79 100644 --- a/spec/api-app-spec.js +++ b/spec/api-app-spec.js @@ -508,11 +508,11 @@ describe('app module', function () { if (process.platform === 'darwin') { return this.skip() // macOS does not support large icons } - app.getFileIcon(iconPath, { size: 'normal' }, function (err, icon) { + app.getFileIcon(iconPath, { size: 'large' }, function (err, icon) { const size = icon.getSize() assert.equal(err, null) - assert.equal(size.height, sizes.normal) - assert.equal(size.width, sizes.normal) + assert.equal(size.height, sizes.large) + assert.equal(size.width, sizes.large) done() }) }) From ee6677645038b10ca855ba5bc3aa197949e048b4 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 7 Feb 2017 10:15:46 -0800 Subject: [PATCH 27/32] Update IconManager for Chrome 56 upgrade --- chromium_src/chrome/browser/icon_manager.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/chromium_src/chrome/browser/icon_manager.cc b/chromium_src/chrome/browser/icon_manager.cc index 17e362c534..95073fcdd7 100644 --- a/chromium_src/chrome/browser/icon_manager.cc +++ b/chromium_src/chrome/browser/icon_manager.cc @@ -27,7 +27,7 @@ IconManager* IconManager::GetInstance() { IconManager::IconManager() {} IconManager::~IconManager() { - STLDeleteValues(&icon_cache_); + base::STLDeleteValues(&icon_cache_); } gfx::Image* IconManager::LookupIconFromFilepath(const base::FilePath& file_name, From 683a758daba7f66884897f33fd54aa143ea6f337 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 7 Feb 2017 10:16:09 -0800 Subject: [PATCH 28/32] Call done instead of skip --- spec/api-app-spec.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/api-app-spec.js b/spec/api-app-spec.js index bf5e767d79..8d754bf65e 100644 --- a/spec/api-app-spec.js +++ b/spec/api-app-spec.js @@ -505,9 +505,9 @@ describe('app module', function () { }) it('fetches large icon', function (done) { - if (process.platform === 'darwin') { - return this.skip() // macOS does not support large icons - } + // macOS does not support large icons + if (process.platform === 'darwin') return done() + app.getFileIcon(iconPath, { size: 'large' }, function (err, icon) { const size = icon.getSize() assert.equal(err, null) From dddc6aec497d461eb8ab9c98bca650bd5079ce2d Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 7 Feb 2017 10:21:20 -0800 Subject: [PATCH 29/32] Tweak spec descriptions --- spec/api-app-spec.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/api-app-spec.js b/spec/api-app-spec.js index 8d754bf65e..71e1df8789 100644 --- a/spec/api-app-spec.js +++ b/spec/api-app-spec.js @@ -465,7 +465,7 @@ describe('app module', function () { large: 48 } - it('fetches non-empty icon', function (done) { + it('fetches a non-empty icon', function (done) { app.getFileIcon(iconPath, function (err, icon) { assert.equal(err, null) assert.equal(icon.isEmpty(), false) @@ -473,7 +473,7 @@ describe('app module', function () { }) }) - it('fetches normal size by default', function (done) { + it('fetches normal icon size by default', function (done) { app.getFileIcon(iconPath, function (err, icon) { const size = icon.getSize() assert.equal(err, null) @@ -484,7 +484,7 @@ describe('app module', function () { }) describe('size option', function () { - it('fetches small icon', function (done) { + it('fetches a small icon', function (done) { app.getFileIcon(iconPath, { size: 'small' }, function (err, icon) { const size = icon.getSize() assert.equal(err, null) @@ -494,7 +494,7 @@ describe('app module', function () { }) }) - it('fetches normal icon', function (done) { + it('fetches a normal icon', function (done) { app.getFileIcon(iconPath, { size: 'normal' }, function (err, icon) { const size = icon.getSize() assert.equal(err, null) @@ -504,7 +504,7 @@ describe('app module', function () { }) }) - it('fetches large icon', function (done) { + it('fetches a large icon', function (done) { // macOS does not support large icons if (process.platform === 'darwin') return done() From a4277bb61699a009c7a00197c61ba623d42c9559 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 7 Feb 2017 10:33:44 -0800 Subject: [PATCH 30/32] Document sizes --- docs/api/app.md | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/docs/api/app.md b/docs/api/app.md index 5285800551..6e159d1dc6 100644 --- a/docs/api/app.md +++ b/docs/api/app.md @@ -405,16 +405,23 @@ You can request the following paths by the name: * `path` String * `options` Object (optional) - * `size` String - Can be `small`, `normal`, `large`. The `large` size is not supported on _macOS_ + * `size` String + * `small` - 16x16 + * `normal` - 32x32 + * `large` - 48x48. This size is not supported on _macOS_. * `callback` Function * `error` Error * `icon` [NativeImage](native-image.md) -Fetches associated icon for using OS rules for handling icons. +Fetches a path's associated icon. + On _Windows_, there a 2 kinds of icons: -- icons associated with certain file extension - `.mp3`, `.png`, etc. -- icons inside file itself, like `.exe`, `.dll`, `.ico`. -On _Linux_ and _macOS_, icons depend on application associated with file mime type. + +- Icons associated with certain file extensions, like `.mp3`, `.png`, etc. +- Icons inside the file itself, like `.exe`, `.dll`, `.ico`. + +On _Linux_ and _macOS_, icons depend on the application associated with file +mime type. ### `app.setPath(name, path)` From fc1b7431c758e88a5f9cd0b99f7f97fc35fe6d20 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 7 Feb 2017 10:35:31 -0800 Subject: [PATCH 31/32] Tweak optional syntax --- docs/api/app.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/api/app.md b/docs/api/app.md index 6e159d1dc6..25a609cb24 100644 --- a/docs/api/app.md +++ b/docs/api/app.md @@ -401,7 +401,7 @@ You can request the following paths by the name: * `videos` Directory for a user's videos. * `pepperFlashSystemPlugin` Full path to the system version of the Pepper Flash plugin. -### `app.getFileIcon(path, [options], callback)` +### `app.getFileIcon(path[, options], callback)` * `path` String * `options` Object (optional) From 82ac4ddf19a8cafcb5cce43a47b58e0da3c93087 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 7 Feb 2017 11:20:27 -0800 Subject: [PATCH 32/32] Large is only 48x48 on Linux --- docs/api/app.md | 2 +- spec/api-app-spec.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/api/app.md b/docs/api/app.md index 25a609cb24..d34daeddde 100644 --- a/docs/api/app.md +++ b/docs/api/app.md @@ -408,7 +408,7 @@ You can request the following paths by the name: * `size` String * `small` - 16x16 * `normal` - 32x32 - * `large` - 48x48. This size is not supported on _macOS_. + * `large` - 48x48 on _Linux_, 32x32 on _Windows_, unsupported on _macOS_. * `callback` Function * `error` Error * `icon` [NativeImage](native-image.md) diff --git a/spec/api-app-spec.js b/spec/api-app-spec.js index 71e1df8789..9bfd77173c 100644 --- a/spec/api-app-spec.js +++ b/spec/api-app-spec.js @@ -462,7 +462,7 @@ describe('app module', function () { const sizes = { small: 16, normal: 32, - large: 48 + large: process.platform === 'win32' ? 32 : 48 } it('fetches a non-empty icon', function (done) {