Task Manager Should remember the most recently enabled columns.

chrome/browser/ui/views/new_task_manager_view.cc

Index: chrome/browser/ui/views/new_task_manager_view.cc
diff --git a/chrome/browser/ui/views/new_task_manager_view.cc b/chrome/browser/ui/views/new_task_manager_view.cc
index 6e07c9c5b8843fcec0ebb040de89472ff4e60622..8ff4ae27ac9eb4cbdeeee9f887aac170d7b32e63 100644
--- a/chrome/browser/ui/views/new_task_manager_view.cc
+++ b/chrome/browser/ui/views/new_task_manager_view.cc
@@ -237,6 +237,86 @@ class TaskManagerValuesStringifier {
   DISALLOW_COPY_AND_ASSIGN(TaskManagerValuesStringifier);
 };
 
+// A collection of data to be used in the construction of a task manager table
+// column.
+struct TableColumnData {
+  // The ID of the column.
+  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.
+const TableColumnData kColumns[] = {

[Lei Zhang, 2015/08/27 01:10:44]

kDefaultColumns ?

[afakhry, 2015/08/27 19:56:52]

Not sure. They're not really the default columns. They're all the columns there
is. Then we use this list to fill the table and then show or hide them based on
TableColumnData::default_visibility. Later if we can successfully retrieve the
columns settings from the local state, we change the visibility accordingly.

+  { IDS_TASK_MANAGER_TASK_COLUMN, ui::TableColumn::LEFT, -1, 1, true, true,
+    true },
+  { IDS_TASK_MANAGER_PROFILE_NAME_COLUMN, ui::TableColumn::LEFT, -1, 0, true,
+    true, false },
+  { IDS_TASK_MANAGER_PHYSICAL_MEM_COLUMN, ui::TableColumn::RIGHT, -1, 0, true,
+    false, true },
+  { IDS_TASK_MANAGER_SHARED_MEM_COLUMN, ui::TableColumn::RIGHT, -1, 0, true,
+    false, false },
+  { IDS_TASK_MANAGER_PRIVATE_MEM_COLUMN, ui::TableColumn::RIGHT, -1, 0, true,
+    false, false },
+  { IDS_TASK_MANAGER_CPU_COLUMN, ui::TableColumn::RIGHT, -1, 0, true, false,
+    true },
+  { IDS_TASK_MANAGER_NET_COLUMN, ui::TableColumn::RIGHT, -1, 0, true, false,
+    true },
+  { IDS_TASK_MANAGER_PROCESS_ID_COLUMN, ui::TableColumn::RIGHT, -1, 0, true,
+    true, true },
+
+#if defined(OS_WIN)
+  { IDS_TASK_MANAGER_GDI_HANDLES_COLUMN, ui::TableColumn::RIGHT, -1, 0, true,
+    false, false },
+  { IDS_TASK_MANAGER_USER_HANDLES_COLUMN, ui::TableColumn::RIGHT, -1, 0, true,
+    false, false },
+#endif
+
+  { IDS_TASK_MANAGER_WEBCORE_IMAGE_CACHE_COLUMN, ui::TableColumn::RIGHT, -1, 0,
+    true, false, false },
+  { IDS_TASK_MANAGER_WEBCORE_SCRIPTS_CACHE_COLUMN, ui::TableColumn::RIGHT, -1,
+    0, true, false, false },
+  { IDS_TASK_MANAGER_WEBCORE_CSS_CACHE_COLUMN, ui::TableColumn::RIGHT, -1, 0,
+    true, false, false },
+  { IDS_TASK_MANAGER_VIDEO_MEMORY_COLUMN, ui::TableColumn::RIGHT, -1, 0, true,
+    false, false },
+  { IDS_TASK_MANAGER_SQLITE_MEMORY_USED_COLUMN, ui::TableColumn::RIGHT, -1, 0,
+    true, false, false },
+
+#if !defined(DISABLE_NACL)
+  { IDS_TASK_MANAGER_NACL_DEBUG_STUB_PORT_COLUMN, ui::TableColumn::RIGHT, -1, 0,
+    true, true, false },
+#endif  // !defined(DISABLE_NACL)
+
+  { IDS_TASK_MANAGER_JAVASCRIPT_MEMORY_ALLOCATED_COLUMN, ui::TableColumn::RIGHT,
+    -1, 0, true, false, false },
+
+#if defined(OS_MACOSX) || defined(OS_LINUX)
+  // TODO(port): Port the idle wakeups per second to platforms other than Linux
+  // and MacOS (http://crbug.com/120488).
+  { IDS_TASK_MANAGER_IDLE_WAKEUPS_COLUMN, ui::TableColumn::RIGHT, -1, 0, true,
+    false, false },
+#endif  // defined(OS_MACOSX) || defined(OS_LINUX)
+};
+
+const size_t kColumnsSize = arraysize(kColumns);
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -280,8 +360,20 @@ class NewTaskManagerView::TableModel
 
   // Called when a column visibility is toggled from the context menu of the
   // table view. This will result in enabling or disabling some resources
-  // refresh types in the task manager.
-  void ToggleColumnVisibility(views::TableView* table, int column_id);
+  // refresh types in the task manager. It will also update the
+  // |visibility_settings| with the new visibility so that it's stored in the
+  // prefs when the task manager window is closed.
+  void ToggleColumnVisibility(views::TableView* table,
+                              base::DictionaryValue* visibility_settings,
+                              int column_id);
+
+  // Based on the given |visibility| and the |column_id|, a particular refresh
+  // type will be enabled or disabled. Multiple columns can map to the same
+  // refresh type, for that we need |table| to determine if any is visible.
+  void UpdateRefreshTypes(views::TableView* table,
+                          int column_id,
+                          bool visibility);
+
 
   // Checks if the task at |row_index| is running on the browser process.
   bool IsBrowserProcess(int row_index) const;
@@ -620,14 +712,28 @@ void NewTaskManagerView::TableModel::KillTask(int row_index) {
 
 void NewTaskManagerView::TableModel::ToggleColumnVisibility(
     views::TableView* table,
+    base::DictionaryValue* visibility_settings,
     int column_id) {
   DCHECK(table);
+  DCHECK(visibility_settings);
 
   bool new_visibility = !table->IsColumnVisible(column_id);
   table->SetColumnVisibility(column_id, new_visibility);
+  visibility_settings->SetBoolean(base::IntToString(column_id), new_visibility);
+
+  UpdateRefreshTypes(table, column_id, new_visibility);
+}
 
+void NewTaskManagerView::TableModel::UpdateRefreshTypes(views::TableView* table,
+                                                        int column_id,
+                                                        bool visibility) {
   RefreshType type = REFRESH_TYPE_NONE;
   switch (column_id) {
+    case IDS_TASK_MANAGER_PROFILE_NAME_COLUMN:
+    case IDS_TASK_MANAGER_TASK_COLUMN:
+    case IDS_TASK_MANAGER_PROCESS_ID_COLUMN:
+      return;

[Lei Zhang, 2015/08/27 01:10:44]

Add a short comment to say why we return early.

// The data in these columns do not change.

[afakhry, 2015/08/27 19:56:52]

Done.

+
     case IDS_TASK_MANAGER_NET_COLUMN:
       type = REFRESH_TYPE_NETWORK_USAGE;
       break;
@@ -636,14 +742,14 @@ void NewTaskManagerView::TableModel::ToggleColumnVisibility(
       type = REFRESH_TYPE_CPU;
       break;
 
+    case IDS_TASK_MANAGER_PHYSICAL_MEM_COLUMN:
     case IDS_TASK_MANAGER_PRIVATE_MEM_COLUMN:
     case IDS_TASK_MANAGER_SHARED_MEM_COLUMN:
-    case IDS_TASK_MANAGER_PHYSICAL_MEM_COLUMN:
       type = REFRESH_TYPE_MEMORY;
-      if (table->IsColumnVisible(IDS_TASK_MANAGER_PRIVATE_MEM_COLUMN) ||
-          table->IsColumnVisible(IDS_TASK_MANAGER_SHARED_MEM_COLUMN) ||
+      if (table->IsColumnVisible(IDS_TASK_MANAGER_PHYSICAL_MEM_COLUMN) ||
+          table->IsColumnVisible(IDS_TASK_MANAGER_PRIVATE_MEM_COLUMN) ||
           table->IsColumnVisible(IDS_TASK_MANAGER_SHARED_MEM_COLUMN)) {

[afakhry, 2015/08/26 01:16:27]

This was a bug as you can see.

[Lei Zhang, 2015/08/27 01:10:44]

Mmm, delicious copy + pasta bug.

-        new_visibility = true;
+        visibility = true;

[Lei Zhang, 2015/08/27 01:10:44]

I don't like changing POD arguments. Can we keep |new_visibility| ?

[afakhry, 2015/08/27 19:56:52]

Done.

       }
       break;
 
@@ -652,7 +758,7 @@ void NewTaskManagerView::TableModel::ToggleColumnVisibility(
       type = REFRESH_TYPE_HANDLES;
       if (table->IsColumnVisible(IDS_TASK_MANAGER_GDI_HANDLES_COLUMN) ||
           table->IsColumnVisible(IDS_TASK_MANAGER_USER_HANDLES_COLUMN)) {
-        new_visibility = true;
+        visibility = true;
       }
       break;
 
@@ -668,7 +774,7 @@ void NewTaskManagerView::TableModel::ToggleColumnVisibility(
           table->IsColumnVisible(
               IDS_TASK_MANAGER_WEBCORE_SCRIPTS_CACHE_COLUMN) ||
           table->IsColumnVisible(IDS_TASK_MANAGER_WEBCORE_CSS_CACHE_COLUMN)) {
-        new_visibility = true;
+        visibility = true;
       }
       break;
 
@@ -693,7 +799,7 @@ void NewTaskManagerView::TableModel::ToggleColumnVisibility(
       return;
   }
 
-  if (new_visibility)
+  if (visibility)
     AddRefreshType(type);
   else
     RemoveRefreshType(type);
@@ -894,6 +1000,7 @@ void NewTaskManagerView::WindowClosing() {
     // owned by the Views hierarchy.
     g_task_manager_view = nullptr;
   }
+  StoreColumnVisibility();
   table_model_->StopUpdating();
 }
 
@@ -968,7 +1075,9 @@ bool NewTaskManagerView::GetAcceleratorForCommandId(
 }
 
 void NewTaskManagerView::ExecuteCommand(int id, int event_flags) {
-  table_model_->ToggleColumnVisibility(tab_table_, id);
+  table_model_->ToggleColumnVisibility(tab_table_,
+                                       column_visibility_settings_.get(),
+                                       id);
 }
 
 NewTaskManagerView::NewTaskManagerView(chrome::HostDesktopType desktop_type)
@@ -976,7 +1085,8 @@ NewTaskManagerView::NewTaskManagerView(chrome::HostDesktopType desktop_type)
         new NewTaskManagerView::TableModel(REFRESH_TYPE_CPU |
                                            REFRESH_TYPE_MEMORY |
                                            REFRESH_TYPE_NETWORK_USAGE)),
-      menu_runner_(nullptr),
+      menu_runner_(),

[Lei Zhang, 2015/08/27 01:10:44]

I thought we agreed to just get rid of these in a previous CL.

[afakhry, 2015/08/27 19:56:52]

Bad habits keep haunting me! :D .. Done. 

+      column_visibility_settings_(),
       always_on_top_menu_text_(),
       kill_button_(nullptr),
       about_memory_link_(nullptr),
@@ -989,134 +1099,33 @@ NewTaskManagerView::NewTaskManagerView(chrome::HostDesktopType desktop_type)
 }
 
 void NewTaskManagerView::Init() {
-  columns_.push_back(ui::TableColumn(IDS_TASK_MANAGER_TASK_COLUMN,
-                                     ui::TableColumn::LEFT, -1, 1));
-  columns_.back().sortable = true;
-
-  columns_.push_back(ui::TableColumn(IDS_TASK_MANAGER_PROFILE_NAME_COLUMN,
-                                     ui::TableColumn::LEFT, -1, 0));
-  columns_.back().sortable = true;
-
-  columns_.push_back(ui::TableColumn(IDS_TASK_MANAGER_PHYSICAL_MEM_COLUMN,
-                                     ui::TableColumn::RIGHT, -1, 0));
-  columns_.back().sortable = true;
-  columns_.back().initial_sort_is_ascending = false;
-
-  columns_.push_back(ui::TableColumn(IDS_TASK_MANAGER_SHARED_MEM_COLUMN,
-                                     ui::TableColumn::RIGHT, -1, 0));
-  columns_.back().sortable = true;
-  columns_.back().initial_sort_is_ascending = false;
-
-  columns_.push_back(ui::TableColumn(IDS_TASK_MANAGER_PRIVATE_MEM_COLUMN,
-                                     ui::TableColumn::RIGHT, -1, 0));
-  columns_.back().sortable = true;
-  columns_.back().initial_sort_is_ascending = false;
-
-  columns_.push_back(ui::TableColumn(IDS_TASK_MANAGER_CPU_COLUMN,
-                                     ui::TableColumn::RIGHT, -1, 0));
-  columns_.back().sortable = true;
-  columns_.back().initial_sort_is_ascending = false;
-
-  columns_.push_back(ui::TableColumn(IDS_TASK_MANAGER_NET_COLUMN,
-                                     ui::TableColumn::RIGHT, -1, 0));
-  columns_.back().sortable = true;
-  columns_.back().initial_sort_is_ascending = false;
-
-  columns_.push_back(ui::TableColumn(IDS_TASK_MANAGER_PROCESS_ID_COLUMN,
-                                     ui::TableColumn::RIGHT, -1, 0));
-  columns_.back().sortable = true;
-
-#if defined(OS_WIN)
-  columns_.push_back(ui::TableColumn(IDS_TASK_MANAGER_GDI_HANDLES_COLUMN,
-                                     ui::TableColumn::RIGHT, -1, 0));
-  columns_.back().sortable = true;
-  columns_.back().initial_sort_is_ascending = false;
-  columns_.push_back(ui::TableColumn(IDS_TASK_MANAGER_USER_HANDLES_COLUMN,
-                                     ui::TableColumn::RIGHT, -1, 0));
-  columns_.back().sortable = true;
-  columns_.back().initial_sort_is_ascending = false;
-#endif
-
-  columns_.push_back(ui::TableColumn(
-      IDS_TASK_MANAGER_WEBCORE_IMAGE_CACHE_COLUMN,
-      ui::TableColumn::RIGHT, -1, 0));
-  columns_.back().sortable = true;
-  columns_.back().initial_sort_is_ascending = false;
-
-  columns_.push_back(ui::TableColumn(
-      IDS_TASK_MANAGER_WEBCORE_SCRIPTS_CACHE_COLUMN,
-      ui::TableColumn::RIGHT, -1, 0));
-  columns_.back().sortable = true;
-  columns_.back().initial_sort_is_ascending = false;
-
-  columns_.push_back(ui::TableColumn(IDS_TASK_MANAGER_WEBCORE_CSS_CACHE_COLUMN,
-                                     ui::TableColumn::RIGHT, -1, 0));
-  columns_.back().sortable = true;
-  columns_.back().initial_sort_is_ascending = false;
-
-  columns_.push_back(ui::TableColumn(IDS_TASK_MANAGER_VIDEO_MEMORY_COLUMN,
-                                     ui::TableColumn::RIGHT, -1, 0));
-  columns_.back().sortable = true;
-  columns_.back().initial_sort_is_ascending = false;
-
-  columns_.push_back(ui::TableColumn(IDS_TASK_MANAGER_SQLITE_MEMORY_USED_COLUMN,
-                                     ui::TableColumn::RIGHT, -1, 0));
-  columns_.back().sortable = true;
-  columns_.back().initial_sort_is_ascending = false;
-
-#if !defined(DISABLE_NACL)
-  columns_.push_back(ui::TableColumn(
-      IDS_TASK_MANAGER_NACL_DEBUG_STUB_PORT_COLUMN,
-      ui::TableColumn::RIGHT, -1, 0));
-  columns_.back().sortable = true;
-#endif  // !defined(DISABLE_NACL)
-
-  columns_.push_back(
-      ui::TableColumn(IDS_TASK_MANAGER_JAVASCRIPT_MEMORY_ALLOCATED_COLUMN,
-                      ui::TableColumn::RIGHT, -1, 0));
-  columns_.back().sortable = true;
-  columns_.back().initial_sort_is_ascending = false;
-
-#if defined(OS_MACOSX) || defined(OS_LINUX)
-  // TODO(port): Port the idle wakeups per second to platforms other than Linux
-  // and MacOS (http://crbug.com/120488).
-  columns_.push_back(ui::TableColumn(IDS_TASK_MANAGER_IDLE_WAKEUPS_COLUMN,
-                                     ui::TableColumn::RIGHT, -1, 0));
-  columns_.back().sortable = true;
-  columns_.back().initial_sort_is_ascending = false;
-#endif  // defined(OS_MACOSX) || defined(OS_LINUX)
+  column_visibility_settings_.reset(new base::DictionaryValue);
+
+  // Create the table columns.
+  for (size_t i = 0; i < kColumnsSize; ++i) {
+    const auto& col_data = kColumns[i];
+    columns_.push_back(ui::TableColumn(col_data.id, col_data.align,
+                                       col_data.width, col_data.percent));
+    columns_.back().sortable = col_data.sortable;
+    columns_.back().initial_sort_is_ascending =
+        col_data.initial_sort_is_ascending;
+
+    column_visibility_settings_->SetBoolean(base::IntToString(col_data.id),
+                                            col_data.default_visibility);
+  }
 
+  // Create the table view.
   tab_table_ = new views::TableView(
       table_model_.get(), columns_, views::ICON_AND_TEXT, false);
   tab_table_->SetGrouper(table_model_.get());
-
-  // Hide some columns by default
-  tab_table_->SetColumnVisibility(IDS_TASK_MANAGER_PROFILE_NAME_COLUMN, false);
-  tab_table_->SetColumnVisibility(IDS_TASK_MANAGER_SHARED_MEM_COLUMN, false);
-  tab_table_->SetColumnVisibility(IDS_TASK_MANAGER_PRIVATE_MEM_COLUMN, false);
-  tab_table_->SetColumnVisibility(IDS_TASK_MANAGER_WEBCORE_IMAGE_CACHE_COLUMN,
-                                  false);
-  tab_table_->SetColumnVisibility(IDS_TASK_MANAGER_WEBCORE_SCRIPTS_CACHE_COLUMN,
-                                  false);
-  tab_table_->SetColumnVisibility(IDS_TASK_MANAGER_WEBCORE_CSS_CACHE_COLUMN,
-                                  false);
-  tab_table_->SetColumnVisibility(IDS_TASK_MANAGER_VIDEO_MEMORY_COLUMN,
-                                  false);
-  tab_table_->SetColumnVisibility(IDS_TASK_MANAGER_SQLITE_MEMORY_USED_COLUMN,
-                                  false);
-  tab_table_->SetColumnVisibility(IDS_TASK_MANAGER_NACL_DEBUG_STUB_PORT_COLUMN,
-                                  false);
-  tab_table_->SetColumnVisibility(
-      IDS_TASK_MANAGER_JAVASCRIPT_MEMORY_ALLOCATED_COLUMN,
-      false);
-  tab_table_->SetColumnVisibility(IDS_TASK_MANAGER_GDI_HANDLES_COLUMN, false);
-  tab_table_->SetColumnVisibility(IDS_TASK_MANAGER_USER_HANDLES_COLUMN, false);
-  tab_table_->SetColumnVisibility(IDS_TASK_MANAGER_IDLE_WAKEUPS_COLUMN, false);
-
   tab_table_->SetObserver(this);
   tab_table_->set_context_menu_controller(this);
   set_context_menu_controller(this);
 
+  // Retrieve and update the table columns visibilities.
+  RetrieveSavedColumnVisibility();
+  UpdateColumnVisibility();
+
   kill_button_ = new views::LabelButton(this,
       l10n_util::GetStringUTF16(IDS_TASK_MANAGER_KILL));
   kill_button_->SetStyle(views::Button::STYLE_BUTTON);
@@ -1154,5 +1163,59 @@ void NewTaskManagerView::RetriveSavedAlwaysOnTopState() {
     dictionary->GetBoolean("always_on_top", &is_always_on_top_);
 }
 
+void NewTaskManagerView::RetrieveSavedColumnVisibility() {
+  if (!g_browser_process->local_state())
+    return;
+
+  const base::DictionaryValue* dictionary =
+      g_browser_process->local_state()->GetDictionary(
+          prefs::kTaskManagerColumnVisibility);
+  if (!dictionary)
+    return;
+
+  int stored_num_columns;
+  if (!dictionary->GetInteger("num_columns", &stored_num_columns))
+    return;
+
+  // If the number of columns change from one version to another, we skip and

[Lei Zhang, 2015/08/27 01:10:44]

Can we use a version number instead. What if:

- we add column A in version N, and then delete column B in version N + 1
- user persists data to disk in version N
- user skips version N + 1 and updates to version N + 2

[afakhry, 2015/08/27 19:56:52]

Thanks for pointing this out. I event found out that the integer values of the
columns IDS_* (generated from generated_resources.grd) can change from one
commit to another. They can't be used as a reliable way to key in the session
restore column settings. This was causing me to fail on the DCHECK(success) at
the Update..() method continuously after I did a rebase. So now I'm using their
string literal values instead like "IDS_TASK_MANAGER_TASK_COLUMN" instead of
15544 which can change and is out of our control.

I aslo added the version number and the sort column ID and the sort direction.

+  // use the default visibility of the columns as defined in |kColumns|.
+  if (stored_num_columns != static_cast<int>(kColumnsSize))
+    return;
+
+  column_visibility_settings_.reset(dictionary->DeepCopy());
+}
+
+void NewTaskManagerView::UpdateColumnVisibility() {
+  for (size_t i = 0; i < kColumnsSize; ++i) {
+    const int col_id = kColumns[i].id;
+    bool visible = false;
+    bool success = column_visibility_settings_->GetBoolean(
+        base::IntToString(col_id), &visible);
+
+    DCHECK(success);
+
+    tab_table_->SetColumnVisibility(col_id, visible);
+    table_model_->UpdateRefreshTypes(tab_table_, col_id, visible);
+  }
+}
+
+void NewTaskManagerView::StoreColumnVisibility() {
+  PrefService* local_state = g_browser_process->local_state();
+
+  if (!local_state)
+    return;
+
+  DictionaryPrefUpdate dict_update(local_state,
+                                   prefs::kTaskManagerColumnVisibility);
+
+  base::DictionaryValue::Iterator it(*column_visibility_settings_);
+  while (!it.IsAtEnd()) {
+    dict_update->Set(it.key(), it.value().CreateDeepCopy());
+    it.Advance();
+  }
+
+  dict_update->SetInteger("num_columns", static_cast<int>(kColumnsSize));
+}
+
 }  // namespace task_management