Add pause and resume to Linux download shelf.

Add pause and resume to Linux download shelf.

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

[Mohamed Mansour, 2009-09-15 16:38:53]

LG but could the conditionals be more compact, less if's.  Evan should comment
as well!

http://codereview.chromium.org/203069/diff/1001/2001
File chrome/browser/gtk/download_item_gtk.cc (right):

http://codereview.chromium.org/203069/diff/1001/2001#newcode111
Line 111: } else {
There seems to be a lot of conditions, could this be cleaned up a little? A lot
of nested if statements.

[Craig Schlenter, 2009-09-15 17:19:30]

On 2009/09/15 16:38:53, Mohamed Mansour wrote:
> LG but could the conditionals be more compact, less if's.  Evan should comment
as well!

Thanks! I concur that the if statement stuff sucks badly ... I'll put my
thinking cap back on to come up with something simpler.

[Craig, 2009-09-15 17:50:05]

On 2009/09/15 17:19:30, Craig Schlenter wrote:
> On 2009/09/15 16:38:53, Mohamed Mansour wrote:
> > LG but could the conditionals be more compact, less if's.  Evan should
comment
> as well!
> 
> Thanks! I concur that the if statement stuff sucks badly ... I'll put my
> thinking cap back on to come up with something simpler.

I've reworked it a bit now ... it's longer but possibly less confusing?

[Evan Stade, 2009-09-15 17:58:07]

http://codereview.chromium.org/203069/diff/6001/6002
File chrome/browser/gtk/download_item_gtk.cc (right):

http://codereview.chromium.org/203069/diff/6001/6002#newcode213
Line 213: MenuCreateMaterial DownloadShelfContextMenuGtk::paused_download_menu[]
= {
why do we have two menus that are almost identical?

delete this second menu and make TOGGLE_PAUSE a MENU_CHECKBOX. And that should
make it possible to revert most of the rest of your changes if I'm not mistaken.

[Mohamed Mansour, 2009-09-15 18:18:18]

Evan, a menu checkbox for pause/resume is not ideal. Having a toggle button is
better since we need to differentiate between pause and resume. This is similar
to the Windows implementation.

I don't know why we can't dynamically build menu items in gtk that is dependent
if a feature is enabled/disabled.

[Evan Stade, 2009-09-15 18:37:45]

On 2009/09/15 18:18:18, Mohamed Mansour wrote:
> Evan, a menu checkbox for pause/resume is not ideal. Having a toggle button is
> better since we need to differentiate between pause and resume. This is
similar
> to the Windows implementation.

I don't understand. This is the exact purpose of a checkbox. Platform
expectations involve that a menu item which represents a binary decision should
be represented by a checkbox.

> I don't know why we can't dynamically build menu items in gtk that is
dependent
> if a feature is enabled/disabled.

[Craig, 2009-09-15 18:38:40]

On 2009/09/15 18:18:18, Mohamed Mansour wrote:
> Evan, a menu checkbox for pause/resume is not ideal. Having a toggle button is
> better since we need to differentiate between pause and resume. This is
similar
> to the Windows implementation.

I've uploaded a menu_checkbox version btw. ... it is just so much cleaner and
simpler that I want to hang my head in shame for my initial attempts.

In my defence, it is important to note that I'm 99.9% clueless about this stuff
... I spotted that there were already two different menu arrays in the original
code and assumed that if it was easy to dynamically change the menus then there
should only have been one structure ... so I added another one.

Anyway, I'm open to suggestions here .. I'm learning stuff which was the aim of
the exercise :)

[Evan Stade, 2009-09-15 18:43:14]

thank you, I believe this to be correct.

http://codereview.chromium.org/203069/diff/8001/8002
File chrome/browser/download/download_shelf.cc (right):

http://codereview.chromium.org/203069/diff/8001/8002#newcode33
Line 33: case ALWAYS_OPEN_TYPE: {
I know this isn't your fault, but could you make the braces consistent here?
(put braces on all cases)

[Craig, 2009-09-15 18:54:59]

Thank you. Braces are in ... will send to try-server.

http://codereview.chromium.org/203069/diff/8001/8002
File chrome/browser/download/download_shelf.cc (right):

http://codereview.chromium.org/203069/diff/8001/8002#newcode33
Line 33: case ALWAYS_OPEN_TYPE: {
On 2009/09/15 18:43:14, Evan Stade wrote:
> I know this isn't your fault, but could you make the braces consistent here?
> (put braces on all cases)

Done.