From d11c840cf036a488fe39c9973548e0e94b0b8056 Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Tue, 17 Sep 2024 04:50:48 -0500 Subject: [PATCH] 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 | 1 - .../browser/api/electron_api_download_item.cc | 1 - shell/browser/api/electron_api_net_log.cc | 1 - shell/browser/api/electron_api_protocol.cc | 18 +++++++--------- .../electron_api_service_worker_context.cc | 1 - shell/browser/api/electron_api_session.cc | 10 ++++----- .../browser/api/electron_api_web_contents.cc | 10 ++++----- shell/browser/hid/hid_chooser_controller.cc | 10 ++++----- shell/browser/native_window_mac.mm | 11 ++++------ shell/browser/usb/usb_chooser_context.cc | 1 - shell/browser/usb/usb_chooser_controller.cc | 12 +++++------ shell/common/crash_keys.cc | 17 +++++++-------- shell/common/node_bindings_mac.cc | 2 -- shell/common/node_util.cc | 21 +++++++++++++++++++ shell/common/node_util.h | 10 +++++++++ shell/common/process_util.cc | 17 --------------- shell/common/process_util.h | 9 +------- shell/common/skia_util.cc | 1 - 18 files changed, 69 insertions(+), 84 deletions(-) diff --git a/shell/browser/api/electron_api_debugger.cc b/shell/browser/api/electron_api_debugger.cc index b1f2ed59fcd6..71f3757bb98e 100755 --- a/shell/browser/api/electron_api_debugger.cc +++ b/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; diff --git a/shell/browser/api/electron_api_download_item.cc b/shell/browser/api/electron_api_download_item.cc index 1e8a142e48a3..feddcc94ae1b 100644 --- a/shell/browser/api/electron_api_download_item.cc +++ b/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 { diff --git a/shell/browser/api/electron_api_net_log.cc b/shell/browser/api/electron_api_net_log.cc index d447d55252ee..2ea08604da83 100644 --- a/shell/browser/api/electron_api_net_log.cc +++ b/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 { diff --git a/shell/browser/api/electron_api_protocol.cc b/shell/browser/api/electron_api_protocol.cc index 9e6cfe3998d2..896570a40267 100644 --- a/shell/browser/api/electron_api_protocol.cc +++ b/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 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::ResolvedPromise( args->isolate(), IsProtocolRegistered(scheme) || IsProtocolIntercepted(scheme) || @@ -279,9 +278,8 @@ void Protocol::HandleOptionalCallback(gin::Arguments* args, ProtocolError error) { base::RepeatingCallback)> 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) diff --git a/shell/browser/api/electron_api_service_worker_context.cc b/shell/browser/api/electron_api_service_worker_context.cc index 814ab8cb3e7c..4cfb23895a7b 100644 --- a/shell/browser/api/electron_api_service_worker_context.cc +++ b/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 { diff --git a/shell/browser/api/electron_api_session.cc b/shell/browser/api/electron_api_session.cc index 8953289886ad..a2169341aa99 100644 --- a/shell/browser/api/electron_api_session.cc +++ b/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 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); diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc index 42ef11724e3c..a8b10d1efe8a 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/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); diff --git a/shell/browser/hid/hid_chooser_controller.cc b/shell/browser/hid/hid_chooser_controller.cc index 8b7c2f31fd55..25ec46b8533e 100644 --- a/shell/browser/hid/hid_chooser_controller.cc +++ b/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({}); } } diff --git a/shell/browser/native_window_mac.mm b/shell/browser/native_window_mac.mm index 6ac5805750cf..2c0a3b3f8c5e 100644 --- a/shell/browser/native_window_mac.mm +++ b/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 diff --git a/shell/browser/usb/usb_chooser_context.cc b/shell/browser/usb/usb_chooser_context.cc index 5151999b425e..8243c3f6982a 100644 --- a/shell/browser/usb/usb_chooser_context.cc +++ b/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 { diff --git a/shell/browser/usb/usb_chooser_controller.cc b/shell/browser/usb/usb_chooser_controller.cc index 706bdae2a86e..ccc8c8c2bb11 100644 --- a/shell/browser/usb/usb_chooser_controller.cc +++ b/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) { diff --git a/shell/common/crash_keys.cc b/shell/common/crash_keys.cc index ccd8ff5cfbfe..3066ac5f01c8 100644 --- a/shell/common/crash_keys.cc +++ b/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; } diff --git a/shell/common/node_bindings_mac.cc b/shell/common/node_bindings_mac.cc index b6a92c54a0bb..b92c9bae0534 100644 --- a/shell/common/node_bindings_mac.cc +++ b/shell/common/node_bindings_mac.cc @@ -10,8 +10,6 @@ #include #include -#include "shell/common/node_includes.h" - namespace electron { NodeBindingsMac::NodeBindingsMac(BrowserEnvironment browser_env) diff --git a/shell/common/node_util.cc b/shell/common/node_util.cc index 344f07f37c03..d75208a9298a 100644 --- a/shell/common/node_util.cc +++ b/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 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 + emit_warning; + process.Get("emitWarning", &emit_warning); + emit_warning.Run(warning_msg, warning_type, ""); +} + } // namespace electron::util diff --git a/shell/common/node_util.h b/shell/common/node_util.h index 2e5f083ffaf8..029a7452e911 100644 --- a/shell/common/node_util.h +++ b/shell/common/node_util.h @@ -5,6 +5,7 @@ #ifndef ELECTRON_SHELL_COMMON_NODE_UTIL_H_ #define ELECTRON_SHELL_COMMON_NODE_UTIL_H_ +#include #include #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. diff --git a/shell/common/process_util.cc b/shell/common/process_util.cc index adcea8e31022..d422cdb8f86f 100644 --- a/shell/common/process_util.cc +++ b/shell/common/process_util.cc @@ -7,26 +7,9 @@ #include #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 - 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); diff --git a/shell/common/process_util.h b/shell/common/process_util.h index fb4c6639153b..c9eb759dd6f3 100644 --- a/shell/common/process_util.h +++ b/shell/common/process_util.h @@ -6,17 +6,10 @@ #define ELECTRON_SHELL_COMMON_PROCESS_UTIL_H_ #include - -namespace node { -class Environment; -} +#include namespace electron { -void EmitWarning(node::Environment* env, - const std::string& warning_msg, - const std::string& warning_type); - std::string GetProcessType(); bool IsBrowserProcess(); diff --git a/shell/common/skia_util.cc b/shell/common/skia_util.cc index bace4213b8e1..bbc7c33f0e95 100644 --- a/shell/common/skia_util.cc +++ b/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"