Task Manager Should remember the most recently enabled columns.

This CL:
1- Makes adding and removing columns to the task manager a lot easier
and cleaner.
2- Stores/retrieves the columns visibility to/from the local state
prefs.
3- Stores/retrieves the sorted column and the sort order.

[email protected]
BUG=452521
TEST=browser_tests --gtest_filter=NewTaskManagerViewTest.*

Committed: https://crrev.com/4e16add803fbb0ed18413496d3fb4424fa4957c6
Cr-Commit-Position: refs/heads/master@{#346524}

Review URL: https://codereview.chromium.org/1320563002

Cr-Commit-Position: refs/heads/master@{#346734}
(cherry picked from commit 4780b87)

Review URL: https://codereview.chromium.org/1332083002 .

Cr-Commit-Position: refs/branch-heads/2490@{#228}
Cr-Branched-From: 7790a35-refs/heads/master@{#344925}

chrome/browser/task_management/providers/child_process_task_provider.cc

@@ -123,12 +123,10 @@ void ChildProcessTaskProvider::ChildProcessDataCollected(
 
 void ChildProcessTaskProvider::CreateTask(
     const content::ChildProcessData& data) {
-  // The following case should never happen since we start observing
-  // |BrowserChildProcessObserver| only after we collect all pre-existing child
-  // processes and are notified (on the UI thread) that the collection is
-  // completed at |ChildProcessDataCollected()|.
   if (tasks_by_handle_.find(data.handle) != tasks_by_handle_.end()) {
-    NOTREACHED();
+    // This case can happen when some of the child process data we collect upon
+    // StartUpdating() might be of BrowserChildProcessHosts whose channels
+    // hadn't connected yet. So we just return.
     return;
   }
 

chrome/browser/task_manager/task_manager.cc

@@ -1500,6 +1500,7 @@ Resource* TaskManagerModel::GetResource(int index) const {
 // static
 void TaskManager::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterDictionaryPref(prefs::kTaskManagerWindowPlacement);
+  registry->RegisterDictionaryPref(prefs::kTaskManagerColumnVisibility);
 }
 
 bool TaskManager::IsBrowserProcess(int index) const {

chrome/browser/ui/views/new_task_manager_view.h

@@ -26,6 +26,44 @@ class View;
 
 namespace task_management {
 
+// A collection of data to be used in the construction of a task manager table
+// column.
+struct TableColumnData {
+  // The generated ID of the column. These can change from one build to another.
+  // Their values are controlled by the generation from generated_resources.grd.
+  int id;
+
+  // The alignment of the text displayed in this column.
+  ui::TableColumn::Alignment align;
+
+  // |width| and |percent| used to define the size of the column. See
+  // ui::TableColumn::width and ui::TableColumn::percent for details.
+  int width;
+  float percent;
+
+  // Is the column sortable.
+  bool sortable;
+
+  // Is the initial sort order ascending?
+  bool initial_sort_is_ascending;
+
+  // The default visibility of this column at startup of the table if no
+  // visibility is stored for it in the prefs.
+  bool default_visibility;
+};
+
+// The task manager table columns and their properties.
+extern const TableColumnData kColumns[];
+extern const size_t kColumnsSize;
+
+// Session Restore Keys.
+extern const char kSortColumnIdKey[];
+extern const char kSortIsAscendingKey[];
+
+// Returns the |column_id| as a string value to be used as keys in the user
+// preferences.
+std::string GetColumnIdAsString(int column_id);
+
 // The new task manager UI container.
 class NewTaskManagerView
     : public views::ButtonListener,
@@ -85,10 +123,13 @@ class NewTaskManagerView
   void ExecuteCommand(int id, int event_flags) override;
 
  private:
+  friend class NewTaskManagerViewTest;
   class TableModel;
 
   explicit NewTaskManagerView(chrome::HostDesktopType desktop_type);
 
+  static NewTaskManagerView* GetInstanceForTests();
+
   // Creates the child controls.
   void Init();
 
@@ -101,10 +142,26 @@ class NewTaskManagerView
   // Restores saved "always on top" state from a previous session.
   void RetriveSavedAlwaysOnTopState();
 
+  // Restores the saved columns settings from a previous session into
+  // |columns_settings_| and updates the table view.
+  void RetrieveSavedColumnsSettingsAndUpdateTable();
+
+  // Stores the current values in |column_settings_| to the user prefs so that
+  // it can be restored later next time the task manager view is opened.
+  void StoreColumnsSettings();
+
+  void ToggleColumnVisibility(int column_id);
+
   scoped_ptr<NewTaskManagerView::TableModel> table_model_;
 
   scoped_ptr<views::MenuRunner> menu_runner_;
 
+  // Contains either the column settings retrieved from user preferences if it
+  // exists, or the default column settings.
+  // The columns settings are the visible columns and the last sorted column
+  // and the direction of the sort.
+  scoped_ptr<base::DictionaryValue> columns_settings_;
+
   // We need to own the text of the menu, the Windows API does not copy it.
   base::string16 always_on_top_menu_text_;
 

chrome/browser/ui/views/new_task_manager_view_browsertest.cc

@@ -0,0 +1,144 @@
+// 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 "base/prefs/pref_service.h"
+#include "base/prefs/scoped_user_pref_update.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/ui/browser_dialogs.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 "content/public/test/test_utils.h"
+#include "ui/views/controls/table/table_view.h"
+
+namespace task_management {
+
+class NewTaskManagerViewTest : public InProcessBrowserTest {
+ public:
+  NewTaskManagerViewTest() {}
+  ~NewTaskManagerViewTest() override {}
+
+  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 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->ToggleColumnVisibility(col_id);
+  }
+
+ 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));
+}
+
+// In the case of no settings stored in the user preferences local store, test
+// that the task manager table starts with the default columns visibility as
+// stored in |kColumns|.
+IN_PROC_BROWSER_TEST_F(NewTaskManagerViewTest, TableStartsWithDefaultColumns) {
+  ASSERT_NO_FATAL_FAILURE(ClearStoredColumnSettings());
+
+  chrome::ShowTaskManager(browser());
+  views::TableView* table = GetTable();
+  ASSERT_TRUE(table);
+
+  EXPECT_FALSE(table->is_sorted());
+  for (size_t i = 0; i < kColumnsSize; ++i) {
+    EXPECT_EQ(kColumns[i].default_visibility,
+              table->IsColumnVisible(kColumns[i].id));
+  }
+}
+
+// Tests that changing columns visibility and sort order will be stored upon
+// closing the task manager view and restored when re-opened.
+IN_PROC_BROWSER_TEST_F(NewTaskManagerViewTest, ColumnsSettingsAreRestored) {
+  ASSERT_NO_FATAL_FAILURE(ClearStoredColumnSettings());
+
+  chrome::ShowTaskManager(browser());
+  NewTaskManagerView* view = GetView();
+  ASSERT_TRUE(view);
+  views::TableView* table = GetTable();
+  ASSERT_TRUE(table);
+
+  // Toggle the visibility of all columns.
+  EXPECT_FALSE(table->is_sorted());
+  for (size_t i = 0; i < kColumnsSize; ++i) {
+    EXPECT_EQ(kColumns[i].default_visibility,
+              table->IsColumnVisible(kColumns[i].id));
+    ToggleColumnVisibility(view, kColumns[i].id);
+  }
+
+  // Sort by the first visible and initially ascending sortable column.
+  bool is_sorted = false;
+  int sorted_col_id = -1;
+  for (size_t i = 0; i < table->visible_columns().size(); ++i) {
+    const ui::TableColumn& column = table->visible_columns()[i].column;
+    if (column.sortable && column.initial_sort_is_ascending) {
+      // Toggle the sort twice for a descending sort.
+      table->ToggleSortOrder(static_cast<int>(i));
+      table->ToggleSortOrder(static_cast<int>(i));
+      is_sorted = true;
+      return;
+    }
+  }
+
+  if (is_sorted) {
+    EXPECT_TRUE(table->is_sorted());
+    EXPECT_FALSE(table->sort_descriptors().front().ascending);
+    EXPECT_EQ(table->sort_descriptors().front().column_id, sorted_col_id);
+  }
+
+  // Close the task manager view and re-open. Expect the inverse of the default
+  // visibility, and the last sort order.
+  chrome::HideTaskManager();
+  content::RunAllPendingInMessageLoop();
+  ASSERT_FALSE(GetView());
+  chrome::ShowTaskManager(browser());
+  view = GetView();
+  ASSERT_TRUE(view);
+  table = GetTable();
+  ASSERT_TRUE(table);
+
+  if (is_sorted) {
+    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));
+  }
+}
+
+}  // namespace task_management
+

chrome/chrome_tests.gypi

@@ -643,6 +643,7 @@
       'browser/ui/views/frame/browser_view_browsertest.cc',
       'browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc',
       'browser/ui/views/media_router/media_router_ui_browsertest.cc',
+      'browser/ui/views/new_task_manager_view_browsertest.cc',
       'browser/ui/views/profiles/avatar_menu_button_browsertest.cc',
       'browser/ui/views/profiles/profile_chooser_view_browsertest.cc',
       'browser/ui/views/toolbar/browser_actions_container_browsertest.cc',

chrome/common/pref_names.cc

@@ -1357,6 +1357,10 @@ const char kBrowserWindowPlacementPopup[] = "browser.window_placement_popup";
 // manager window to restore on startup.
 const char kTaskManagerWindowPlacement[] = "task_manager.window_placement";
 
+// The most recent stored column visibility of the task manager table to be
+// restored on startup.
+const char kTaskManagerColumnVisibility[] = "task_manager.column_visibility";
+
 // A collection of position, size, and other data relating to app windows to
 // restore on startup.
 const char kAppWindowPlacement[] = "browser.app_window_placement";

chrome/common/pref_names.h

@@ -478,6 +478,7 @@ extern const char kBrowserSuppressDefaultBrowserPrompt[];
 extern const char kBrowserWindowPlacement[];
 extern const char kBrowserWindowPlacementPopup[];
 extern const char kTaskManagerWindowPlacement[];
+extern const char kTaskManagerColumnVisibility[];
 extern const char kAppWindowPlacement[];
 
 extern const char kDownloadDefaultDirectory[];