Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1992623002:
Task Manager: Preserve selection when rows are added (Closed)
|
Created:
4 years, 7 months ago by ncarter (slow) Modified:
4 years, 7 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTask Manager: Preserve selection when rows are added
Add a test that fails without the fix.
BUG=527455
Committed: https://crrev.com/01227686310ddde2290666504f28abcecbfcb110
Cr-Commit-Position: refs/heads/master@{#394930}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Fix compile #Patch Set 4 : Self-review fixes. #
Total comments: 26
Patch Set 5 : Fixes from review. #
Total comments: 4
Patch Set 6 : fixes from sky #
Dependent Patchsets: Messages
Total messages: 17 (6 generated)
Description was changed from ========== Task Manager: improve selection tracking when rows are added. Add a test that fails without the fix. BUG= ========== to ========== Task Manager: Preserve selection when rows are added Add a test that fails without the fix. BUG=527455 ==========
nick@chromium.org changed reviewers: + afakhry@chromium.org
Ahmed, please review
All looks good. A couple of minor comments. Thanks for adding the browser test! https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/task_... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/task_... chrome/browser/ui/task_manager/task_manager_table_model.cc:587: if (table_model_observer_) { Nit: Please remove the braces. https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/new_task_manager_view_browsertest.cc (right): https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:35: using browsertest_util::WaitForTaskManagerRows; Porting those tests paid off really well after all! Now we can use those utils here! :) Thanks! Was definitely worth it! https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:44: ASSERT_TRUE(embedded_test_server()->Start()); Shouldn't you be calling here the parent's SetUpOnMainThread() i.e. InProcessBrowserTest::SetUpOnMainThread()? https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:83: if (SessionTabHelper::IdForTab(*it) == tab_id) { Nit: ditto for the curly braces. https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:90: int GetRowForTab(content::WebContents* tab) { 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. https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:92: auto tester = TaskManagerTester::Create(base::Closure()); 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? https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:93: for (int i = 0; i < tester->GetRowCount(); i++) { Nit: (Not that it matters much here, but ...) ++i https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:213: for (int i = 0; i < tester->GetRowCount(); i++) { Nit: ++i https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:217: tabs.push_back(FindTabById(tester->GetTabId(i))); Since FindTabById() can return nullptr, maybe consider avoiding pushing_back() a nullptr if the tab_id is not found? https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:226: for (int i = 0; i < 3; i++) { Nit: ++i https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:234: for (int i = 0; i < 2; i++) { Nit: ++i
PTAL https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/task_... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/task_... chrome/browser/ui/task_manager/task_manager_table_model.cc:587: if (table_model_observer_) { On 2016/05/18 23:12:18, afakhry wrote: > Nit: Please remove the braces. Done. https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/new_task_manager_view_browsertest.cc (right): https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:35: using browsertest_util::WaitForTaskManagerRows; On 2016/05/18 23:12:19, afakhry wrote: > Porting those tests paid off really well after all! Now we can use those utils > here! :) > > Thanks! Was definitely worth it! Acknowledged. https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:44: ASSERT_TRUE(embedded_test_server()->Start()); On 2016/05/18 23:12:19, afakhry wrote: > Shouldn't you be calling here the parent's SetUpOnMainThread() i.e. > InProcessBrowserTest::SetUpOnMainThread()? InProcessBrowserTest::SetUpOnMainThread has an empty body, so the superclass call is not really needed when we're deriving directly from InProcessBrowserTest. https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:83: if (SessionTabHelper::IdForTab(*it) == tab_id) { On 2016/05/18 23:12:19, afakhry wrote: > Nit: ditto for the curly braces. Done. https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:90: int GetRowForTab(content::WebContents* tab) { On 2016/05/18 23:12:19, afakhry wrote: > 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. 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(). https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:92: auto tester = TaskManagerTester::Create(base::Closure()); On 2016/05/18 23:12:19, afakhry wrote: > 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? 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. https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:93: for (int i = 0; i < tester->GetRowCount(); i++) { On 2016/05/18 23:12:19, afakhry wrote: > Nit: (Not that it matters much here, but ...) ++i Done. https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:213: for (int i = 0; i < tester->GetRowCount(); i++) { On 2016/05/18 23:12:19, afakhry wrote: > Nit: ++i Done. https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:217: tabs.push_back(FindTabById(tester->GetTabId(i))); On 2016/05/18 23:12:19, afakhry wrote: > Since FindTabById() can return nullptr, maybe consider avoiding pushing_back() a > nullptr if the tab_id is not found? Done; I just added an EXPECT. https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:226: for (int i = 0; i < 3; i++) { On 2016/05/18 23:12:19, afakhry wrote: > Nit: ++i Done. https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:234: for (int i = 0; i < 2; i++) { On 2016/05/18 23:12:19, afakhry wrote: > Nit: ++i Done.
lgtm https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/new_task_manager_view_browsertest.cc (right): https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:44: ASSERT_TRUE(embedded_test_server()->Start()); On 2016/05/19 00:12:59, ncarter wrote: > On 2016/05/18 23:12:19, afakhry wrote: > > Shouldn't you be calling here the parent's SetUpOnMainThread() i.e. > > InProcessBrowserTest::SetUpOnMainThread()? > > InProcessBrowserTest::SetUpOnMainThread has an empty body, so the superclass > call is not really needed when we're deriving directly from > InProcessBrowserTest. 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. https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:90: int GetRowForTab(content::WebContents* tab) { On 2016/05/19 00:12:59, ncarter wrote: > On 2016/05/18 23:12:19, afakhry wrote: > > 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. > > 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(). Acknowledged. https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:92: auto tester = TaskManagerTester::Create(base::Closure()); On 2016/05/19 00:12:59, ncarter wrote: > On 2016/05/18 23:12:19, afakhry wrote: > > 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? > > 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. Acknowledged.
https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/new_task_manager_view_browsertest.cc (right): https://codereview.chromium.org/1992623002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:44: ASSERT_TRUE(embedded_test_server()->Start()); On 2016/05/19 00:57:33, afakhry wrote: > On 2016/05/19 00:12:59, ncarter wrote: > > On 2016/05/18 23:12:19, afakhry wrote: > > > Shouldn't you be calling here the parent's SetUpOnMainThread() i.e. > > > InProcessBrowserTest::SetUpOnMainThread()? > > > > InProcessBrowserTest::SetUpOnMainThread has an empty body, so the superclass > > call is not really needed when we're deriving directly from > > InProcessBrowserTest. > > 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. 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.
nick@chromium.org changed reviewers: + sky@chromium.org
+sky as chrome/ OWNER
LGTM https://codereview.chromium.org/1992623002/diff/80001/chrome/browser/ui/task_... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/1992623002/diff/80001/chrome/browser/ui/task_... chrome/browser/ui/task_manager/task_manager_table_model.cc:586: int index = std::find(tasks_.begin(), tasks_.end(), id) - tasks_.begin(); Move this inside if (tabl_model_observer_) as it's only needed there. I'm surprised you don't need a static_cast here. https://codereview.chromium.org/1992623002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/new_task_manager_view_browsertest.cc (right): https://codereview.chromium.org/1992623002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:94: auto tester = TaskManagerTester::Create(base::Closure()); This may be obvious to you, but it's totally not obvious what the type of tester is here. I recommend not using auto here.
https://codereview.chromium.org/1992623002/diff/80001/chrome/browser/ui/task_... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/1992623002/diff/80001/chrome/browser/ui/task_... chrome/browser/ui/task_manager/task_manager_table_model.cc:586: int index = std::find(tasks_.begin(), tasks_.end(), id) - tasks_.begin(); On 2016/05/19 19:08:47, sky wrote: > Move this inside if (tabl_model_observer_) as it's only needed there. > > I'm surprised you don't need a static_cast here. I was surprised too. It turns out we have no warning when assigning from int64 to int32 (crbug.com/167187). difference_type is signed, so there's no signed/unsigned warning here, which is what usually we hit with size_t's. I've added a cast anyway. https://codereview.chromium.org/1992623002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/new_task_manager_view_browsertest.cc (right): https://codereview.chromium.org/1992623002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/new_task_manager_view_browsertest.cc:94: auto tester = TaskManagerTester::Create(base::Closure()); On 2016/05/19 19:08:47, sky wrote: > This may be obvious to you, but it's totally not obvious what the type of tester > is here. I recommend not using auto here. Done.
The CQ bit was checked by nick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1992623002/#ps100001 (title: "fixes from sky")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1992623002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1992623002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Task Manager: Preserve selection when rows are added Add a test that fails without the fix. BUG=527455 ========== to ========== Task Manager: Preserve selection when rows are added Add a test that fails without the fix. BUG=527455 Committed: https://crrev.com/01227686310ddde2290666504f28abcecbfcb110 Cr-Commit-Position: refs/heads/master@{#394930} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/01227686310ddde2290666504f28abcecbfcb110 Cr-Commit-Position: refs/heads/master@{#394930} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
