Mac: Add sort support for task manager.

Mac: Add sort support for task manager.

Also fix a leak in test Init reported by valgrind. The problem was that the test called a rogue init, which didn't cause the window to be shown, and hence -close didn't send a -windowWillClose: notification. Also restore -windowWillClose:, it accidentally got deleted in http://codereview.chromium.org/536086

BUG=32148, 30398
TEST=Click a column in the task manager. Should sort.

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

[Robert Sesek, 2010-07-28 22:33:10]

http://codereview.chromium.org/2873075/diff/8001/9001
File chrome/browser/cocoa/task_manager_mac.h (right):

http://codereview.chromium.org/2873075/diff/8001/9001#newcode10
chrome/browser/cocoa/task_manager_mac.h:10: 
Remove

http://codereview.chromium.org/2873075/diff/8001/9001#newcode34
chrome/browser/cocoa/task_manager_mac.h:34: std::vector<int> indexShuffle_;
This isn't "just" used for sorting, though.  This maps Cocoa rows to their model
indices, yes?

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

http://codereview.chromium.org/2873075/diff/8001/9002#newcode66
chrome/browser/cocoa/task_manager_mac.mm:66: // TODO(thakis): Do grouping like
on GTK.
Create a bug for this?

http://codereview.chromium.org/2873075/diff/8001/9002#newcode174
chrome/browser/cocoa/task_manager_mac.mm:174: BOOL ascending = columnId ==
IDS_TASK_MANAGER_PAGE_COLUMN;
Comment: The page column should by default be sorted ascending.

http://codereview.chromium.org/2873075/diff/8001/9002#newcode177
chrome/browser/cocoa/task_manager_mac.mm:177: ascending:ascending]);  //
|selector| is ignored by SortHelper.
I don't understand this comment.  What |selector| are you referencing?

http://codereview.chromium.org/2873075/diff/8001/9002#newcode408
chrome/browser/cocoa/task_manager_mac.mm:408:
sortDescriptorsDidChange:(NSArray*)oldDescriptors {
align colons

[Nico, 2010-07-28 23:03:11]

thanks!

http://codereview.chromium.org/2873075/diff/8001/9001
File chrome/browser/cocoa/task_manager_mac.h (right):

http://codereview.chromium.org/2873075/diff/8001/9001#newcode10
chrome/browser/cocoa/task_manager_mac.h:10: 
On 2010/07/28 22:33:10, rsesek wrote:
> Remove

Done.

http://codereview.chromium.org/2873075/diff/8001/9001#newcode34
chrome/browser/cocoa/task_manager_mac.h:34: std::vector<int> indexShuffle_;
On 2010/07/28 22:33:10, rsesek wrote:
> This isn't "just" used for sorting, though.  This maps Cocoa rows to their
model
> indices, yes?

Well, it wouldn't be necessary if there was no sorting. Reworded comment
slightly

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

http://codereview.chromium.org/2873075/diff/8001/9002#newcode66
chrome/browser/cocoa/task_manager_mac.mm:66: // TODO(thakis): Do grouping like
on GTK.
On 2010/07/28 22:33:10, rsesek wrote:
> Create a bug for this?

I'll keep the sorting bug open until this is done. I intend to do this tomorrow.

http://codereview.chromium.org/2873075/diff/8001/9002#newcode174
chrome/browser/cocoa/task_manager_mac.mm:174: BOOL ascending = columnId ==
IDS_TASK_MANAGER_PAGE_COLUMN;
On 2010/07/28 22:33:10, rsesek wrote:
> Comment: The page column should by default be sorted ascending.

Done.

http://codereview.chromium.org/2873075/diff/8001/9002#newcode177
chrome/browser/cocoa/task_manager_mac.mm:177: ascending:ascending]);  //
|selector| is ignored by SortHelper.
On 2010/07/28 22:33:10, rsesek wrote:
> I don't understand this comment.  What |selector| are you referencing?

NSSortDescriptor has one. Removed the comment.

http://codereview.chromium.org/2873075/diff/8001/9002#newcode408
chrome/browser/cocoa/task_manager_mac.mm:408:
sortDescriptorsDidChange:(NSArray*)oldDescriptors {
On 2010/07/28 22:33:10, rsesek wrote:
> align colons

Done. (I tried!)

[Robert Sesek, 2010-07-29 14:23:23]

The code LGTM. How about a unit test?

[Nico, 2010-07-29 15:38:58]

added a test (didn't run it yet: on shuttle and out of battery)

[Robert Sesek, 2010-07-29 15:56:09]

LGTM!

http://codereview.chromium.org/2873075/diff/20001/21001
File chrome/browser/cocoa/task_manager_mac.h (right):

http://codereview.chromium.org/2873075/diff/20001/21001#newcode89
chrome/browser/cocoa/task_manager_mac.h:89: // The task manager.
Indent fix.

http://codereview.chromium.org/2873075/diff/20001/21003
File chrome/browser/cocoa/task_manager_mac_unittest.mm (right):

http://codereview.chromium.org/2873075/diff/20001/21003#newcode23
chrome/browser/cocoa/task_manager_mac_unittest.mm:23: return
base::GetCurrentProcessHandle();  // FIXME:
FIXME what?

[Nico, 2010-07-29 19:06:37]

The test now passes, even under valgrind. I needed to change some things around.
Will commit in 40 min or so unless I hear otherwise.

http://codereview.chromium.org/2873075/diff/20001/21001
File chrome/browser/cocoa/task_manager_mac.h (right):

http://codereview.chromium.org/2873075/diff/20001/21001#newcode89
chrome/browser/cocoa/task_manager_mac.h:89: // The task manager.
On 2010/07/29 15:56:09, rsesek wrote:
> Indent fix.

Done.

http://codereview.chromium.org/2873075/diff/20001/21003
File chrome/browser/cocoa/task_manager_mac_unittest.mm (right):

http://codereview.chromium.org/2873075/diff/20001/21003#newcode23
chrome/browser/cocoa/task_manager_mac_unittest.mm:23: return
base::GetCurrentProcessHandle();  // FIXME:
On 2010/07/29 15:56:09, rsesek wrote:
> FIXME what?

removed

[Robert Sesek, 2010-07-29 19:21:22]

LGTM with nits

http://codereview.chromium.org/2873075/diff/32001/33001
File chrome/browser/cocoa/task_manager_mac.h (right):

http://codereview.chromium.org/2873075/diff/32001/33001#newcode56
chrome/browser/cocoa/task_manager_mac.h:56: - (NSTableView*) tableView;
No space

http://codereview.chromium.org/2873075/diff/32001/33002
File chrome/browser/cocoa/task_manager_mac.mm (right):

http://codereview.chromium.org/2873075/diff/32001/33002#newcode146
chrome/browser/cocoa/task_manager_mac.mm:146: - (NSTableView*) tableView {
no space

http://codereview.chromium.org/2873075/diff/32001/33003
File chrome/browser/cocoa/task_manager_mac_unittest.mm (right):

http://codereview.chromium.org/2873075/diff/32001/33003#newcode31
chrome/browser/cocoa/task_manager_mac_unittest.mm:31: }  // namespace
Can you put in the entire test in the namespace?

[Nico, 2010-07-29 20:30:39]

http://codereview.chromium.org/2873075/diff/32001/33001
File chrome/browser/cocoa/task_manager_mac.h (right):

http://codereview.chromium.org/2873075/diff/32001/33001#newcode56
chrome/browser/cocoa/task_manager_mac.h:56: - (NSTableView*) tableView;
On 2010/07/29 19:21:23, rsesek wrote:
> No space

Done.

http://codereview.chromium.org/2873075/diff/32001/33003
File chrome/browser/cocoa/task_manager_mac_unittest.mm (right):

http://codereview.chromium.org/2873075/diff/32001/33003#newcode31
chrome/browser/cocoa/task_manager_mac_unittest.mm:31: }  // namespace
On 2010/07/29 19:21:23, rsesek wrote:
> Can you put in the entire test in the namespace?

No, else the FRIEND_TEST stuff won't work

[Robert Sesek, 2010-07-29 20:33:09]

http://codereview.chromium.org/2873075/diff/32001/33003
File chrome/browser/cocoa/task_manager_mac_unittest.mm (right):

http://codereview.chromium.org/2873075/diff/32001/33003#newcode31
chrome/browser/cocoa/task_manager_mac_unittest.mm:31: }  // namespace
On 2010/07/29 20:30:39, Nico wrote:
> On 2010/07/29 19:21:23, rsesek wrote:
> > Can you put in the entire test in the namespace?
> 
> No, else the FRIEND_TEST stuff won't work

Ah. I usually do this instead:
namespace {
class MyTest;
}
...
  friend class ::MyTest;

Not a big deal though.