Reland "Use default icons for PE files lacking their own."
This is a reland of 676d0cb1f6ac3488102c1024c70ae3c272861431.
[email protected]
Bug: 1032250
Original change's description:
> Use default icons for PE files lacking their own.
>
> PrivateExtractIcon (underlying the icon loading service) may fail to
> supply an icon for PE files that do not have one embedded and would
> display as a blank area when downloaded. This change fetches the
> default for the file extension prior to fetching the file's embedded
> icon. It is difficult to do this only on failure as the ownership of
> IconLoader means it has already been deleted when we learn about the
> failure. It should be relatively cheap to fetch the default icons as
> these will be cached by the OS, and this occurs in the main process.
>
> Tests: Added LoadDefaultIcon in icon_loader_browser_test
> Bug: 1032250
> Change-Id: Ifad1c9f013d0e4fe87aaec5ec0cdeae08d06799e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2242371
> Commit-Queue: Alex Gough <[email protected]>
> Reviewed-by: François Doray <[email protected]>
> Reviewed-by: Avi Drissman <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#779542}
Change-Id: Ie7f5e57dbbfb1ebf5d9fc588e15b1a2687212bce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2254823
Reviewed-by: Alex Gough <[email protected]>
Reviewed-by: Avi Drissman <[email protected]>
Commit-Queue: Alex Gough <[email protected]>
Cr-Commit-Position: refs/heads/master@{#781108}
diff --git a/chrome/browser/icon_loader_win.cc b/chrome/browser/icon_loader_win.cc
index bd2622b..a6c4d17c 100644
--- a/chrome/browser/icon_loader_win.cc
+++ b/chrome/browser/icon_loader_win.cc
@@ -28,10 +28,13 @@
static void ExecuteLoadIcon(
base::FilePath filename,
chrome::mojom::IconSize size,
+ gfx::Image default_icon,
scoped_refptr<base::SingleThreadTaskRunner> target_task_runner,
IconLoader::IconLoadedCallback icon_loaded_callback);
- IconLoaderHelper(base::FilePath filename, chrome::mojom::IconSize size);
+ IconLoaderHelper(base::FilePath filename,
+ chrome::mojom::IconSize size,
+ gfx::Image default_icon);
private:
void StartReadIconRequest();
@@ -52,6 +55,7 @@
chrome::mojom::IconSize size_;
// This callback owns the object until work is done.
IconLoaderHelperCallback finally_;
+ gfx::Image default_icon_;
SEQUENCE_CHECKER(sequence_checker_);
@@ -61,10 +65,12 @@
void IconLoaderHelper::ExecuteLoadIcon(
base::FilePath filename,
chrome::mojom::IconSize size,
+ gfx::Image default_icon,
scoped_refptr<base::SingleThreadTaskRunner> target_task_runner,
IconLoader::IconLoadedCallback icon_loaded_callback) {
// Self-deleting helper manages service lifetime.
- auto helper = std::make_unique<IconLoaderHelper>(filename, size);
+ auto helper = std::make_unique<IconLoaderHelper>(filename, size,
+ std::move(default_icon));
auto* helper_raw = helper.get();
// This callback owns the helper and extinguishes itself once work is done.
auto finally_callback = base::BindOnce(
@@ -83,8 +89,9 @@
}
IconLoaderHelper::IconLoaderHelper(base::FilePath filename,
- chrome::mojom::IconSize size)
- : filename_(filename), size_(size) {
+ chrome::mojom::IconSize size,
+ gfx::Image default_icon)
+ : filename_(filename), size_(size), default_icon_(std::move(default_icon)) {
remote_read_icon_ = LaunchIconReaderInstance();
remote_read_icon_.set_disconnect_handler(base::BindOnce(
&IconLoaderHelper::OnConnectionError, base::Unretained(this)));
@@ -102,16 +109,55 @@
if (finally_.is_null())
return;
- gfx::Image image;
- std::move(finally_).Run(std::move(image), filename_.value());
+ std::move(finally_).Run(std::move(default_icon_), filename_.value());
}
void IconLoaderHelper::OnReadIconExecuted(const gfx::ImageSkia& icon,
const base::string16& group) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
- gfx::Image image(icon);
- std::move(finally_).Run(std::move(image), group);
+ if (icon.isNull()) {
+ std::move(finally_).Run(std::move(default_icon_), group);
+ } else {
+ gfx::Image image(icon);
+ std::move(finally_).Run(std::move(image), group);
+ }
+}
+
+// Must be called in a COM context. |group| should be a file extension.
+gfx::Image GetIconForFileExtension(base::string16 group,
+ IconLoader::IconSize icon_size) {
+ 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();
+ }
+
+ gfx::Image image;
+
+ SHFILEINFO file_info = {0};
+ if (SHGetFileInfo(group.c_str(), FILE_ATTRIBUTE_NORMAL, &file_info,
+ sizeof(file_info),
+ SHGFI_ICON | size | SHGFI_USEFILEATTRIBUTES)) {
+ const SkBitmap bitmap = IconUtil::CreateSkBitmapFromHICON(file_info.hIcon);
+ if (!bitmap.isNull()) {
+ gfx::ImageSkia image_skia(
+ gfx::ImageSkiaRep(bitmap, display::win::GetDPIScale()));
+ image_skia.MakeThreadSafe();
+ image = gfx::Image(image_skia);
+ }
+ DestroyIcon(file_info.hIcon);
+ }
+ return image;
}
} // namespace
@@ -139,9 +185,8 @@
group_ = GroupForFilepath(file_path_);
if (group_ == file_path_.value()) {
- // Calls a Windows API that parses the file so must be sandboxed. Call on
- // target sequence as we don't need COM in this process.
- target_task_runner_->PostTask(
+ // Calls a Windows API that parses the file so must be sandboxed.
+ GetReadIconTaskRunner()->PostTask(
FROM_HERE,
base::BindOnce(&IconLoader::ReadIconInSandbox, base::Unretained(this)));
} else {
@@ -153,44 +198,21 @@
}
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();
- }
-
- gfx::Image image;
-
- SHFILEINFO file_info = { 0 };
- if (SHGetFileInfo(group_.c_str(), FILE_ATTRIBUTE_NORMAL, &file_info,
- sizeof(file_info),
- SHGFI_ICON | size | SHGFI_USEFILEATTRIBUTES)) {
- const SkBitmap bitmap = IconUtil::CreateSkBitmapFromHICON(file_info.hIcon);
- if (!bitmap.isNull()) {
- gfx::ImageSkia image_skia(
- gfx::ImageSkiaRep(bitmap, display::win::GetDPIScale()));
- image_skia.MakeThreadSafe();
- image = gfx::Image(image_skia);
- }
- DestroyIcon(file_info.hIcon);
- }
+ auto image = GetIconForFileExtension(group_, icon_size_);
target_task_runner_->PostTask(
FROM_HERE,
base::BindOnce(std::move(callback_), std::move(image), group_));
+
delete this;
}
void IconLoader::ReadIconInSandbox() {
+ // Get default first as loader is deleted before ExecuteLoadIcon
+ // completes.
+ auto path = base::FilePath(group_);
+ auto default_icon = GetIconForFileExtension(path.Extension(), icon_size_);
+
chrome::mojom::IconSize size = chrome::mojom::IconSize::kNormal;
switch (icon_size_) {
case IconLoader::SMALL:
@@ -206,8 +228,10 @@
NOTREACHED();
}
- IconLoaderHelper::ExecuteLoadIcon(base::FilePath(group_), size,
- target_task_runner_, std::move(callback_));
+ target_task_runner_->PostTask(
+ FROM_HERE, base::BindOnce(&IconLoaderHelper::ExecuteLoadIcon,
+ std::move(path), size, std::move(default_icon),
+ target_task_runner_, std::move(callback_)));
delete this;
}
