Task Manager: Preserve selection when rows are added

chrome/browser/ui/views/new_task_manager_view_browsertest.cc

 // Copyright 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 <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();
     content::RunAllPendingInMessageLoop();
     ASSERT_FALSE(GetView());
 
     InProcessBrowserTest::TearDownOnMainThread();
   }
 
   NewTaskManagerView* GetView() const {
     return NewTaskManagerView::GetInstanceForTests();
   }
 
   views::TableView* GetTable() const {
     return GetView() ? GetView()->tab_table_ : nullptr;
   }
 
+  void PressKillButton() { GetView()->Accept(); }
+
   void ClearStoredColumnSettings() const {
     PrefService* local_state = g_browser_process->local_state();
     if (!local_state)
       FAIL();
 
     DictionaryPrefUpdate dict_update(local_state,
                                      prefs::kTaskManagerColumnVisibility);
     dict_update->Clear();
   }
 
   void ToggleColumnVisibility(NewTaskManagerView* view, int col_id) {
     DCHECK(view);
     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);
 };
 
 // Tests that all defined columns have a corresponding string IDs for keying
 // into the user preferences dictionary.
 IN_PROC_BROWSER_TEST_F(NewTaskManagerViewTest, AllColumnsHaveStringIds) {
   for (size_t i = 0; i < kColumnsSize; ++i)
     EXPECT_NE("", GetColumnIdAsString(kColumns[i].id));
 }
@@ skipping 70 matching lines @@
     EXPECT_TRUE(table->is_sorted());
     EXPECT_FALSE(table->sort_descriptors().front().ascending);
     EXPECT_EQ(table->sort_descriptors().front().column_id, sorted_col_id);
   }
   for (size_t i = 0; i < kColumnsSize; ++i) {
     EXPECT_EQ(!kColumns[i].default_visibility,
               table->IsColumnVisible(kColumns[i].id));
   }
 }
 
+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