Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1320563002:
Task Manager Should remember the most recently enabled columns. (Closed)
DescriptionTask 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.
R=thestig@chromium.org
BUG=452521
TEST=browser_tests --gtest_filter=NewTaskManagerViewTest.*
Committed: https://crrev.com/4e16add803fbb0ed18413496d3fb4424fa4957c6
Cr-Commit-Position: refs/heads/master@{#346524}
Committed: https://crrev.com/4780b87d1c25f68c39abc623199f3e71a477d32d
Cr-Commit-Position: refs/heads/master@{#346734}
Patch Set 1 #
Total comments: 14
Patch Set 2 : thestig's comments #
Total comments: 13
Patch Set 3 : comments #
Total comments: 6
Patch Set 4 : #Patch Set 5 : Added browser tests #
Total comments: 2
Patch Set 6 : nit #Patch Set 7 : Removed the un-necessary NOTREACHED() #Patch Set 8 : Making sure tests end in good conditions #
Total comments: 2
Patch Set 9 : Tearing Down #
Messages
Total messages: 39 (8 generated)
thestig@chromium.org: Could you please review this CL? https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... File chrome/browser/ui/views/new_task_manager_view.cc (left): https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... chrome/browser/ui/views/new_task_manager_view.cc:645: table->IsColumnVisible(IDS_TASK_MANAGER_SHARED_MEM_COLUMN)) { This was a bug as you can see.
On 2015/08/26 01:16:27, afakhry wrote: > mailto:thestig@chromium.org: Could you please review this CL? > > https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... > File chrome/browser/ui/views/new_task_manager_view.cc (left): > > https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... > chrome/browser/ui/views/new_task_manager_view.cc:645: > table->IsColumnVisible(IDS_TASK_MANAGER_SHARED_MEM_COLUMN)) { > This was a bug as you can see. thestig@chromium.org: Friendly ping :)
On 2015/08/27 00:30:06, afakhry wrote: > On 2015/08/26 01:16:27, afakhry wrote: > > mailto:thestig@chromium.org: Could you please review this CL? > > > > > https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... > > File chrome/browser/ui/views/new_task_manager_view.cc (left): > > > > > https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... > > chrome/browser/ui/views/new_task_manager_view.cc:645: > > table->IsColumnVisible(IDS_TASK_MANAGER_SHARED_MEM_COLUMN)) { > > This was a bug as you can see. > > mailto:thestig@chromium.org: Friendly ping :) Looking... shrike@ had some additional comments on the bug, BTW.
On 2015/08/27 00:47:44, Lei Zhang wrote: > On 2015/08/27 00:30:06, afakhry wrote: > > On 2015/08/26 01:16:27, afakhry wrote: > > > mailto:thestig@chromium.org: Could you please review this CL? > > > > > > > > > https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... > > > File chrome/browser/ui/views/new_task_manager_view.cc (left): > > > > > > > > > https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... > > > chrome/browser/ui/views/new_task_manager_view.cc:645: > > > table->IsColumnVisible(IDS_TASK_MANAGER_SHARED_MEM_COLUMN)) { > > > This was a bug as you can see. > > > > mailto:thestig@chromium.org: Friendly ping :) > > Looking... shrike@ had some additional comments on the bug, BTW. Yes I just saw them. I'm working on adding support for the last sorted-by-column now. Thanks!
https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... File chrome/browser/ui/views/new_task_manager_view.cc (left): https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... chrome/browser/ui/views/new_task_manager_view.cc:645: table->IsColumnVisible(IDS_TASK_MANAGER_SHARED_MEM_COLUMN)) { On 2015/08/26 01:16:27, afakhry wrote: > This was a bug as you can see. Mmm, delicious copy + pasta bug. https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... File chrome/browser/ui/views/new_task_manager_view.cc (right): https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... chrome/browser/ui/views/new_task_manager_view.cc:266: const TableColumnData kColumns[] = { kDefaultColumns ? https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... chrome/browser/ui/views/new_task_manager_view.cc:735: return; Add a short comment to say why we return early. // The data in these columns do not change. https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... chrome/browser/ui/views/new_task_manager_view.cc:752: visibility = true; I don't like changing POD arguments. Can we keep |new_visibility| ? https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... chrome/browser/ui/views/new_task_manager_view.cc:1088: menu_runner_(), I thought we agreed to just get rid of these in a previous CL. https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... chrome/browser/ui/views/new_task_manager_view.cc:1180: // If the number of columns change from one version to another, we skip and 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 https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... File chrome/browser/ui/views/new_task_manager_view.h (right): https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... chrome/browser/ui/views/new_task_manager_view.h:121: scoped_ptr<base::DictionaryValue> column_visibility_settings_; Does this need to be a scoped_ptr? It's always going to a 'non-NULL' scoped_ptr.
PTAL. Thanks! https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... File chrome/browser/ui/views/new_task_manager_view.cc (right): https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... chrome/browser/ui/views/new_task_manager_view.cc:266: const TableColumnData kColumns[] = { On 2015/08/27 01:10:44, Lei Zhang wrote: > kDefaultColumns ? 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. https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... chrome/browser/ui/views/new_task_manager_view.cc:735: return; On 2015/08/27 01:10:44, Lei Zhang wrote: > Add a short comment to say why we return early. > > // The data in these columns do not change. Done. https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... chrome/browser/ui/views/new_task_manager_view.cc:752: visibility = true; On 2015/08/27 01:10:44, Lei Zhang wrote: > I don't like changing POD arguments. Can we keep |new_visibility| ? Done. https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... chrome/browser/ui/views/new_task_manager_view.cc:1088: menu_runner_(), On 2015/08/27 01:10:44, Lei Zhang wrote: > I thought we agreed to just get rid of these in a previous CL. Bad habits keep haunting me! :D .. Done. https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... chrome/browser/ui/views/new_task_manager_view.cc:1180: // If the number of columns change from one version to another, we skip and On 2015/08/27 01:10:44, Lei Zhang wrote: > 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 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. https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... File chrome/browser/ui/views/new_task_manager_view.h (right): https://codereview.chromium.org/1320563002/diff/1/chrome/browser/ui/views/new... chrome/browser/ui/views/new_task_manager_view.h:121: scoped_ptr<base::DictionaryValue> column_visibility_settings_; On 2015/08/27 01:10:44, Lei Zhang wrote: > Does this need to be a scoped_ptr? It's always going to a 'non-NULL' scoped_ptr. It has to be a scoped_ptr<>. DictionaryValue is owned by us and it's non copiable non assignable. We need to reset it to an new DictionaryValue if there's one to retrieve from the local state. We can only get it as a pointer or a scoped_ptr<>. https://codereview.chromium.org/1320563002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/new_task_manager_view.cc (right): https://codereview.chromium.org/1320563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.cc:1268: // a column ID. 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%...
thestig@: PTAL.
https://codereview.chromium.org/1320563002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/new_task_manager_view.cc (right): https://codereview.chromium.org/1320563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.cc:274: // number. Remind readers about COLUMNS_LITS too? https://codereview.chromium.org/1320563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.cc:1165: col_data.default_visibility); nit: indentation https://codereview.chromium.org/1320563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.cc:1231: // If the number of columns changes from one version to another (i.e. the 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? https://codereview.chromium.org/1320563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.cc:1253: DCHECK(success); 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? https://codereview.chromium.org/1320563002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/new_task_manager_view.h (right): https://codereview.chromium.org/1320563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.h:107: // Updates the table view based on the values currently stored in nit: extra space before "stored" https://codereview.chromium.org/1320563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.h:119: // Contains either the retrieved from user preferences columns settings if I have a bit of trouble parsing this sentence. How about: Contains either the column settings retrieved from user preferences if it exists, or the default column settings.
https://codereview.chromium.org/1320563002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/new_task_manager_view.cc (right): https://codereview.chromium.org/1320563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.cc:274: // number. On 2015/08/28 22:47:44, Lei Zhang wrote: > Remind readers about COLUMNS_LITS too? Done. https://codereview.chromium.org/1320563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.cc:1165: col_data.default_visibility); On 2015/08/28 22:47:45, Lei Zhang wrote: > nit: indentation Done. https://codereview.chromium.org/1320563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.cc:1231: // If the number of columns changes from one version to another (i.e. the On 2015/08/28 22:47:44, Lei Zhang wrote: > 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? 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());|. https://codereview.chromium.org/1320563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.cc:1253: DCHECK(success); On 2015/08/28 22:47:44, Lei Zhang wrote: > 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? Done. https://codereview.chromium.org/1320563002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/new_task_manager_view.h (right): https://codereview.chromium.org/1320563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.h:107: // Updates the table view based on the values currently stored in On 2015/08/28 22:47:45, Lei Zhang wrote: > nit: extra space before "stored" Done. https://codereview.chromium.org/1320563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.h:119: // Contains either the retrieved from user preferences columns settings if On 2015/08/28 22:47:45, Lei Zhang wrote: > I have a bit of trouble parsing this sentence. How about: > > Contains either the column settings retrieved from user preferences if it > exists, or the default column settings. Done.
Looking good. What are your plans to test this code? https://codereview.chromium.org/1320563002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/new_task_manager_view.cc (right): https://codereview.chromium.org/1320563002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.cc:1225: const std::string col_id_key(GetColumnIdAsString(col_id)); Continue to the next loop iteration if |col_id_key| is empty. https://codereview.chromium.org/1320563002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.cc:1230: // If the above GetBoolean() fails, the |col_visibility| will remain the "remain" -> "remain at" ? https://codereview.chromium.org/1320563002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.cc:1259: nit: remove blank line
I wasn't planning on writing any tests for this code, since the whole task manager view doesn't have a single unit test anywhere! But maybe I should? https://codereview.chromium.org/1320563002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/new_task_manager_view.cc (right): https://codereview.chromium.org/1320563002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.cc:1225: const std::string col_id_key(GetColumnIdAsString(col_id)); On 2015/08/29 00:47:09, Lei Zhang wrote: > Continue to the next loop iteration if |col_id_key| is empty. Done. https://codereview.chromium.org/1320563002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.cc:1230: // If the above GetBoolean() fails, the |col_visibility| will remain the On 2015/08/29 00:47:09, Lei Zhang wrote: > "remain" -> "remain at" ? Done. https://codereview.chromium.org/1320563002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view.cc:1259: On 2015/08/29 00:47:09, Lei Zhang wrote: > nit: remove blank line Done.
On 2015/08/29 01:48:14, afakhry wrote: > I wasn't planning on writing any tests for this code, since the whole task > manager view doesn't have a single unit test anywhere! > > But maybe I should? maybe... yes? :)
thestig@chromium.org: How about these browser tests? PTAL.
lgtm https://codereview.chromium.org/1320563002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/new_task_manager_view_browsertest.cc (right): https://codereview.chromium.org/1320563002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:63: ASSERT_NE(nullptr, table); I think you can also be written as ASSERT_TRUE(table);
https://codereview.chromium.org/1320563002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/new_task_manager_view_browsertest.cc (right): https://codereview.chromium.org/1320563002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:63: ASSERT_NE(nullptr, table); On 2015/08/31 21:06:57, Lei Zhang wrote: > I think you can also be written as ASSERT_TRUE(table); Done for this and all similar asserts.
The CQ bit was checked by afakhry@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/1320563002/#ps100001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320563002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320563002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by afakhry@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320563002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320563002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/4e16add803fbb0ed18413496d3fb4424fa4957c6 Cr-Commit-Position: refs/heads/master@{#346524}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1323853005/ by horo@chromium.org. The reason for reverting is: NewTaskManagerViewTest.ColumnsSettingsAreRestored crashes on Linux ChromiumOS Tests (dbg)(1). https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%....
The CQ bit was checked by afakhry@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/1320563002/#ps140001 (title: "Making sure tests end in good conditions")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320563002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320563002/140001
note: It's better to start a new CL rather than reusing an old CL. You are piggybacking off of the l-g-t-m from patch set 6, and the new changes haven't been reviewed.
The CQ bit was unchecked by afakhry@chromium.org
On 2015/09/01 19:03:40, afakhry wrote: > The CQ bit was unchecked by mailto:afakhry@chromium.org Ok, sorry, I thought that's how we reland a reverted CL specially if the change is minor. I unchecked the CQ bit. Please take a look and let me know.
https://codereview.chromium.org/1320563002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/new_task_manager_view_browsertest.cc (right): https://codereview.chromium.org/1320563002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:72: chrome::HideTaskManager(); I think all the teardown should go in NewTaskManagerViewTest::TearDown().
On 2015/09/01 19:04:54, afakhry wrote: > On 2015/09/01 19:03:40, afakhry wrote: > > The CQ bit was unchecked by mailto:afakhry@chromium.org > > Ok, sorry, I thought that's how we reland a reverted CL specially if the change > is minor. I unchecked the CQ bit. Please take a look and let me know. It's been a while, but the last discussion on the topic was https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/pUCEqaugA9M I'm firmly in the recycling coke cans, but not CLs camp.
https://codereview.chromium.org/1320563002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/new_task_manager_view_browsertest.cc (right): https://codereview.chromium.org/1320563002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:72: chrome::HideTaskManager(); On 2015/09/01 19:08:20, Lei Zhang wrote: > I think all the teardown should go in NewTaskManagerViewTest::TearDown(). Done! Move it to TearDownOnMainThread().
++lgtm
The CQ bit was checked by afakhry@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320563002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320563002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/4780b87d1c25f68c39abc623199f3e71a477d32d Cr-Commit-Position: refs/heads/master@{#346734} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
