Refactor ViewsDelegate singleton

Currently, the variable containing ViewsDelegate singleton object can be set by anyone and there are cases (at least in tests) where it is freely replaced without properly deleting the old one which can lead to situations where multiple ViewsDelegate objects are in existence. This patch tries to ensure that there is at most one instance available at each time. Also, cleaned up some ViewsDelegate related includes. BUG=492991 Review URL: https://codereview.chromium.org/1159033008 Cr-Commit-Position: refs/heads/master@{#333692}
cbchan · Jun 10, 2015 · 0ff8c0e · 0ff8c0e
1 parent 78356a0
commit 0ff8c0e
Show file tree

Hide file tree

Showing 43 changed files with 128 additions and 115 deletions.
diff --git a/apps/ui/views/app_window_frame_view.cc b/apps/ui/views/app_window_frame_view.cc
@@ -20,7 +20,6 @@
 #include "ui/strings/grit/ui_strings.h"  // Accessibility names
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/layout/grid_layout.h"
-#include "ui/views/views_delegate.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_delegate.h"
 

diff --git a/ash/content/display/screen_orientation_controller_chromeos_unittest.cc b/ash/content/display/screen_orientation_controller_chromeos_unittest.cc
@@ -135,15 +135,15 @@ ScreenOrientationControllerTest::~ScreenOrientationControllerTest() {
 }
 
 content::WebContents* ScreenOrientationControllerTest::CreateWebContents() {
-  return views::ViewsDelegate::views_delegate->CreateWebContents(
+  return views::ViewsDelegate::GetInstance()->CreateWebContents(
       ash_test_helper()->test_shell_delegate()->GetActiveBrowserContext(),
       nullptr);
 }
 
 content::WebContents*
 ScreenOrientationControllerTest::CreateSecondaryWebContents() {
   secondary_browser_context_.reset(new content::TestBrowserContext());
-  return views::ViewsDelegate::views_delegate->CreateWebContents(
+  return views::ViewsDelegate::GetInstance()->CreateWebContents(
       secondary_browser_context_.get(), nullptr);
 }
 

diff --git a/ash/drag_drop/drag_drop_controller.cc b/ash/drag_drop/drag_drop_controller.cc
@@ -26,7 +26,6 @@
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/rect_conversions.h"
 #include "ui/gfx/path.h"
-#include "ui/views/views_delegate.h"
 #include "ui/views/widget/native_widget_aura.h"
 #include "ui/wm/core/coordinate_conversion.h"
 #include "ui/wm/public/drag_drop_delegate.h"

diff --git a/ash/drag_drop/drag_drop_controller_unittest.cc b/ash/drag_drop/drag_drop_controller_unittest.cc
@@ -29,7 +29,6 @@
 #include "ui/gfx/animation/linear_animation.h"
 #include "ui/gfx/image/image_skia_rep.h"
 #include "ui/views/view.h"
-#include "ui/views/views_delegate.h"
 #include "ui/views/widget/native_widget_aura.h"
 #include "ui/views/widget/native_widget_delegate.h"
 #include "ui/views/widget/widget.h"

diff --git a/ash/shell/content_client/shell_browser_main_parts.cc b/ash/shell/content_client/shell_browser_main_parts.cc
@@ -108,8 +108,8 @@ void ShellBrowserMainParts::PreMainMessageLoopRun() {
       false, net_log_.get()));
 
   // A ViewsDelegate is required.
-  if (!views::ViewsDelegate::views_delegate)
-    views::ViewsDelegate::views_delegate = new ShellViewsDelegate;
+  if (!views::ViewsDelegate::GetInstance())
+    views_delegate_.reset(new ShellViewsDelegate);
 
   delegate_ = new ash::shell::ShellDelegateImpl;
   // The global message center state must be initialized absent
@@ -159,6 +159,8 @@ void ShellBrowserMainParts::PostMainMessageLoopRun() {
 
   aura::Env::DeleteInstance();
 
+  views_delegate_.reset();
+
   // The keyboard may have created a WebContents. The WebContents is destroyed
   // with the UI, and it needs the BrowserContext to be alive during its
   // destruction. So destroy all of the UI elements before destroying the

diff --git a/ash/shell/content_client/shell_browser_main_parts.h b/ash/shell/content_client/shell_browser_main_parts.h
@@ -22,6 +22,10 @@ namespace net {
 class NetLog;
 }
 
+namespace views {
+class ViewsDelegate;
+}
+
 namespace wm {
 class WMState;
 }
@@ -53,6 +57,7 @@ class ShellBrowserMainParts : public content::BrowserMainParts {
  private:
   scoped_ptr<net::NetLog> net_log_;
   scoped_ptr<content::ShellBrowserContext> browser_context_;
+  scoped_ptr<views::ViewsDelegate> views_delegate_;
   scoped_ptr<ash::shell::WindowWatcher> window_watcher_;
   ShellDelegateImpl* delegate_;  // owned by Shell
   scoped_ptr<wm::WMState> wm_state_;

diff --git a/chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc b/chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc
@@ -22,8 +22,8 @@ ChromeBrowserMainExtraPartsViews::~ChromeBrowserMainExtraPartsViews() {
 void ChromeBrowserMainExtraPartsViews::ToolkitInitialized() {
   // The delegate needs to be set before any UI is created so that windows
   // display the correct icon.
-  if (!views::ViewsDelegate::views_delegate)
-    views::ViewsDelegate::views_delegate = new ChromeViewsDelegate;
+  if (!views::ViewsDelegate::GetInstance())
+    views_delegate_.reset(new ChromeViewsDelegate);
 
   SetConstrainedWindowViewsClient(CreateChromeConstrainedWindowViewsClient());
 

diff --git a/chrome/browser/ui/views/chrome_browser_main_extra_parts_views.h b/chrome/browser/ui/views/chrome_browser_main_extra_parts_views.h
@@ -9,6 +9,10 @@
 #include "base/memory/scoped_ptr.h"
 #include "chrome/browser/chrome_browser_main_extra_parts.h"
 
+namespace views {
+class ViewsDelegate;
+}
+
 #if defined(USE_AURA)
 namespace wm {
 class WMState;
@@ -24,6 +28,8 @@ class ChromeBrowserMainExtraPartsViews : public ChromeBrowserMainExtraParts {
   void ToolkitInitialized() override;
 
  private:
+  scoped_ptr<views::ViewsDelegate> views_delegate_;
+
 #if defined(USE_AURA)
   scoped_ptr<wm::WMState> wm_state_;
 #endif

diff --git a/chrome/browser/ui/views/find_bar_host_interactive_uitest.cc b/chrome/browser/ui/views/find_bar_host_interactive_uitest.cc
@@ -24,7 +24,6 @@
 #include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/views/focus/focus_manager.h"
 #include "ui/views/view.h"
-#include "ui/views/views_delegate.h"
 
 using base::ASCIIToUTF16;
 using content::WebContents;

diff --git a/chrome/browser/ui/views/frame/opaque_browser_frame_view.cc b/chrome/browser/ui/views/frame/opaque_browser_frame_view.cc
@@ -582,10 +582,10 @@ bool OpaqueBrowserFrameView::ShouldShowWindowTitleBar() const {
 
   // Do not show caption buttons if the window manager is forcefully providing a
   // title bar (e.g., in Ubuntu Unity, if the window is maximized).
-  if (!views::ViewsDelegate::views_delegate)
+  if (!views::ViewsDelegate::GetInstance())
     return true;
-  return !views::ViewsDelegate::views_delegate->WindowManagerProvidesTitleBar(
-              IsMaximized());
+  return !views::ViewsDelegate::GetInstance()->WindowManagerProvidesTitleBar(
+      IsMaximized());
 }
 
 void OpaqueBrowserFrameView::PaintRestoredFrameBorder(gfx::Canvas* canvas) {

diff --git a/chrome/browser/ui/views/menu_test_base.cc b/chrome/browser/ui/views/menu_test_base.cc
@@ -10,6 +10,7 @@
 #include "ui/views/controls/button/menu_button.h"
 #include "ui/views/controls/menu/menu_item_view.h"
 #include "ui/views/controls/menu/menu_runner.h"
+#include "ui/views/widget/widget.h"
 
 MenuTestBase::MenuTestBase()
     : ViewEventTestBase(),

diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_views.cc b/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
@@ -50,7 +50,6 @@
 #include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/ime/input_method.h"
 #include "ui/views/layout/fill_layout.h"
-#include "ui/views/views_delegate.h"
 #include "ui/views/widget/widget.h"
 #include "url/gurl.h"
 

diff --git a/chrome/test/base/view_event_test_base.cc b/chrome/test/base/view_event_test_base.cc
@@ -68,7 +68,6 @@ void ViewEventTestBase::SetUpTestCase() {
 }
 
 void ViewEventTestBase::SetUp() {
-  views::ViewsDelegate::views_delegate = &views_delegate_;
   ui::InitializeInputMethodForTesting();
 
   // The ContextFactory must exist before any Compositors are created.
@@ -97,7 +96,6 @@ void ViewEventTestBase::TearDown() {
   ui::TerminateContextFactoryForTests();
 
   ui::ShutdownInputMethodForTesting();
-  views::ViewsDelegate::views_delegate = NULL;
 }
 
 bool ViewEventTestBase::CanResize() const {

diff --git a/ui/app_list/views/app_list_view_unittest.cc b/ui/app_list/views/app_list_view_unittest.cc
@@ -32,6 +32,7 @@
 #include "ui/app_list/views/tile_item_view.h"
 #include "ui/events/event_utils.h"
 #include "ui/views/controls/textfield/textfield.h"
+#include "ui/views/test/test_views_delegate.h"
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/views_delegate.h"
 #include "ui/views/widget/desktop_aura/desktop_native_widget_aura.h"
@@ -705,7 +706,7 @@ class AppListViewTestDesktop : public views::ViewsTestBase,
 
   // testing::Test overrides:
   void SetUp() override {
-    set_views_delegate(new AppListViewTestViewsDelegate(this));
+    set_views_delegate(make_scoped_ptr(new AppListViewTestViewsDelegate(this)));
     views::ViewsTestBase::SetUp();
     test_context_.reset(new AppListViewTestContext(GetParam(), NULL));
   }

diff --git a/ui/message_center/views/message_popup_collection.cc b/ui/message_center/views/message_popup_collection.cc
@@ -171,8 +171,8 @@ void MessagePopupCollection::UpdateWidgets() {
     else
       base -= view_height + kToastMarginY;
 
-    if (views::ViewsDelegate::views_delegate) {
-      views::ViewsDelegate::views_delegate->NotifyAccessibilityEvent(
+    if (views::ViewsDelegate::GetInstance()) {
+      views::ViewsDelegate::GetInstance()->NotifyAccessibilityEvent(
           toast, ui::AX_EVENT_ALERT);
     }
 

diff --git a/ui/views/accessibility/native_view_accessibility_auralinux.cc b/ui/views/accessibility/native_view_accessibility_auralinux.cc
@@ -119,17 +119,17 @@ class AuraLinuxApplication
   AuraLinuxApplication()
       : platform_node_(ui::AXPlatformNode::Create(this)) {
     data_.role = ui::AX_ROLE_APPLICATION;
-    if (ViewsDelegate::views_delegate) {
+    if (ViewsDelegate::GetInstance()) {
       data_.AddStringAttribute(
           ui::AX_ATTR_NAME,
-          ViewsDelegate::views_delegate->GetApplicationName());
+          ViewsDelegate::GetInstance()->GetApplicationName());
     }
     ui::AXPlatformNodeAuraLinux::SetApplication(platform_node_);
-    if (ViewsDelegate::views_delegate) {
+    if (ViewsDelegate::GetInstance()) {
       // This should be on the a blocking pool thread so that we can open
       // libatk-bridge.so without blocking this thread.
       scoped_refptr<base::TaskRunner> init_task_runner =
-          ViewsDelegate::views_delegate->GetBlockingPoolTaskRunner();
+          ViewsDelegate::GetInstance()->GetBlockingPoolTaskRunner();
       if (init_task_runner)
         ui::AXPlatformNodeAuraLinux::StaticInitialize(init_task_runner);
     }

diff --git a/ui/views/bubble/bubble_border_unittest.cc b/ui/views/bubble/bubble_border_unittest.cc
@@ -6,6 +6,7 @@
 
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/stringprintf.h"
+#include "ui/gfx/geometry/rect.h"
 #include "ui/views/test/views_test_base.h"
 
 namespace views {

diff --git a/ui/views/cocoa/bridged_native_widget.mm b/ui/views/cocoa/bridged_native_widget.mm
@@ -932,10 +932,10 @@ void SetupDragEventMonitor() {
 void BridgedNativeWidget::CreateCompositor() {
   DCHECK(!compositor_);
   DCHECK(!compositor_widget_);
-  DCHECK(ViewsDelegate::views_delegate);
+  DCHECK(ViewsDelegate::GetInstance());
 
   ui::ContextFactory* context_factory =
-      ViewsDelegate::views_delegate->GetContextFactory();
+      ViewsDelegate::GetInstance()->GetContextFactory();
   DCHECK(context_factory);
 
   AddCompositorSuperview();

diff --git a/ui/views/controls/menu/menu_controller.cc b/ui/views/controls/menu/menu_controller.cc
@@ -361,8 +361,8 @@ MenuItemView* MenuController::Run(Widget* parent,
     pressed_lock_.reset(new MenuButton::PressedLock(button));
 
   // Make sure Chrome doesn't attempt to shut down while the menu is showing.
-  if (ViewsDelegate::views_delegate)
-    ViewsDelegate::views_delegate->AddRef();
+  if (ViewsDelegate::GetInstance())
+    ViewsDelegate::GetInstance()->AddRef();
 
   // We need to turn on nestable tasks as in some situations (pressing alt-f for
   // one) the menus are run from a task. If we don't do this and are invoked
@@ -373,8 +373,8 @@ MenuItemView* MenuController::Run(Widget* parent,
   RunMessageLoop(nested_menu);
   message_loop_depth_--;
 
-  if (ViewsDelegate::views_delegate)
-    ViewsDelegate::views_delegate->ReleaseRef();
+  if (ViewsDelegate::GetInstance())
+    ViewsDelegate::GetInstance()->ReleaseRef();
 
   // Close any open menus.
   SetSelection(NULL, SELECTION_UPDATE_IMMEDIATELY | SELECTION_EXIT);
@@ -2125,8 +2125,8 @@ void MenuController::RepostEvent(SubmenuView* source,
   // coordinates to be in pixels.
   // PostMessage() to metro windows isn't allowed (access will be denied). Don't
   // try to repost with Win32 if the window under the mouse press is in metro.
-  if (!ViewsDelegate::views_delegate ||
-      !ViewsDelegate::views_delegate->IsWindowInMetro(window)) {
+  if (!ViewsDelegate::GetInstance() ||
+      !ViewsDelegate::GetInstance()->IsWindowInMetro(window)) {
     gfx::Point screen_loc_pixels = gfx::win::DIPToScreenPoint(screen_loc);
     HWND target_window = window ? HWNDForNativeWindow(window) :
                                   WindowFromPoint(screen_loc_pixels.ToPOINT());

diff --git a/ui/views/controls/menu/menu_model_adapter.cc b/ui/views/controls/menu/menu_model_adapter.cc
@@ -9,7 +9,6 @@
 #include "ui/gfx/image/image.h"
 #include "ui/views/controls/menu/menu_item_view.h"
 #include "ui/views/controls/menu/submenu_view.h"
-#include "ui/views/views_delegate.h"
 
 namespace views {
 

diff --git a/ui/views/controls/textfield/textfield.cc b/ui/views/controls/textfield/textfield.cc
@@ -284,9 +284,10 @@ Textfield::Textfield()
   SetBorder(scoped_ptr<Border>(new FocusableBorder()));
   SetFocusable(true);
 
-  if (ViewsDelegate::views_delegate) {
-    password_reveal_duration_ = ViewsDelegate::views_delegate->
-        GetDefaultTextfieldObscuredRevealDuration();
+  if (ViewsDelegate::GetInstance()) {
+    password_reveal_duration_ =
+        ViewsDelegate::GetInstance()
+            ->GetDefaultTextfieldObscuredRevealDuration();
   }
 }
 

diff --git a/ui/views/controls/webview/webview.cc b/ui/views/controls/webview/webview.cc
@@ -407,9 +407,9 @@ void WebView::NotifyMaybeTextInputClientAndAccessibilityChanged() {
 content::WebContents* WebView::CreateWebContents(
       content::BrowserContext* browser_context) {
   content::WebContents* contents = NULL;
-  if (ViewsDelegate::views_delegate) {
-    contents = ViewsDelegate::views_delegate->CreateWebContents(
-        browser_context, NULL);
+  if (ViewsDelegate::GetInstance()) {
+    contents =
+        ViewsDelegate::GetInstance()->CreateWebContents(browser_context, NULL);
   }
 
   if (!contents) {

diff --git a/ui/views/controls/webview/webview_interactive_uitest.cc b/ui/views/controls/webview/webview_interactive_uitest.cc
@@ -44,8 +44,7 @@ class WebViewInteractiveUiTest : public views::test::WidgetTest {
 
   void SetUp() override {
     gfx::GLSurface::InitializeOneOffForTests();
-    // The ViewsDelegate is deleted when the ViewsTestBase class is torn down.
-    WidgetTest::set_views_delegate(new WebViewTestViewsDelegate);
+    set_views_delegate(make_scoped_ptr(new WebViewTestViewsDelegate));
     WidgetTest::SetUp();
   }
 

diff --git a/ui/views/controls/webview/webview_unittest.cc b/ui/views/controls/webview/webview_unittest.cc
@@ -129,8 +129,7 @@ class WebViewUnitTest : public views::test::WidgetTest {
   ~WebViewUnitTest() override {}
 
   void SetUp() override {
-    // The ViewsDelegate is deleted when the ViewsTestBase class is torn down.
-    WidgetTest::set_views_delegate(new WebViewTestViewsDelegate);
+    set_views_delegate(make_scoped_ptr(new WebViewTestViewsDelegate));
     browser_context_.reset(new content::TestBrowserContext);
     WidgetTest::SetUp();
     // Set the test content browser client to avoid pulling in needless
@@ -176,7 +175,6 @@ class WebViewUnitTest : public views::test::WidgetTest {
   content::TestBrowserThread file_blocking_thread_;
   content::TestBrowserThread io_thread_;
   scoped_ptr<content::TestBrowserContext> browser_context_;
-  scoped_ptr<WebViewTestViewsDelegate> views_delegate_;
   content::TestContentBrowserClient test_browser_client_;
 
   Widget* top_level_widget_;

diff --git a/ui/views/test/test_views_delegate_aura.cc b/ui/views/test/test_views_delegate_aura.cc
@@ -18,16 +18,12 @@ TestViewsDelegate::TestViewsDelegate()
     : context_factory_(nullptr),
       use_desktop_native_widgets_(false),
       use_transparent_windows_(false) {
-  DCHECK(!ViewsDelegate::views_delegate);
-  ViewsDelegate::views_delegate = this;
 #if defined(USE_AURA)
   wm_state_.reset(new wm::WMState);
 #endif
 }
 
 TestViewsDelegate::~TestViewsDelegate() {
-  if (ViewsDelegate::views_delegate == this)
-    ViewsDelegate::views_delegate = NULL;
 }
 
 #if defined(OS_WIN)

diff --git a/ui/views/test/test_views_delegate_mac.mm b/ui/views/test/test_views_delegate_mac.mm
@@ -12,13 +12,9 @@
     : context_factory_(nullptr),
       use_desktop_native_widgets_(false),
       use_transparent_windows_(false) {
-  DCHECK(!ViewsDelegate::views_delegate);
-  ViewsDelegate::views_delegate = this;
 }
 
 TestViewsDelegate::~TestViewsDelegate() {
-  if (ViewsDelegate::views_delegate == this)
-    ViewsDelegate::views_delegate = NULL;
 }
 
 void TestViewsDelegate::OnBeforeWidgetInit(

diff --git a/ui/views/test/views_test_base.cc b/ui/views/test/views_test_base.cc
@@ -27,7 +27,7 @@ ViewsTestBase::~ViewsTestBase() {
 void ViewsTestBase::SetUp() {
   testing::Test::SetUp();
   setup_called_ = true;
-  if (!views_delegate_.get())
+  if (!views_delegate_)
     views_delegate_.reset(new TestViewsDelegate());
 
   // The ContextFactory must exist before any Compositors are created.
@@ -53,6 +53,8 @@ void ViewsTestBase::TearDown() {
   ui::ShutdownInputMethodForTesting();
   test_helper_->TearDown();
   ui::TerminateContextFactoryForTests();
+
+  views_delegate_.reset();
 }
 
 void ViewsTestBase::RunPendingMessages() {