refactor: remove path from nativeImage converter (#27181)

* refactor: remove path from nativeImage converter

* chore: address some review feedback

* Abstract out conversion

* Use converter for DockSetIcon

* Fix startDrag error

* Remove unnecessary FromV8 converter

* Allow DockSetIcon to take null

* Update shell/browser/browser_mac.mm

Co-authored-by: Jeremy Rose <[email protected]>

Co-authored-by: Shelley Vohr <[email protected]>
Co-authored-by: Jeremy Rose <[email protected]>

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<NativeImage> icon;
-    if (options.Get(options::kIcon, &icon) && !icon.IsEmpty())
-      SetIcon(icon);
-    if (try_catch.HasCaught())
-      LOG(ERROR) << "Failed to convert NativeImage";
+  v8::Local<v8::Value> icon;
+  if (options.Get(options::kIcon, &icon)) {
+    SetIcon(isolate, icon);
   }
 #endif
 }
@@ -998,14 +994,18 @@ bool BaseWindow::SetThumbarButtons(gin_helper::Arguments* args) {
 }
 
 #if defined(TOOLKIT_VIEWS)
-void BaseWindow::SetIcon(gin::Handle<NativeImage> icon) {
+void BaseWindow::SetIcon(v8::Isolate* isolate, v8::Local<v8::Value> icon) {
+  NativeImage* native_image = nullptr;
+  if (!NativeImage::TryConvertNativeImage(isolate, icon, &native_image))
+    return;
+
 #if defined(OS_WIN)
   static_cast<NativeWindowViews*>(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<NativeWindowViews*>(window_.get())
-      ->SetIcon(icon->image().AsImageSkia());
+      ->SetIcon(native_image->image().AsImageSkia());
 #endif
 }
 #endif

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<BaseWindow>,
   // Extra APIs added in JS.
   bool SetThumbarButtons(gin_helper::Arguments* args);
 #if defined(TOOLKIT_VIEWS)
-  void SetIcon(gin::Handle<NativeImage> icon);
+  void SetIcon(v8::Isolate* isolate, v8::Local<v8::Value> icon);
 #endif
 #if defined(OS_WIN)
   typedef base::RepeatingCallback<void(v8::Local<v8::Value>,

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<NativeImage> image,
-           base::Optional<UUID> guid,
-           gin::Arguments* args)
+Tray::Tray(v8::Isolate* isolate,
+           v8::Local<v8::Value> image,
+           base::Optional<UUID> 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> Tray::New(gin_helper::ErrorThrower thrower,
-                            gin::Handle<NativeImage> image,
+                            v8::Local<v8::Value> image,
                             base::Optional<UUID> guid,
                             gin::Arguments* args) {
   if (!Browser::Get()->is_ready()) {
@@ -88,7 +89,8 @@ gin::Handle<Tray> 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<NativeImage> image) {
+void Tray::SetImage(v8::Isolate* isolate, v8::Local<v8::Value> 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<NativeImage> image) {
+void Tray::SetPressedImage(v8::Isolate* isolate, v8::Local<v8::Value> 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<NativeImage> icon;
-  options.Get("icon", &icon);
+  v8::Local<v8::Value> 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));

shell/browser/api/electron_api_tray.h

@@ -43,7 +43,7 @@ class Tray : public gin::Wrappable<Tray>,
  public:
   // gin_helper::Constructible
   static gin::Handle<Tray> New(gin_helper::ErrorThrower thrower,
-                               gin::Handle<NativeImage> image,
+                               v8::Local<v8::Value> image,
                                base::Optional<UUID> guid,
                                gin::Arguments* args);
   static v8::Local<v8::ObjectTemplate> FillObjectTemplate(
@@ -54,9 +54,9 @@ class Tray : public gin::Wrappable<Tray>,
   static gin::WrapperInfo kWrapperInfo;
 
  private:
-  Tray(gin::Handle<NativeImage> image,
-       base::Optional<UUID> guid,
-       gin::Arguments* args);
+  Tray(v8::Isolate* isolate,
+       v8::Local<v8::Value> image,
+       base::Optional<UUID> guid);
   ~Tray() override;
 
   // TrayIconObserver:
@@ -83,8 +83,8 @@ class Tray : public gin::Wrappable<Tray>,
   // JS API:
   void Destroy();
   bool IsDestroyed();
-  void SetImage(gin::Handle<NativeImage> image);
-  void SetPressedImage(gin::Handle<NativeImage> image);
+  void SetImage(v8::Isolate* isolate, v8::Local<v8::Value> image);
+  void SetPressedImage(v8::Isolate* isolate, v8::Local<v8::Value> image);
   void SetToolTip(const std::string& tool_tip);
   void SetTitle(const std::string& title,
                 const base::Optional<gin_helper::Dictionary>& options,

shell/browser/api/electron_api_web_contents.cc

@@ -2859,10 +2859,16 @@ void WebContents::StartDrag(const gin_helper::Dictionary& item,
     files.push_back(file);
   }
 
-  gin::Handle<NativeImage> icon;
-  if (!item.Get("icon", &icon) || icon->image().IsEmpty()) {
+  v8::Local<v8::Value> 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;
   }
 

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<v8::Value> icon);
 
 #endif  // defined(OS_MAC)
 

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"
@@ -451,7 +452,16 @@ void Browser::DockSetMenu(ElectronMenuModel* model) {
   [delegate setApplicationDockMenu:model];
 }
 
-void Browser::DockSetIcon(const gfx::Image& image) {
+void Browser::DockSetIcon(v8::Isolate* isolate, v8::Local<v8::Value> 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()];
 }

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<v8::Value> 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<v8::Value> Converter<electron::api::NativeImage*>::ToV8(
-    v8::Isolate* isolate,
-    electron::api::NativeImage* val) {
-  v8::Local<v8::Object> ret;
-  if (val && val->GetWrapper(isolate).ToLocal(&ret))
-    return ret;
-  else
-    return v8::Null(isolate);
-}
-
-bool Converter<electron::api::NativeImage*>::FromV8(
-    v8::Isolate* isolate,
-    v8::Local<v8::Value> 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<NativeImage>.
-  *out = static_cast<electron::api::NativeImage*>(
-      static_cast<gin::WrappableBase*>(gin::internal::FromV8Impl(
-          isolate, val, &electron::api::NativeImage::kWrapperInfo)));
-  return *out != nullptr;
-}
-
-}  // namespace gin
-
 namespace {
 
 using electron::api::NativeImage;

shell/common/api/electron_api_native_image.h

@@ -77,6 +77,10 @@ class NativeImage : public gin::Wrappable<NativeImage> {
 
   static v8::Local<v8::FunctionTemplate> GetConstructor(v8::Isolate* isolate);
 
+  static bool TryConvertNativeImage(v8::Isolate* isolate,
+                                    v8::Local<v8::Value> image,
+                                    NativeImage** native_image);
+
   // gin::Wrappable
   static gin::WrapperInfo kWrapperInfo;
   gin::ObjectTemplateBuilder GetObjectTemplateBuilder(
@@ -133,18 +137,4 @@ class NativeImage : public gin::Wrappable<NativeImage> {
 
 }  // namespace electron
 
-namespace gin {
-
-// A custom converter that allows converting path to NativeImage.
-template <>
-struct Converter<electron::api::NativeImage*> {
-  static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,
-                                   electron::api::NativeImage* val);
-  static bool FromV8(v8::Isolate* isolate,
-                     v8::Local<v8::Value> val,
-                     electron::api::NativeImage** out);
-};
-
-}  // namespace gin
-
 #endif  // SHELL_COMMON_API_ELECTRON_API_NATIVE_IMAGE_H_

shell/common/gin_converters/image_converter.cc

@@ -27,9 +27,19 @@ bool Converter<gfx::Image>::FromV8(v8::Isolate* isolate,
   if (val->IsNull())
     return true;
 
-  gin::Handle<electron::api::NativeImage> 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;

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);

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!';

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 (.+)/);
     });
   });