Update stylus settings for lock screen note taking

Fixes a number of issues with stylus settings for note taking on lock
screen:
 *  Provide a NoteTakingHelper observer interface called when the
    preferred app changes (or when it's lock screen status changes)
     *  Settings UI can use this to update itself when the preferred
        app changes.
     *  Switch lock_screen_apps::AppManagerImpl to observe this event
        for preferred app changes (instead of observing note taking
        pref directly)
  *  Introduces kNoteTakingAppsAllowedOnLockScreen pref, that will be
    used by a user policy to whitelist apps available on the lock
    screen (to be added in dependent patch)
 *  Disable lock screen support for note taking apps in non-primary
    profiles (the profile that supports lock screen use case is set
    by lock_screen_apps::StateController during its initialization).
 *  Redo settings UI for enabling apps on the lock screen so its
    state (whether it's disabled, the policy indicator) does not
    depend on prefs directly, instead derive the state from the
    note taking app's NoteAppInfo (in particular lockScreenSupport
    property)

While here, did some cleanup in test code:
 *  Provided utility methods to NoteTakingHelper unit tests to
    reduce code duplication:
     *  Method to create/install lock screen enabled note taking app
     *  Methods for verifying preferred app and available apps info
 *  In stylus device page browser tests, made fake browser proxy
    smarter, so test don't have to "manually" trigger note taking app
    changes

[email protected]

(cherry picked from commit e4a5c06)

Bug: 741940
Bug: 741053
Bug: 746116
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I5e2ee138df620d3832a8ad0d1b4d0db285fba0da
Reviewed-on: https://chromium-review.googlesource.com/572842
Commit-Queue: Toni Barzic <[email protected]>
Reviewed-by: Jacob Dufault <[email protected]>
Reviewed-by: Steven Bennetts <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#489142}
Reviewed-on: https://chromium-review.googlesource.com/588341
Reviewed-by: Toni Barzic <[email protected]>
Cr-Commit-Position: refs/branch-heads/3163@{#72}
Cr-Branched-From: ff259ba-refs/heads/master@{#488528}

Author: Toni Barzic

chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc

@@ -113,7 +113,9 @@ void InstallExtensionCopy(
 }  // namespace
 
 AppManagerImpl::AppManagerImpl()
-    : extensions_observer_(this), weak_ptr_factory_(this) {}
+    : extensions_observer_(this),
+      note_taking_helper_observer_(this),
+      weak_ptr_factory_(this) {}
 
 AppManagerImpl::~AppManagerImpl() = default;
 
@@ -138,15 +140,7 @@ void AppManagerImpl::Initialize(Profile* primary_profile,
   lock_screen_profile_ = lock_screen_profile;
   state_ = State::kInactive;
 
-  pref_change_registrar_.Init(primary_profile->GetPrefs());
-  pref_change_registrar_.Add(
-      prefs::kNoteTakingAppId,
-      base::Bind(&AppManagerImpl::OnNoteTakingExtensionChanged,
-                 base::Unretained(this)));
-  pref_change_registrar_.Add(
-      prefs::kNoteTakingAppEnabledOnLockScreen,
-      base::Bind(&AppManagerImpl::OnNoteTakingExtensionChanged,
-                 base::Unretained(this)));
+  note_taking_helper_observer_.Add(chromeos::NoteTakingHelper::Get());
 }
 
 void AppManagerImpl::Start(const base::Closure& note_taking_changed_callback) {
@@ -233,6 +227,15 @@ void AppManagerImpl::OnExtensionUnloaded(
     OnNoteTakingExtensionChanged();
 }
 
+void AppManagerImpl::OnAvailableNoteTakingAppsUpdated() {}
+
+void AppManagerImpl::OnPreferredNoteTakingAppUpdated(Profile* profile) {
+  if (profile != primary_profile_)
+    return;
+
+  OnNoteTakingExtensionChanged();
+}
+
 void AppManagerImpl::OnNoteTakingExtensionChanged() {
   if (state_ == State::kInactive)
     return;
@@ -259,9 +262,9 @@ std::string AppManagerImpl::FindLockScreenNoteTakingApp() const {
       chromeos::NoteTakingHelper::Get()->GetPreferredChromeAppInfo(
           primary_profile_);
 
-  if (!note_taking_app ||
+  if (!note_taking_app || !note_taking_app->preferred ||
       note_taking_app->lock_screen_support !=
-          chromeos::NoteTakingLockScreenSupport::kSelected) {
+          chromeos::NoteTakingLockScreenSupport::kEnabled) {
     return std::string();
   }
 

chrome/browser/chromeos/lock_screen_apps/app_manager_impl.h

@@ -12,7 +12,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/scoped_observer.h"
 #include "chrome/browser/chromeos/lock_screen_apps/app_manager.h"
-#include "components/prefs/pref_change_registrar.h"
+#include "chrome/browser/chromeos/note_taking_helper.h"
 #include "extensions/browser/extension_registry_observer.h"
 
 class Profile;
@@ -26,6 +26,7 @@ namespace lock_screen_apps {
 
 // The default implementation of lock_screen_apps::AppManager.
 class AppManagerImpl : public AppManager,
+                       public chromeos::NoteTakingHelper::Observer,
                        public extensions::ExtensionRegistryObserver {
  public:
   AppManagerImpl();
@@ -40,13 +41,17 @@ class AppManagerImpl : public AppManager,
   bool IsNoteTakingAppAvailable() const override;
   std::string GetNoteTakingAppId() const override;
 
-  // extensions::ExtensionRegistryObserver implementation:
+  // extensions::ExtensionRegistryObserver:
   void OnExtensionLoaded(content::BrowserContext* browser_context,
                          const extensions::Extension* extension) override;
   void OnExtensionUnloaded(content::BrowserContext* browser_context,
                            const extensions::Extension* extension,
                            extensions::UnloadedExtensionReason reason) override;
 
+  // chromeos::NoteTakingHelper::Observer:
+  void OnAvailableNoteTakingAppsUpdated() override;
+  void OnPreferredNoteTakingAppUpdated(Profile* profile) override;
+
  private:
   enum class State {
     // The manager has not yet been initialized.
@@ -101,11 +106,14 @@ class AppManagerImpl : public AppManager,
   State state_ = State::kNotInitialized;
   std::string lock_screen_app_id_;
 
-  PrefChangeRegistrar pref_change_registrar_;
   ScopedObserver<extensions::ExtensionRegistry,
                  extensions::ExtensionRegistryObserver>
       extensions_observer_;
 
+  ScopedObserver<chromeos::NoteTakingHelper,
+                 chromeos::NoteTakingHelper::Observer>
+      note_taking_helper_observer_;
+
   base::Closure note_taking_changed_callback_;
 
   // Counts app installs. Passed to app install callback as install request

chrome/browser/chromeos/lock_screen_apps/app_manager_impl_unittest.cc

@@ -144,14 +144,21 @@ class LockScreenAppManagerImplTest
             base::Bind(&ArcSessionFactory)));
 
     chromeos::NoteTakingHelper::Initialize();
+    chromeos::NoteTakingHelper::Get()->SetProfileWithEnabledLockScreenApps(
+        profile());
 
     ResetAppManager();
   }
 
   void TearDown() override {
+    // App manager has to be destroyed before NoteTakingHelper is shutdown - it
+    // removes itself from the NoteTakingHelper observer list during its
+    // destruction.
+    app_manager_.reset();
+
+    chromeos::NoteTakingHelper::Shutdown();
     extensions::ExtensionSystem::Get(profile())->Shutdown();
     extensions::ExtensionSystem::Get(lock_screen_profile())->Shutdown();
-    chromeos::NoteTakingHelper::Shutdown();
   }
 
   void InitExtensionSystem(Profile* profile) {
@@ -323,8 +330,8 @@ class LockScreenAppManagerImplTest
         ->AddExtension(app.get());
 
     chromeos::NoteTakingHelper::Get()->SetPreferredApp(profile, app_id);
-    profile->GetPrefs()->SetBoolean(prefs::kNoteTakingAppEnabledOnLockScreen,
-                                    enable_on_lock_screen);
+    chromeos::NoteTakingHelper::Get()->SetPreferredAppEnabledOnLockScreen(
+        profile, enable_on_lock_screen);
     return app;
   }
 
@@ -515,8 +522,8 @@ TEST_P(LockScreenAppManagerImplTest, LockScreenNoteTakingDisabledWhileStarted) {
             lock_app->path());
   EXPECT_TRUE(base::PathExists(note_taking_app->path()));
 
-  profile()->GetPrefs()->SetBoolean(prefs::kNoteTakingAppEnabledOnLockScreen,
-                                    false);
+  chromeos::NoteTakingHelper::Get()->SetPreferredAppEnabledOnLockScreen(
+      profile(), false);
 
   EXPECT_EQ(1, note_taking_changed_count());
   ResetNoteTakingChangedCount();
@@ -560,8 +567,8 @@ TEST_P(LockScreenAppManagerImplTest, LockScreenNoteTakingEnabledWhileStarted) {
                              extensions::ExtensionRegistry::EVERYTHING);
   EXPECT_FALSE(lock_app);
 
-  profile()->GetPrefs()->SetBoolean(prefs::kNoteTakingAppEnabledOnLockScreen,
-                                    true);
+  chromeos::NoteTakingHelper::Get()->SetPreferredAppEnabledOnLockScreen(
+      profile(), true);
 
   EXPECT_EQ(1, note_taking_changed_count());
   ResetNoteTakingChangedCount();
@@ -604,7 +611,7 @@ TEST_P(LockScreenAppManagerImplTest, LockScreenNoteTakingChangedWhileStarted) {
   scoped_refptr<const extensions::Extension> dev_note_taking_app =
       AddTestAppWithLockScreenSupport(
           profile(), chromeos::NoteTakingHelper::kDevKeepExtensionId, "1.0",
-          false /* enable_on_lock_screen */);
+          true /* enable_on_lock_screen */);
 
   scoped_refptr<const extensions::Extension> prod_note_taking_app =
       AddTestAppWithLockScreenSupport(
@@ -684,6 +691,81 @@ TEST_P(LockScreenAppManagerImplTest, LockScreenNoteTakingChangedWhileStarted) {
   EXPECT_TRUE(base::PathExists(prod_note_taking_app->path()));
 }
 
+TEST_P(LockScreenAppManagerImplTest, NoteTakingChangedToLockScreenSupported) {
+  scoped_refptr<const extensions::Extension> dev_note_taking_app =
+      AddTestAppWithLockScreenSupport(
+          profile(), chromeos::NoteTakingHelper::kDevKeepExtensionId, "1.0",
+          true /* enable_on_lock_screen */);
+
+  scoped_refptr<const extensions::Extension> prod_note_taking_app =
+      CreateTestAppInProfile(profile(),
+                             chromeos::NoteTakingHelper::kProdKeepExtensionId,
+                             "1.0", false /* supports_lock_screen */);
+  extensions::ExtensionSystem::Get(profile())
+      ->extension_service()
+      ->AddExtension(prod_note_taking_app.get());
+  chromeos::NoteTakingHelper::Get()->SetPreferredApp(
+      profile(), chromeos::NoteTakingHelper::kProdKeepExtensionId);
+
+  // Initialize app manager - the note taking should be disabled initially
+  // because the preferred app (prod) is not enabled on lock screen.
+  InitializeAndStartAppManager(profile());
+  RunExtensionServiceTaskRunner(lock_screen_profile());
+  EXPECT_EQ(0, note_taking_changed_count());
+  EXPECT_EQ(false, app_manager()->IsNoteTakingAppAvailable());
+
+  // Setting dev app, which is enabled on lock screen, as preferred will enable
+  // lock screen note taking,
+  chromeos::NoteTakingHelper::Get()->SetPreferredApp(
+      profile(), chromeos::NoteTakingHelper::kDevKeepExtensionId);
+
+  EXPECT_EQ(1, note_taking_changed_count());
+  ResetNoteTakingChangedCount();
+  // If test app is installed asynchronously. the app won't be enabled on
+  // lock screen until extension service task runner tasks are run.
+  EXPECT_EQ(!IsInstallAsync(), app_manager()->IsNoteTakingAppAvailable());
+  RunExtensionServiceTaskRunner(lock_screen_profile());
+
+  EXPECT_EQ(NoteTakingChangedCountOnStart(), note_taking_changed_count());
+  ResetNoteTakingChangedCount();
+  EXPECT_TRUE(app_manager()->IsNoteTakingAppAvailable());
+  EXPECT_EQ(chromeos::NoteTakingHelper::kDevKeepExtensionId,
+            app_manager()->GetNoteTakingAppId());
+
+  // Verify the dev app copy is installed in the lock screen app profile.
+  const extensions::Extension* lock_app =
+      extensions::ExtensionRegistry::Get(lock_screen_profile())
+          ->GetExtensionById(chromeos::NoteTakingHelper::kDevKeepExtensionId,
+                             extensions::ExtensionRegistry::ENABLED);
+  ASSERT_TRUE(lock_app);
+  EXPECT_TRUE(base::PathExists(lock_app->path()));
+  EXPECT_EQ(GetLockScreenAppPath(dev_note_taking_app->id(),
+                                 dev_note_taking_app->VersionString()),
+            lock_app->path());
+
+  // Verify the prod app was not coppied to the lock screen profile (for
+  // unpacked apps, the lock screen extension path is the same as the original
+  // app path, so it's expected to still exist).
+  EXPECT_EQ(
+      GetParam() == TestAppLocation::kUnpacked,
+      base::PathExists(GetLockScreenAppPath(
+          prod_note_taking_app->id(), prod_note_taking_app->VersionString())));
+
+  app_manager()->Stop();
+
+  // Stopping app manager will disable lock screen note taking.
+  EXPECT_EQ(0, note_taking_changed_count());
+  EXPECT_FALSE(app_manager()->IsNoteTakingAppAvailable());
+  EXPECT_TRUE(app_manager()->GetNoteTakingAppId().empty());
+
+  RunExtensionServiceTaskRunner(lock_screen_profile());
+  RunExtensionServiceTaskRunner(profile());
+
+  // Make sure original app paths are not deleted.
+  EXPECT_TRUE(base::PathExists(dev_note_taking_app->path()));
+  EXPECT_TRUE(base::PathExists(prod_note_taking_app->path()));
+}
+
 TEST_P(LockScreenAppManagerImplTest, LockScreenNoteTakingReloadedWhileStarted) {
   scoped_refptr<const extensions::Extension> note_taking_app =
       AddTestAppWithLockScreenSupport(

chrome/browser/chromeos/lock_screen_apps/note_taking_browsertest.cc

@@ -35,8 +35,9 @@ class LockScreenNoteTakingTest : public ExtensionBrowserTest {
 
   bool EnableLockScreenAppLaunch(const std::string& app_id) {
     chromeos::NoteTakingHelper::Get()->SetPreferredApp(profile(), app_id);
-    profile()->GetPrefs()->SetBoolean(prefs::kNoteTakingAppEnabledOnLockScreen,
-                                      true);
+    chromeos::NoteTakingHelper::Get()->SetPreferredAppEnabledOnLockScreen(
+        profile(), true);
+
     session_manager::SessionManager::Get()->SetSessionState(
         session_manager::SessionState::LOCKED);
 

chrome/browser/chromeos/lock_screen_apps/state_controller.cc

@@ -15,6 +15,7 @@
 #include "base/strings/string16.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/lock_screen_apps/app_manager_impl.h"
+#include "chrome/browser/chromeos/note_taking_helper.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/chrome_paths.h"
@@ -210,6 +211,9 @@ void StateController::InitializeWithCryptoKey(Profile* profile,
           profile, g_browser_process->local_state(), crypto_key,
           base_path.AppendASCII("lock_screen_app_data"));
 
+  chromeos::NoteTakingHelper::Get()->SetProfileWithEnabledLockScreenApps(
+      profile);
+
   // App manager might have been set previously by a test.
   if (!app_manager_)
     app_manager_ = base::MakeUnique<AppManagerImpl>();

chrome/browser/chromeos/lock_screen_apps/state_controller_unittest.cc

@@ -14,8 +14,10 @@
 #include "base/files/file_path.h"
 #include "base/memory/ptr_util.h"
 #include "base/test/scoped_command_line.h"
+#include "chrome/browser/chromeos/arc/arc_session_manager.h"
 #include "chrome/browser/chromeos/lock_screen_apps/app_manager.h"
 #include "chrome/browser/chromeos/lock_screen_apps/state_observer.h"
+#include "chrome/browser/chromeos/note_taking_helper.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/test_extension_system.h"
@@ -27,6 +29,8 @@
 #include "chrome/test/base/testing_profile_manager.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/fake_power_manager_client.h"
+#include "components/arc/arc_service_manager.h"
+#include "components/arc/arc_session.h"
 #include "components/session_manager/core/session_manager.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
@@ -62,6 +66,11 @@ const char kPrimaryProfileName[] = "primary_profile";
 // Key for pref containing lock screen data crypto key.
 constexpr char kDataCryptoKeyPref[] = "lockScreenAppDataCryptoKey";
 
+std::unique_ptr<arc::ArcSession> ArcSessionFactory() {
+  ADD_FAILURE() << "Attempt to create arc session.";
+  return nullptr;
+}
+
 scoped_refptr<extensions::Extension> CreateTestNoteTakingApp(
     const std::string& app_id) {
   ListBuilder action_handlers;
@@ -350,6 +359,13 @@ class LockScreenAppStateTest : public BrowserWithTestWindowTest {
     session_manager_->SetSessionState(
         session_manager::SessionState::LOGIN_PRIMARY);
 
+    // Initialize arc session manager - NoteTakingHelper expects it to be set.
+    arc_session_manager_ = base::MakeUnique<arc::ArcSessionManager>(
+        base::MakeUnique<arc::ArcSessionRunner>(
+            base::Bind(&ArcSessionFactory)));
+
+    chromeos::NoteTakingHelper::Initialize();
+
     ASSERT_TRUE(lock_screen_apps::StateController::IsEnabled());
 
     // Create fake lock screen app profile.
@@ -381,6 +397,8 @@ class LockScreenAppStateTest : public BrowserWithTestWindowTest {
 
     state_controller_->RemoveObserver(&observer_);
     state_controller_->Shutdown();
+    chromeos::NoteTakingHelper::Shutdown();
+
     session_manager_.reset();
     app_manager_ = nullptr;
     app_window_.reset();
@@ -548,6 +566,11 @@ class LockScreenAppStateTest : public BrowserWithTestWindowTest {
   // owned by DBusThreadManager.
   chromeos::FakePowerManagerClient* power_manager_client_ = nullptr;
 
+  // The StateController does not really have dependency on ARC, but this is
+  // needed to properly initialize NoteTakingHelper.
+  std::unique_ptr<arc::ArcServiceManager> arc_service_manager_;
+  std::unique_ptr<arc::ArcSessionManager> arc_session_manager_;
+
   std::unique_ptr<session_manager::SessionManager> session_manager_;
 
   std::unique_ptr<lock_screen_apps::StateController> state_controller_;