Move Chrome terminating notification out of //apps

Part of removing the chrome dependency from //apps. Make the embedder (chrome) extension system notify the app restore service when it will terminate, instead of relying on a Chrome notification via NotificationService. This builds on https://codereview.chromium.org/425303002 to some extent. It would be nicer not to use the Chrome notification at all, but I'm not sure how to do that -- at least this way, the responsibility is in c/b/extensions rather than elsewhere in Chrome. R=benwells@chromium.org BUG=679971 Review-Url: https://codereview.chromium.org/2623273011 Cr-Commit-Position: refs/heads/master@{#443643}
hylschwy · Jan 13, 2017 · 2fc6af9 · 2fc6af9
1 parent 4480968
commit 2fc6af9
Show file tree

Hide file tree

Showing 9 changed files with 34 additions and 36 deletions.
diff --git a/apps/app_lifetime_monitor.cc b/apps/app_lifetime_monitor.cc
@@ -4,7 +4,6 @@
 
 #include "apps/app_lifetime_monitor.h"
 
-#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
@@ -28,9 +27,6 @@ AppLifetimeMonitor::AppLifetimeMonitor(Profile* profile)
   registrar_.Add(this,
                  extensions::NOTIFICATION_EXTENSION_HOST_DESTROYED,
                  content::NotificationService::AllSources());
-  registrar_.Add(
-      this, chrome::NOTIFICATION_APP_TERMINATING,
-      content::NotificationService::AllSources());
 
   AppWindowRegistry* app_window_registry =
       AppWindowRegistry::Factory::GetForBrowserContext(profile_,
@@ -72,11 +68,6 @@ void AppLifetimeMonitor::Observe(int type,
       NotifyAppStop(extension->id());
       break;
     }
-
-    case chrome::NOTIFICATION_APP_TERMINATING: {
-      NotifyChromeTerminating();
-      break;
-    }
   }
 }
 
@@ -144,9 +135,4 @@ void AppLifetimeMonitor::NotifyAppStop(const std::string& app_id) {
     observer.OnAppStop(profile_, app_id);
 }
 
-void AppLifetimeMonitor::NotifyChromeTerminating() {
-  for (auto& observer : observers_)
-    observer.OnChromeTerminating();
-}
-
 }  // namespace apps
diff --git a/apps/app_lifetime_monitor.h b/apps/app_lifetime_monitor.h
@@ -37,10 +37,6 @@ class AppLifetimeMonitor : public KeyedService,
     }
     // Called when the app stops running.
     virtual void OnAppStop(Profile* profile, const std::string& app_id) {}
-    // Called when chrome is about to terminate. This gives observers a chance
-    // to do something before the apps shut down. This is a system-wide event
-    // so there is no associated profile and app id.
-    virtual void OnChromeTerminating() {}
 
    protected:
     virtual ~Observer() {}
@@ -73,7 +69,6 @@ class AppLifetimeMonitor : public KeyedService,
   void NotifyAppActivated(const std::string& app_id);
   void NotifyAppDeactivated(const std::string& app_id);
   void NotifyAppStop(const std::string& app_id);
-  void NotifyChromeTerminating();
 
   content::NotificationRegistrar registrar_;
   Profile* profile_;

diff --git a/apps/app_restore_service.cc b/apps/app_restore_service.cc
@@ -66,6 +66,12 @@ bool AppRestoreService::IsAppRestorable(const std::string& extension_id) {
   return ExtensionPrefs::Get(profile_)->IsExtensionRunning(extension_id);
 }
 
+void AppRestoreService::OnApplicationTerminating() {
+  // We want to preserve the state when the app begins terminating, so stop
+  // listening to app lifetime events.
+  StopObservingAppLifetime();
+}
+
 // static
 AppRestoreService* AppRestoreService::Get(Profile* profile) {
   return apps::AppRestoreServiceFactory::GetForProfile(profile);
@@ -90,12 +96,6 @@ void AppRestoreService::OnAppStop(Profile* profile, const std::string& app_id) {
   RecordAppStop(app_id);
 }
 
-void AppRestoreService::OnChromeTerminating() {
-  // We want to preserve the state when the app begins terminating, so stop
-  // listening to app lifetime events.
-  StopObservingAppLifetime();
-}
-
 void AppRestoreService::Shutdown() {
   StopObservingAppLifetime();
 }

diff --git a/apps/app_restore_service.h b/apps/app_restore_service.h
@@ -38,6 +38,9 @@ class AppRestoreService : public KeyedService,
   // running when Chrome was last terminated.
   bool IsAppRestorable(const std::string& extension_id);
 
+  // Called to notify that the application has begun to exit.
+  void OnApplicationTerminating();
+
   static AppRestoreService* Get(Profile* profile);
 
  private:
@@ -46,7 +49,6 @@ class AppRestoreService : public KeyedService,
   void OnAppActivated(Profile* profile, const std::string& app_id) override;
   void OnAppDeactivated(Profile* profile, const std::string& app_id) override;
   void OnAppStop(Profile* profile, const std::string& app_id) override;
-  void OnChromeTerminating() override;
 
   // KeyedService.
   void Shutdown() override;

diff --git a/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc b/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc
@@ -735,8 +735,6 @@ void ExtensionAppShimHandler::OnAppDeactivated(Profile* profile,
 void ExtensionAppShimHandler::OnAppStop(Profile* profile,
                                         const std::string& app_id) {}
 
-void ExtensionAppShimHandler::OnChromeTerminating() {}
-
 // The BrowserWindow may be NULL when this is called.
 // Therefore we listen for the notification
 // chrome::NOTIFICATION_BROWSER_WINDOW_READY and then call OnAppActivated.

diff --git a/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h b/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h
@@ -129,7 +129,6 @@ class ExtensionAppShimHandler : public AppShimHandler,
   void OnAppActivated(Profile* profile, const std::string& app_id) override;
   void OnAppDeactivated(Profile* profile, const std::string& app_id) override;
   void OnAppStop(Profile* profile, const std::string& app_id) override;
-  void OnChromeTerminating() override;
 
   // content::NotificationObserver overrides:
   void Observe(int type,

diff --git a/chrome/browser/extensions/DEPS b/chrome/browser/extensions/DEPS
@@ -1,10 +1,4 @@
 include_rules = [
-  # TODO(benwells): Once the extensions component is established
-  # and there are only chrome specific extension things left in
-  # chrome/browser/extensions, the restriction of not being able
-  # to depend on apps will be lifted.
-  "-apps",
-  "-chrome/browser/apps",
   "+components/chrome_apps",
   "+components/crx_file",
   "+components/strings/grit/components_strings.h",

diff --git a/chrome/browser/extensions/extension_system_impl.cc b/chrome/browser/extensions/extension_system_impl.cc
@@ -6,6 +6,8 @@
 
 #include <algorithm>
 
+#include "apps/app_restore_service.h"
+#include "apps/app_restore_service_factory.h"
 #include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/command_line.h"
@@ -16,6 +18,7 @@
 #include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/chrome_app_sorting.h"
 #include "chrome/browser/extensions/chrome_content_verifier_delegate.h"
 #include "chrome/browser/extensions/component_loader.h"
@@ -40,6 +43,7 @@
 #include "chrome/browser/ui/webui/extensions/extension_icon_source.h"
 #include "chrome/common/chrome_switches.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/browser/url_data_source.h"
 #include "extensions/browser/content_verifier.h"
 #include "extensions/browser/extension_pref_store.h"
@@ -97,6 +101,8 @@ UninstallPingSender::FilterResult ShouldSendUninstallPing(
 
 ExtensionSystemImpl::Shared::Shared(Profile* profile)
     : profile_(profile) {
+  registrar_.Add(this, chrome::NOTIFICATION_APP_TERMINATING,
+                 content::NotificationService::AllSources());
 }
 
 ExtensionSystemImpl::Shared::~Shared() {
@@ -324,6 +330,16 @@ ContentVerifier* ExtensionSystemImpl::Shared::content_verifier() {
   return content_verifier_.get();
 }
 
+void ExtensionSystemImpl::Shared::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK_EQ(chrome::NOTIFICATION_APP_TERMINATING, type);
+  CHECK(apps::AppRestoreServiceFactory::GetForProfile(profile_));
+  apps::AppRestoreServiceFactory::GetForProfile(profile_)
+      ->OnApplicationTerminating();
+}
+
 //
 // ExtensionSystemImpl
 //

diff --git a/chrome/browser/extensions/extension_system_impl.h b/chrome/browser/extensions/extension_system_impl.h
@@ -9,6 +9,8 @@
 
 #include "base/macros.h"
 #include "build/build_config.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/common/one_shot_event.h"
 
@@ -78,7 +80,7 @@ class ExtensionSystemImpl : public ExtensionSystem {
 
   // Owns the Extension-related systems that have a single instance
   // shared between normal and incognito profiles.
-  class Shared : public KeyedService {
+  class Shared : public KeyedService, public content::NotificationObserver {
    public:
     explicit Shared(Profile* profile);
     ~Shared() override;
@@ -108,10 +110,16 @@ class ExtensionSystemImpl : public ExtensionSystem {
     ContentVerifier* content_verifier();
 
    private:
+    // content::NotificationObserver implementation.
+    void Observe(int type,
+                 const content::NotificationSource& source,
+                 const content::NotificationDetails& details) override;
+
     Profile* profile_;
 
     // The services that are shared between normal and incognito profiles.
 
+    content::NotificationRegistrar registrar_;
     std::unique_ptr<StateStore> state_store_;
     std::unique_ptr<StateStoreNotificationObserver>
         state_store_notification_observer_;