Delete activity upon window deletion

 (there was no leak reported because they're deleted upon shutdown)

A few minor cleanup/improvements
* Added shortcut to close athena. This doesn't update overview/split view mode.
  I'll address it in separate CL.
* Delete unnecessary athena_content_support_lib target. (I somehow missed this in review)
* Separate app_shell specific impl in extensions_delegate  (this is prep for athena+chrome)
* Remove ActivityWidget wrapper and use views::Widget instead, as it seems to be unnecessary. This is also prep for athena+chrome, where AppActivity should use NativeAppWindowViews.

BUG=None
TEST=coverted by unittests.
TBR=jamescook@chromium.org

Review URL: https://codereview.chromium.org/498023003

Cr-Commit-Position: refs/heads/master@{#291935}

athena/activity/activity_manager_impl.cc

@@ -2,10 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "athena/activity/public/activity_manager.h"
+#include "athena/activity/activity_manager_impl.h"
 
 #include <algorithm>
-#include <vector>
 
 #include "athena/activity/public/activity.h"
 #include "athena/activity/public/activity_view_manager.h"
@@ -17,54 +16,43 @@ namespace {
 
 ActivityManager* instance = NULL;
 
-class ActivityManagerImpl : public ActivityManager {
- public:
-  ActivityManagerImpl() {
+}  // namespace
+
+ActivityManagerImpl::ActivityManagerImpl() {
   CHECK(!instance);
   instance = this;
-  }
-  virtual ~ActivityManagerImpl() {
+}
+
+ActivityManagerImpl::~ActivityManagerImpl() {
   while (!activities_.empty())
     delete activities_.front();
 
   CHECK_EQ(this, instance);
   instance = NULL;
-  }
+}
 
-  // ActivityManager:
-  virtual void AddActivity(Activity* activity) OVERRIDE {
-    CHECK(activities_.end() == std::find(activities_.begin(),
-                                         activities_.end(),
-                                         activity));
+void ActivityManagerImpl::AddActivity(Activity* activity) {
+  CHECK(activities_.end() ==
+        std::find(activities_.begin(), activities_.end(), activity));
   activities_.push_back(activity);
   ActivityViewManager* manager = ActivityViewManager::Get();
   manager->AddActivity(activity);
-  }
+}
 
-  virtual void RemoveActivity(Activity* activity) OVERRIDE {
-    std::vector<Activity*>::iterator find = std::find(activities_.begin(),
-                                                      activities_.end(),
-                                                      activity);
+void ActivityManagerImpl::RemoveActivity(Activity* activity) {
+  std::vector<Activity*>::iterator find =
+      std::find(activities_.begin(), activities_.end(), activity);
   if (find != activities_.end()) {
     activities_.erase(find);
-
     ActivityViewManager* manager = ActivityViewManager::Get();
     manager->RemoveActivity(activity);
   }
-  }
+}
 
-  virtual void UpdateActivity(Activity* activity) OVERRIDE {
+void ActivityManagerImpl::UpdateActivity(Activity* activity) {
   ActivityViewManager* manager = ActivityViewManager::Get();
   manager->UpdateActivity(activity);
-  }
-
- private:
-  std::vector<Activity*> activities_;
-
-  DISALLOW_COPY_AND_ASSIGN(ActivityManagerImpl);
-};
-
-}  // namespace
+}
 
 // static
 ActivityManager* ActivityManager::Create() {

athena/activity/activity_manager_impl.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "athena/activity/public/activity_manager.h"
+
+#include <vector>
+
+#include "base/macros.h"
+
+namespace athena {
+
+class ActivityManagerImpl : public ActivityManager {
+ public:
+  ActivityManagerImpl();
+  virtual ~ActivityManagerImpl();
+
+  int num_activities() const { return activities_.size(); }
+
+  // ActivityManager:
+  virtual void AddActivity(Activity* activity) OVERRIDE;
+  virtual void RemoveActivity(Activity* activity) OVERRIDE;
+  virtual void UpdateActivity(Activity* activity) OVERRIDE;
+
+ private:
+  std::vector<Activity*> activities_;
+
+  DISALLOW_COPY_AND_ASSIGN(ActivityManagerImpl);
+};
+
+}  // namespace athena

athena/activity/activity_manager_unittest.cc

@@ -2,14 +2,36 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "athena/activity/public/activity_manager.h"
+#include "athena/activity/activity_manager_impl.h"
 
+#include "athena/activity/public/activity.h"
 #include "athena/activity/public/activity_factory.h"
 #include "athena/test/athena_test_base.h"
+#include "ui/aura/window.h"
 
-typedef athena::test::AthenaTestBase ActivityManagerTest;
+namespace athena {
+
+typedef test::AthenaTestBase ActivityManagerTest;
 
 TEST_F(ActivityManagerTest, Basic) {
-  athena::ActivityManager::Get()->AddActivity(
+  ActivityManagerImpl* activity_manager =
+      static_cast<ActivityManagerImpl*>(ActivityManager::Get());
+  scoped_ptr<Activity> activity1(
       athena::ActivityFactory::Get()->CreateWebActivity(NULL, GURL()));
+  activity_manager->AddActivity(activity1.get());
+  EXPECT_EQ(1, activity_manager->num_activities());
+
+  Activity* activity2 =
+      athena::ActivityFactory::Get()->CreateWebActivity(NULL, GURL());
+  activity_manager->AddActivity(activity2);
+  EXPECT_EQ(2, activity_manager->num_activities());
+
+  activity1.reset();
+  EXPECT_EQ(1, activity_manager->num_activities());
+
+  // Deleting the activity's window should delete the activity itself.
+  delete activity2->GetWindow();
+  EXPECT_EQ(0, activity_manager->num_activities());
 }
+
+}  // namespace athena

athena/activity/activity_view_manager_impl.cc

@@ -9,59 +9,40 @@
 
 #include "athena/activity/activity_widget_delegate.h"
 #include "athena/activity/public/activity.h"
+#include "athena/activity/public/activity_manager.h"
 #include "athena/activity/public/activity_view_model.h"
 #include "athena/screen/public/screen_manager.h"
 #include "ui/aura/window.h"
 #include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
+#include "ui/views/widget/widget_observer.h"
 
 namespace athena {
 namespace {
 
-class ActivityWidget {
- public:
-  explicit ActivityWidget(Activity* activity)
-      : activity_(activity), widget_(NULL) {
+typedef std::map<Activity*, views::Widget*> ActivityWidgetMap;
+
+views::Widget* CreateWidget(Activity* activity) {
   ActivityViewModel* view_model = activity->GetActivityViewModel();
-    widget_ = new views::Widget;
+  views::Widget* widget = new views::Widget;
   views::Widget::InitParams params(views::Widget::InitParams::TYPE_WINDOW);
-    params.context = ScreenManager::Get()->GetContext();
   params.delegate = new ActivityWidgetDelegate(view_model);
   params.activatable = views::Widget::InitParams::ACTIVATABLE_YES;
-    widget_->Init(params);
-    activity_->GetActivityViewModel()->Init();
-  }
-
-  virtual ~ActivityWidget() {
-    widget_->Close();
-  }
-
-  void Show() {
-    Update();
-    widget_->Show();
-    widget_->Activate();
-  }
-
-  void Update() {
-    widget_->UpdateWindowTitle();
-  }
-
- private:
-
-  Activity* activity_;
-  views::Widget* widget_;
-
-  DISALLOW_COPY_AND_ASSIGN(ActivityWidget);
-};
+  widget->Init(params);
+  activity->GetActivityViewModel()->Init();
+  return widget;
+}
 
 ActivityViewManager* instance = NULL;
 
-class ActivityViewManagerImpl : public ActivityViewManager {
+class ActivityViewManagerImpl : public ActivityViewManager,
+                                public views::WidgetObserver {
  public:
   ActivityViewManagerImpl() {
     CHECK(!instance);
     instance = this;
   }
+
   virtual ~ActivityViewManagerImpl() {
     CHECK_EQ(this, instance);
     instance = NULL;
@@ -70,30 +51,44 @@ class ActivityViewManagerImpl : public ActivityViewManager {
   // ActivityViewManager:
   virtual void AddActivity(Activity* activity) OVERRIDE {
     CHECK(activity_widgets_.end() == activity_widgets_.find(activity));
-    ActivityWidget* container = new ActivityWidget(activity);
+    views::Widget* container = CreateWidget(activity);
+    container->AddObserver(this);
     activity_widgets_[activity] = container;
+    container->UpdateWindowTitle();
     container->Show();
+    container->Activate();
   }
 
   virtual void RemoveActivity(Activity* activity) OVERRIDE {
-    std::map<Activity*, ActivityWidget*>::iterator find =
-        activity_widgets_.find(activity);
+    ActivityWidgetMap::iterator find = activity_widgets_.find(activity);
     if (find != activity_widgets_.end()) {
-      ActivityWidget* widget = find->second;
+      views::Widget* widget = find->second;
+      widget->RemoveObserver(this);
+      widget->Close();
       activity_widgets_.erase(activity);
-      delete widget;
     }
   }
 
   virtual void UpdateActivity(Activity* activity) OVERRIDE {
-    std::map<Activity*, ActivityWidget*>::iterator find =
-        activity_widgets_.find(activity);
+    ActivityWidgetMap::iterator find = activity_widgets_.find(activity);
     if (find != activity_widgets_.end())
-      find->second->Update();
+      find->second->UpdateWindowTitle();
+  }
+
+  // views::WidgetObserver:
+  virtual void OnWidgetDestroying(views::Widget* widget) OVERRIDE {
+    for (ActivityWidgetMap::iterator iter = activity_widgets_.begin();
+         iter != activity_widgets_.end();
+         ++iter) {
+      if (iter->second == widget) {
+        delete iter->first;
+        break;
+      }
+    }
   }
 
  private:
-  std::map<Activity*, ActivityWidget*> activity_widgets_;
+  ActivityWidgetMap activity_widgets_;
 
   DISALLOW_COPY_AND_ASSIGN(ActivityViewManagerImpl);
 };

athena/athena.gyp

@@ -32,6 +32,7 @@
         'activity/activity.cc',
         'activity/activity_factory.cc',
         'activity/activity_manager_impl.cc',
+        'activity/activity_manager_impl.h',
         'activity/activity_view_manager_impl.cc',
         'activity/activity_frame_view.cc',
         'activity/activity_frame_view.h',
@@ -115,9 +116,6 @@
         '../ui/views/controls/webview/webview.gyp:webview',
         '../skia/skia.gyp:skia',
       ],
-      'defines': [
-        'ATHENA_IMPLEMENTATION',
-      ],
       'sources': [
         'content/app_activity.cc',
         'content/app_activity.h',
@@ -128,6 +126,8 @@
         'content/app_registry_impl.cc',
         'content/content_activity_factory.cc',
         'content/content_app_model_builder.cc',
+        'content/delegate/app_content_control_delegate_impl.cc',
+        'content/public/app_content_control_delegate.h',
         'content/public/app_registry.h',
         'content/public/content_activity_factory.h',
         'content/public/content_app_model_builder.h',
@@ -138,20 +138,19 @@
         'content/web_activity.h',
         'content/web_contents_view_delegate_factory_impl.cc',
         'extensions/public/extensions_delegate.h',
-        'extensions/extensions_delegate_impl.cc',
+        'extensions/extensions_delegate.cc',
         'virtual_keyboard/public/virtual_keyboard_manager.h',
         'virtual_keyboard/virtual_keyboard_manager_impl.cc',
       ],
     },
     {
-      'target_name': 'athena_content_support_lib',
+      'target_name': 'athena_app_shell_lib',
       'type': 'static_library',
       'dependencies': [
-        '../content/content.gyp:content_browser',
+        '../extensions/shell/app_shell.gyp:app_shell_lib',
       ],
       'sources': [
-        'content/delegate/app_content_control_delegate_impl.cc',        
-        'content/public/app_content_control_delegate.h',
+        'extensions/shell/extensions_delegate_impl.cc',
       ],
     },
     {

athena/content/web_activity.cc

@@ -27,6 +27,7 @@ class WebActivityController : public AcceleratorHandler {
     CMD_FORWARD,
     CMD_RELOAD,
     CMD_RELOAD_IGNORE_CACHE,
+    CMD_CLOSE,
   };
 
   explicit WebActivityController(views::WebView* web_view)
@@ -48,6 +49,7 @@ class WebActivityController : public AcceleratorHandler {
          AF_NONE},
         {TRIGGER_ON_PRESS, ui::VKEY_BROWSER_BACK, ui::EF_NONE, CMD_BACK,
          AF_NONE},
+        {TRIGGER_ON_PRESS, ui::VKEY_W, ui::EF_CONTROL_DOWN, CMD_CLOSE, AF_NONE},
     };
     accelerator_manager_->RegisterAccelerators(
         accelerator_data, arraysize(accelerator_data), this);
@@ -85,11 +87,15 @@ class WebActivityController : public AcceleratorHandler {
   virtual bool IsCommandEnabled(int command_id) const OVERRIDE {
     switch (command_id) {
       case CMD_RELOAD:
+      case CMD_RELOAD_IGNORE_CACHE:
         return true;
       case CMD_BACK:
         return web_view_->GetWebContents()->GetController().CanGoBack();
       case CMD_FORWARD:
         return web_view_->GetWebContents()->GetController().CanGoForward();
+      case CMD_CLOSE:
+        // TODO(oshima): check onbeforeunload handler.
+        return true;
     }
     return false;
   }
@@ -109,6 +115,9 @@ class WebActivityController : public AcceleratorHandler {
       case CMD_FORWARD:
         web_view_->GetWebContents()->GetController().GoForward();
         return true;
+      case CMD_CLOSE:
+        web_view_->GetWidget()->Close();
+        return true;
     }
     return false;
   }

athena/extensions/DEPS

 include_rules = [
   "+extensions/common",
-  "+extensions/shell/browser",
 ]

athena/extensions/extensions_delegate.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "athena/extensions/public/extensions_delegate.h"
+
+#include "base/logging.h"
+
+namespace athena {
+namespace {
+
+ExtensionsDelegate* instance = NULL;
+
+}  // namespace
+
+ExtensionsDelegate::ExtensionsDelegate() {
+  DCHECK(!instance);
+  instance = this;
+}
+
+ExtensionsDelegate::~ExtensionsDelegate() {
+  DCHECK(instance);
+  instance = NULL;
+}
+
+// static
+ExtensionsDelegate* ExtensionsDelegate::Get(content::BrowserContext* context) {
+  DCHECK(instance);
+  DCHECK_EQ(context, instance->GetBrowserContext());
+  return instance;
+}
+
+// static
+void ExtensionsDelegate::Shutdown() {
+  DCHECK(instance);
+  delete instance;
+}
+
+}  // namespace athena

athena/extensions/shell/DEPS

+include_rules = [
+  "+extensions/shell/browser",
+]

athena/extensions/extensions_delegate_impl.cc → athena/extensions/shell/extensions_delegate_impl.cc

@@ -10,8 +10,6 @@
 namespace athena {
 namespace {
 
-ExtensionsDelegate* instance = NULL;
-
 class ShellExtensionsDelegate : public ExtensionsDelegate {
  public:
   explicit ShellExtensionsDelegate(content::BrowserContext* context)
@@ -45,33 +43,10 @@ class ShellExtensionsDelegate : public ExtensionsDelegate {
 
 }  // namespace
 
-ExtensionsDelegate::ExtensionsDelegate() {
-  DCHECK(!instance);
-  instance = this;
-}
-
-ExtensionsDelegate::~ExtensionsDelegate() {
-  DCHECK(instance);
-  instance = NULL;
-}
-
-// static
-ExtensionsDelegate* ExtensionsDelegate::Get(content::BrowserContext* context) {
-  DCHECK(instance);
-  DCHECK_EQ(context, instance->GetBrowserContext());
-  return instance;
-}
-
 // static
 void ExtensionsDelegate::CreateExtensionsDelegateForShell(
     content::BrowserContext* context) {
   new ShellExtensionsDelegate(context);
 }
 
-// static
-void ExtensionsDelegate::Shutdown() {
-  DCHECK(instance);
-  delete instance;
-}
-
 }  // namespace athena

athena/main/athena_launcher.cc

@@ -89,6 +89,7 @@ class AthenaViewsDelegate : public views::ViewsDelegate {
   virtual void OnBeforeWidgetInit(
       views::Widget::InitParams* params,
       views::internal::NativeWidgetDelegate* delegate) OVERRIDE {
+    params->context = athena::ScreenManager::Get()->GetContext();
   }
 
   DISALLOW_COPY_AND_ASSIGN(AthenaViewsDelegate);
@@ -130,7 +131,6 @@ void StartAthenaEnv(scoped_refptr<base::TaskRunner> file_runner) {
 }
 
 void StartAthenaSessionWithContext(content::BrowserContext* context) {
-  athena::ExtensionsDelegate::CreateExtensionsDelegateForShell(context);
   StartAthenaSession(new athena::ContentActivityFactory(),
                      new athena::ContentAppModelBuilder(context));
   athena::VirtualKeyboardManager::Create(context);

athena/main/athena_main.cc

@@ -6,6 +6,8 @@
 #include "athena/activity/public/activity_manager.h"
 #include "athena/content/public/web_contents_view_delegate_creator.h"
 #include "athena/env/public/athena_env.h"
+#include "athena/extensions/public/extensions_delegate.h"
+#include "athena/main/athena_app_window_controller.h"
 #include "athena/main/athena_launcher.h"
 #include "athena/screen/public/screen_manager.h"
 #include "base/command_line.h"
@@ -87,6 +89,7 @@ class AthenaBrowserMainDelegate : public extensions::ShellBrowserMainDelegate {
 
     athena::StartAthenaEnv(content::BrowserThread::GetMessageLoopProxyForThread(
         content::BrowserThread::FILE));
+    athena::ExtensionsDelegate::CreateExtensionsDelegateForShell(context);
     athena::StartAthenaSessionWithContext(context);
   }
 

athena/main/athena_main.gyp

@@ -13,7 +13,6 @@
       'dependencies': [
         '../athena.gyp:athena_lib',
         '../athena.gyp:athena_content_lib',
-        '../athena.gyp:athena_content_support_lib',
         '../resources/athena_resources.gyp:athena_resources',
 	# debug_widow.cc depends on this. Remove this once debug_window
 	# is removed.
@@ -55,8 +54,8 @@
       'type': 'executable',
       'dependencies': [
         '../../ui/accessibility/accessibility.gyp:ax_gen',
+	'../athena.gyp:athena_app_shell_lib',
         '../resources/athena_resources.gyp:athena_pak',
-	'../../extensions/shell/app_shell.gyp:app_shell_lib',
         'athena_main_lib',
       ],
       'include_dirs': [