From 3455136e9dec4a693b59f8c42cf00baad412c6d2 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Mon, 4 Jan 2021 12:58:31 -0800 Subject: [PATCH] refactor: remove path from nativeImage converter (#26546) --- shell/browser/api/electron_api_base_window.cc | 22 +++--- shell/browser/api/electron_api_base_window.h | 3 +- shell/browser/api/electron_api_tray.cc | 49 +++++++++---- shell/browser/api/electron_api_tray.h | 12 ++-- .../browser/api/electron_api_web_contents.cc | 12 +++- shell/browser/browser.h | 2 +- shell/browser/browser_mac.mm | 12 +++- shell/common/api/electron_api_native_image.cc | 72 ++++++++----------- shell/common/api/electron_api_native_image.h | 18 ++--- .../common/gin_converters/image_converter.cc | 16 ++++- spec-main/api-app-spec.ts | 9 +++ spec-main/api-tray-spec.ts | 39 +++++++++- spec-main/api-web-contents-spec.ts | 4 +- 13 files changed, 167 insertions(+), 103 deletions(-) diff --git a/shell/browser/api/electron_api_base_window.cc b/shell/browser/api/electron_api_base_window.cc index 40a33f210a6e..1cff430d281a 100644 --- a/shell/browser/api/electron_api_base_window.cc +++ b/shell/browser/api/electron_api_base_window.cc @@ -99,13 +99,9 @@ BaseWindow::BaseWindow(v8::Isolate* isolate, window_->AddObserver(this); #if defined(TOOLKIT_VIEWS) - { - v8::TryCatch try_catch(isolate); - gin::Handle icon; - if (options.Get(options::kIcon, &icon) && !icon.IsEmpty()) - SetIcon(icon); - if (try_catch.HasCaught()) - LOG(ERROR) << "Failed to convert NativeImage"; + v8::Local icon; + if (options.Get(options::kIcon, &icon)) { + SetIcon(isolate, icon); } #endif } @@ -1003,14 +999,18 @@ bool BaseWindow::SetThumbarButtons(gin_helper::Arguments* args) { } #if defined(TOOLKIT_VIEWS) -void BaseWindow::SetIcon(gin::Handle icon) { +void BaseWindow::SetIcon(v8::Isolate* isolate, v8::Local icon) { + NativeImage* native_image = nullptr; + if (!NativeImage::TryConvertNativeImage(isolate, icon, &native_image)) + return; + #if defined(OS_WIN) static_cast(window_.get()) - ->SetIcon(icon->GetHICON(GetSystemMetrics(SM_CXSMICON)), - icon->GetHICON(GetSystemMetrics(SM_CXICON))); + ->SetIcon(native_image->GetHICON(GetSystemMetrics(SM_CXSMICON)), + native_image->GetHICON(GetSystemMetrics(SM_CXICON))); #elif defined(OS_LINUX) static_cast(window_.get()) - ->SetIcon(icon->image().AsImageSkia()); + ->SetIcon(native_image->image().AsImageSkia()); #endif } #endif diff --git a/shell/browser/api/electron_api_base_window.h b/shell/browser/api/electron_api_base_window.h index 1555da8a1a1d..c8e53633ffcf 100644 --- a/shell/browser/api/electron_api_base_window.h +++ b/shell/browser/api/electron_api_base_window.h @@ -17,6 +17,7 @@ #include "shell/browser/native_window.h" #include "shell/browser/native_window_observer.h" #include "shell/common/api/electron_api_native_image.h" +#include "shell/common/gin_helper/error_thrower.h" #include "shell/common/gin_helper/trackable_object.h" namespace electron { @@ -221,7 +222,7 @@ class BaseWindow : public gin_helper::TrackableObject, // Extra APIs added in JS. bool SetThumbarButtons(gin_helper::Arguments* args); #if defined(TOOLKIT_VIEWS) - void SetIcon(gin::Handle icon); + void SetIcon(v8::Isolate* isolate, v8::Local icon); #endif #if defined(OS_WIN) typedef base::RepeatingCallback, diff --git a/shell/browser/api/electron_api_tray.cc b/shell/browser/api/electron_api_tray.cc index 4b120b724881..1ac892305763 100644 --- a/shell/browser/api/electron_api_tray.cc +++ b/shell/browser/api/electron_api_tray.cc @@ -14,6 +14,7 @@ #include "shell/browser/browser.h" #include "shell/browser/javascript_environment.h" #include "shell/common/api/electron_api_native_image.h" +#include "shell/common/gin_converters/file_path_converter.h" #include "shell/common/gin_converters/gfx_converter.h" #include "shell/common/gin_converters/guid_converter.h" #include "shell/common/gin_converters/image_converter.h" @@ -61,11 +62,11 @@ namespace api { gin::WrapperInfo Tray::kWrapperInfo = {gin::kEmbedderNativeGin}; -Tray::Tray(gin::Handle image, - base::Optional guid, - gin::Arguments* args) +Tray::Tray(v8::Isolate* isolate, + v8::Local image, + base::Optional guid) : tray_icon_(TrayIcon::Create(guid)) { - SetImage(image); + SetImage(isolate, image); tray_icon_->AddObserver(this); } @@ -73,7 +74,7 @@ Tray::~Tray() = default; // static gin::Handle Tray::New(gin_helper::ErrorThrower thrower, - gin::Handle image, + v8::Local image, base::Optional guid, gin::Arguments* args) { if (!Browser::Get()->is_ready()) { @@ -88,7 +89,8 @@ gin::Handle Tray::New(gin_helper::ErrorThrower thrower, } #endif - return gin::CreateHandle(thrower.isolate(), new Tray(image, guid, args)); + return gin::CreateHandle(thrower.isolate(), + new Tray(args->isolate(), image, guid)); } void Tray::OnClicked(const gfx::Rect& bounds, @@ -186,23 +188,34 @@ bool Tray::IsDestroyed() { return !tray_icon_; } -void Tray::SetImage(gin::Handle image) { +void Tray::SetImage(v8::Isolate* isolate, v8::Local image) { if (!CheckAlive()) return; + + NativeImage* native_image = nullptr; + if (!NativeImage::TryConvertNativeImage(isolate, image, &native_image)) + return; + #if defined(OS_WIN) - tray_icon_->SetImage(image->GetHICON(GetSystemMetrics(SM_CXSMICON))); + tray_icon_->SetImage(native_image->GetHICON(GetSystemMetrics(SM_CXSMICON))); #else - tray_icon_->SetImage(image->image()); + tray_icon_->SetImage(native_image->image()); #endif } -void Tray::SetPressedImage(gin::Handle image) { +void Tray::SetPressedImage(v8::Isolate* isolate, v8::Local image) { if (!CheckAlive()) return; + + NativeImage* native_image = nullptr; + if (!NativeImage::TryConvertNativeImage(isolate, image, &native_image)) + return; + #if defined(OS_WIN) - tray_icon_->SetPressedImage(image->GetHICON(GetSystemMetrics(SM_CXSMICON))); + tray_icon_->SetPressedImage( + native_image->GetHICON(GetSystemMetrics(SM_CXSMICON))); #else - tray_icon_->SetPressedImage(image->image()); + tray_icon_->SetPressedImage(native_image->image()); #endif } @@ -282,14 +295,20 @@ void Tray::DisplayBalloon(gin_helper::ErrorThrower thrower, return; } - gin::Handle icon; - options.Get("icon", &icon); + v8::Local icon_value; + NativeImage* icon = nullptr; + if (options.Get("icon", &icon_value) && + !NativeImage::TryConvertNativeImage(thrower.isolate(), icon_value, + &icon)) { + return; + } + options.Get("iconType", &balloon_options.icon_type); options.Get("largeIcon", &balloon_options.large_icon); options.Get("noSound", &balloon_options.no_sound); options.Get("respectQuietTime", &balloon_options.respect_quiet_time); - if (!icon.IsEmpty()) { + if (icon) { #if defined(OS_WIN) balloon_options.icon = icon->GetHICON( GetSystemMetrics(balloon_options.large_icon ? SM_CXICON : SM_CXSMICON)); diff --git a/shell/browser/api/electron_api_tray.h b/shell/browser/api/electron_api_tray.h index 1bfacb2fa577..9fde1b143427 100644 --- a/shell/browser/api/electron_api_tray.h +++ b/shell/browser/api/electron_api_tray.h @@ -43,7 +43,7 @@ class Tray : public gin::Wrappable, public: // gin_helper::Constructible static gin::Handle New(gin_helper::ErrorThrower thrower, - gin::Handle image, + v8::Local image, base::Optional guid, gin::Arguments* args); static v8::Local FillObjectTemplate( @@ -54,9 +54,9 @@ class Tray : public gin::Wrappable, static gin::WrapperInfo kWrapperInfo; private: - Tray(gin::Handle image, - base::Optional guid, - gin::Arguments* args); + Tray(v8::Isolate* isolate, + v8::Local image, + base::Optional guid); ~Tray() override; // TrayIconObserver: @@ -83,8 +83,8 @@ class Tray : public gin::Wrappable, // JS API: void Destroy(); bool IsDestroyed(); - void SetImage(gin::Handle image); - void SetPressedImage(gin::Handle image); + void SetImage(v8::Isolate* isolate, v8::Local image); + void SetPressedImage(v8::Isolate* isolate, v8::Local image); void SetToolTip(const std::string& tool_tip); void SetTitle(const std::string& title, const base::Optional& options, diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc index 93358c5e3865..a1b50478a59b 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/shell/browser/api/electron_api_web_contents.cc @@ -2819,10 +2819,16 @@ void WebContents::StartDrag(const gin_helper::Dictionary& item, files.push_back(file); } - gin::Handle icon; - if (!item.Get("icon", &icon) || icon->image().IsEmpty()) { + v8::Local icon_value; + if (!item.Get("icon", &icon_value)) { gin_helper::ErrorThrower(args->isolate()) - .ThrowError("Must specify non-empty 'icon' option"); + .ThrowError("'icon' parameter is required"); + return; + } + + NativeImage* icon = nullptr; + if (!NativeImage::TryConvertNativeImage(args->isolate(), icon_value, &icon) || + icon->image().IsEmpty()) { return; } diff --git a/shell/browser/browser.h b/shell/browser/browser.h index 9e624abf45c2..1f3803163b9b 100644 --- a/shell/browser/browser.h +++ b/shell/browser/browser.h @@ -223,7 +223,7 @@ class Browser : public WindowListObserver { void DockSetMenu(ElectronMenuModel* model); // Set docks' icon. - void DockSetIcon(const gfx::Image& image); + void DockSetIcon(v8::Isolate* isolate, v8::Local icon); #endif // defined(OS_MAC) diff --git a/shell/browser/browser_mac.mm b/shell/browser/browser_mac.mm index 56a875a9e869..b09ef1052232 100644 --- a/shell/browser/browser_mac.mm +++ b/shell/browser/browser_mac.mm @@ -20,6 +20,7 @@ #include "shell/browser/mac/electron_application_delegate.h" #include "shell/browser/native_window.h" #include "shell/browser/window_list.h" +#include "shell/common/api/electron_api_native_image.h" #include "shell/common/application_info.h" #include "shell/common/gin_converters/image_converter.h" #include "shell/common/gin_helper/arguments.h" @@ -454,7 +455,16 @@ void Browser::DockSetMenu(ElectronMenuModel* model) { [delegate setApplicationDockMenu:model]; } -void Browser::DockSetIcon(const gfx::Image& image) { +void Browser::DockSetIcon(v8::Isolate* isolate, v8::Local icon) { + gfx::Image image; + + if (!icon->IsNull()) { + api::NativeImage* native_image = nullptr; + if (!api::NativeImage::TryConvertNativeImage(isolate, icon, &native_image)) + return; + image = native_image->image(); + } + [[AtomApplication sharedApplication] setApplicationIconImage:image.AsNSImage()]; } diff --git a/shell/common/api/electron_api_native_image.cc b/shell/common/api/electron_api_native_image.cc index 9781f9b1af0f..b353d4bf2217 100644 --- a/shell/common/api/electron_api_native_image.cc +++ b/shell/common/api/electron_api_native_image.cc @@ -17,6 +17,7 @@ #include "gin/arguments.h" #include "gin/object_template_builder.h" #include "gin/per_isolate_data.h" +#include "gin/wrappable.h" #include "net/base/data_url.h" #include "shell/common/asar/asar_util.h" #include "shell/common/gin_converters/file_path_converter.h" @@ -134,6 +135,33 @@ NativeImage::~NativeImage() { } } +// static +bool NativeImage::TryConvertNativeImage(v8::Isolate* isolate, + v8::Local image, + NativeImage** native_image) { + base::FilePath icon_path; + if (gin::ConvertFromV8(isolate, image, &icon_path)) { + *native_image = NativeImage::CreateFromPath(isolate, icon_path).get(); + if ((*native_image)->image().IsEmpty()) { +#if defined(OS_WIN) + const auto img_path = base::UTF16ToUTF8(icon_path.value()); +#else + const auto img_path = icon_path.value(); +#endif + isolate->ThrowException(v8::Exception::Error(gin::StringToV8( + isolate, "Failed to load image from path '" + img_path + "'"))); + return false; + } + } else { + if (!gin::ConvertFromV8(isolate, image, native_image)) { + isolate->ThrowException(v8::Exception::Error(gin::StringToV8( + isolate, "Argument must be a file path or a NativeImage"))); + return false; + } + } + return true; +} + #if defined(OS_WIN) HICON NativeImage::GetHICON(int size) { auto iter = hicons_.find(size); @@ -571,50 +599,6 @@ gin::WrapperInfo NativeImage::kWrapperInfo = {gin::kEmbedderNativeGin}; } // namespace electron -namespace gin { - -v8::Local Converter::ToV8( - v8::Isolate* isolate, - electron::api::NativeImage* val) { - v8::Local ret; - if (val && val->GetWrapper(isolate).ToLocal(&ret)) - return ret; - else - return v8::Null(isolate); -} - -bool Converter::FromV8( - v8::Isolate* isolate, - v8::Local val, - electron::api::NativeImage** out) { - // Try converting from file path. - base::FilePath path; - if (ConvertFromV8(isolate, val, &path)) { - *out = electron::api::NativeImage::CreateFromPath(isolate, path).get(); - if ((*out)->image().IsEmpty()) { -#if defined(OS_WIN) - const auto img_path = base::UTF16ToUTF8(path.value()); -#else - const auto img_path = path.value(); -#endif - isolate->ThrowException(v8::Exception::Error( - StringToV8(isolate, "Image could not be created from " + img_path))); - return false; - } - - return true; - } - - // reinterpret_cast is safe here because NativeImage is the only subclass of - // gin::Wrappable. - *out = static_cast( - static_cast(gin::internal::FromV8Impl( - isolate, val, &electron::api::NativeImage::kWrapperInfo))); - return *out != nullptr; -} - -} // namespace gin - namespace { using electron::api::NativeImage; diff --git a/shell/common/api/electron_api_native_image.h b/shell/common/api/electron_api_native_image.h index 5e0ec3e0933c..1c89fcdfa6d9 100644 --- a/shell/common/api/electron_api_native_image.h +++ b/shell/common/api/electron_api_native_image.h @@ -77,6 +77,10 @@ class NativeImage : public gin::Wrappable { static v8::Local GetConstructor(v8::Isolate* isolate); + static bool TryConvertNativeImage(v8::Isolate* isolate, + v8::Local image, + NativeImage** native_image); + // gin::Wrappable static gin::WrapperInfo kWrapperInfo; gin::ObjectTemplateBuilder GetObjectTemplateBuilder( @@ -133,18 +137,4 @@ class NativeImage : public gin::Wrappable { } // namespace electron -namespace gin { - -// A custom converter that allows converting path to NativeImage. -template <> -struct Converter { - static v8::Local ToV8(v8::Isolate* isolate, - electron::api::NativeImage* val); - static bool FromV8(v8::Isolate* isolate, - v8::Local val, - electron::api::NativeImage** out); -}; - -} // namespace gin - #endif // SHELL_COMMON_API_ELECTRON_API_NATIVE_IMAGE_H_ diff --git a/shell/common/gin_converters/image_converter.cc b/shell/common/gin_converters/image_converter.cc index a0b73e23c4c3..0241d7bbf245 100644 --- a/shell/common/gin_converters/image_converter.cc +++ b/shell/common/gin_converters/image_converter.cc @@ -27,9 +27,19 @@ bool Converter::FromV8(v8::Isolate* isolate, if (val->IsNull()) return true; - gin::Handle native_image; - if (!gin::ConvertFromV8(isolate, val, &native_image)) - return false; + // First see if the user has passed a path. + electron::api::NativeImage* native_image = nullptr; + base::FilePath icon_path; + if (gin::ConvertFromV8(isolate, val, &icon_path)) { + native_image = + electron::api::NativeImage::CreateFromPath(isolate, icon_path).get(); + if (native_image->image().IsEmpty()) + return false; + } else { + // Try a normal nativeImage if that fails. + if (!gin::ConvertFromV8(isolate, val, &native_image)) + return false; + } *out = native_image->image(); return true; diff --git a/spec-main/api-app-spec.ts b/spec-main/api-app-spec.ts index 10a54b64f2ad..672f298170c6 100644 --- a/spec-main/api-app-spec.ts +++ b/spec-main/api-app-spec.ts @@ -1541,6 +1541,15 @@ describe('app module', () => { }); }); + describe('dock.setIcon', () => { + it('throws a descriptive error for a bad icon path', () => { + const badPath = path.resolve('I', 'Do', 'Not', 'Exist'); + expect(() => { + app.dock.setIcon(badPath); + }).to.throw(/Failed to load image from path (.+)/); + }); + }); + describe('dock.bounce', () => { it('should return -1 for unknown bounce type', () => { expect(app.dock.bounce('bad type' as any)).to.equal(-1); diff --git a/spec-main/api-tray-spec.ts b/spec-main/api-tray-spec.ts index de158360ab75..1cabcee4284c 100644 --- a/spec-main/api-tray-spec.ts +++ b/spec-main/api-tray-spec.ts @@ -19,10 +19,10 @@ describe('tray module', () => { const badPath = path.resolve('I', 'Do', 'Not', 'Exist'); expect(() => { tray = new Tray(badPath); - }).to.throw(/Error processing argument at index 0/); + }).to.throw(/Failed to load image from path (.+)/); }); - ifit(process.platform === 'win32')('throws a descriptive error if an invlaid guid is given', () => { + ifit(process.platform === 'win32')('throws a descriptive error if an invalid guid is given', () => { expect(() => { tray = new Tray(nativeImage.createEmpty(), 'I am not a guid'); }).to.throw('Invalid GUID format'); @@ -132,17 +132,52 @@ describe('tray module', () => { }); describe('tray.setImage(image)', () => { + it('throws a descriptive error for a missing file', () => { + const badPath = path.resolve('I', 'Do', 'Not', 'Exist'); + expect(() => { + tray.setImage(badPath); + }).to.throw(/Failed to load image from path (.+)/); + }); + it('accepts empty image', () => { tray.setImage(nativeImage.createEmpty()); }); }); describe('tray.setPressedImage(image)', () => { + it('throws a descriptive error for a missing file', () => { + const badPath = path.resolve('I', 'Do', 'Not', 'Exist'); + expect(() => { + tray.setPressedImage(badPath); + }).to.throw(/Failed to load image from path (.+)/); + }); + it('accepts empty image', () => { tray.setPressedImage(nativeImage.createEmpty()); }); }); + ifdescribe(process.platform === 'win32')('tray.displayBalloon(image)', () => { + it('throws a descriptive error for a missing file', () => { + const badPath = path.resolve('I', 'Do', 'Not', 'Exist'); + expect(() => { + tray.displayBalloon({ + title: 'title', + content: 'wow content', + icon: badPath + }); + }).to.throw(/Failed to load image from path (.+)/); + }); + + it('accepts an empty image', () => { + tray.displayBalloon({ + title: 'title', + content: 'wow content', + icon: nativeImage.createEmpty() + }); + }); + }); + ifdescribe(process.platform === 'darwin')('tray get/set title', () => { it('sets/gets non-empty title', () => { const title = 'Hello World!'; diff --git a/spec-main/api-web-contents-spec.ts b/spec-main/api-web-contents-spec.ts index deb756f63cad..c1348af72e39 100644 --- a/spec-main/api-web-contents-spec.ts +++ b/spec-main/api-web-contents-spec.ts @@ -764,11 +764,11 @@ describe('webContents module', () => { expect(() => { w.webContents.startDrag({ file: __filename } as any); - }).to.throw('Must specify non-empty \'icon\' option'); + }).to.throw('\'icon\' parameter is required'); expect(() => { w.webContents.startDrag({ file: __filename, icon: path.join(mainFixturesPath, 'blank.png') }); - }).to.throw('Must specify non-empty \'icon\' option'); + }).to.throw(/Failed to load image from path (.+)/); }); });