Mac: Show download item menu on mouse down instead of mouse up.

Show download item menu on mouse down instead of mouse up.

BUG=20812
TEST=
* Click right part of download item. Menu should appear on mouse down.
* After menu is closed, mousing over the menu should not show the depressed state.
* Click left part of download item. File should be opened on mouse up.
* http://crbug.com/28215 should not have regressed.

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

[Nico, 2009-11-08 23:27:51]

This change is simpler than it looks, but since I only get a "fire action" event
in the cell, I needed to introduce a button subclass to intercept mouseDown. But
it really is just moving some code around.

[Nico, 2009-11-08 23:27:54]

This change is simpler than it looks, but since I only get a "fire action" event
in the cell, I needed to introduce a button subclass to intercept mouseDown. But
it really is just moving some code around.

[pink (ping after 24hrs), 2009-11-09 15:37:40]

Needs a unit test on new class.

http://codereview.chromium.org/380002/diff/2001/3002
File chrome/browser/cocoa/download_item_button.h (right):

http://codereview.chromium.org/380002/diff/2001/3002#newcode9
Line 9: @interface DownloadItemButton : NSButton
Rather than name this with where it will be used, name it as per what it does.
That will allow someone to re-use it elsewhere in the app w/out the confusion of
"why are we using a download button here"?

http://codereview.chromium.org/380002/diff/2001/3003
File chrome/browser/cocoa/download_item_button.mm (right):

http://codereview.chromium.org/380002/diff/2001/3003#newcode12
Line 12: - (void)mouseDown:(NSEvent*)event {
Comment on why you need to override this?

http://codereview.chromium.org/380002/diff/2001/3003#newcode22
Line 22: withEvent:[NSApp currentEvent]
line up colons

http://codereview.chromium.org/380002/diff/2001/3003#newcode27
Line 27: - (void)menuDidClose:(NSMenu*)menu {
is this a delegate method? who calls this?

http://codereview.chromium.org/380002/diff/2001/3004
File chrome/browser/cocoa/download_item_cell.h (right):

http://codereview.chromium.org/380002/diff/2001/3004#newcode27
Line 27: @interface DownloadItemCell : GradientButtonCell<NSAnimationDelegate> {
per comment in new button class, would be nice to abstract this so that it could
be re-used and isn't specific to a "download item".

[Nico, 2009-11-09 16:41:50]

I Addressed the easy comments. I will probably not get around to the other
comments in a while. I will ping this CL once it's ready for another look.

Two questions below.

http://codereview.chromium.org/380002/diff/2001/3002
File chrome/browser/cocoa/download_item_button.h (right):

http://codereview.chromium.org/380002/diff/2001/3002#newcode9
Line 9: @interface DownloadItemButton : NSButton
On 2009/11/09 15:37:41, pink wrote:
> Rather than name this with where it will be used, name it as per what it does.
> That will allow someone to re-use it elsewhere in the app w/out the confusion
of
> "why are we using a download button here"?

Will do.

http://codereview.chromium.org/380002/diff/2001/3003
File chrome/browser/cocoa/download_item_button.mm (right):

http://codereview.chromium.org/380002/diff/2001/3003#newcode12
Line 12: - (void)mouseDown:(NSEvent*)event {
On 2009/11/09 15:37:41, pink wrote:
> Comment on why you need to override this?

Done.

http://codereview.chromium.org/380002/diff/2001/3003#newcode22
Line 22: withEvent:[NSApp currentEvent]
On 2009/11/09 15:37:41, pink wrote:
> line up colons

Done.

http://codereview.chromium.org/380002/diff/2001/3003#newcode27
Line 27: - (void)menuDidClose:(NSMenu*)menu {
On 2009/11/09 15:37:41, pink wrote:
> is this a delegate method? who calls this?

Yes, it's a delegate method of the menu. The context menu calls this when it
closes. Do we have "Delegate method for <foo>" comments for delegate methods?

http://codereview.chromium.org/380002/diff/2001/3004
File chrome/browser/cocoa/download_item_cell.h (right):

http://codereview.chromium.org/380002/diff/2001/3004#newcode27
Line 27: @interface DownloadItemCell : GradientButtonCell<NSAnimationDelegate> {
On 2009/11/09 15:37:41, pink wrote:
> per comment in new button class, would be nice to abstract this so that it
could
> be re-used and isn't specific to a "download item".

By "this", you mean the context menu handling part?

[pink (ping after 24hrs), 2009-11-09 16:54:46]

On Mon, Nov 9, 2009 at 11:41 AM,  <thakis@chromium.org> wrote:
> I Addressed the easy comments. I will probably not get around to the other
> comments in a while. I will ping this CL once it's ready for another look.
>
> Two questions below.
>
>
> http://codereview.chromium.org/380002/diff/2001/3002
> File chrome/browser/cocoa/download_item_button.h (right):
>
> http://codereview.chromium.org/380002/diff/2001/3002#newcode9
> Line 9: @interface DownloadItemButton : NSButton
> On 2009/11/09 15:37:41, pink wrote:
>>
>> Rather than name this with where it will be used, name it as per what
>
> it does.
>>
>> That will allow someone to re-use it elsewhere in the app w/out the
>
> confusion of
>>
>> "why are we using a download button here"?
>
> Will do.
>
> http://codereview.chromium.org/380002/diff/2001/3003
> File chrome/browser/cocoa/download_item_button.mm (right):
>
> http://codereview.chromium.org/380002/diff/2001/3003#newcode12
> Line 12: - (void)mouseDown:(NSEvent*)event {
> On 2009/11/09 15:37:41, pink wrote:
>>
>> Comment on why you need to override this?
>
> Done.
>
> http://codereview.chromium.org/380002/diff/2001/3003#newcode22
> Line 22: withEvent:[NSApp currentEvent]
> On 2009/11/09 15:37:41, pink wrote:
>>
>> line up colons
>
> Done.
>
> http://codereview.chromium.org/380002/diff/2001/3003#newcode27
> Line 27: - (void)menuDidClose:(NSMenu*)menu {
> On 2009/11/09 15:37:41, pink wrote:
>>
>> is this a delegate method? who calls this?
>
> Yes, it's a delegate method of the menu. The context menu calls this
> when it closes. Do we have "Delegate method for <foo>" comments for
> delegate methods?

It's nice to have comments for every method, whether or not it's a
delegate. The only time you don't need a comment is when there's
already a comment in the header for the method, which in this case
there isn't.

> http://codereview.chromium.org/380002/diff/2001/3004
> File chrome/browser/cocoa/download_item_cell.h (right):
>
> http://codereview.chromium.org/380002/diff/2001/3004#newcode27
> Line 27: @interface DownloadItemCell :
> GradientButtonCell<NSAnimationDelegate> {
> On 2009/11/09 15:37:41, pink wrote:
>>
>> per comment in new button class, would be nice to abstract this so
>
> that it could
>>
>> be re-used and isn't specific to a "download item".
>
> By "this", you mean the context menu handling part?
>
> http://codereview.chromium.org/380002
>

By "this" i meant anything that wasn't specific to downloads. We now
have a button view class that seems like it has potential for re-use
except the naming suggests it's specific to downloads. I'm not sure
how much of it actually is, it would be nice to put in some
abstraction so the view could be re-used elsewhere. I would imagine
very little is specifically tied to a download item.


-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[Nico, 2010-01-31 03:11:45]

I dusted this off. I had to add a DownloadButton for other reasons, so I could
just edit that file.

Had I gone down the "general class" route, then I would now want to use
behaviors from two different classes. There's no good way to do that in OOP :-/

Please take another look. The patch is pretty small by now.

[pink (ping after 24hrs), 2010-02-01 14:37:57]

lgtm with one comment.

http://codereview.chromium.org/380002/diff/6014/7011
File chrome/browser/cocoa/download_item_controller.mm (right):

http://codereview.chromium.org/380002/diff/6014/7011#newcode101
chrome/browser/cocoa/download_item_controller.mm:101: - (void)dealloc {
just to be safe do you want to set the progressView_'s controller to nil, since
it's a weak reference and there's no guarantee when it will go away?

[Nico, 2010-02-01 15:07:21]

Thanks.

http://codereview.chromium.org/380002/diff/6014/7011
File chrome/browser/cocoa/download_item_controller.mm (right):

http://codereview.chromium.org/380002/diff/6014/7011#newcode101
chrome/browser/cocoa/download_item_controller.mm:101: - (void)dealloc {
On 2010/02/01 14:37:57, pink wrote:
> just to be safe do you want to set the progressView_'s controller to nil,
since
> it's a weak reference and there's no guarantee when it will go away?

Done.