Browse Source

refactor: use direct aggregation in NativeWindowViews (#38559)

* refactor: in NativeWindowViews, aggregate root_view_ directly

* refactor: in NativeWindowViews, aggregate keyboard_event_handler_ directly

* refactor: make NativeWindowClientView::window_ a raw_ref

Xref: https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md\#non_owning-pointers-in-class-fields

Prefer const raw_ref<T> whenever the held pointer will never be null

* chore: make lint happy
Charles Kerr 1 year ago
parent
commit
83d023747c
2 changed files with 34 additions and 40 deletions
  1. 30 33
      shell/browser/native_window_views.cc
  2. 4 7
      shell/browser/native_window_views.h

+ 30 - 33
shell/browser/native_window_views.cc

@@ -36,7 +36,6 @@
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/native_widget_types.h"
 #include "ui/views/background.h"
-#include "ui/views/controls/webview/unhandled_keyboard_event_handler.h"
 #include "ui/views/controls/webview/webview.h"
 #include "ui/views/widget/native_widget_private.h"
 #include "ui/views/widget/widget.h"
@@ -166,7 +165,8 @@ class NativeWindowClientView : public views::ClientView {
   NativeWindowClientView(views::Widget* widget,
                          views::View* root_view,
                          NativeWindowViews* window)
-      : views::ClientView(widget, root_view), window_(window) {}
+      : views::ClientView{widget, root_view},
+        window_{raw_ref<NativeWindowViews>::from_ptr(window)} {}
   ~NativeWindowClientView() override = default;
 
   // disable copy
@@ -179,22 +179,19 @@ class NativeWindowClientView : public views::ClientView {
   }
 
  private:
-  raw_ptr<NativeWindowViews> window_;
+  const raw_ref<NativeWindowViews> window_;
 };
 
 }  // namespace
 
 NativeWindowViews::NativeWindowViews(const gin_helper::Dictionary& options,
                                      NativeWindow* parent)
-    : NativeWindow(options, parent),
-      root_view_(std::make_unique<RootView>(this)),
-      keyboard_event_handler_(
-          std::make_unique<views::UnhandledKeyboardEventHandler>()) {
+    : 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);
+    root_view_.SetAutoHideMenuBar(menu_bar_autohide);
 
 #if BUILDFLAG(IS_WIN)
   // On Windows we rely on the CanResize() to indicate whether window can be
@@ -455,12 +452,12 @@ void NativeWindowViews::SetGTKDarkThemeEnabled(bool use_dark_theme) {
 
 void NativeWindowViews::SetContentView(views::View* view) {
   if (content_view()) {
-    root_view_->RemoveChildView(content_view());
+    root_view_.RemoveChildView(content_view());
   }
   set_content_view(view);
   focused_view_ = view;
-  root_view_->AddChildView(content_view());
-  root_view_->Layout();
+  root_view_.AddChildView(content_view());
+  root_view_.Layout();
 }
 
 void NativeWindowViews::Close() {
@@ -1097,7 +1094,7 @@ bool NativeWindowViews::IsTabletMode() const {
 }
 
 SkColor NativeWindowViews::GetBackgroundColor() {
-  auto* background = root_view_->background();
+  auto* background = root_view_.background();
   if (!background)
     return SK_ColorTRANSPARENT;
   return background->get_color();
@@ -1105,7 +1102,7 @@ SkColor NativeWindowViews::GetBackgroundColor() {
 
 void NativeWindowViews::SetBackgroundColor(SkColor background_color) {
   // web views' background color.
-  root_view_->SetBackground(views::CreateSolidBackground(background_color));
+  root_view_.SetBackground(views::CreateSolidBackground(background_color));
 
 #if BUILDFLAG(IS_WIN)
   // Set the background color of native window.
@@ -1240,7 +1237,7 @@ void NativeWindowViews::SetMenu(ElectronMenuModel* menu_model) {
   // Remove global menu bar.
   if (global_menu_bar_ && menu_model == nullptr) {
     global_menu_bar_.reset();
-    root_view_->UnregisterAcceleratorsWithFocusManager();
+    root_view_.UnregisterAcceleratorsWithFocusManager();
     return;
   }
 
@@ -1249,7 +1246,7 @@ void NativeWindowViews::SetMenu(ElectronMenuModel* menu_model) {
     if (!global_menu_bar_)
       global_menu_bar_ = std::make_unique<GlobalMenuBarX11>(this);
     if (global_menu_bar_->IsServerStarted()) {
-      root_view_->RegisterAcceleratorsWithFocusManager(menu_model);
+      root_view_.RegisterAcceleratorsWithFocusManager(menu_model);
       global_menu_bar_->SetMenu(menu_model);
       return;
     }
@@ -1260,13 +1257,13 @@ void NativeWindowViews::SetMenu(ElectronMenuModel* menu_model) {
   gfx::Size content_size = GetContentSize();
   bool should_reset_size = use_content_size_ && has_frame() &&
                            !IsMenuBarAutoHide() &&
-                           ((!!menu_model) != root_view_->HasMenu());
+                           ((!!menu_model) != root_view_.HasMenu());
 
-  root_view_->SetMenu(menu_model);
+  root_view_.SetMenu(menu_model);
 
   if (should_reset_size) {
     // Enlarge the size constraints for the menu.
-    int menu_bar_height = root_view_->GetMenuBarHeight();
+    int menu_bar_height = root_view_.GetMenuBarHeight();
     extensions::SizeConstraints constraints = GetContentSizeConstraints();
     if (constraints.HasMinimumSize()) {
       gfx::Size min_size = constraints.GetMinimumSize();
@@ -1393,19 +1390,19 @@ void NativeWindowViews::SetOverlayIcon(const gfx::Image& overlay,
 }
 
 void NativeWindowViews::SetAutoHideMenuBar(bool auto_hide) {
-  root_view_->SetAutoHideMenuBar(auto_hide);
+  root_view_.SetAutoHideMenuBar(auto_hide);
 }
 
 bool NativeWindowViews::IsMenuBarAutoHide() {
-  return root_view_->IsMenuBarAutoHide();
+  return root_view_.IsMenuBarAutoHide();
 }
 
 void NativeWindowViews::SetMenuBarVisibility(bool visible) {
-  root_view_->SetMenuBarVisibility(visible);
+  root_view_.SetMenuBarVisibility(visible);
 }
 
 bool NativeWindowViews::IsMenuBarVisible() {
-  return root_view_->IsMenuBarVisible();
+  return root_view_.IsMenuBarVisible();
 }
 
 void NativeWindowViews::SetBackgroundMaterial(const std::string& material) {
@@ -1503,8 +1500,8 @@ gfx::Rect NativeWindowViews::ContentBoundsToWindowBounds(
   }
 #endif
 
-  if (root_view_->HasMenu() && root_view_->IsMenuBarVisible()) {
-    int menu_bar_height = root_view_->GetMenuBarHeight();
+  if (root_view_.HasMenu() && root_view_.IsMenuBarVisible()) {
+    int menu_bar_height = root_view_.GetMenuBarHeight();
     window_bounds.set_y(window_bounds.y() - menu_bar_height);
     window_bounds.set_height(window_bounds.height() + menu_bar_height);
   }
@@ -1530,8 +1527,8 @@ gfx::Rect NativeWindowViews::WindowBoundsToContentBounds(
   content_bounds.set_size(ScreenToDIPRect(hwnd, content_bounds).size());
 #endif
 
-  if (root_view_->HasMenu() && root_view_->IsMenuBarVisible()) {
-    int menu_bar_height = root_view_->GetMenuBarHeight();
+  if (root_view_.HasMenu() && root_view_.IsMenuBarVisible()) {
+    int menu_bar_height = root_view_.GetMenuBarHeight();
     content_bounds.set_y(content_bounds.y() + menu_bar_height);
     content_bounds.set_height(content_bounds.height() - menu_bar_height);
   }
@@ -1574,7 +1571,7 @@ void NativeWindowViews::OnWidgetActivationChanged(views::Widget* changed_widget,
   if (!active && IsMenuBarAutoHide() && IsMenuBarVisible())
     SetMenuBarVisibility(false);
 
-  root_view_->ResetAltState();
+  root_view_.ResetAltState();
 }
 
 void NativeWindowViews::OnWidgetBoundsChanged(views::Widget* changed_widget,
@@ -1630,7 +1627,7 @@ std::u16string NativeWindowViews::GetWindowTitle() const {
 }
 
 views::View* NativeWindowViews::GetContentsView() {
-  return root_view_.get();
+  return &root_view_;
 }
 
 bool NativeWindowViews::ShouldDescendIntoChildForEventHandling(
@@ -1640,7 +1637,7 @@ bool NativeWindowViews::ShouldDescendIntoChildForEventHandling(
 }
 
 views::ClientView* NativeWindowViews::CreateClientView(views::Widget* widget) {
-  return new NativeWindowClientView(widget, root_view_.get(), this);
+  return new NativeWindowClientView{widget, GetContentsView(), this};
 }
 
 std::unique_ptr<views::NonClientFrameView>
@@ -1679,9 +1676,9 @@ void NativeWindowViews::HandleKeyboardEvent(
     NotifyWindowExecuteAppCommand(kBrowserForward);
 #endif
 
-  keyboard_event_handler_->HandleKeyboardEvent(event,
-                                               root_view_->GetFocusManager());
-  root_view_->HandleKeyEvent(event);
+  keyboard_event_handler_.HandleKeyboardEvent(event,
+                                              root_view_.GetFocusManager());
+  root_view_.HandleKeyEvent(event);
 }
 
 void NativeWindowViews::OnMouseEvent(ui::MouseEvent* event) {
@@ -1689,7 +1686,7 @@ void NativeWindowViews::OnMouseEvent(ui::MouseEvent* event) {
     return;
 
   // Alt+Click should not toggle menu bar.
-  root_view_->ResetAltState();
+  root_view_.ResetAltState();
 
 #if BUILDFLAG(IS_LINUX)
   if (event->changed_button_flags() == ui::EF_BACK_MOUSE_BUTTON)

+ 4 - 7
shell/browser/native_window_views.h

@@ -13,6 +13,8 @@
 #include <vector>
 
 #include "base/memory/raw_ptr.h"
+#include "shell/browser/ui/views/root_view.h"
+#include "ui/views/controls/webview/unhandled_keyboard_event_handler.h"
 #include "ui/views/widget/widget_observer.h"
 
 #if defined(USE_OZONE)
@@ -28,14 +30,9 @@
 
 #endif
 
-namespace views {
-class UnhandledKeyboardEventHandler;
-}
-
 namespace electron {
 
 class GlobalMenuBarX11;
-class RootView;
 class WindowStateWatcher;
 
 #if defined(USE_OZONE_PLATFORM_X11)
@@ -251,7 +248,7 @@ class NativeWindowViews : public NativeWindow,
   // Maintain window placement.
   void MoveBehindTaskBarIfNeeded();
 
-  std::unique_ptr<RootView> root_view_;
+  RootView root_view_{this};
 
   // The view should be focused by default.
   raw_ptr<views::View> focused_view_ = nullptr;
@@ -322,7 +319,7 @@ class NativeWindowViews : public NativeWindow,
 #endif
 
   // Handles unhandled keyboard messages coming back from the renderer process.
-  std::unique_ptr<views::UnhandledKeyboardEventHandler> keyboard_event_handler_;
+  views::UnhandledKeyboardEventHandler keyboard_event_handler_;
 
   // Whether the window should be enabled based on user calls to SetEnabled()
   bool is_enabled_ = true;