Task Manager Should remember the most recently enabled columns.

chrome/browser/ui/views/new_task_manager_view.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 "chrome/browser/ui/views/new_task_manager_view.h"
 
 #include <map>
 
 #include "base/i18n/number_formatting.h"
 #include "base/prefs/pref_service.h"
@@ skipping 219 matching lines @@
   // The string "*" that is used to show that there exists duplicates in the
   // GPU memory.
   const base::string16 asterisk_string_;
 
   // The string "Unknown".
   const base::string16 unknown_string_;
 
   DISALLOW_COPY_AND_ASSIGN(TaskManagerValuesStringifier);
 };
 
+// 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 version number we manually assign for the list of columns. Any addition/
+// removal to/from the columns list must be accompanied by incrementing the
+// version number. This is needed so that we can restore the visible columns
+// correctly.
+const int kColumnsVersion = 1;
+
+// The task manager table columns and their properties.
+// IMPORTANT: Do NOT change the below list without changing the above version
+// number.

[Lei Zhang, 2015/08/28 22:47:44]

Remind readers about COLUMNS_LITS too?

[afakhry, 2015/08/29 00:30:29]

Done.

+const TableColumnData kColumns[] = {
+  { 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);
+
+// We can't use the integer IDs of the columns converted to strings as session
+// restore keys. These integer values can change from one build to another as
+// they are generated. Instead we use the literal string value of the column
+// ID symbol (i.e. for the ID IDS_TASK_MANAGER_TASK_COLUMN, we use the literal
+// string "IDS_TASK_MANAGER_TASK_COLUMN". The following macros help us
+// efficiently get the literal ID for the integer value.
+#define COLUMNS_LITS(def) \
+  def(IDS_TASK_MANAGER_TASK_COLUMN) \
+  def(IDS_TASK_MANAGER_PROFILE_NAME_COLUMN) \
+  def(IDS_TASK_MANAGER_PHYSICAL_MEM_COLUMN) \
+  def(IDS_TASK_MANAGER_SHARED_MEM_COLUMN) \
+  def(IDS_TASK_MANAGER_PRIVATE_MEM_COLUMN) \
+  def(IDS_TASK_MANAGER_CPU_COLUMN) \
+  def(IDS_TASK_MANAGER_NET_COLUMN) \
+  def(IDS_TASK_MANAGER_PROCESS_ID_COLUMN) \
+  def(IDS_TASK_MANAGER_GDI_HANDLES_COLUMN) \
+  def(IDS_TASK_MANAGER_USER_HANDLES_COLUMN) \
+  def(IDS_TASK_MANAGER_WEBCORE_IMAGE_CACHE_COLUMN) \
+  def(IDS_TASK_MANAGER_WEBCORE_SCRIPTS_CACHE_COLUMN) \
+  def(IDS_TASK_MANAGER_WEBCORE_CSS_CACHE_COLUMN) \
+  def(IDS_TASK_MANAGER_VIDEO_MEMORY_COLUMN) \
+  def(IDS_TASK_MANAGER_SQLITE_MEMORY_USED_COLUMN) \
+  def(IDS_TASK_MANAGER_NACL_DEBUG_STUB_PORT_COLUMN) \
+  def(IDS_TASK_MANAGER_JAVASCRIPT_MEMORY_ALLOCATED_COLUMN) \
+  def(IDS_TASK_MANAGER_IDLE_WAKEUPS_COLUMN)
+// Add to the above list in the macro any new IDs added in the future. Also
+// remove the removed ones.
+
+#define COLUMN_ID_AS_STRING(col_id) case col_id: return std::string(#col_id);
+
+std::string GetColumnIdAsString(int column_id) {
+  switch (column_id) {
+    COLUMNS_LITS(COLUMN_ID_AS_STRING)
+    default:
+      NOTREACHED();
+      return std::string();
+  }
+}
+
+// Session Restore Keys.
+const char kColumnVersionKey[] = "columns_version";
+const char kSortColumnIdKey[] = "sort_column_id";
+const char kSortIsAscendingKey[] = "sort_is_ascending";
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
 
 // The table model of the task manager table view that will observe the
 // task manager backend and adapt its interface to match the requirements of the
 // TableView.
 class NewTaskManagerView::TableModel
     : public TaskManagerObserver,
       public ui::TableModel,
@@ skipping 23 matching lines @@
 
   // Activates the browser tab associated with the process in the specified
   // |row_index|.
   void ActivateTask(int row_index);
 
   // Kills the process on which the task at |row_index| is running.
   void KillTask(int row_index);
 
   // 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;
 
  private:
   void OnRefresh();
 
   // Checks whether the task at |row_index| is the first task in its process
   // group of tasks.
   bool IsTaskFirstInGroup(int row_index) const;
@@ skipping 318 matching lines @@
       tasks_[row_index]);
 
   DCHECK_NE(proc_id, base::GetCurrentProcId());
 
   base::Process process = base::Process::Open(proc_id);
   process.Terminate(content::RESULT_CODE_KILLED, false);
 }
 
 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(GetColumnIdAsString(column_id),
+                                  new_visibility);
 
+  UpdateRefreshTypes(table, column_id, new_visibility);
+}
+
+void NewTaskManagerView::TableModel::UpdateRefreshTypes(views::TableView* table,
+                                                        int column_id,
+                                                        bool visibility) {
+  bool new_visibility = 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;  // The data is these columns do not change.
+
     case IDS_TASK_MANAGER_NET_COLUMN:
       type = REFRESH_TYPE_NETWORK_USAGE;
       break;
 
     case IDS_TASK_MANAGER_CPU_COLUMN:
       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)) {
         new_visibility = true;
       }
       break;
 
     case IDS_TASK_MANAGER_GDI_HANDLES_COLUMN:
     case IDS_TASK_MANAGER_USER_HANDLES_COLUMN:
       type = REFRESH_TYPE_HANDLES;
       if (table->IsColumnVisible(IDS_TASK_MANAGER_GDI_HANDLES_COLUMN) ||
           table->IsColumnVisible(IDS_TASK_MANAGER_USER_HANDLES_COLUMN)) {
@@ skipping 232 matching lines @@
 
 void NewTaskManagerView::WindowClosing() {
   // Now that the window is closed, we can allow a new one to be opened.
   // (WindowClosing comes in asynchronously from the call to Close() and we
   // may have already opened a new instance).
   if (g_task_manager_view == this) {
     // We don't have to delete |g_task_manager_view| as we don't own it. It's
     // owned by the Views hierarchy.
     g_task_manager_view = nullptr;
   }
+  StoreColumnsSettings();
   table_model_->StopUpdating();
 }
 
 bool NewTaskManagerView::UseNewStyleForThisDialog() const {
   return false;
 }
 
 void NewTaskManagerView::OnSelectionChanged() {
   const ui::ListSelectionModel::SelectedIndices& selections(
       tab_table_->selection_model().selected_indices());
@@ skipping 54 matching lines @@
   return true;
 }
 
 bool NewTaskManagerView::GetAcceleratorForCommandId(
     int command_id,
     ui::Accelerator* accelerator) {
   return false;
 }
 
 void NewTaskManagerView::ExecuteCommand(int id, int event_flags) {
-  table_model_->ToggleColumnVisibility(tab_table_, id);
+  table_model_->ToggleColumnVisibility(tab_table_,
+                                       columns_settings_.get(),
+                                       id);
 }
 
 NewTaskManagerView::NewTaskManagerView(chrome::HostDesktopType desktop_type)
     : table_model_(
         new NewTaskManagerView::TableModel(REFRESH_TYPE_CPU |
                                            REFRESH_TYPE_MEMORY |
                                            REFRESH_TYPE_NETWORK_USAGE)),
-      menu_runner_(nullptr),
-      always_on_top_menu_text_(),
       kill_button_(nullptr),
       about_memory_link_(nullptr),
       tab_table_(nullptr),
       tab_table_parent_(nullptr),
-      columns_(),
       desktop_type_(desktop_type),
       is_always_on_top_(false) {
   Init();
 }
 
 void NewTaskManagerView::Init() {
-  columns_.push_back(ui::TableColumn(IDS_TASK_MANAGER_TASK_COLUMN,
-                                     ui::TableColumn::LEFT, -1, 1));
-  columns_.back().sortable = true;
+  columns_settings_.reset(new base::DictionaryValue);
 
-  columns_.push_back(ui::TableColumn(IDS_TASK_MANAGER_PROFILE_NAME_COLUMN,
-                                     ui::TableColumn::LEFT, -1, 0));
-  columns_.back().sortable = true;
+  // 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;
 
-  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_settings_->SetBoolean(GetColumnIdAsString(col_data.id),
+                                            col_data.default_visibility);

[Lei Zhang, 2015/08/28 22:47:45]

nit: indentation

[afakhry, 2015/08/29 00:30:29]

Done.

+  }
 
-  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)
-
+  // 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.
+  RetrieveSavedColumnsSettings();
+  UpdateTableFromColumnsSettings();
+
   kill_button_ = new views::LabelButton(this,
       l10n_util::GetStringUTF16(IDS_TASK_MANAGER_KILL));
   kill_button_->SetStyle(views::Button::STYLE_BUTTON);
 
   about_memory_link_ = new views::Link(
       l10n_util::GetStringUTF16(IDS_TASK_MANAGER_ABOUT_MEMORY_LINK));
   about_memory_link_->set_listener(this);
 
   // Makes sure our state is consistent.
   OnSelectionChanged();
@@ skipping 17 matching lines @@
 
   if (!g_browser_process->local_state())
     return;
 
   const base::DictionaryValue* dictionary =
     g_browser_process->local_state()->GetDictionary(GetWindowName());
   if (dictionary)
     dictionary->GetBoolean("always_on_top", &is_always_on_top_);
 }
 
+void NewTaskManagerView::RetrieveSavedColumnsSettings() {
+  if (!g_browser_process->local_state())
+    return;
+
+  const base::DictionaryValue* dictionary =
+      g_browser_process->local_state()->GetDictionary(
+          prefs::kTaskManagerColumnVisibility);
+  if (!dictionary)
+    return;
+
+  int columns_version;
+  if (!dictionary->GetInteger(kColumnVersionKey, &columns_version))
+    return;
+
+  // If the number of columns changes from one version to another (i.e. the

[Lei Zhang, 2015/08/28 22:47:44]

So the other possible way of doing this is to be very tolerant of bad/outdated
data in UpdateTableFromColumnsSettings() and just try to make a best effort
attempt at restoring the settings from the pref, ignoring any pref that do not
make sense. Then maybe we won't need a version number. Do you think that may
work better?

[afakhry, 2015/08/29 00:30:29]

I agree. I actually combined the Retrieve() and UpdateTable() functions into
one. So now we loop through the values we're sure they're correct. try to
retrieve their last stored values from the local state, if they succeed, fine,
if not, we're still using the default values. And then update the table view
while we're there. Now |columns_settings_| is always guaranteed to contain
correct values rather than possibly garbage we got from the below line that I
removed |columns_settings_.reset(dictionary->DeepCopy());|.

+  // columns version number changes), we skip and use the default visibility of
+  // the columns as defined in |kColumns|.
+  if (columns_version != kColumnsVersion)
+    return;
+
+  columns_settings_.reset(dictionary->DeepCopy());
+}
+
+void NewTaskManagerView::UpdateTableFromColumnsSettings() {
+  std::string sorted_col_id;
+  bool sort_is_ascending = true;
+  columns_settings_->GetString(kSortColumnIdKey, &sorted_col_id);
+  columns_settings_->GetBoolean(kSortIsAscendingKey, &sort_is_ascending);
+
+  int current_visible_column_index = 0;
+  for (size_t i = 0; i < kColumnsSize; ++i) {
+    const int col_id = kColumns[i].id;
+    bool visible = false;
+    bool success = columns_settings_->GetBoolean(
+        GetColumnIdAsString(col_id), &visible);
+
+    DCHECK(success);

[Lei Zhang, 2015/08/28 22:47:44]

Given the existence of malware and other external programs that try to mess with
Chrome's setting files, (boo!!!) I would not count on this DCHECK() to hold in
real life. Can we just use the default visibility/sorting on failure?

[afakhry, 2015/08/29 00:30:29]

Done.

+
+    tab_table_->SetColumnVisibility(col_id, visible);
+    table_model_->UpdateRefreshTypes(tab_table_, col_id, visible);
+
+    if (visible) {
+      if (sorted_col_id == GetColumnIdAsString(col_id)) {
+        if (sort_is_ascending == kColumns[i].initial_sort_is_ascending) {
+          tab_table_->ToggleSortOrder(current_visible_column_index);
+        } else {
+          // Unfortunately the API of ui::TableView doesn't provide a clean way
+          // to sort by a particular column ID and a sort direction. If the
+          // retrieved sort direction is different than the initial one, we have
+          // to toggle the sort order twice!
+          // Note that the function takes the visible_column_index rather than
+          // a column ID.

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

Obviously not a big fan of the TableView API. I personally like SWT:
http://help.eclipse.org/juno/index.jsp?topic=%2Forg.eclipse.platform.doc.isv%...

+          tab_table_->ToggleSortOrder(current_visible_column_index);
+          tab_table_->ToggleSortOrder(current_visible_column_index);
+        }
+      }
+
+      ++current_visible_column_index;
+    }
+  }
+}
+
+void NewTaskManagerView::StoreColumnsSettings() {
+  PrefService* local_state = g_browser_process->local_state();
+
+  if (!local_state)
+    return;
+
+  DictionaryPrefUpdate dict_update(local_state,
+                                   prefs::kTaskManagerColumnVisibility);
+
+  base::DictionaryValue::Iterator it(*columns_settings_);
+  while (!it.IsAtEnd()) {
+    dict_update->Set(it.key(), it.value().CreateDeepCopy());
+    it.Advance();
+  }
+
+  // Store the current sort status to be restored again at startup.
+  if (tab_table_->sort_descriptors().empty()) {
+    dict_update->SetString(kSortColumnIdKey, "");
+  } else {
+    const auto& sort_descriptor = tab_table_->sort_descriptors().front();
+    dict_update->SetString(kSortColumnIdKey,
+                           GetColumnIdAsString(sort_descriptor.column_id));
+    dict_update->SetBoolean(kSortIsAscendingKey, sort_descriptor.ascending);
+  }
+
+  dict_update->SetInteger(kColumnVersionKey, kColumnsVersion);
+}
+
 }  // namespace task_management