refactor: ginify Menu (#22916)

lib/browser/api/menu.js

@@ -10,43 +10,51 @@ const { Menu } = bindings;
 let applicationMenu = null;
 let groupIdIndex = 0;
 
-Object.setPrototypeOf(Menu.prototype, EventEmitter.prototype);
-
-// Menu Delegate.
-// This object should hold no reference to |Menu| to avoid cyclic reference.
-const delegate = {
-  isCommandIdChecked: (menu, id) => menu.commandsMap[id] ? menu.commandsMap[id].checked : undefined,
-  isCommandIdEnabled: (menu, id) => menu.commandsMap[id] ? menu.commandsMap[id].enabled : undefined,
-  shouldCommandIdWorkWhenHidden: (menu, id) => menu.commandsMap[id] ? menu.commandsMap[id].acceleratorWorksWhenHidden : undefined,
-  isCommandIdVisible: (menu, id) => menu.commandsMap[id] ? menu.commandsMap[id].visible : undefined,
-  getAcceleratorForCommandId: (menu, id, useDefaultAccelerator) => {
-    const command = menu.commandsMap[id];
-    if (!command) return;
-    if (command.accelerator != null) return command.accelerator;
-    if (useDefaultAccelerator) return command.getDefaultRoleAccelerator();
-  },
-  shouldRegisterAcceleratorForCommandId: (menu, id) => menu.commandsMap[id] ? menu.commandsMap[id].registerAccelerator : undefined,
-  executeCommand: (menu, event, id) => {
-    const command = menu.commandsMap[id];
-    if (!command) return;
-    command.click(event, TopLevelWindow.getFocusedWindow(), webContents.getFocusedWebContents());
-  },
-  menuWillShow: (menu) => {
-    // Ensure radio groups have at least one menu item selected
-    for (const id of Object.keys(menu.groupsMap)) {
-      const found = menu.groupsMap[id].find(item => item.checked) || null;
-      if (!found) v8Util.setHiddenValue(menu.groupsMap[id][0], 'checked', true);
-    }
-  }
-};
-
 /* Instance Methods */
 
 Menu.prototype._init = function () {
   this.commandsMap = {};
   this.groupsMap = {};
   this.items = [];
-  this.delegate = delegate;
+};
+
+Menu.prototype._isCommandIdChecked = function (id) {
+  return this.commandsMap[id] ? this.commandsMap[id].checked : undefined;
+};
+
+Menu.prototype._isCommandIdEnabled = function (id) {
+  return this.commandsMap[id] ? this.commandsMap[id].enabled : undefined;
+};
+Menu.prototype._shouldCommandIdWorkWhenHidden = function (id) {
+  return this.commandsMap[id] ? this.commandsMap[id].acceleratorWorksWhenHidden : undefined;
+};
+Menu.prototype._isCommandIdVisible = function (id) {
+  return this.commandsMap[id] ? this.commandsMap[id].visible : undefined;
+};
+
+Menu.prototype._getAcceleratorForCommandId = function (id, useDefaultAccelerator) {
+  const command = this.commandsMap[id];
+  if (!command) return;
+  if (command.accelerator != null) return command.accelerator;
+  if (useDefaultAccelerator) return command.getDefaultRoleAccelerator();
+};
+
+Menu.prototype._shouldRegisterAcceleratorForCommandId = function (id) {
+  return this.commandsMap[id] ? this.commandsMap[id].registerAccelerator : undefined;
+};
+
+Menu.prototype._executeCommand = function (event, id) {
+  const command = this.commandsMap[id];
+  if (!command) return;
+  command.click(event, TopLevelWindow.getFocusedWindow(), webContents.getFocusedWebContents());
+};
+
+Menu.prototype._menuWillShow = function () {
+  // Ensure radio groups have at least one menu item selected
+  for (const id of Object.keys(this.groupsMap)) {
+    const found = this.groupsMap[id].find(item => item.checked) || null;
+    if (!found) v8Util.setHiddenValue(this.groupsMap[id][0], 'checked', true);
+  }
 };
 
 Menu.prototype.popup = function (options = {}) {

patches/chromium/.patches

@@ -93,4 +93,5 @@ gpu_notify_when_dxdiag_request_fails.patch
 feat_allow_embedders_to_add_observers_on_created_hunspell.patch
 feat_add_onclose_to_messageport.patch
 gin_allow_passing_an_objecttemplate_to_objecttemplatebuilder.patch
+gin_forward_args_when_dispatching.patch
 fix_undo_redo_broken_in_webviews.patch

patches/chromium/gin_forward_args_when_dispatching.patch

@@ -0,0 +1,94 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Jeremy Apthorp <[email protected]>
+Date: Wed, 1 Apr 2020 01:06:45 +0000
+Subject: gin: forward args when dispatching
+
+This allows passing arguments with move-only semantics.
+
+Change-Id: I852eb343398e6f763abfe2a44e623f28353d79a5
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2129029
+Commit-Queue: Jeremy Apthorp <[email protected]>
+Reviewed-by: Jeremy Roman <[email protected]>
+Auto-Submit: Jeremy Apthorp <[email protected]>
+Cr-Commit-Position: refs/heads/master@{#755196}
+
+diff --git a/gin/converter_unittest.cc b/gin/converter_unittest.cc
+index 6f9205f06a3e4747e70149c440c4548e54045001..162572385bdd8a54612eb64414fe80e0f9d619d1 100644
+--- a/gin/converter_unittest.cc
++++ b/gin/converter_unittest.cc
+@@ -12,6 +12,7 @@
+ #include "base/stl_util.h"
+ #include "base/strings/string16.h"
+ #include "base/strings/utf_string_conversions.h"
++#include "gin/function_template.h"
+ #include "gin/handle.h"
+ #include "gin/public/isolate_holder.h"
+ #include "gin/test/v8_test.h"
+@@ -224,4 +225,45 @@ TEST_F(ConverterTest, VectorOfWrappables) {
+   EXPECT_THAT(out_value2, testing::ContainerEq(vector));
+ }
+ 
++namespace {
++
++class MoveOnlyObject {
++ public:
++  MoveOnlyObject() = default;
++  MoveOnlyObject(const MoveOnlyObject&) = delete;
++  MoveOnlyObject& operator=(const MoveOnlyObject&) = delete;
++
++  MoveOnlyObject(MoveOnlyObject&&) noexcept = default;
++  MoveOnlyObject& operator=(MoveOnlyObject&&) noexcept = default;
++};
++
++}  // namespace
++
++template <>
++struct Converter<MoveOnlyObject> {
++  static v8::Local<v8::Value> ToV8(v8::Isolate* isolate, MoveOnlyObject in) {
++    return v8::Undefined(isolate);
++  }
++  static bool FromV8(v8::Isolate* isolate,
++                     v8::Local<v8::Value> val,
++                     MoveOnlyObject* out) {
++    *out = MoveOnlyObject();
++    return true;
++  }
++};
++
++TEST_F(ConverterTest, MoveOnlyParameters) {
++  v8::Isolate* isolate = instance_->isolate();
++  v8::HandleScope handle_scope(isolate);
++
++  auto receives_move_only_obj = [](MoveOnlyObject obj) {};
++  auto func_templ = gin::CreateFunctionTemplate(
++      isolate, base::BindRepeating(receives_move_only_obj));
++
++  v8::Local<v8::Context> context = instance_->isolate()->GetCurrentContext();
++  auto func = func_templ->GetFunction(context).ToLocalChecked();
++  v8::Local<v8::Value> argv[] = {v8::Undefined(isolate)};
++  func->Call(context, v8::Undefined(isolate), 1, argv).ToLocalChecked();
++}
++
+ }  // namespace gin
+diff --git a/gin/function_template.h b/gin/function_template.h
+index 7edcc9e20dfa6367dde5f58237cca5f4ca637f7a..8c641d934fdeebb9a90f6eb49960e4fe06217913 100644
+--- a/gin/function_template.h
++++ b/gin/function_template.h
+@@ -166,14 +166,15 @@ class Invoker<std::index_sequence<indices...>, ArgTypes...>
+   template <typename ReturnType>
+   void DispatchToCallback(
+       base::RepeatingCallback<ReturnType(ArgTypes...)> callback) {
+-    args_->Return(callback.Run(ArgumentHolder<indices, ArgTypes>::value...));
++    args_->Return(
++        callback.Run(std::move(ArgumentHolder<indices, ArgTypes>::value)...));
+   }
+ 
+   // In C++, you can declare the function foo(void), but you can't pass a void
+   // expression to foo. As a result, we must specialize the case of Callbacks
+   // that have the void return type.
+   void DispatchToCallback(base::RepeatingCallback<void(ArgTypes...)> callback) {
+-    callback.Run(ArgumentHolder<indices, ArgTypes>::value...);
++    callback.Run(std::move(ArgumentHolder<indices, ArgTypes>::value)...);
+   }
+ 
+  private:

shell/browser/api/electron_api_desktop_capturer.cc

@@ -155,6 +155,9 @@ void DesktopCapturer::UpdateSourcesList(DesktopMediaList* list) {
       // |media_list_sources|.
       if (!webrtc::DxgiDuplicatorController::Instance()->GetDeviceNames(
               &device_names)) {
+        v8::Isolate* isolate = v8::Isolate::GetCurrent();
+        v8::Locker locker(isolate);
+        v8::HandleScope scope(isolate);
         gin_helper::CallMethod(this, "_onerror", "Failed to get sources.");
         return;
       }
@@ -184,9 +187,13 @@ void DesktopCapturer::UpdateSourcesList(DesktopMediaList* list) {
               std::back_inserter(captured_sources_));
   }
 
-  if (!capture_window_ && !capture_screen_)
+  if (!capture_window_ && !capture_screen_) {
+    v8::Isolate* isolate = v8::Isolate::GetCurrent();
+    v8::Locker locker(isolate);
+    v8::HandleScope scope(isolate);
     gin_helper::CallMethod(this, "_onfinished", captured_sources_,
                            fetch_window_icons_);
+  }
 }
 
 // static

shell/browser/api/electron_api_menu.cc

@@ -16,21 +16,13 @@
 #include "shell/common/gin_helper/object_template_builder.h"
 #include "shell/common/node_includes.h"
 
-namespace {
-
-// We need this map to keep references to currently opened menus.
-// Without this menus would be destroyed by js garbage collector
-// even when they are still displayed.
-std::map<uint32_t, v8::Global<v8::Object>> g_menus;
-
-}  // unnamed namespace
-
 namespace electron {
 
 namespace api {
 
+gin::WrapperInfo Menu::kWrapperInfo = {gin::kEmbedderNativeGin};
+
 Menu::Menu(gin::Arguments* args) : model_(new ElectronMenuModel(this)) {
-  InitWithArgs(args);
   model_->AddObserver(this);
 }
 
@@ -40,74 +32,82 @@ Menu::~Menu() {
   }
 }
 
-void Menu::AfterInit(v8::Isolate* isolate) {
-  gin::Dictionary wrappable(isolate, GetWrapper());
-  gin::Dictionary delegate(nullptr);
-  if (!wrappable.Get("delegate", &delegate))
-    return;
-
-  delegate.Get("isCommandIdChecked", &is_checked_);
-  delegate.Get("isCommandIdEnabled", &is_enabled_);
-  delegate.Get("isCommandIdVisible", &is_visible_);
-  delegate.Get("shouldCommandIdWorkWhenHidden", &works_when_hidden_);
-  delegate.Get("getAcceleratorForCommandId", &get_accelerator_);
-  delegate.Get("shouldRegisterAcceleratorForCommandId",
-               &should_register_accelerator_);
-  delegate.Get("executeCommand", &execute_command_);
-  delegate.Get("menuWillShow", &menu_will_show_);
+bool InvokeBoolMethod(const Menu* menu,
+                      const char* method,
+                      int command_id,
+                      bool default_value = false) {
+  v8::Isolate* isolate = v8::Isolate::GetCurrent();
+  v8::HandleScope scope(isolate);
+  // We need to cast off const here because GetWrapper() is non-const, but
+  // ui::SimpleMenuModel::Delegate's methods are const.
+  v8::Local<v8::Value> val = gin_helper::CallMethod(
+      isolate, const_cast<Menu*>(menu), method, command_id);
+  bool ret = false;
+  return gin::ConvertFromV8(isolate, val, &ret) ? ret : default_value;
 }
 
 bool Menu::IsCommandIdChecked(int command_id) const {
-  v8::Locker locker(isolate());
-  v8::HandleScope handle_scope(isolate());
-  return is_checked_.Run(GetWrapper(), command_id);
+  return InvokeBoolMethod(this, "_isCommandIdChecked", command_id);
 }
 
 bool Menu::IsCommandIdEnabled(int command_id) const {
-  v8::Locker locker(isolate());
-  v8::HandleScope handle_scope(isolate());
-  return is_enabled_.Run(GetWrapper(), command_id);
+  return InvokeBoolMethod(this, "_isCommandIdEnabled", command_id);
 }
 
 bool Menu::IsCommandIdVisible(int command_id) const {
-  v8::Locker locker(isolate());
-  v8::HandleScope handle_scope(isolate());
-  return is_visible_.Run(GetWrapper(), command_id);
+  return InvokeBoolMethod(this, "_isCommandIdVisible", command_id);
 }
 
 bool Menu::ShouldCommandIdWorkWhenHidden(int command_id) const {
-  v8::Locker locker(isolate());
-  v8::HandleScope handle_scope(isolate());
-  return works_when_hidden_.Run(GetWrapper(), command_id);
+  return InvokeBoolMethod(this, "_shouldCommandIdWorkWhenHidden", command_id);
 }
 
 bool Menu::GetAcceleratorForCommandIdWithParams(
     int command_id,
     bool use_default_accelerator,
     ui::Accelerator* accelerator) const {
-  v8::Locker locker(isolate());
-  v8::HandleScope handle_scope(isolate());
-  v8::Local<v8::Value> val =
-      get_accelerator_.Run(GetWrapper(), command_id, use_default_accelerator);
-  return gin::ConvertFromV8(isolate(), val, accelerator);
+  v8::Isolate* isolate = v8::Isolate::GetCurrent();
+  v8::HandleScope scope(isolate);
+  v8::Local<v8::Value> val = gin_helper::CallMethod(
+      isolate, const_cast<Menu*>(this), "_getAcceleratorForCommandId",
+      command_id, use_default_accelerator);
+  return gin::ConvertFromV8(isolate, val, accelerator);
 }
 
 bool Menu::ShouldRegisterAcceleratorForCommandId(int command_id) const {
-  v8::Locker locker(isolate());
-  v8::HandleScope handle_scope(isolate());
-  return should_register_accelerator_.Run(GetWrapper(), command_id);
+  return InvokeBoolMethod(this, "_shouldRegisterAcceleratorForCommandId",
+                          command_id);
 }
 
 void Menu::ExecuteCommand(int command_id, int flags) {
-  v8::Locker locker(isolate());
-  v8::HandleScope handle_scope(isolate());
-  execute_command_.Run(GetWrapper(), CreateEventFromFlags(flags), command_id);
+  v8::Isolate* isolate = v8::Isolate::GetCurrent();
+  v8::HandleScope scope(isolate);
+  gin_helper::CallMethod(isolate, const_cast<Menu*>(this), "_executeCommand",
+                         CreateEventFromFlags(flags), command_id);
 }
 
 void Menu::OnMenuWillShow(ui::SimpleMenuModel* source) {
-  v8::Locker locker(isolate());
-  v8::HandleScope handle_scope(isolate());
-  menu_will_show_.Run(GetWrapper());
+  v8::Isolate* isolate = v8::Isolate::GetCurrent();
+  v8::HandleScope scope(isolate);
+  gin_helper::CallMethod(isolate, const_cast<Menu*>(this), "_menuWillShow");
+}
+
+base::OnceClosure Menu::BindSelfToClosure(base::OnceClosure callback) {
+  // return ((callback, ref) => { callback() }).bind(null, callback, this)
+  v8::Isolate* isolate = v8::Isolate::GetCurrent();
+  v8::Locker locker(isolate);
+  v8::HandleScope scope(isolate);
+  v8::Local<v8::Object> self;
+  if (GetWrapper(isolate).ToLocal(&self)) {
+    v8::Global<v8::Value> ref(isolate, self);
+    return base::BindOnce(
+        [](base::OnceClosure callback, v8::Global<v8::Value> ref) {
+          std::move(callback).Run();
+        },
+        std::move(callback), std::move(ref));
+  } else {
+    return base::DoNothing();
+  }
 }
 
 void Menu::InsertItemAt(int index,
@@ -208,32 +208,20 @@ bool Menu::WorksWhenHiddenAt(int index) const {
 }
 
 void Menu::OnMenuWillClose() {
-  g_menus.erase(weak_map_id());
+  Unpin();
   Emit("menu-will-close");
 }
 
 void Menu::OnMenuWillShow() {
-  v8::HandleScope scope(isolate());
-  g_menus[weak_map_id()] = v8::Global<v8::Object>(isolate(), GetWrapper());
+  Pin(v8::Isolate::GetCurrent());
   Emit("menu-will-show");
 }
 
-base::OnceClosure Menu::BindSelfToClosure(base::OnceClosure callback) {
-  // return ((callback, ref) => { callback() }).bind(null, callback, this)
-  v8::Global<v8::Value> ref(isolate(), GetWrapper());
-  return base::BindOnce(
-      [](base::OnceClosure callback, v8::Global<v8::Value> ref) {
-        std::move(callback).Run();
-      },
-      std::move(callback), std::move(ref));
-}
-
 // static
-void Menu::BuildPrototype(v8::Isolate* isolate,
-                          v8::Local<v8::FunctionTemplate> prototype) {
-  prototype->SetClassName(gin::StringToV8(isolate, "Menu"));
-  gin_helper::Destroyable::MakeDestroyable(isolate, prototype);
-  gin_helper::ObjectTemplateBuilder(isolate, prototype->PrototypeTemplate())
+v8::Local<v8::ObjectTemplate> Menu::FillObjectTemplate(
+    v8::Isolate* isolate,
+    v8::Local<v8::ObjectTemplate> templ) {
+  return gin::ObjectTemplateBuilder(isolate, "Menu", templ)
       .SetMethod("insertItem", &Menu::InsertItemAt)
       .SetMethod("insertCheckItem", &Menu::InsertCheckItemAt)
       .SetMethod("insertRadioItem", &Menu::InsertRadioItemAt)
@@ -256,7 +244,8 @@ void Menu::BuildPrototype(v8::Isolate* isolate,
       .SetMethod("worksWhenHiddenAt", &Menu::WorksWhenHiddenAt)
       .SetMethod("isVisibleAt", &Menu::IsVisibleAt)
       .SetMethod("popupAt", &Menu::PopupAt)
-      .SetMethod("closePopupAt", &Menu::ClosePopupAt);
+      .SetMethod("closePopupAt", &Menu::ClosePopupAt)
+      .Build();
 }
 
 }  // namespace api
@@ -272,12 +261,9 @@ void Initialize(v8::Local<v8::Object> exports,
                 v8::Local<v8::Context> context,
                 void* priv) {
   v8::Isolate* isolate = context->GetIsolate();
-  Menu::SetConstructor(isolate, base::BindRepeating(&Menu::New));
 
   gin_helper::Dictionary dict(isolate, exports);
-  dict.Set(
-      "Menu",
-      Menu::GetConstructor(isolate)->GetFunction(context).ToLocalChecked());
+  dict.Set("Menu", Menu::GetConstructor(context));
 #if defined(OS_MACOSX)
   dict.SetMethod("setApplicationMenu", &Menu::SetApplicationMenu);
   dict.SetMethod("sendActionToFirstResponder",

shell/browser/api/electron_api_menu.h

@@ -11,21 +11,30 @@
 #include "base/callback.h"
 #include "gin/arguments.h"
 #include "shell/browser/api/electron_api_top_level_window.h"
+#include "shell/browser/event_emitter_mixin.h"
 #include "shell/browser/ui/electron_menu_model.h"
-#include "shell/common/gin_helper/trackable_object.h"
+#include "shell/common/gin_helper/constructible.h"
+#include "shell/common/gin_helper/pinnable.h"
 
 namespace electron {
 
 namespace api {
 
-class Menu : public gin_helper::TrackableObject<Menu>,
+class Menu : public gin::Wrappable<Menu>,
+             public gin_helper::EventEmitterMixin<Menu>,
+             public gin_helper::Constructible<Menu>,
+             public gin_helper::Pinnable<Menu>,
              public ElectronMenuModel::Delegate,
              public ElectronMenuModel::Observer {
  public:
-  static gin_helper::WrappableBase* New(gin::Arguments* args);
+  // gin_helper::Constructible
+  static gin::Handle<Menu> New(gin::Arguments* args);
+  static v8::Local<v8::ObjectTemplate> FillObjectTemplate(
+      v8::Isolate*,
+      v8::Local<v8::ObjectTemplate>);
 
-  static void BuildPrototype(v8::Isolate* isolate,
-                             v8::Local<v8::FunctionTemplate> prototype);
+  // gin::Wrappable
+  static gin::WrapperInfo kWrapperInfo;
 
 #if defined(OS_MACOSX)
   // Set the global menubar.
@@ -41,8 +50,9 @@ class Menu : public gin_helper::TrackableObject<Menu>,
   explicit Menu(gin::Arguments* args);
   ~Menu() override;
 
-  // gin_helper::Wrappable:
-  void AfterInit(v8::Isolate* isolate) override;
+  // Returns a new callback which keeps references of the JS wrapper until the
+  // passed |callback| is called.
+  base::OnceClosure BindSelfToClosure(base::OnceClosure callback);
 
   // ui::SimpleMenuModel::Delegate:
   bool IsCommandIdChecked(int command_id) const override;
@@ -71,11 +81,6 @@ class Menu : public gin_helper::TrackableObject<Menu>,
   void OnMenuWillClose() override;
   void OnMenuWillShow() override;
 
- protected:
-  // Returns a new callback which keeps references of the JS wrapper until the
-  // passed |callback| is called.
-  base::OnceClosure BindSelfToClosure(base::OnceClosure callback);
-
  private:
   void InsertItemAt(int index, int command_id, const base::string16& label);
   void InsertSeparatorAt(int index);
@@ -107,19 +112,6 @@ class Menu : public gin_helper::TrackableObject<Menu>,
   bool IsVisibleAt(int index) const;
   bool WorksWhenHiddenAt(int index) const;
 
-  // Stored delegate methods.
-  base::RepeatingCallback<bool(v8::Local<v8::Value>, int)> is_checked_;
-  base::RepeatingCallback<bool(v8::Local<v8::Value>, int)> is_enabled_;
-  base::RepeatingCallback<bool(v8::Local<v8::Value>, int)> is_visible_;
-  base::RepeatingCallback<bool(v8::Local<v8::Value>, int)> works_when_hidden_;
-  base::RepeatingCallback<v8::Local<v8::Value>(v8::Local<v8::Value>, int, bool)>
-      get_accelerator_;
-  base::RepeatingCallback<bool(v8::Local<v8::Value>, int)>
-      should_register_accelerator_;
-  base::RepeatingCallback<void(v8::Local<v8::Value>, v8::Local<v8::Value>, int)>
-      execute_command_;
-  base::RepeatingCallback<void(v8::Local<v8::Value>)> menu_will_show_;
-
   DISALLOW_COPY_AND_ASSIGN(Menu);
 };
 

shell/browser/api/electron_api_menu_mac.mm

@@ -177,8 +177,11 @@ void Menu::SendActionToFirstResponder(const std::string& action) {
 }
 
 // static
-gin_helper::WrappableBase* Menu::New(gin::Arguments* args) {
-  return new MenuMac(args);
+gin::Handle<Menu> Menu::New(gin::Arguments* args) {
+  auto handle =
+      gin::CreateHandle(args->isolate(), static_cast<Menu*>(new MenuMac(args)));
+  gin_helper::CallMethod(args->isolate(), handle.get(), "_init");
+  return handle;
 }
 
 }  // namespace api

shell/browser/api/electron_api_menu_views.cc

@@ -84,8 +84,11 @@ void MenuViews::OnClosed(int32_t window_id, base::OnceClosure callback) {
 }
 
 // static
-gin_helper::WrappableBase* Menu::New(gin::Arguments* args) {
-  return new MenuViews(args);
+gin::Handle<Menu> Menu::New(gin::Arguments* args) {
+  auto handle = gin::CreateHandle(args->isolate(),
+                                  static_cast<Menu*>(new MenuViews(args)));
+  gin_helper::CallMethod(args->isolate(), handle.get(), "_init");
+  return handle;
 }
 
 }  // namespace api

shell/browser/api/electron_api_top_level_window.cc

@@ -687,7 +687,6 @@ void TopLevelWindow::SetMenu(v8::Isolate* isolate, v8::Local<v8::Value> value) {
   gin::Handle<Menu> menu;
   v8::Local<v8::Object> object;
   if (value->IsObject() && value->ToObject(context).ToLocal(&object) &&
-      gin::V8ToString(isolate, object->GetConstructorName()) == "Menu" &&
       gin::ConvertFromV8(isolate, value, &menu) && !menu.IsEmpty()) {
     menu_.Reset(isolate, menu.ToV8());
     window_->SetMenu(menu->model());

shell/common/gin_helper/event_emitter_caller.cc

@@ -21,7 +21,7 @@ v8::Local<v8::Value> CallMethodWithArgs(v8::Isolate* isolate,
   // Use node::MakeCallback to call the callback, and it will also run pending
   // tasks in Node.js.
   v8::MaybeLocal<v8::Value> ret = node::MakeCallback(
-      isolate, obj, method, args->size(), &args->front(), {0, 0});
+      isolate, obj, method, args->size(), args->data(), {0, 0});
   // If the JS function throws an exception (doesn't return a value) the result
   // of MakeCallback will be empty and therefore ToLocal will be false, in this
   // case we need to return "false" as that indicates that the event emitter did

shell/common/gin_helper/event_emitter_caller.h

@@ -69,11 +69,11 @@ v8::Local<v8::Value> CallMethod(v8::Isolate* isolate,
                                 gin::Wrappable<T>* object,
                                 const char* method_name,
                                 Args&&... args) {
-  v8::HandleScope scope(isolate);
+  v8::EscapableHandleScope scope(isolate);
   v8::Local<v8::Object> v8_object;
   if (object->GetWrapper(isolate).ToLocal(&v8_object))
-    return CustomEmit(isolate, v8_object, method_name,
-                      std::forward<Args>(args)...);
+    return scope.Escape(CustomEmit(isolate, v8_object, method_name,
+                                   std::forward<Args>(args)...));
   else
     return v8::Local<v8::Value>();
 }

shell/common/gin_helper/wrappable.h

@@ -25,7 +25,7 @@ class Wrappable : public WrappableBase {
   template <typename Sig>
   static void SetConstructor(v8::Isolate* isolate,
                              const base::Callback<Sig>& constructor) {
-    v8::Local<v8::FunctionTemplate> templ = CreateFunctionTemplate(
+    v8::Local<v8::FunctionTemplate> templ = gin_helper::CreateFunctionTemplate(
         isolate, base::Bind(&internal::InvokeNew<Sig>, constructor));
     templ->InstanceTemplate()->SetInternalFieldCount(1);
     T::BuildPrototype(isolate, templ);

spec-main/ambient.d.ts

@@ -2,10 +2,8 @@ declare let standardScheme: string;
 
 declare namespace Electron {
   interface Menu {
-    delegate: {
-      executeCommand(menu: Menu, event: any, id: number): void;
-      menuWillShow(menu: Menu): void;
-    };
+    _executeCommand(event: any, id: number): void;
+    _menuWillShow(): void;
     getAcceleratorTextAt(index: number): string;
   }
 

spec-main/api-menu-item-spec.ts

@@ -48,7 +48,7 @@ describe('MenuItems', () => {
           done();
         }
       }]);
-      menu.delegate.executeCommand(menu, {}, menu.items[0].commandId);
+      menu._executeCommand({}, menu.items[0].commandId);
     });
   });
 
@@ -60,7 +60,7 @@ describe('MenuItems', () => {
       }]);
 
       expect(menu.items[0].checked).to.be.false('menu item checked');
-      menu.delegate.executeCommand(menu, {}, menu.items[0].commandId);
+      menu._executeCommand({}, menu.items[0].commandId);
       expect(menu.items[0].checked).to.be.true('menu item checked');
     });
 
@@ -70,9 +70,9 @@ describe('MenuItems', () => {
         type: 'radio'
       }]);
 
-      menu.delegate.executeCommand(menu, {}, menu.items[0].commandId);
+      menu._executeCommand({}, menu.items[0].commandId);
       expect(menu.items[0].checked).to.be.true('menu item checked');
-      menu.delegate.executeCommand(menu, {}, menu.items[0].commandId);
+      menu._executeCommand({}, menu.items[0].commandId);
       expect(menu.items[0].checked).to.be.true('menu item checked');
     });
 
@@ -123,7 +123,7 @@ describe('MenuItems', () => {
 
       it('at least have one item checked in each group', () => {
         const menu = Menu.buildFromTemplate(template);
-        menu.delegate.menuWillShow(menu);
+        menu._menuWillShow();
 
         const groups = findRadioGroups(template);