[Extensions + Apps] Move application termination observation to apps

Currently, the ExtensionsSystem watches for application termination, and
then forwards the notification along to the apps code. This causes a
dependency between the extensions system and apps.

Move the application termination observation to a dedicated KeyedService
in platform_apps code, and remove knowledge of platform apps from
the extensions system.

Bug: 873872
Change-Id: I2dc6ae147acc4bad428542f5c5ed31154355304b
Reviewed-on: https://chromium-review.googlesource.com/1173765Reviewed-by: Ben Wells <benwells@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583158}

chrome/browser/apps/platform_apps/BUILD.gn

@@ -14,6 +14,8 @@ source_set("platform_apps") {
     "app_load_service.h",
     "app_load_service_factory.cc",
     "app_load_service_factory.h",
+    "app_termination_observer.cc",
+    "app_termination_observer.h",
     "app_window_registry_util.cc",
     "app_window_registry_util.h",
     "browser_context_keyed_service_factories.cc",

chrome/browser/apps/platform_apps/app_termination_observer.cc

+// Copyright 2018 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 "chrome/browser/apps/platform_apps/app_termination_observer.h"
+
+#include "apps/browser_context_keyed_service_factories.h"
+#include "base/no_destructor.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/profiles/profile.h"
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/keyed_service/content/browser_context_keyed_service_factory.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/notification_details.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_source.h"
+
+namespace chrome_apps {
+
+namespace {
+
+class AppTerminationObserverFactory : public BrowserContextKeyedServiceFactory {
+ public:
+  AppTerminationObserverFactory();
+  ~AppTerminationObserverFactory() override;
+
+  // BrowserContextKeyedServiceFactory:
+  KeyedService* BuildServiceInstanceFor(
+      content::BrowserContext* context) const override;
+  content::BrowserContext* GetBrowserContextToUse(
+      content::BrowserContext* context) const override;
+  bool ServiceIsCreatedWithBrowserContext() const override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(AppTerminationObserverFactory);
+};
+
+AppTerminationObserverFactory::AppTerminationObserverFactory()
+    : BrowserContextKeyedServiceFactory(
+          "AppTerminationObserver",
+          BrowserContextDependencyManager::GetInstance()) {}
+AppTerminationObserverFactory::~AppTerminationObserverFactory() = default;
+
+KeyedService* AppTerminationObserverFactory::BuildServiceInstanceFor(
+    content::BrowserContext* browser_context) const {
+  return new AppTerminationObserver(browser_context);
+}
+
+content::BrowserContext* AppTerminationObserverFactory::GetBrowserContextToUse(
+    content::BrowserContext* browser_context) const {
+  return Profile::FromBrowserContext(browser_context)->GetOriginalProfile();
+}
+
+bool AppTerminationObserverFactory::ServiceIsCreatedWithBrowserContext() const {
+  return true;
+}
+
+}  // namespace
+
+AppTerminationObserver::AppTerminationObserver(
+    content::BrowserContext* browser_context)
+    : browser_context_(browser_context) {
+  registrar_.Add(this, chrome::NOTIFICATION_APP_TERMINATING,
+                 content::NotificationService::AllSources());
+}
+
+AppTerminationObserver::~AppTerminationObserver() = default;
+
+// static
+BrowserContextKeyedServiceFactory*
+AppTerminationObserver::GetFactoryInstance() {
+  static base::NoDestructor<AppTerminationObserverFactory> factory;
+  return factory.get();
+}
+
+void AppTerminationObserver::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK_EQ(chrome::NOTIFICATION_APP_TERMINATING, type);
+  // NOTE: This fires on application termination, but passes in an associated
+  // BrowserContext. If a BrowserContext is actually destroyed *before*
+  // application termination, we won't call NotifyApplicationTerminating() for
+  // that context. We could instead monitor BrowserContext destruction if this
+  // is an issue.
+  apps::NotifyApplicationTerminating(browser_context_);
+}
+
+}  // namespace chrome_apps

chrome/browser/apps/platform_apps/app_termination_observer.h

+// Copyright 2018 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.
+
+#ifndef CHROME_BROWSER_APPS_PLATFORM_APPS_APP_TERMINATION_OBSERVER_H_
+#define CHROME_BROWSER_APPS_PLATFORM_APPS_APP_TERMINATION_OBSERVER_H_
+
+#include "components/keyed_service/core/keyed_service.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+
+class BrowserContextKeyedServiceFactory;
+
+namespace content {
+class BrowserContext;
+}
+
+namespace chrome_apps {
+
+// A helper class to observe application termination, and notify the
+// appropriate apps systems.
+class AppTerminationObserver : public content::NotificationObserver,
+                               public KeyedService {
+ public:
+  explicit AppTerminationObserver(content::BrowserContext* browser_context);
+  ~AppTerminationObserver() override;
+
+  static BrowserContextKeyedServiceFactory* GetFactoryInstance();
+
+  void Observe(int type,
+               const content::NotificationSource& source,
+               const content::NotificationDetails& details) override;
+
+ private:
+  content::BrowserContext* browser_context_;
+
+  content::NotificationRegistrar registrar_;
+
+  DISALLOW_COPY_AND_ASSIGN(AppTerminationObserver);
+};
+
+}  // namespace chrome_apps
+
+#endif  // CHROME_BROWSER_APPS_PLATFORM_APPS_APP_TERMINATION_OBSERVER_H_

chrome/browser/apps/platform_apps/browser_context_keyed_service_factories.cc

@@ -6,19 +6,16 @@
 
 #include "apps/browser_context_keyed_service_factories.h"
 #include "chrome/browser/apps/platform_apps/app_load_service_factory.h"
+#include "chrome/browser/apps/platform_apps/app_termination_observer.h"
 #include "chrome/browser/apps/platform_apps/shortcut_manager_factory.h"
-#include "content/public/browser/browser_context.h"
 
 namespace chrome_apps {
 
 void EnsureBrowserContextKeyedServiceFactoriesBuilt() {
   apps::EnsureBrowserContextKeyedServiceFactoriesBuilt();
+  AppTerminationObserver::GetFactoryInstance();
   AppShortcutManagerFactory::GetInstance();
   apps::AppLoadServiceFactory::GetInstance();
 }
 
-void NotifyApplicationTerminating(content::BrowserContext* browser_context) {
-  apps::NotifyApplicationTerminating(browser_context);
-}
-
 }  // namespace chrome_apps

chrome/browser/apps/platform_apps/browser_context_keyed_service_factories.h

@@ -5,20 +5,12 @@
 #ifndef CHROME_BROWSER_APPS_PLATFORM_APPS_BROWSER_CONTEXT_KEYED_SERVICE_FACTORIES_H_
 #define CHROME_BROWSER_APPS_PLATFORM_APPS_BROWSER_CONTEXT_KEYED_SERVICE_FACTORIES_H_
 
-namespace content {
-class BrowserContext;
-}
-
 namespace chrome_apps {
 
 // Ensures the existence of any BrowserContextKeyedServiceFactory provided by
 // the Chrome apps code.
 void EnsureBrowserContextKeyedServiceFactoriesBuilt();
 
-// Notifies the relevant BrowserContextKeyedServices for the browser context
-// that the application is being terminated.
-void NotifyApplicationTerminating(content::BrowserContext* browser_context);
-
 }  // namespace chrome_apps
 
 #endif  // CHROME_BROWSER_APPS_PLATFORM_APPS_BROWSER_CONTEXT_KEYED_SERVICE_FACTORIES_H_

chrome/browser/extensions/DEPS

@@ -20,11 +20,6 @@ include_rules = [
   "-chrome/browser/apps/",
 ]
 specific_include_rules = {
-  "extension_system_impl\.cc": [
-    # TODO(https://crbug.com/873872): Remove this.
-    "+chrome/browser/apps/platform_apps/browser_context_keyed_service_factories.h",
-  ],
-
   ".*test(_chromeos)?\.(cc|h)$": [
     # For now, tests are allowed to depend on app_browsertest_util.h, since
     # that's where PlatformAppBrowserTest is defined. Ideally, we'd eventually

chrome/browser/extensions/extension_system_impl.cc

@@ -16,9 +16,7 @@
 #include "base/strings/string_tokenizer.h"
 #include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
-#include "chrome/browser/apps/platform_apps/browser_context_keyed_service_factories.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"
@@ -42,7 +40,6 @@
 #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"
@@ -99,11 +96,7 @@ UninstallPingSender::FilterResult ShouldSendUninstallPing(
 // ExtensionSystemImpl::Shared
 //
 
-ExtensionSystemImpl::Shared::Shared(Profile* profile)
-    : profile_(profile) {
-  registrar_.Add(this, chrome::NOTIFICATION_APP_TERMINATING,
-                 content::NotificationService::AllSources());
-}
+ExtensionSystemImpl::Shared::Shared(Profile* profile) : profile_(profile) {}
 
 ExtensionSystemImpl::Shared::~Shared() {
 }
@@ -341,14 +334,6 @@ 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);
-  chrome_apps::NotifyApplicationTerminating(profile_);
-}
-
 //
 // ExtensionSystemImpl
 //

chrome/browser/extensions/extension_system_impl.h

@@ -10,8 +10,6 @@
 #include "base/macros.h"
 #include "build/build_config.h"
 #include "chrome/browser/extensions/extension_cookie_notifier.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"
 
@@ -89,7 +87,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, public content::NotificationObserver {
+  class Shared : public KeyedService {
    public:
     explicit Shared(Profile* profile);
     ~Shared() override;
@@ -119,16 +117,10 @@ 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_;