From 2e460c0ffb79b7efd10bf9fcf5b8dff9da4b906a Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Sat, 19 Jul 2025 10:42:19 +0200 Subject: [PATCH] refactor: reduce scope of temporaries when getting dictionary values (#47799) refactor: reduce scale of temporaries when getting dictionary values Co-authored-by: Charles Kerr --- shell/browser/native_window.cc | 78 +++++++++++----------------- shell/browser/native_window_mac.mm | 9 ++-- shell/browser/native_window_views.cc | 49 ++++++----------- 3 files changed, 48 insertions(+), 88 deletions(-) diff --git a/shell/browser/native_window.cc b/shell/browser/native_window.cc index 36fff501b204..5a9a189298f6 100644 --- a/shell/browser/native_window.cc +++ b/shell/browser/native_window.cc @@ -134,19 +134,17 @@ NativeWindow::~NativeWindow() { void NativeWindow::InitFromOptions(const gin_helper::Dictionary& options) { // Setup window from options. - int x = -1, y = -1; - bool center; - if (options.Get(options::kX, &x) && options.Get(options::kY, &y)) { - SetPosition(gfx::Point(x, y)); + if (int x, y; options.Get(options::kX, &x) && options.Get(options::kY, &y)) { + SetPosition(gfx::Point{x, y}); #if BUILDFLAG(IS_WIN) // FIXME(felixrieseberg): Dirty, dirty workaround for // https://github.com/electron/electron/issues/10862 // Somehow, we need to call `SetBounds` twice to get // usable results. The root cause is still unknown. - SetPosition(gfx::Point(x, y)); + SetPosition(gfx::Point{x, y}); #endif - } else if (options.Get(options::kCenter, ¢er) && center) { + } else if (bool center; options.Get(options::kCenter, ¢er) && center) { Center(); } @@ -185,27 +183,21 @@ void NativeWindow::InitFromOptions(const gin_helper::Dictionary& options) { SetSizeConstraints(size_constraints); } #if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_LINUX) - bool closable; - if (options.Get(options::kClosable, &closable)) { - SetClosable(closable); - } + if (bool val; options.Get(options::kClosable, &val)) + SetClosable(val); #endif - bool movable; - if (options.Get(options::kMovable, &movable)) { - SetMovable(movable); - } - bool has_shadow; - if (options.Get(options::kHasShadow, &has_shadow)) { - SetHasShadow(has_shadow); - } - double opacity; - if (options.Get(options::kOpacity, &opacity)) { - SetOpacity(opacity); - } - bool top; - if (options.Get(options::kAlwaysOnTop, &top) && top) { + + if (bool val; options.Get(options::kMovable, &val)) + SetMovable(val); + + if (bool val; options.Get(options::kHasShadow, &val)) + SetHasShadow(val); + + if (double val; options.Get(options::kOpacity, &val)) + SetOpacity(val); + + if (bool val; options.Get(options::kAlwaysOnTop, &val) && val) SetAlwaysOnTop(ui::ZOrderLevel::kFloatingWindow); - } bool fullscreenable = true; bool fullscreen = false; @@ -222,29 +214,21 @@ void NativeWindow::InitFromOptions(const gin_helper::Dictionary& options) { if (fullscreen) SetFullScreen(true); - bool resizable; - if (options.Get(options::kResizable, &resizable)) { - SetResizable(resizable); - } + if (bool val; options.Get(options::kResizable, &val)) + SetResizable(val); + + if (bool val; options.Get(options::kSkipTaskbar, &val)) + SetSkipTaskbar(val); + + if (bool val; options.Get(options::kKiosk, &val) && val) + SetKiosk(val); - bool skip; - if (options.Get(options::kSkipTaskbar, &skip)) { - SetSkipTaskbar(skip); - } - bool kiosk; - if (options.Get(options::kKiosk, &kiosk) && kiosk) { - SetKiosk(kiosk); - } #if BUILDFLAG(IS_MAC) - std::string type; - if (options.Get(options::kVibrancyType, &type)) { - SetVibrancy(type, 0); - } + if (std::string val; options.Get(options::kVibrancyType, &val)) + SetVibrancy(val, 0); #elif BUILDFLAG(IS_WIN) - std::string material; - if (options.Get(options::kBackgroundMaterial, &material)) { - SetBackgroundMaterial(material); - } + if (std::string val; options.Get(options::kBackgroundMaterial, &val)) + SetBackgroundMaterial(val); #endif SkColor background_color = SK_ColorWHITE; @@ -255,9 +239,7 @@ void NativeWindow::InitFromOptions(const gin_helper::Dictionary& options) { } SetBackgroundColor(background_color); - std::string title(Browser::Get()->GetName()); - options.Get(options::kTitle, &title); - SetTitle(title); + SetTitle(options.ValueOrDefault(options::kTitle, Browser::Get()->GetName())); // Then show it. if (options.ValueOrDefault(options::kShow, true)) diff --git a/shell/browser/native_window_mac.mm b/shell/browser/native_window_mac.mm index 4e3c14b399eb..2b26687624bb 100644 --- a/shell/browser/native_window_mac.mm +++ b/shell/browser/native_window_mac.mm @@ -239,8 +239,7 @@ NativeWindowMac::NativeWindowMac(const gin_helper::Dictionary& options, [window_ setLevel:NSFloatingWindowLevel]; } - bool focusable; - if (options.Get(options::kFocusable, &focusable) && !focusable) + if (bool val; options.Get(options::kFocusable, &val) && !val) [window_ setDisableKeyOrMainWindow:YES]; if (transparent() || !has_frame()) { @@ -285,12 +284,10 @@ NativeWindowMac::NativeWindowMac(const gin_helper::Dictionary& options, // NOTE(@mlaurencin) Spec requirements can be found here: // https://developer.mozilla.org/en-US/docs/Web/API/Window/open#width constexpr int kMinSizeReqdBySpec = 100; - int inner_width = 0; - int inner_height = 0; + const int inner_width = options.ValueOrDefault(options::kinnerWidth, 0); + const int inner_height = options.ValueOrDefault(options::kinnerHeight, 0); bool use_content_size = options.ValueOrDefault(options::kUseContentSize, false); - options.Get(options::kinnerWidth, &inner_width); - options.Get(options::kinnerHeight, &inner_height); if (inner_width || inner_height) { use_content_size = true; if (inner_width) diff --git a/shell/browser/native_window_views.cc b/shell/browser/native_window_views.cc index fff2c9dd9a88..02a9bb3eab43 100644 --- a/shell/browser/native_window_views.cc +++ b/shell/browser/native_window_views.cc @@ -197,9 +197,8 @@ NativeWindowViews::NativeWindowViews(const gin_helper::Dictionary& options, : NativeWindow(options, parent) { options.Get(options::kTitle, &title_); - bool menu_bar_autohide; - if (options.Get(options::kAutoHideMenuBar, &menu_bar_autohide)) - root_view_.SetAutoHideMenuBar(menu_bar_autohide); + if (bool val; options.Get(options::kAutoHideMenuBar, &val)) + root_view_.SetAutoHideMenuBar(val); #if BUILDFLAG(IS_WIN) // On Windows we rely on the CanResize() to indicate whether window can be @@ -216,35 +215,20 @@ NativeWindowViews::NativeWindowViews(const gin_helper::Dictionary& options, overlay_button_color_ = color_utils::GetSysSkColor(COLOR_BTNFACE); overlay_symbol_color_ = color_utils::GetSysSkColor(COLOR_BTNTEXT); - bool accent_color = true; - std::string accent_color_string; - if (options.Get(options::kAccentColor, &accent_color_string)) { - accent_color_ = ParseCSSColor(accent_color_string); - } else if (options.Get(options::kAccentColor, &accent_color)) { - accent_color_ = accent_color; + if (std::string str; options.Get(options::kAccentColor, &str)) { + accent_color_ = ParseCSSColor(str); + } else if (bool flag; options.Get(options::kAccentColor, &flag)) { + accent_color_ = flag; } #endif - v8::Local titlebar_overlay; - if (options.Get(options::ktitleBarOverlay, &titlebar_overlay) && - titlebar_overlay->IsObject()) { - auto titlebar_overlay_obj = - gin_helper::Dictionary::CreateEmpty(options.isolate()); - options.Get(options::ktitleBarOverlay, &titlebar_overlay_obj); - - std::string overlay_color_string; - if (titlebar_overlay_obj.Get(options::kOverlayButtonColor, - &overlay_color_string)) { - bool success = content::ParseCssColorString(overlay_color_string, - &overlay_button_color_); + if (gin_helper::Dictionary od; options.Get(options::ktitleBarOverlay, &od)) { + if (std::string val; od.Get(options::kOverlayButtonColor, &val)) { + bool success = content::ParseCssColorString(val, &overlay_button_color_); DCHECK(success); } - - std::string overlay_symbol_color_string; - if (titlebar_overlay_obj.Get(options::kOverlaySymbolColor, - &overlay_symbol_color_string)) { - bool success = content::ParseCssColorString(overlay_symbol_color_string, - &overlay_symbol_color_); + if (std::string val; od.Get(options::kOverlaySymbolColor, &val)) { + bool success = content::ParseCssColorString(val, &overlay_symbol_color_); DCHECK(success); } } @@ -290,8 +274,7 @@ NativeWindowViews::NativeWindowViews(const gin_helper::Dictionary& options, if (IsTranslucent() && !has_frame()) params.shadow_type = InitParams::ShadowType::kNone; - bool focusable; - if (options.Get(options::kFocusable, &focusable) && !focusable) + if (bool val; options.Get(options::kFocusable, &val) && !val) params.activatable = InitParams::Activatable::kNo; #if BUILDFLAG(IS_WIN) @@ -414,11 +397,9 @@ NativeWindowViews::NativeWindowViews(const gin_helper::Dictionary& options, // NOTE(@mlaurencin) Spec requirements can be found here: // https://developer.mozilla.org/en-US/docs/Web/API/Window/open#width - int kMinSizeReqdBySpec = 100; - int inner_width = 0; - int inner_height = 0; - options.Get(options::kinnerWidth, &inner_width); - options.Get(options::kinnerHeight, &inner_height); + constexpr int kMinSizeReqdBySpec = 100; + const int inner_width = options.ValueOrDefault(options::kinnerWidth, 0); + const int inner_height = options.ValueOrDefault(options::kinnerHeight, 0); if (inner_width || inner_height) { use_content_size_ = true; if (inner_width)