refactor: add `EmitWarning(v8::Isolate*)` helper (#43722)

* refactor: add EmitWarning(Isolate*, ...) warning

* chore: remove EmitWarning(node::Environment*, ...)

* chore: add code comments

* fixup! refactor: add EmitWarning(Isolate*, ...) warning

* chore: remove unused node #includes

shell/browser/api/electron_api_debugger.cc

@@ -18,7 +18,6 @@
 #include "shell/browser/javascript_environment.h"
 #include "shell/common/gin_converters/value_converter.h"
 #include "shell/common/gin_helper/promise.h"
-#include "shell/common/node_includes.h"
 
 using content::DevToolsAgentHost;
 

shell/browser/api/electron_api_download_item.cc

@@ -15,7 +15,6 @@
 #include "shell/common/gin_converters/gurl_converter.h"
 #include "shell/common/gin_helper/dictionary.h"
 #include "shell/common/gin_helper/object_template_builder.h"
-#include "shell/common/node_includes.h"
 #include "url/gurl.h"
 
 namespace gin {

shell/browser/api/electron_api_net_log.cc

@@ -22,7 +22,6 @@
 #include "shell/browser/net/system_network_context_manager.h"
 #include "shell/common/gin_converters/file_path_converter.h"
 #include "shell/common/gin_helper/dictionary.h"
-#include "shell/common/node_includes.h"
 
 namespace gin {
 

shell/browser/api/electron_api_protocol.cc

@@ -21,8 +21,8 @@
 #include "shell/common/gin_helper/object_template_builder.h"
 #include "shell/common/gin_helper/promise.h"
 #include "shell/common/node_includes.h"
+#include "shell/common/node_util.h"
 #include "shell/common/options_switches.h"
-#include "shell/common/process_util.h"
 #include "url/url_util.h"
 
 namespace {
@@ -256,12 +256,11 @@ bool Protocol::IsProtocolIntercepted(const std::string& scheme) {
 
 v8::Local<v8::Promise> Protocol::IsProtocolHandled(const std::string& scheme,
                                                    gin::Arguments* args) {
-  node::Environment* env = node::Environment::GetCurrent(args->isolate());
-  EmitWarning(env,
-              "The protocol.isProtocolHandled API is deprecated, use "
-              "protocol.isProtocolRegistered or protocol.isProtocolIntercepted "
-              "instead.",
-              "ProtocolDeprecateIsProtocolHandled");
+  util::EmitWarning(args->isolate(),
+                    "The protocol.isProtocolHandled API is deprecated, "
+                    "use protocol.isProtocolRegistered "
+                    "or protocol.isProtocolIntercepted instead.",
+                    "ProtocolDeprecateIsProtocolHandled");
   return gin_helper::Promise<bool>::ResolvedPromise(
       args->isolate(),
       IsProtocolRegistered(scheme) || IsProtocolIntercepted(scheme) ||
@@ -279,9 +278,8 @@ void Protocol::HandleOptionalCallback(gin::Arguments* args,
                                       ProtocolError error) {
   base::RepeatingCallback<void(v8::Local<v8::Value>)> callback;
   if (args->GetNext(&callback)) {
-    node::Environment* env = node::Environment::GetCurrent(args->isolate());
-    EmitWarning(
-        env,
+    util::EmitWarning(
+        args->isolate(),
         "The callback argument of protocol module APIs is no longer needed.",
         "ProtocolDeprecateCallback");
     if (error == ProtocolError::kOK)

shell/browser/api/electron_api_service_worker_context.cc

@@ -18,7 +18,6 @@
 #include "shell/common/gin_converters/gurl_converter.h"
 #include "shell/common/gin_converters/value_converter.h"
 #include "shell/common/gin_helper/dictionary.h"
-#include "shell/common/node_includes.h"
 
 namespace electron::api {
 

shell/browser/api/electron_api_session.cc

@@ -87,8 +87,8 @@
 #include "shell/common/gin_helper/object_template_builder.h"
 #include "shell/common/gin_helper/promise.h"
 #include "shell/common/node_includes.h"
+#include "shell/common/node_util.h"
 #include "shell/common/options_switches.h"
-#include "shell/common/process_util.h"
 #include "third_party/blink/public/common/storage_key/storage_key.h"
 #include "third_party/blink/public/mojom/mediastream/media_stream.mojom.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -1111,11 +1111,9 @@ v8::Local<v8::Promise> Session::LoadExtension(
              const extensions::Extension* extension,
              const std::string& error_msg) {
             if (extension) {
-              if (!error_msg.empty()) {
-                node::Environment* env =
-                    node::Environment::GetCurrent(promise.isolate());
-                EmitWarning(env, error_msg, "ExtensionLoadWarning");
-              }
+              if (!error_msg.empty())
+                util::EmitWarning(promise.isolate(), error_msg,
+                                  "ExtensionLoadWarning");
               promise.Resolve(extension);
             } else {
               promise.RejectWithErrorMessage(error_msg);

shell/browser/api/electron_api_web_contents.cc

@@ -21,6 +21,7 @@
 #include "base/files/file_util.h"
 #include "base/json/json_reader.h"
 #include "base/no_destructor.h"
+#include "base/strings/strcat.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/current_thread.h"
 #include "base/threading/scoped_blocking_call.h"
@@ -129,8 +130,8 @@
 #include "shell/common/gin_helper/promise.h"
 #include "shell/common/language_util.h"
 #include "shell/common/node_includes.h"
+#include "shell/common/node_util.h"
 #include "shell/common/options_switches.h"
-#include "shell/common/process_util.h"
 #include "shell/common/thread_restrictions.h"
 #include "shell/common/v8_value_serializer.h"
 #include "storage/browser/file_system/isolated_context.h"
@@ -2113,10 +2114,9 @@ void WebContents::DidFinishNavigation(
 
     // Do not emit "did-fail-load" for canceled requests.
     if (code != net::ERR_ABORTED) {
-      EmitWarning(
-          node::Environment::GetCurrent(JavascriptEnvironment::GetIsolate()),
-          "Failed to load URL: " + url.possibly_invalid_spec() +
-              " with error: " + description,
+      util::EmitWarning(
+          base::StrCat({"Failed to load URL: ", url.possibly_invalid_spec(),
+                        " with error: ", description}),
           "electron");
       Emit("did-fail-load", code, description, url, is_main_frame,
            frame_process_id, frame_routing_id);

shell/browser/hid/hid_chooser_controller.cc

@@ -24,8 +24,7 @@
 #include "shell/common/gin_converters/content_converter.h"
 #include "shell/common/gin_converters/hid_device_info_converter.h"
 #include "shell/common/gin_converters/value_converter.h"
-#include "shell/common/node_includes.h"
-#include "shell/common/process_util.h"
+#include "shell/common/node_util.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace {
@@ -204,10 +203,9 @@ void HidChooserController::OnDeviceChosen(gin::Arguments* args) {
       }
       RunCallback(std::move(devices));
     } else {
-      v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
-      node::Environment* env = node::Environment::GetCurrent(isolate);
-      EmitWarning(env, "The device id " + device_id + " was not found.",
-                  "UnknownHIDDeviceId");
+      util::EmitWarning(
+          base::StrCat({"The device id ", device_id, " was not found."}),
+          "UnknownHIDDeviceId");
       RunCallback({});
     }
   }

shell/browser/native_window_mac.mm

@@ -36,9 +36,8 @@
 #include "shell/browser/window_list.h"
 #include "shell/common/gin_converters/gfx_converter.h"
 #include "shell/common/gin_helper/dictionary.h"
-#include "shell/common/node_includes.h"
+#include "shell/common/node_util.h"
 #include "shell/common/options_switches.h"
-#include "shell/common/process_util.h"
 #include "skia/ext/skia_utils_mac.h"
 #include "third_party/webrtc/modules/desktop_capture/mac/window_list_utils.h"
 #include "ui/base/hit_test.h"
@@ -183,11 +182,9 @@ NativeWindowMac::NativeWindowMac(const gin_helper::Dictionary& options,
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wdeprecated-declarations"
   if (windowType == "textured" || transparent() || !has_frame()) {
-    node::Environment* env =
-        node::Environment::GetCurrent(JavascriptEnvironment::GetIsolate());
-    EmitWarning(env,
-                "The 'textured' window type is deprecated and will be removed",
-                "DeprecationWarning");
+    util::EmitWarning(
+        "The 'textured' window type is deprecated and will be removed",
+        "DeprecationWarning");
     styleMask |= NSWindowStyleMaskTexturedBackground;
   }
 #pragma clang diagnostic pop

shell/browser/usb/usb_chooser_context.cc

@@ -23,7 +23,6 @@
 #include "shell/common/electron_constants.h"
 #include "shell/common/gin_converters/usb_device_info_converter.h"
 #include "shell/common/gin_helper/dictionary.h"
-#include "shell/common/node_includes.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace {

shell/browser/usb/usb_chooser_controller.cc

@@ -23,8 +23,7 @@
 #include "shell/common/gin_converters/content_converter.h"
 #include "shell/common/gin_converters/frame_converter.h"
 #include "shell/common/gin_converters/usb_device_info_converter.h"
-#include "shell/common/node_includes.h"
-#include "shell/common/process_util.h"
+#include "shell/common/node_util.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "url/gurl.h"
 
@@ -91,10 +90,9 @@ void UsbChooserController::OnDeviceChosen(gin::Arguments* args) {
     if (device_info) {
       RunCallback(device_info->Clone());
     } else {
-      v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
-      node::Environment* env = node::Environment::GetCurrent(isolate);
-      EmitWarning(env, "The device id " + device_id + " was not found.",
-                  "UnknownUsbDeviceId");
+      util::EmitWarning(
+          base::StrCat({"The device id ", device_id, " was not found."}),
+          "UnknownUsbDeviceId");
       RunCallback(/*device_info=*/nullptr);
     }
   }
@@ -118,7 +116,7 @@ void UsbChooserController::GotUsbDeviceList(
   if (session) {
     auto* rfh = content::RenderFrameHost::FromID(render_frame_host_id_);
     v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
-    v8::HandleScope scope(isolate);
+    v8::HandleScope handle_scope{isolate};
 
     // "select-usb-device" should respect |filters|.
     std::erase_if(devices, [this](const auto& device_info) {

shell/common/crash_keys.cc

@@ -12,14 +12,14 @@
 #include "base/command_line.h"
 #include "base/environment.h"
 #include "base/no_destructor.h"
-#include "base/strings/stringprintf.h"
+#include "base/strings/strcat.h"
+#include "base/strings/string_number_conversions.h"
 #include "components/crash/core/common/crash_key.h"
 #include "content/public/common/content_switches.h"
 #include "electron/buildflags/buildflags.h"
 #include "electron/fuses.h"
-#include "shell/browser/javascript_environment.h"
 #include "shell/common/electron_constants.h"
-#include "shell/common/node_includes.h"
+#include "shell/common/node_util.h"
 #include "shell/common/options_switches.h"
 #include "shell/common/process_util.h"
 #include "third_party/crashpad/crashpad/client/annotation.h"
@@ -54,13 +54,10 @@ void SetCrashKey(const std::string& key, const std::string& value) {
   // Chrome DCHECK()s if we try to set an annotation with a name longer than
   // the max.
   if (key.size() >= kMaxCrashKeyNameLength) {
-    node::Environment* env =
-        node::Environment::GetCurrent(JavascriptEnvironment::GetIsolate());
-    EmitWarning(
-        env,
-        base::StringPrintf("The crash key name, \"%s\", is longer than %" PRIu32
-                           " bytes, ignoring it.",
-                           key.c_str(), kMaxCrashKeyNameLength),
+    util::EmitWarning(
+        base::StrCat({"The crash key name, '", key, "', is longer than ",
+                      base::NumberToString(kMaxCrashKeyNameLength),
+                      " bytes, ignoring it."}),
         "electron");
     return;
   }

shell/common/gin_converters/osr_converter.cc

@@ -15,7 +15,7 @@
 #include "shell/common/gin_converters/gfx_converter.h"
 #include "shell/common/gin_converters/optional_converter.h"
 #include "shell/common/node_includes.h"
-#include "shell/common/process_util.h"
+#include "shell/common/node_util.h"
 
 namespace gin {
 
@@ -146,13 +146,11 @@ v8::Local<v8::Value> Converter<electron::OffscreenSharedTextureValue>::ToV8(
           data.SetSecondPassCallback([](const v8::WeakCallbackInfo<
                                          OffscreenReleaseHolderMonitor>& data) {
             auto* iso = data.GetIsolate();
-            node::Environment* env = node::Environment::GetCurrent(iso);
-
             // Emit warning only once
             static std::once_flag flag;
             std::call_once(flag, [=] {
-              electron::EmitWarning(
-                  env,
+              electron::util::EmitWarning(
+                  iso,
                   "[OSR TEXTURE LEAKED] When using OSR with "
                   "`useSharedTexture`, `texture.release()` "
                   "must be called explicitly as soon as the texture is "

shell/common/node_bindings_mac.cc

@@ -10,8 +10,6 @@
 #include <sys/time.h>
 #include <sys/types.h>
 
-#include "shell/common/node_includes.h"
-
 namespace electron {
 
 NodeBindingsMac::NodeBindingsMac(BrowserEnvironment browser_env)

shell/common/node_util.cc

@@ -6,6 +6,9 @@
 
 #include "base/logging.h"
 #include "gin/converter.h"
+#include "gin/dictionary.h"
+#include "shell/browser/javascript_environment.h"
+#include "shell/common/gin_converters/callback_converter.h"
 #include "shell/common/node_includes.h"
 
 namespace electron::util {
@@ -44,4 +47,22 @@ v8::MaybeLocal<v8::Value> CompileAndCall(
   return ret;
 }
 
+void EmitWarning(const std::string_view warning_msg,
+                 const std::string_view warning_type) {
+  EmitWarning(JavascriptEnvironment::GetIsolate(), warning_msg, warning_type);
+}
+
+void EmitWarning(v8::Isolate* isolate,
+                 const std::string_view warning_msg,
+                 const std::string_view warning_type) {
+  v8::HandleScope scope{isolate};
+  gin::Dictionary process{
+      isolate, node::Environment::GetCurrent(isolate)->process_object()};
+  base::RepeatingCallback<void(std::string_view, std::string_view,
+                               std::string_view)>
+      emit_warning;
+  process.Get("emitWarning", &emit_warning);
+  emit_warning.Run(warning_msg, warning_type, "");
+}
+
 }  // namespace electron::util

shell/common/node_util.h

@@ -5,6 +5,7 @@
 #ifndef ELECTRON_SHELL_COMMON_NODE_UTIL_H_
 #define ELECTRON_SHELL_COMMON_NODE_UTIL_H_
 
+#include <string_view>
 #include <vector>
 
 #include "v8/include/v8-forward.h"
@@ -15,6 +16,15 @@ class Environment;
 
 namespace electron::util {
 
+// Emit a warning via node's process.emitWarning()
+void EmitWarning(v8::Isolate* isolate,
+                 std::string_view warning_msg,
+                 std::string_view warning_type);
+
+// Emit a warning via node's process.emitWarning(),
+// using JavscriptEnvironment's isolate
+void EmitWarning(std::string_view warning_msg, std::string_view warning_type);
+
 // Run a script with JS source bundled inside the binary as if it's wrapped
 // in a function called with a null receiver and arguments specified in C++.
 // The returned value is empty if an exception is encountered.

shell/common/process_util.cc

@@ -9,26 +9,9 @@
 
 #include "base/command_line.h"
 #include "content/public/common/content_switches.h"
-#include "gin/dictionary.h"
-#include "shell/common/gin_converters/callback_converter.h"
-#include "shell/common/node_includes.h"
 
 namespace electron {
 
-void EmitWarning(node::Environment* env,
-                 const std::string& warning_msg,
-                 const std::string& warning_type) {
-  v8::HandleScope scope(env->isolate());
-  gin::Dictionary process(env->isolate(), env->process_object());
-
-  base::RepeatingCallback<void(std::string_view, std::string_view,
-                               std::string_view)>
-      emit_warning;
-  process.Get("emitWarning", &emit_warning);
-
-  emit_warning.Run(warning_msg, warning_type, "");
-}
-
 std::string GetProcessType() {
   auto* command_line = base::CommandLine::ForCurrentProcess();
   return command_line->GetSwitchValueASCII(switches::kProcessType);

shell/common/process_util.h

@@ -6,17 +6,10 @@
 #define ELECTRON_SHELL_COMMON_PROCESS_UTIL_H_
 
 #include <string>
-
-namespace node {
-class Environment;
-}
+#include <string_view>
 
 namespace electron {
 
-void EmitWarning(node::Environment* env,
-                 const std::string& warning_msg,
-                 const std::string& warning_type);
-
 std::string GetProcessType();
 
 bool IsBrowserProcess();

shell/common/skia_util.cc

@@ -10,7 +10,6 @@
 #include "base/strings/string_util.h"
 #include "net/base/data_url.h"
 #include "shell/common/asar/asar_util.h"
-#include "shell/common/node_includes.h"
 #include "shell/common/skia_util.h"
 #include "shell/common/thread_restrictions.h"
 #include "third_party/skia/include/core/SkBitmap.h"