Gtk: Allow all columns in task manager to be sortable.

Gtk: Allow all columns in task manager to be sortable.

BUG=21048
TEST=Open task manager and sort any column

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

[Mohamed Mansour, 2009-09-07 23:24:52]

Please review, if there is a shorter way instead of being repetitive, please let
me know. I have been told to not merge gcallbacks together into one function.

This CL will make the columns sortable.

[James Hawkins, 2009-09-08 20:27:43]

http://codereview.chromium.org/196040/diff/1/2
File chrome/browser/gtk/task_manager_gtk.cc (right):

http://codereview.chromium.org/196040/diff/1/2#newcode759
Line 759: return reinterpret_cast<TaskManagerGtk*>(window)->model_->
nit: try this on for size

TaskManagerModel* model =
    reinterpret_cast<TaskManagerGtk*>(task_manager)->model_;
return model->CompareValues(row1, row2, IDS_TASK_MANAGER_PAGE_COLUMN);

It's not a deal-breaker, but you might find it clearer.

http://codereview.chromium.org/196040/diff/1/3
File chrome/browser/gtk/task_manager_gtk.h (right):

http://codereview.chromium.org/196040/diff/1/3#newcode94
Line 94: // GTK sorting callbacks.
Seems like your missing the 'Goats Teleported' column.

http://codereview.chromium.org/196040/diff/1/3#newcode96
Line 96: GtkTreeIter* b, gpointer window);
s/window/task_manager/ for all of these functions.

[Mohamed Mansour, 2009-09-08 22:16:44]

Thanks for the review. Goats teleported seems to be manually added, I moved it
to generated resources so it would be translated.

Please re-review.

http://codereview.chromium.org/196040/diff/1/2
File chrome/browser/gtk/task_manager_gtk.cc (right):

http://codereview.chromium.org/196040/diff/1/2#newcode759
Line 759: return reinterpret_cast<TaskManagerGtk*>(window)->model_->
On 2009/09/08 20:27:43, James Hawkins wrote:
> nit: try this on for size
> 
> TaskManagerModel* model =
>     reinterpret_cast<TaskManagerGtk*>(task_manager)->model_;
> return model->CompareValues(row1, row2, IDS_TASK_MANAGER_PAGE_COLUMN);
> 
> It's not a deal-breaker, but you might find it clearer.

Done.

http://codereview.chromium.org/196040/diff/1/3
File chrome/browser/gtk/task_manager_gtk.h (right):

http://codereview.chromium.org/196040/diff/1/3#newcode94
Line 94: // GTK sorting callbacks.
On 2009/09/08 20:27:43, James Hawkins wrote:
> Seems like your missing the 'Goats Teleported' column.

Done.

http://codereview.chromium.org/196040/diff/1/3#newcode96
Line 96: GtkTreeIter* b, gpointer window);
On 2009/09/08 20:27:43, James Hawkins wrote:
> s/window/task_manager/ for all of these functions.

Done.

[James Hawkins, 2009-09-08 22:22:47]

LGTM

[Evan Stade, 2009-09-08 22:35:44]

http://codereview.chromium.org/196040/diff/2002/2004
File chrome/browser/gtk/task_manager_gtk.cc (right):

http://codereview.chromium.org/196040/diff/2002/2004#newcode830
Line 830: GtkTreeIter* a,
tabbing off here.

[Mohamed Mansour, 2009-09-08 23:36:19]

Is this cleaner approach? I tried using anonymous class approach but it was
turning out to be ugly, I hate to reinterpret cast everytime. This one just
calls CompareImpl.

[Evan Stade, 2009-09-08 23:52:11]

this seems fine to me, LG

[James Hawkins, 2009-09-08 23:53:31]

I still don't think you're gaining much by factoring out the comparisons into
CompareImpl.  The motivation for using one factored callback function was to not
have N callback functions, yet we still have that.  I think your first version
was cleanest at the cost of a few extra lines of code.

http://codereview.chromium.org/196040/diff/2006/3004
File chrome/browser/gtk/task_manager_gtk.cc (right):

http://codereview.chromium.org/196040/diff/2006/3004#newcode762
Line 762: TaskManagerModel* model =
You can remove these two lines and just use model_->CompareValues once you make
this method non-static.

http://codereview.chromium.org/196040/diff/2006/3005
File chrome/browser/gtk/task_manager_gtk.h (right):

http://codereview.chromium.org/196040/diff/2006/3005#newcode98
Line 98: return CompareImpl(model, a, b, task_manager,
Please don't add these implementations to the header file.  Keep them in the
source file.

http://codereview.chromium.org/196040/diff/2006/3005#newcode105
Line 105: return CompareImpl(model, a, b, task_manager,
This will be:

return reinterpret_cast<TaskManagerGtk*>(task_manager)->CompareImpl(...);

after you make CompareImpl non-static.

http://codereview.chromium.org/196040/diff/2006/3005#newcode151
Line 151: // GTK sort callback.
We're calling this method so there's no need to make this static.

[Evan Stade, 2009-09-08 23:55:20]

http://codereview.chromium.org/196040/diff/2006/3005
File chrome/browser/gtk/task_manager_gtk.h (right):

http://codereview.chromium.org/196040/diff/2006/3005#newcode98
Line 98: return CompareImpl(model, a, b, task_manager,
On 2009/09/08 23:53:31, James Hawkins wrote:
> Please don't add these implementations to the header file.  Keep them in the
> source file.

there's nothing wrong with putting thunk implementations in the header, we do it
all over the place.

[Mohamed Mansour, 2009-09-11 01:21:07]

Please review :)

[Evan Stade, 2009-09-11 01:45:21]

since you made goats teleported a resource can you fix the views implementation
as well

[Evan Stade, 2009-09-11 01:45:34]

http://codereview.chromium.org/196040/diff/7002/7005
File chrome/browser/gtk/task_manager_gtk.h (right):

http://codereview.chromium.org/196040/diff/7002/7005#newcode68
Line 68: // GTK sort callback.
nit: this isn't a callback.

[Mohamed Mansour, 2009-09-11 01:58:24]

Sure, I can do the Windows one, I would rather do it in a separate CL so I could
test it in the windows environment.

I renamed the comment, safe to commit since we ruled out the crash is happening
prior to this patch.

http://codereview.chromium.org/196040/diff/7002/7005
File chrome/browser/gtk/task_manager_gtk.h (right):

http://codereview.chromium.org/196040/diff/7002/7005#newcode68
Line 68: // GTK sort callback.
On 2009/09/11 01:45:34, Evan Stade wrote:
> nit: this isn't a callback.

Done.

[Evan Stade, 2009-09-11 01:59:21]

On 2009/09/11 01:58:24, Mohamed Mansour wrote:
> Sure, I can do the Windows one, I would rather do it in a separate CL so I
could
> test it in the windows environment.

sounds fine. LGTM.

[Mohamed Mansour, 2009-09-11 02:03:28]

Sorry, I added the windows view, it is only 3 lines change, simple change could
be added to this CL. Please just review that.

[Evan Stade, 2009-09-11 02:07:29]

http://codereview.chromium.org/196040/diff/9004/3012
File chrome/browser/views/task_manager_view.cc (right):

http://codereview.chromium.org/196040/diff/9004/3012#newcode38
Line 38: static const int64 kNuthMagicNumber = 1737350766;
you can delete this whole block it seems

[Mohamed Mansour, 2009-09-11 02:12:07]

Deleted dead code.

http://codereview.chromium.org/196040/diff/9004/3012
File chrome/browser/views/task_manager_view.cc (right):

http://codereview.chromium.org/196040/diff/9004/3012#newcode38
Line 38: static const int64 kNuthMagicNumber = 1737350766;
On 2009/09/11 02:07:29, Evan Stade wrote:
> you can delete this whole block it seems

Done.

[Evan Stade, 2009-09-11 02:31:10]

lg