refactor: add EmitWarning(v8::Isolate*) helper (#43736)

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,11 +256,10 @@ 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.",
+  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(),
@@ -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

@@ -19,7 +19,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"
@@ -128,8 +129,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"
@@ -2098,10 +2099,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,9 +203,8 @@ 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.",
+      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,9 +182,7 @@ 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,
+    util::EmitWarning(
         "The 'textured' window type is deprecated and will be removed",
         "DeprecationWarning");
     styleMask |= NSWindowStyleMaskTexturedBackground;

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,9 +90,8 @@ 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.",
+      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/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

@@ -7,26 +7,9 @@
 #include <string_view>
 #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"