Fix the browser match rules.

chrome/browser/ui/browser_finder_chromeos_unittest.cc

Index: chrome/browser/ui/browser_finder_chromeos_unittest.cc
diff --git a/chrome/browser/ui/browser_finder_chromeos_unittest.cc b/chrome/browser/ui/browser_finder_chromeos_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..6057f05f4828051fb071a12cc03c13a6a68373a4
--- /dev/null
+++ b/chrome/browser/ui/browser_finder_chromeos_unittest.cc
@@ -0,0 +1,194 @@
+// Copyright (c) 2015 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/browser_finder.h"
+
+#include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"

[msw, 2015/06/30 18:42:33]

This doesn't appear to be used/needed.

[xdai1, 2015/07/01 01:06:20]

Removed. 

+#include "chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h"
+#include "chrome/browser/ui/ash/multi_user/multi_user_window_manager.h"
+#include "chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_list.h"

[msw, 2015/06/30 18:42:33]

This doesn't appear to be used/needed.

[xdai1, 2015/07/01 01:06:20]

Removed.

+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/test/base/browser_with_test_window_test.h"
+#include "chrome/test/base/testing_browser_process.h"
+#include "chrome/test/base/testing_profile_manager.h"
+
+namespace test {
+
+namespace {
+
+const char kTestAccount1[] = "user1@test.com";
+const char kTestAccount2[] = "user2@test.com";
+
+class TestBrowserWindowAura : public TestBrowserWindow {

[msw, 2015/06/30 18:42:33]

Should this file include chrome/test/base/test_browser_window.h?

[xdai1, 2015/07/01 01:06:20]

It has been included in "chrome/test/base/browser_with_test_window_test.h" file.

+ public:
+  // |native_window| is owned by the caller.
+  explicit TestBrowserWindowAura(aura::Window* native_window)
+      : native_window_(native_window) {}
+  ~TestBrowserWindowAura() override {}
+
+  // TestBrowserWindow override.
+  aura::Window* GetNativeWindow() const override {
+    return native_window_.get();
+  }
+
+  Browser* browser() { return browser_.get(); }
+
+  void CreateBrowser(const Browser::CreateParams& params) {
+    Browser::CreateParams create_params = params;
+    create_params.window = this;
+    browser_.reset(new Browser(create_params));
+  }
+
+ private:
+  scoped_ptr<Browser> browser_;
+  scoped_ptr<aura::Window> native_window_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestBrowserWindowAura);
+};
+
+}  // namespace
+
+class BrowserFinderChromeOSTest : public BrowserWithTestWindowTest {
+ protected:
+  BrowserFinderChromeOSTest() : multi_user_window_manager_(NULL) {}

[msw, 2015/06/30 18:42:34]

nit: nullptr

[xdai1, 2015/07/01 01:06:20]

Done.

+
+  TestingProfile* CreateMultiUserProfile(const std::string& user_email) {

[msw, 2015/06/30 18:42:33]

Why not inline this in the CreateProfile override and keep that protected?

[xdai1, 2015/07/01 01:06:20]

CreateProfile() is override from BrowserWithTestWindowTest and will be called in
BrowserWithTestWindowTest::SetUp() to create a default profile. I think it's
necessary to have a function that could create another profile.

+    TestingProfile* profile =
+        profile_manager()->CreateTestingProfile(user_email);

[msw, 2015/06/30 18:42:34]

nit: use profile_manager_ and remove the accessor.

[xdai1, 2015/07/01 01:06:20]

Done.

+    created_profiles_[profile] = user_email;
+    GetUserWindowManager()->AddUser(profile);
+    return profile;
+  }
+
+  scoped_ptr<TestBrowserWindowAura> CreateBrowserWindowWithProfile(

[msw, 2015/06/30 18:42:33]

This and TestBrowserWindowAura seem very odd to me... Please try to reuse more
existing test utility code to simplify this file. Try using
BrowserWithTestWindowTest::CreateBrowser (with
profile()->GetOriginalProfile()/GetOffTheRecordProfile() as needed), use
[Test]BrowserWindow::GetNativeWindow() to avoid the need for
TestBrowserWindowAura, etc.

[xdai1, 2015/07/01 01:06:21]

In BrowserWithTestWindowTest, the |window_| is usually a TestBrowserWindow,
which doesn't have a native window, and that's why I use TestBrowserWindowAura
class to associated a native window with it. 

+      Profile* profile,
+      bool is_incognito) {
+    // Create a window.
+    aura::Window* window = new aura::Window(NULL);
+    window->set_id(0);
+    window->SetType(ui::wm::WINDOW_TYPE_NORMAL);
+    window->Init(ui::LAYER_TEXTURED);
+    window->Show();
+
+    scoped_ptr<TestBrowserWindowAura> browser_window(
+        new TestBrowserWindowAura(window));
+    if (is_incognito) {
+      Browser::CreateParams params(profile->GetOffTheRecordProfile(),
+                                   chrome::HOST_DESKTOP_TYPE_ASH);
+      browser_window->CreateBrowser(params);
+    } else {
+      Browser::CreateParams params(profile->GetOriginalProfile(),
+                                   chrome::HOST_DESKTOP_TYPE_ASH);
+      browser_window->CreateBrowser(params);
+    }
+    return browser_window.Pass();
+  }
+
+  chrome::MultiUserWindowManagerChromeOS* GetUserWindowManager() {
+    if (!multi_user_window_manager_) {
+      multi_user_window_manager_ =
+          new chrome::MultiUserWindowManagerChromeOS(kTestAccount1);
+      multi_user_window_manager_->Init();
+      chrome::MultiUserWindowManager::SetInstanceForTest(
+          multi_user_window_manager_,
+          chrome::MultiUserWindowManager::MULTI_PROFILE_MODE_SEPARATED);
+    }
+    return multi_user_window_manager_;
+  }
+
+ private:
+  void SetUp() override {
+    profile_manager_.reset(
+        new TestingProfileManager(TestingBrowserProcess::GetGlobal()));
+    ASSERT_TRUE(profile_manager_->SetUp());
+    profile_manager_->SetLoggedIn(true);
+    chromeos::WallpaperManager::Initialize();

[msw, 2015/06/30 18:42:33]

q: why do we need this?

[xdai1, 2015/07/01 01:06:20]

This will be used in CreateMultiUserProfile() function, see line 62 in this
patch. Add another user into the session will cause user transition animation,
which requires wallpaper transition as well.

+    BrowserWithTestWindowTest::SetUp();
+  }
+
+  void TearDown() override {
+    chrome::MultiUserWindowManager::DeleteInstance();
+    BrowserWithTestWindowTest::TearDown();
+    chromeos::WallpaperManager::Shutdown();
+    for (ProfileToNameMap::iterator it = created_profiles_.begin();
+         it != created_profiles_.end(); ++it) {
+      profile_manager_->DeleteTestingProfile(it->second);
+    }
+  }
+
+  TestingProfileManager* profile_manager() { return profile_manager_.get(); }
+
+  // Override BrowserWithTestWindowTest.
+  TestingProfile* CreateProfile() override {
+    return CreateMultiUserProfile(kTestAccount1);
+  }
+  void DestroyProfile(TestingProfile* profile) override {
+    ProfileToNameMap::iterator it = created_profiles_.find(profile);
+    DCHECK(it != created_profiles_.end());
+    profile_manager_->DeleteTestingProfile(it->second);
+    created_profiles_.erase(it);
+  }
+
+  scoped_ptr<TestingProfileManager> profile_manager_;
+  chrome::MultiUserWindowManagerChromeOS* multi_user_window_manager_;
+
+  typedef std::map<Profile*, std::string> ProfileToNameMap;

[msw, 2015/06/30 18:42:33]

Why is this needed? Can't you just use GetProfileUserName() and keep an std::set
of profiles (or really just keep one extra scoped_ptr or raw pointer of the
second profile that you need, besides the BrowserWithTestWindowTest's member)?

[xdai1, 2015/07/01 01:06:20]

Removed this and maintained a raw pointer of the second profile instead.

+  ProfileToNameMap created_profiles_;
+};
+
+// Test the matching rules for incognito browser.

[msw, 2015/06/30 18:42:33]

nit: this comment doesn't add much value, remove it

[xdai1, 2015/07/01 01:06:21]

Done.

+TEST_F(BrowserFinderChromeOSTest, IncognitoBrowserMatchTest) {
+  // GetBrowserCount() use kMatchAll to find all browser windows for profile().
+  EXPECT_EQ(1u,
+            chrome::GetBrowserCount(profile(), chrome::HOST_DESKTOP_TYPE_ASH));
+  EXPECT_TRUE(
+      chrome::FindAnyBrowser(profile(), true, chrome::HOST_DESKTOP_TYPE_ASH));
+  EXPECT_TRUE(
+      chrome::FindAnyBrowser(profile(), false, chrome::HOST_DESKTOP_TYPE_ASH));
+  set_browser(nullptr);
+
+  scoped_ptr<TestBrowserWindowAura> incognito_browser_window =
+      CreateBrowserWindowWithProfile(profile(), true);
+  // Incognito windows are excluded in GetBrowserCount() because kMatchAll
+  // doesn't match original profile of the browser with the given profile.
+  EXPECT_EQ(0u,
+            chrome::GetBrowserCount(profile(), chrome::HOST_DESKTOP_TYPE_ASH));
+  EXPECT_TRUE(
+      chrome::FindAnyBrowser(profile(), true, chrome::HOST_DESKTOP_TYPE_ASH));
+  EXPECT_FALSE(
+      chrome::FindAnyBrowser(profile(), false, chrome::HOST_DESKTOP_TYPE_ASH));
+}
+
+TEST_F(BrowserFinderChromeOSTest, MultiProfileBrowserMatchTest) {

[msw, 2015/06/30 18:42:33]

nit: name this test something like "FindBrowserOwnedByAnotherProfile"

[xdai1, 2015/07/01 01:06:20]

Done.

+  set_browser(nullptr);
+
+  // Add another profile.
+  CreateMultiUserProfile(kTestAccount2);
+
+  scoped_ptr<TestBrowserWindowAura> browser_window =
+      CreateBrowserWindowWithProfile(profile(), false);
+  GetUserWindowManager()->SetWindowOwner(browser_window->GetNativeWindow(),
+                                         kTestAccount1);
+  EXPECT_EQ(1u,
+            chrome::GetBrowserCount(profile(), chrome::HOST_DESKTOP_TYPE_ASH));
+  EXPECT_TRUE(
+      chrome::FindAnyBrowser(profile(), true, chrome::HOST_DESKTOP_TYPE_ASH));
+  EXPECT_TRUE(
+      chrome::FindAnyBrowser(profile(), false, chrome::HOST_DESKTOP_TYPE_ASH));
+
+  // Move the browser window to another user's desktop. Then no window should
+  // be avaiable for the current profile.

[msw, 2015/06/30 18:42:33]

nit: available

[xdai1, 2015/07/01 01:06:21]

Done.

+  GetUserWindowManager()->ShowWindowForUser(browser_window->GetNativeWindow(),
+                                            kTestAccount2);
+  EXPECT_EQ(0u,
+            chrome::GetBrowserCount(profile(), chrome::HOST_DESKTOP_TYPE_ASH));
+  EXPECT_FALSE(
+      chrome::FindAnyBrowser(profile(), true, chrome::HOST_DESKTOP_TYPE_ASH));
+  EXPECT_FALSE(
+      chrome::FindAnyBrowser(profile(), false, chrome::HOST_DESKTOP_TYPE_ASH));
+}
+
+}  // namespace test