arc: Implement uninstall confirmation dialog for Arc app.

chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc

Index: chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc
diff --git a/chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc b/chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b40506ae207fd0edffaf54c87b3112b736d7d2e2
--- /dev/null
+++ b/chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc
@@ -0,0 +1,174 @@
+// Copyright 2016 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/ui/app_list/arc/arc_app_dialog.h"
+
+#include "base/command_line.h"
+#include "base/macros.h"
+#include "chrome/browser/chromeos/arc/arc_session_manager.h"
+#include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
+#include "chrome/browser/chromeos/login/users/scoped_user_manager_enabler.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/app_list/app_list_controller_delegate.h"
+#include "chrome/browser/ui/app_list/app_list_service.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_utils.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chromeos/chromeos_switches.h"
+#include "components/arc/arc_bridge_service.h"
+#include "components/arc/common/app.mojom.h"
+#include "components/arc/test/fake_app_instance.h"
+#include "content/public/test/test_utils.h"
+
+namespace arc {
+
+namespace {
+
+chromeos::FakeChromeUserManager* GetUserManager() {

[msw, 2016/11/29 20:47:37]

optional nit: inline and cache a ptr below. (I see this is a copy-paste of
arc_app_test.cc and it'd be nice to fix both, or optional to keep both as-is...)

[lgcheng, 2016/11/30 19:28:47]

These lines turn out to be not needed for browser_test. Removed.

+  return static_cast<chromeos::FakeChromeUserManager*>(
+      user_manager::UserManager::Get());
+  }
+
+const user_manager::User* CreateUserAndLogin(Profile* profile) {
+  std::string gaia_id = "1234567890";
+  const AccountId account_id(
+      AccountId::FromUserEmailGaiaId(profile->GetProfileUserName(), gaia_id));
+  const user_manager::User* user = GetUserManager()->AddUser(account_id);
+  GetUserManager()->LoginUser(account_id);
+  return user;
+}
+
+}  // namespace
+
+class ArcAppUninstallDialogViewBrowserTest : public InProcessBrowserTest {
+ public:
+  ArcAppUninstallDialogViewBrowserTest() {}
+
+  // InProcessBrowserTest:
+  ~ArcAppUninstallDialogViewBrowserTest() override {}
+
+  void SetUpAppInstance() {

[msw, 2016/11/29 20:47:37]

It would be nice if there was a common utility function or shared arc
browser_tests base class to do this for these tests and other tests. It seems
like a fair amount of the setup code is shared with other test suites. Can you
create an ArcAppTest instance in this class and call its SetUp function to
remove some redundancy?

[lgcheng, 2016/11/30 19:28:47]

ArcAppTest::Setup() is very different from what is here although they share a
lot of Arc enable code. ArcAppTest is designed as a unit_test helper, where
ArcBridgeService and ArcSessionManager are not initialized so ArcAppTest creates
FakeArcBridgeService and then create ArcSessionManager and manages their
lifetime. In browser_test ArcBridgeService and ArcSessionManager are pre
initialized and their life time is also managed by browser_test base.
Initializing a ArcAppTest would result a lot of test teardown error.

Good news is that after reexamination, a lot of setup code needed in unit_test
is not needed in browser_test. Remove unnecessary code. The code which remains
to Enable Arc does not look that too much now.

+    user_manager_enabler_ =
+        base::MakeUnique<chromeos::ScopedUserManagerEnabler>(
+             new chromeos::FakeChromeUserManager());
+    profile_ = browser()->profile();
+    const user_manager::User* user = CreateUserAndLogin(profile_);
+    // Have the user-to-profile mapping ready to avoid using the real profile
+    // manager (which is null).
+    chromeos::ProfileHelper::Get()->SetUserToProfileMappingForTesting(user,
+                                                                      profile_);
+
+    base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      chromeos::switches::kEnableArc);

[msw, 2016/11/29 20:47:37]

nit: indentation (run "git cl format")

[lgcheng, 2016/11/30 19:28:47]

Done.

+
+    // A valid |arc_app_list_prefs_| is needed for the Arc bridge service and
+    // the Arc session manager.
+    arc_app_list_pref_ = ArcAppListPrefs::Get(profile_);
+    if (!arc_app_list_pref_) {

[msw, 2016/11/29 20:47:37]

Why is this conditional? It seems like we should know in advance whether an
instance exists?

[lgcheng, 2016/11/30 19:28:47]

The browser_test env try to call ArcAppListPrefs::Get(profile_); and keep the
mapping [profile* -> nullptr] in the key_factory map before I turn on the system
switch. The mapping of [profile* -> nullptr] stays in the map and prevents
future ArcAppListPrefs::Get(profile_) call to create the ArcAppListPrefs.

In any cases other than tests, availability of Arc never changes without user
logging out. I think this is current workaround for Arc environment in browser
tests that the availability of Arc changes.

[msw, 2016/11/30 23:26:57]

Hmm, okay.

+      ArcAppListPrefsFactory::GetInstance()->RecreateServiceInstanceForTesting(
+          profile_);
+    }
+
+    DCHECK(ArcBridgeService::Get());
+    ArcSessionManager* session_manager = ArcSessionManager::Get();
+    DCHECK(session_manager);
+    ArcSessionManager::DisableUIForTesting();
+    session_manager->OnPrimaryUserProfilePrepared(profile_);
+    session_manager->EnableArc();
+
+    arc_app_list_pref_ = ArcAppListPrefs::Get(profile_);
+    DCHECK(arc_app_list_pref_);
+
+    base::RunLoop run_loop;
+    arc_app_list_pref_->SetDefaltAppsReadyCallback(run_loop.QuitClosure());
+    run_loop.Run();
+
+    app_instance_.reset(new arc::FakeAppInstance(arc_app_list_pref_));
+    arc_app_list_pref_->app_instance_holder()->SetInstance(app_instance_.get());
+
+    mojom::AppInfo app;
+    app.name = base::StringPrintf("Fake App %d", 0);
+    app.package_name = base::StringPrintf("fake.app.%d", 0);
+    app.activity = base::StringPrintf("fake.app.%d.activity", 0);
+    app.sticky = false;
+    app_instance_->SendRefreshAppList(std::vector<mojom::AppInfo>(1, app));
+
+    mojom::ArcPackageInfo package;
+    package.package_name = base::StringPrintf("fake.app.%d", 0);
+    package.package_version = 0;
+    package.last_backup_android_id = 0;
+    package.last_backup_time = 0;
+    package.sync = false;
+    app_instance_->SendRefreshPackageList(
+      std::vector<mojom::ArcPackageInfo>(1, package));
+  }
+
+  void TearDownOnMainThread() override {
+    ArcSessionManager::Get()->Shutdown();
+    user_manager_enabler_.reset();
+    InProcessBrowserTest::TearDownOnMainThread();
+  }
+
+  // Ensures the ArcAppDialogView is destoryed.
+  void TearDown() override {
+    ASSERT_FALSE(IsArcAppDialogViewAliveForTest());
+  }
+
+  ArcAppListPrefs* arc_app_list_pref() { return arc_app_list_pref_; }
+
+  FakeAppInstance* instance() { return app_instance_.get(); }
+
+ private:
+  ArcAppListPrefs* arc_app_list_pref_;

[msw, 2016/11/29 20:47:37]

nit: = nullptr;

[lgcheng, 2016/11/30 19:28:47]

Done.

+
+  Profile* profile_;

[msw, 2016/11/29 20:47:37]

nit: = nullptr;

[lgcheng, 2016/11/30 19:28:47]

Done.

+
+  std::unique_ptr<chromeos::ScopedUserManagerEnabler> user_manager_enabler_;
+
+  std::unique_ptr<arc::FakeAppInstance> app_instance_;
+
+  DISALLOW_COPY_AND_ASSIGN(ArcAppUninstallDialogViewBrowserTest);
+};
+
+// User confirms/cancels Arc App uninstall.
+IN_PROC_BROWSER_TEST_F(ArcAppUninstallDialogViewBrowserTest,

[msw, 2016/11/29 20:47:37]

Nice test fixture; should we have a similar one for shortcut removal?

[lgcheng, 2016/11/30 19:28:47]

Shortcut removal test added.

+                       UserConfirmsUninstall) {
+  SetUpAppInstance();
+
+  std::vector<std::string> app_ids = arc_app_list_pref()->GetAppIds();
+  EXPECT_EQ(app_ids.size(), 1u);
+  std::string app_id = app_ids[0];
+
+  AppListService* service = AppListService::Get();
+  ASSERT_TRUE(service);
+  service->ShowForProfile(browser()->profile());
+  AppListControllerDelegate* controller(service->GetControllerDelegate());
+  ASSERT_TRUE(controller);
+  ShowArcAppUninstallDialog(browser()->profile(), controller, app_id,
+                                     base::Bind(UninstallArcApp));

[msw, 2016/11/29 20:47:37]

nit: indentation (run "git cl format")

[lgcheng, 2016/11/30 19:28:47]

Done.

+  content::RunAllPendingInMessageLoop();
+
+  EXPECT_TRUE(CloseAppDialogViewAndConfirmForTest(false));
+  content::RunAllPendingInMessageLoop();
+  app_ids = arc_app_list_pref()->GetAppIds();
+  EXPECT_EQ(app_ids.size(), 1u);
+  controller->DismissView();
+
+  service->ShowForProfile(browser()->profile());
+  ShowArcAppUninstallDialog(browser()->profile(), controller, app_id,
+                                     base::Bind(UninstallArcApp));

[msw, 2016/11/29 20:47:37]

nit: indentation (run "git cl format")

[lgcheng, 2016/11/30 19:28:47]

Done.

+  content::RunAllPendingInMessageLoop();
+
+  EXPECT_TRUE(CloseAppDialogViewAndConfirmForTest(true));
+  content::RunAllPendingInMessageLoop();
+  app_ids = arc_app_list_pref()->GetAppIds();
+  EXPECT_EQ(app_ids.size(), 0u);
+  controller->DismissView();
+}
+
+}  // namespace arc