Mac: Fix kill button and selection in task manager

Mac: Fix kill button and selection in task manager

BUG=51488, 51486
TEST=Sort on a changing column such as %cpu or goats teleported. The selection should follow the selected rows (i.e. if the "maps" process jumps from row 2 to row 4, the selected row should follow). "Kill process" should kill the currently selected process, not the process that's at that index when no sorting is happening.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55361

[Nico, 2010-08-07 19:26:17]

blah

[viettrungluu, 2010-08-07 19:54:04]

The complexity has gotten to the point where I'd be happier if you had a unit
test.

http://codereview.chromium.org/3047055/diff/1/2
File chrome/browser/cocoa/task_manager_mac.h (right):

http://codereview.chromium.org/3047055/diff/1/2#newcode33
chrome/browser/cocoa/task_manager_mac.h:33: std::vector<int> viewToModelIndex_;
I was momentarily confused by the term "index", since it's often used for a
single integer. Maybe "map" would be better?

http://codereview.chromium.org/3047055/diff/1/3
File chrome/browser/cocoa/task_manager_mac.mm (right):

http://codereview.chromium.org/3047055/diff/1/3#newcode136
chrome/browser/cocoa/task_manager_mac.mm:136: NSIndexSet* view_selection =
[tableView_ selectedRowIndexes];
viewSelection

http://codereview.chromium.org/3047055/diff/1/3#newcode137
chrome/browser/cocoa/task_manager_mac.mm:137: std::vector<int> model_selection;
modelSelection

http://codereview.chromium.org/3047055/diff/1/3#newcode151
chrome/browser/cocoa/task_manager_mac.mm:151: DCHECK_EQ([tableView_
numberOfRows], model_->ResourceCount());
I find if () DCHECK...; kind of weird, but whatever.

http://codereview.chromium.org/3047055/diff/1/3#newcode152
chrome/browser/cocoa/task_manager_mac.mm:152: [tableView_ deselectAll:self];
Wouldn't it be better to use an NSMutableIndexSet and do the selection all at
once?

[Miranda Callahan, 2010-08-07 20:01:52]

http://codereview.chromium.org/3047055/diff/1/2
File chrome/browser/cocoa/task_manager_mac.h (right):

http://codereview.chromium.org/3047055/diff/1/2#newcode31
chrome/browser/cocoa/task_manager_mac.h:31: // Contain a permutation of
[0..|model_->ResourceCount() - 1|]. Used to
Drive-by: "Contains" is actually correct, as the answer to the question "What is
the role of this variable?"  "It ... "

[viettrungluu, 2010-08-07 20:03:50]

http://codereview.chromium.org/3047055/diff/1/2
File chrome/browser/cocoa/task_manager_mac.h (right):

http://codereview.chromium.org/3047055/diff/1/2#newcode31
chrome/browser/cocoa/task_manager_mac.h:31: // Contain a permutation of
[0..|model_->ResourceCount() - 1|]. Used to
On 2010/08/07 20:01:52, Miranda Callahan wrote:
> Drive-by: "Contains" is actually correct, as the answer to the question "What
is
> the role of this variable?"  "It ... "

Unless, of course, the question is "What is the role of these variables?".
"They....".

[Nico, 2010-08-07 20:05:00]

working on a test…

http://codereview.chromium.org/3047055/diff/1/2
File chrome/browser/cocoa/task_manager_mac.h (right):

http://codereview.chromium.org/3047055/diff/1/2#newcode31
chrome/browser/cocoa/task_manager_mac.h:31: // Contain a permutation of
[0..|model_->ResourceCount() - 1|]. Used to
On 2010/08/07 20:01:52, Miranda Callahan wrote:
> Drive-by: "Contains" is actually correct, as the answer to the question "What
is
> the role of this variable?"  "It ... "

It now refers to two variables, hence the change ("What is the role of these
variables?" "They contain…". I think.)

http://codereview.chromium.org/3047055/diff/1/2#newcode33
chrome/browser/cocoa/task_manager_mac.h:33: std::vector<int> viewToModelIndex_;
On 2010/08/07 19:54:04, viettrungluu wrote:
> I was momentarily confused by the term "index", since it's often used for a
> single integer. Maybe "map" would be better?

Done.

http://codereview.chromium.org/3047055/diff/1/3
File chrome/browser/cocoa/task_manager_mac.mm (right):

http://codereview.chromium.org/3047055/diff/1/3#newcode136
chrome/browser/cocoa/task_manager_mac.mm:136: NSIndexSet* view_selection =
[tableView_ selectedRowIndexes];
On 2010/08/07 19:54:04, viettrungluu wrote:
> viewSelection

Done.

http://codereview.chromium.org/3047055/diff/1/3#newcode137
chrome/browser/cocoa/task_manager_mac.mm:137: std::vector<int> model_selection;
On 2010/08/07 19:54:04, viettrungluu wrote:
> modelSelection

Done.

http://codereview.chromium.org/3047055/diff/1/3#newcode151
chrome/browser/cocoa/task_manager_mac.mm:151: DCHECK_EQ([tableView_
numberOfRows], model_->ResourceCount());
On 2010/08/07 19:54:04, viettrungluu wrote:
> I find if () DCHECK...; kind of weird, but whatever.

Me too, but
* it's more readable than DCHECK(size == 0 || numRows == resourceCount)
* gives a better error message if it fails than ^

so i figured it's ok

http://codereview.chromium.org/3047055/diff/1/3#newcode152
chrome/browser/cocoa/task_manager_mac.mm:152: [tableView_ deselectAll:self];
On 2010/08/07 19:54:04, viettrungluu wrote:
> Wouldn't it be better to use an NSMutableIndexSet and do the selection all at
> once?

Probably. Done. Also below.

(I didn't know NSMutableIndexSet existed.)

[Miranda Callahan, 2010-08-07 20:09:22]

http://codereview.chromium.org/3047055/diff/1/2
File chrome/browser/cocoa/task_manager_mac.h (right):

http://codereview.chromium.org/3047055/diff/1/2#newcode31
chrome/browser/cocoa/task_manager_mac.h:31: // Contain a permutation of
[0..|model_->ResourceCount() - 1|]. Used to
On 2010/08/07 20:05:00, Nico wrote:
> On 2010/08/07 20:01:52, Miranda Callahan wrote:
> > Drive-by: "Contains" is actually correct, as the answer to the question
"What
> is
> > the role of this variable?"  "It ... "
> 
> It now refers to two variables, hence the change ("What is the role of these
> variables?" "They contain…". I think.)

I see -- I was thrown by trungl's comment between the two vars, and by the fact
that I still find it an unwieldy way of phrasing.  I would put something like
"These variables ..." in front, to make it clearer.

[Nico, 2010-08-07 20:20:17]

Writing a test without rewriting the task manager stuff seems to be difficult.
Here's what I tried:

TEST_F(TaskManagerWindowControllerTest, Kill) {
  TaskManager task_manager;

  TestResource resource1(UTF8ToUTF16("zzz"), 1);
  TestResource resource2(UTF8ToUTF16("aaa"), 2);

  task_manager.AddResource(&resource1);
  task_manager.AddResource(&resource2);

  TaskManagerMac* bridge(new TaskManagerMac(&task_manager));
  TaskManagerWindowController* controller = bridge->cocoa_controller();
  NSTableView* table = [controller tableView];
  ASSERT_EQ(2, [controller numberOfRowsInTableView:table]);

  // Select row 0 in the table (corresponds to row 1 in the model) and kill it.
  [table selectRowIndexes:
      [NSIndexSet indexSetWithIndex:0 byExtendingSelection:NO]];
  [controller killSelectedProcesses:nil];

  ASSERT_EQ(0, [[table selectedRowIndexes] count]);
  ASSERT_EQ(1, [controller numberOfRowsInTableView:table]);

  NSTableColumn* title_column = [table tableColumnWithIdentifier:
      [NSNumber numberWithInt:IDS_TASK_MANAGER_PAGE_COLUMN]];
  NSCell* cell;
  cell = [controller tableView:table dataCellForTableColumn:title_column row:0];
  EXPECT_TRUE([@"zzz" isEqualToString:[cell title]]);

  // Releases the controller, which in turn deletes |bridge|.
  [controller close];

  task_manager.RemoveResource(&resource1);
//  task_manager.RemoveResource(&resource2);
}


However, this is how TaskManager::KillProcess looks:

void TaskManager::KillProcess(int index) {
  base::ProcessHandle process = model_->GetResourceProcessHandle(index);
  DCHECK(process);
  if (process != base::GetCurrentProcessHandle())
    base::KillProcess(process, base::PROCESS_END_KILLED_BY_USER, false);
}

(i.e. it relies on the whole multiprocess architecture to get a notification
that the process really died, which then removes the resource).

I could make that function virtual and create a TestTaskManager subclass that
overrides KillProcess(), but TaskManager is a singleton with a private
constructor (and since nothing is mocked out at the moment, using more than one
TaskManager object in a test leads to breakage). If I make TestTaskManager a
friend of TaskManager, then I'd need to make TestTaskManager's constructor
private with a note to be careful to friend TestTaskManager, and it's a lot of
brittle changes to test a change to a single line. So I'd rather not do it in
this CL.

Looking at a test for the selection code next…

[Nico, 2010-08-07 20:28:59]

Added a test for the selection behavior (since I'm on 10.5, I need to uncomment
pieces of chrome_tests.gyp, run runhooks, and wait for xcode to check
dependencies, so I haven't checked if the test actually passes yet. Should I get
an LG, I will do so before checking in :-P But it compiles!)

http://codereview.chromium.org/3047055/diff/1/2
File chrome/browser/cocoa/task_manager_mac.h (right):

http://codereview.chromium.org/3047055/diff/1/2#newcode31
chrome/browser/cocoa/task_manager_mac.h:31: // Contain a permutation of
[0..|model_->ResourceCount() - 1|]. Used to
On 2010/08/07 20:09:22, Miranda Callahan wrote:
> On 2010/08/07 20:05:00, Nico wrote:
> > On 2010/08/07 20:01:52, Miranda Callahan wrote:
> > > Drive-by: "Contains" is actually correct, as the answer to the question
> "What
> > is
> > > the role of this variable?"  "It ... "
> > 
> > It now refers to two variables, hence the change ("What is the role of these
> > variables?" "They contain…". I think.)
> 
> I see -- I was thrown by trungl's comment between the two vars, and by the
fact
> that I still find it an unwieldy way of phrasing.  I would put something like
> "These variables ..." in front, to make it clearer.
> 
> 

Done.

[viettrungluu, 2010-08-07 20:34:46]

LGETM.

http://codereview.chromium.org/3047055/diff/8001/9002
File chrome/browser/cocoa/task_manager_mac.mm (right):

http://codereview.chromium.org/3047055/diff/8001/9002#newcode137
chrome/browser/cocoa/task_manager_mac.mm:137: std::vector<int> modelSelection;
You might consider factoring out the algorithmic bits into a pure functional
helper (which would be easy to test).

http://codereview.chromium.org/3047055/diff/8001/9002#newcode506
chrome/browser/cocoa/task_manager_mac.mm:506: [window_controller_ deselectRows];
You might consider making -reloadData just take a BOOL parameter for whether to
deselect rows or not (or, more precisely, start with the current selection or
none).