Chrome OS: New window on teleported browser window should show on current desktop

chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc

Index: chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
diff --git a/chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc b/chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
index aa7d11f77ccf03f108bb0e3afdeb224b9653102d..c57e9b0756e57c71ef2eaa6ec05ab74c66e72fbf 100644
--- a/chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
+++ b/chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
@@ -29,6 +29,7 @@
 #include "base/macros.h"
 #include "base/run_loop.h"
 #include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
 #include "chrome/browser/chromeos/login/users/scoped_user_manager_enabler.h"
@@ -39,12 +40,16 @@
 #include "chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.h"
 #include "chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.h"
 #include "chrome/browser/ui/ash/session_util.h"
+#include "chrome/browser/ui/browser_commands.h"
+#include "chrome/test/base/browser_with_test_window_test.h"
+#include "chrome/test/base/test_browser_window_aura.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chrome/test/base/testing_profile_manager.h"
 #include "components/signin/core/account_id/account_id.h"
 #include "components/user_manager/user_info.h"
 #include "ui/aura/client/aura_constants.h"
+#include "ui/aura/client/window_parenting_client.h"
 #include "ui/aura/window_event_dispatcher.h"
 #include "ui/base/ui_base_types.h"
 #include "ui/wm/core/window_modality_controller.h"
@@ -60,6 +65,9 @@ const char kBAccountIdString[] =
 const char kArrowBAccountIdString[] =
     "->{\"account_type\":\"unknown\",\"email\":\"B\"}";
 
+const char kTestAccount1[] = "user1@test.com";
+const char kTestAccount2[] = "user2@test.com";
+
 // TOOD(beng): This implementation seems only superficially different to the
 //             production impl. Evaluate whether or not we can just use that
 //             one.
@@ -1530,3 +1538,162 @@ TEST_F(MultiUserWindowManagerChromeOSTest, WindowsOrderPreservedTests) {
 
 }  // namespace test
 }  // namespace ash
+
+namespace test {
+
+// The testing framework to test browser window management under multi profile
+// scenarios.
+// TODO(warx): consider creating a test class base. It can also be used in other
+// tests such as BrowserFinderChromeOSTest.
+class MultiUserBrowserWindowChromeOSTest : public BrowserWithTestWindowTest {
+ protected:
+  MultiUserBrowserWindowChromeOSTest()
+      : test_account_id1_(EmptyAccountId()),
+        test_account_id2_(EmptyAccountId()),
+        multi_user_window_manager_(nullptr),
+        fake_user_manager_(new chromeos::FakeChromeUserManager),
+        user_manager_enabler_(fake_user_manager_) {}
+
+  ~MultiUserBrowserWindowChromeOSTest() override {}
+
+  TestingProfile* CreateMultiUserProfile(const AccountId& account_id) {
+    TestingProfile* profile =
+        profile_manager_->CreateTestingProfile(account_id.GetUserEmail());
+    const user_manager::User* user = fake_user_manager_->AddUser(account_id);
+    chromeos::ProfileHelper::Get()->SetUserToProfileMappingForTesting(
+        const_cast<user_manager::User*>(user), profile);

[sky, 2017/01/20 01:03:08]

The const_casts here seem wrong. Doesn't SetUserToProfileMappingForTesting()
take a const user?

+    chromeos::ProfileHelper::Get()->SetProfileToUserMappingForTesting(
+        const_cast<user_manager::User*>(user));
+    ash::test::AshTestHelper::GetTestSessionStateDelegate()->AddUser(
+        account_id);
+    GetUserWindowManager()->AddUser(profile);
+    return profile;
+  }
+
+  chrome::MultiUserWindowManagerChromeOS* GetUserWindowManager() {
+    if (!multi_user_window_manager_) {
+      multi_user_window_manager_ =
+          new chrome::MultiUserWindowManagerChromeOS(test_account_id1_);

[sky, 2017/01/20 01:03:08]

Who owns this?

+      multi_user_window_manager_->Init();
+      chrome::MultiUserWindowManager::SetInstanceForTest(
+          multi_user_window_manager_,
+          chrome::MultiUserWindowManager::MULTI_PROFILE_MODE_SEPARATED);
+    }
+    return multi_user_window_manager_;
+  }
+
+  void SwitchActiveUser(const AccountId& account_id) {
+    ash::test::AshTestHelper::GetTestSessionStateDelegate()->SwitchActiveUser(
+        account_id);
+    fake_user_manager_->SwitchActiveUser(account_id);
+    GetUserWindowManager()->SetAnimationSpeedForTest(

[sky, 2017/01/20 01:03:08]

Can this be done early on rather than for each switch?

+        chrome::MultiUserWindowManagerChromeOS::ANIMATION_SPEED_DISABLED);
+    GetUserWindowManager()->ActiveUserChanged(account_id);
+  }
+
+  std::unique_ptr<Browser> CreateBrowserAndTab(Browser* browser,
+                                               const std::string& title,
+                                               const std::string& url) {
+    std::unique_ptr<Browser> new_browser(CreateBrowserWithTestWindowForProfile(
+        browser->profile()->GetOriginalProfile()));
+    chrome::NewTab(new_browser.get());
+
+    new_browser->window()->Show();
+    NavigateAndCommitActiveTabWithTitle(new_browser.get(), GURL(url),
+                                        base::ASCIIToUTF16(title));
+
+    return new_browser;
+  }
+
+  std::unique_ptr<Browser> CreateBrowserWithTestWindowForProfile(
+      Profile* profile) {
+    TestBrowserWindow* browser_window = CreateTestBrowserWindowAura();
+    new TestBrowserWindowOwner(browser_window);
+    return base::WrapUnique(
+        CreateBrowser(profile, Browser::TYPE_TABBED, false, browser_window));
+  }
+
+  TestBrowserWindow* CreateTestBrowserWindowAura() {
+    std::unique_ptr<aura::Window> window(new aura::Window(nullptr));
+    window->set_id(0);
+    window->SetType(ui::wm::WINDOW_TYPE_NORMAL);
+    window->Init(ui::LAYER_TEXTURED);
+    aura::client::ParentWindowWithContext(window.get(), GetContext(),
+                                          gfx::Rect(200, 200));
+    return new TestBrowserWindowAura(std::move(window));
+  }
+
+  void SetUp() override {
+    profile_manager_.reset(
+        new TestingProfileManager(TestingBrowserProcess::GetGlobal()));
+    ASSERT_TRUE(profile_manager_->SetUp());
+    test_account_id1_ = AccountId::FromUserEmail(kTestAccount1);
+    test_account_id2_ = AccountId::FromUserEmail(kTestAccount2);
+    profile_manager_->SetLoggedIn(true);
+    chromeos::WallpaperManager::Initialize();
+    BrowserWithTestWindowTest::SetUp();
+    second_profile_ = CreateMultiUserProfile(test_account_id2_);
+    ash::test::AshTestHelper::GetTestSessionStateDelegate()
+        ->set_logged_in_users(2);
+  }
+
+  void TearDown() override {
+    chrome::MultiUserWindowManager::DeleteInstance();
+    BrowserWithTestWindowTest::TearDown();
+    chromeos::WallpaperManager::Shutdown();
+    if (second_profile_) {
+      DestroyProfile(second_profile_);
+      second_profile_ = nullptr;
+    }
+  }
+
+  TestingProfile* CreateProfile() override {
+    return CreateMultiUserProfile(test_account_id1_);
+  }
+
+  void DestroyProfile(TestingProfile* profile) override {
+    profile_manager_->DeleteTestingProfile(profile->GetProfileUserName());
+  }
+
+  AccountId test_account_id1_;
+  AccountId test_account_id2_;
+
+ private:
+  TestingProfile* second_profile_;
+  std::unique_ptr<TestingProfileManager> profile_manager_;
+  chrome::MultiUserWindowManagerChromeOS* multi_user_window_manager_;
+
+  // |fake_user_manager_| is owned by |user_manager_enabler_|
+  chromeos::FakeChromeUserManager* fake_user_manager_;
+  chromeos::ScopedUserManagerEnabler user_manager_enabler_;
+
+  DISALLOW_COPY_AND_ASSIGN(MultiUserBrowserWindowChromeOSTest);
+};
+
+// Test that creating new browser window against teleported browser window
+// should show on current active desktop (crbug.com/608840).
+TEST_F(MultiUserBrowserWindowChromeOSTest, NewWindowOnTeleportedBrowserWindow) {
+  // Switch to |test_account_id2_|'s desktop and teleport the browser window in
+  // |test_account_id1_|.
+  SwitchActiveUser(test_account_id2_);
+  aura::Window* window1 = window()->GetNativeWindow();
+  GetUserWindowManager()->ShowWindowForUser(window1, test_account_id2_);
+  EXPECT_TRUE(GetUserWindowManager()->IsWindowOnDesktopOfUser(
+      window1, test_account_id2_));
+
+  std::unique_ptr<Browser> browser2(

[sky, 2017/01/20 01:03:08]

When you close the tab in the browser that should trigger deleting the window.
So, I don't think you want a unique_ptr here.

+      CreateBrowserAndTab(browser(), "test", "http://test"));
+  ASSERT_TRUE(browser2);

[sky, 2017/01/20 01:03:08]

IMO this check isn't useful, CreateBrowserAndTab always returns a browser.

+  aura::Window* window2 = browser2->window()->GetNativeWindow();
+  EXPECT_NE(window1, window2);
+  // New browser is using the profile with |test_account_id1_| but show on
+  // |test_account_id2_|'s desktop.
+  EXPECT_EQ(test_account_id1_,
+            multi_user_util::GetAccountIdFromProfile(browser2->profile()));
+  EXPECT_TRUE(GetUserWindowManager()->IsWindowOnDesktopOfUser(
+      window2, test_account_id2_));
+
+  chrome::CloseTab(browser2.get());
+}
+
+}  // namespace test