Task Manager: Preserve selection when rows are added

chrome/browser/ui/views/new_task_manager_view_browsertest.cc

Index: chrome/browser/ui/views/new_task_manager_view_browsertest.cc
diff --git a/chrome/browser/ui/views/new_task_manager_view_browsertest.cc b/chrome/browser/ui/views/new_task_manager_view_browsertest.cc
index e46dd0322eb444d8c99858d57c1b488f52f88c76..17d4bc1e17825e41598b48cc15a6771be7ba8e51 100644
--- a/chrome/browser/ui/views/new_task_manager_view_browsertest.cc
+++ b/chrome/browser/ui/views/new_task_manager_view_browsertest.cc
@@ -5,25 +5,45 @@
 #include <stddef.h>
 
 #include "base/macros.h"
+#include "base/strings/pattern.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/sessions/session_tab_helper.h"
+#include "chrome/browser/task_management/task_manager_browsertest_util.h"
+#include "chrome/browser/task_management/task_manager_tester.h"
+#include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_dialogs.h"
+#include "chrome/browser/ui/browser_tabstrip.h"
+#include "chrome/browser/ui/tab_contents/tab_contents_iterator.h"
 #include "chrome/browser/ui/task_manager/task_manager_columns.h"
 #include "chrome/browser/ui/task_manager/task_manager_table_model.h"
 #include "chrome/browser/ui/views/new_task_manager_view.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_delegate.h"
+#include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"
+#include "net/dns/mock_host_resolver.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
 #include "ui/views/controls/table/table_view.h"
 
 namespace task_management {
 
+using browsertest_util::WaitForTaskManagerRows;

[afakhry, 2016/05/18 23:12:19]

Porting those tests paid off really well after all! Now we can use those utils
here! :)

Thanks! Was definitely worth it!

[ncarter (slow), 2016/05/19 00:12:58]

Acknowledged.

+
 class NewTaskManagerViewTest : public InProcessBrowserTest {
  public:
   NewTaskManagerViewTest() {}
   ~NewTaskManagerViewTest() override {}
 
+  void SetUpOnMainThread() override {
+    host_resolver()->AddRule("*", "127.0.0.1");
+    ASSERT_TRUE(embedded_test_server()->Start());

[afakhry, 2016/05/18 23:12:19]

Shouldn't you be calling here the parent's SetUpOnMainThread() i.e.
InProcessBrowserTest::SetUpOnMainThread()?

[ncarter (slow), 2016/05/19 00:12:59]

InProcessBrowserTest::SetUpOnMainThread has an empty body, so the superclass
call is not really needed when we're deriving directly from
InProcessBrowserTest.

[afakhry, 2016/05/19 00:57:33]

It would be safer to call it though, just in case someone decided to fill it
with some code in the future.
Up to you.

[ncarter (slow), 2016/05/19 17:12:34]

I don't think this is likely to happen. This function is a hook that the
superclass provides to its subclasses; the empty body is a default
implementation.

+  }
+
   void TearDownOnMainThread() override {
     // Make sure the task manager is closed (if any).
     chrome::HideTaskManager();
@@ -41,6 +61,8 @@ class NewTaskManagerViewTest : public InProcessBrowserTest {
     return GetView() ? GetView()->tab_table_ : nullptr;
   }
 
+  void PressKillButton() { GetView()->Accept(); }
+
   void ClearStoredColumnSettings() const {
     PrefService* local_state = g_browser_process->local_state();
     if (!local_state)
@@ -56,6 +78,25 @@ class NewTaskManagerViewTest : public InProcessBrowserTest {
     view->table_model_->ToggleColumnVisibility(col_id);
   }
 
+  content::WebContents* FindTabById(int32_t tab_id) {
+    for (TabContentsIterator it; !it.done(); it.Next()) {
+      if (SessionTabHelper::IdForTab(*it) == tab_id) {

[afakhry, 2016/05/18 23:12:19]

Nit: ditto for the curly braces.

[ncarter (slow), 2016/05/19 00:12:58]

Done.

+        return *it;
+      }
+    }
+    return nullptr;
+  }
+
+  int GetRowForTab(content::WebContents* tab) {

[afakhry, 2016/05/18 23:12:19]

Nit: (optional) Maybe prefer to call this: 

int GetRowIndexForTabContents(WC* tab_contents);

and the previous: 

WC* FindWebContentsForTabId(int32_t tab_id);?

Or something similar. Just for clarity. Up to you really. All is good.

[ncarter (slow), 2016/05/19 00:12:59]

I used FindWebContentsForTabId and FindRowForTab(), and added some comments.

I think it's enough to say Row (not RowIndex), because we're usually comparing
the result of FindRowForTab() with the result of FirstSelectedRow().

[afakhry, 2016/05/19 00:57:33]

Acknowledged.

+    int32_t tab_id = SessionTabHelper::IdForTab(tab);
+    auto tester = TaskManagerTester::Create(base::Closure());

[afakhry, 2016/05/18 23:12:19]

Do we create the TaskManagerTester every time we call GetRowForTab(), and also
in the |SelectionConsistency| test as well?

How about we do something similar to what you have done in
TaskManagerBrowserTest::ShowTaskManager() 
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/tas...

and then we assert before accessing the |tester_| that ShowTaskManager() had
been called first?

[ncarter (slow), 2016/05/19 00:12:59]

I'll consider this if we need to use the tester in more places. For now, there's
no harm or real expense to creating multiple testers, so I'm optimizing for the
fewest lines of code.

[afakhry, 2016/05/19 00:57:33]

Acknowledged.

+    for (int i = 0; i < tester->GetRowCount(); i++) {

[afakhry, 2016/05/18 23:12:19]

Nit: (Not that it matters much here, but ...) ++i

[ncarter (slow), 2016/05/19 00:12:58]

Done.

+      if (tester->GetTabId(i) == tab_id)
+        return i;
+    }
+    return -1;
+  }
+
  private:
   DISALLOW_COPY_AND_ASSIGN(NewTaskManagerViewTest);
 };
@@ -146,5 +187,94 @@ IN_PROC_BROWSER_TEST_F(NewTaskManagerViewTest, ColumnsSettingsAreRestored) {
   }
 }
 
+IN_PROC_BROWSER_TEST_F(NewTaskManagerViewTest, SelectionConsistency) {
+  chrome::ShowTaskManager(browser());
+
+  // Set up a total of three tabs in different processes.
+  ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL("a.com", "/title2.html"));
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), embedded_test_server()->GetURL("b.com", "/title2.html"),
+      NEW_FOREGROUND_TAB, ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), embedded_test_server()->GetURL("c.com", "/title2.html"),
+      NEW_FOREGROUND_TAB, ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+
+  // Wait for their titles to appear in the TaskManager. There should be three
+  // rows.
+  auto pattern = browsertest_util::MatchTab("Title *");
+  int rows = 3;
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(rows, pattern));
+
+  // Find the three tabs we set up, in TaskManager model order. Because we have
+  // not sorted the table yet, this should also be their UI display order.
+  auto tester = TaskManagerTester::Create(base::Closure());
+  std::vector<content::WebContents*> tabs;
+  for (int i = 0; i < tester->GetRowCount(); i++) {

[afakhry, 2016/05/18 23:12:19]

Nit: ++i

[ncarter (slow), 2016/05/19 00:12:58]

Done.

+    // Filter based on our title.
+    if (!base::MatchPattern(tester->GetRowTitle(i), pattern))
+      continue;
+    tabs.push_back(FindTabById(tester->GetTabId(i)));

[afakhry, 2016/05/18 23:12:19]

Since FindTabById() can return nullptr, maybe consider avoiding pushing_back() a
nullptr if the tab_id is not found?

[ncarter (slow), 2016/05/19 00:12:58]

Done; I just added an EXPECT.

+  }
+  EXPECT_EQ(3U, tabs.size());
+
+  // Select the middle row, and store its tab id.
+  GetTable()->Select(GetRowForTab(tabs[1]));
+  EXPECT_EQ(GetTable()->FirstSelectedRow(), GetRowForTab(tabs[1]));
+
+  // Add 3 rows above the selection. The selected tab should not change.
+  for (int i = 0; i < 3; i++) {

[afakhry, 2016/05/18 23:12:19]

Nit: ++i

[ncarter (slow), 2016/05/19 00:12:58]

Done.

+    ASSERT_TRUE(content::ExecuteScript(tabs[0], "window.open('title3.html');"));
+    EXPECT_EQ(GetTable()->FirstSelectedRow(), GetRowForTab(tabs[1]));
+  }
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows((rows += 3), pattern));
+  EXPECT_EQ(GetTable()->FirstSelectedRow(), GetRowForTab(tabs[1]));
+
+  // Add 2 rows below the selection. The selected tab should not change.
+  for (int i = 0; i < 2; i++) {

[afakhry, 2016/05/18 23:12:19]

Nit: ++i

[ncarter (slow), 2016/05/19 00:12:59]

Done.

+    ASSERT_TRUE(content::ExecuteScript(tabs[2], "window.open('title3.html');"));
+    EXPECT_EQ(GetTable()->FirstSelectedRow(), GetRowForTab(tabs[1]));
+  }
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows((rows += 2), pattern));
+  EXPECT_EQ(GetTable()->FirstSelectedRow(), GetRowForTab(tabs[1]));
+
+  // Add a new row in the same process as the selection. The selected tab should
+  // not change.
+  ASSERT_TRUE(content::ExecuteScript(tabs[1], "window.open('title3.html');"));
+  EXPECT_EQ(GetTable()->FirstSelectedRow(), GetRowForTab(tabs[1]));
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows((rows += 1), pattern));
+  EXPECT_EQ(GetTable()->FirstSelectedRow(), GetRowForTab(tabs[1]));
+  EXPECT_EQ(1, GetTable()->SelectedRowCount());
+
+  // Press the button, which kills the process of the selected row.
+  PressKillButton();
+
+  // Two rows should disappear.
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows((rows -= 2), pattern));
+
+  // A later row should now be selected. The selection should be after the 4
+  // rows sharing the tabs[0] process, and it should be at or before
+  // the tabs[2] row.
+  ASSERT_LT(GetRowForTab(tabs[0]) + 3, GetTable()->FirstSelectedRow());
+  ASSERT_LE(GetTable()->FirstSelectedRow(), GetRowForTab(tabs[2]));
+
+  // Now select tabs[2].
+  GetTable()->Select(GetRowForTab(tabs[2]));
+
+  // Focus and reload one of the sad tabs. It should reappear in the TM. The
+  // other sad tab should not reappear.
+  tabs[1]->GetDelegate()->ActivateContents(tabs[1]);
+  chrome::Reload(browser(), CURRENT_TAB);
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows((rows += 1), pattern));
+
+  // tabs[2] should still be selected.
+  EXPECT_EQ(GetTable()->FirstSelectedRow(), GetRowForTab(tabs[2]));
+
+  // Close tabs[0]. The selection should not change.
+  chrome::CloseWebContents(browser(), tabs[0], false);
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows((rows -= 1), pattern));
+  EXPECT_EQ(GetTable()->FirstSelectedRow(), GetRowForTab(tabs[2]));
+}
+
 }  // namespace task_management