Make download items drag sources on OS X.

Make download items drag sources on OS X.

Extract button dragging out of BookmarkButton into DraggableButton. Make BookmarkButton a subclass of DraggableButton. Create new class DownloadItemButton and make it a subclass of DraggableButton.

xib change: Make download item a DownloadItemButton instead of an NSButton.

BUG=15776
TEST=Download something, wait for it to complete, then drag it from the download shelf to somewhere. It should now work. Bookmarks should still be draggable in the bookmarks bar.

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

[Nico, 2010-01-30 03:41:11]

This looks larger than it is. It's really just splitting BookmarkButton into two
classes. The new code for supporting dnd for download items is tiny.

[Nico, 2010-01-30 03:48:28]

On 2010/01/30 03:41:11, Nico wrote:
> This looks larger than it is. It's really just splitting BookmarkButton into
two
> classes. The new code for supporting dnd for download items is tiny.

Also, I kept draggable_button as close as possible to how its code looked in
bookmark_button. I intend to clean it up a bit in a followup CL (for example,
beingDragged_ is never really written – I left it that way for now, but I _will_
clean it up as soon as this is in.)

[viettrungluu, 2010-01-30 04:40:53]

LGTM with a few changes. Could you also file a bug for the cleanup, in case you
get distracted?

http://codereview.chromium.org/180036/diff/9016/10002
File chrome/browser/cocoa/bookmark_button.h (right):

http://codereview.chromium.org/180036/diff/9016/10002#newcode1
chrome/browser/cocoa/bookmark_button.h:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
The year thing is so stupid that this comment doesn't exist. Nothing to see
here. Move along.

http://codereview.chromium.org/180036/diff/9016/10003
File chrome/browser/cocoa/bookmark_button.mm (right):

http://codereview.chromium.org/180036/diff/9016/10003#newcode24
chrome/browser/cocoa/bookmark_button.mm:24: - (id)initWithFrame:(NSRect)frame {
This whole init is now redundant, isn't it?

http://codereview.chromium.org/180036/diff/9016/10003#newcode46
chrome/browser/cocoa/bookmark_button.mm:46: scoped_nsobject<BookmarkButton>
keepSelfAlive([self retain]);
I worry a bit about this change, since -release is different from -autorelease.

http://codereview.chromium.org/180036/diff/9016/10004
File chrome/browser/cocoa/bookmark_button_unittest.mm (right):

http://codereview.chromium.org/180036/diff/9016/10004#newcode12
chrome/browser/cocoa/bookmark_button_unittest.mm:12: public:
Aagh.

http://codereview.chromium.org/180036/diff/9016/10004#newcode15
chrome/browser/cocoa/bookmark_button_unittest.mm:15: // Make sure nothing leaks
Sigh. This file now does so little.

http://codereview.chromium.org/180036/diff/9016/10005
File chrome/browser/cocoa/download_item_button.h (right):

http://codereview.chromium.org/180036/diff/9016/10005#newcode12
chrome/browser/cocoa/download_item_button.h:12: FilePath downloadPath_;
@private?

http://codereview.chromium.org/180036/diff/9016/10005#newcode16
chrome/browser/cocoa/download_item_button.h:16: -
(void)setDownload:(FilePath)path;
Is this a mutator for downloadPath_? Then it should have an accessor too
(general principle). And its name should be |-setDownloadPath:|. Can you use|
@property|?

http://codereview.chromium.org/180036/diff/9016/10005#newcode19
chrome/browser/cocoa/download_item_button.h:19: -
(void)beginDrag:(NSEvent*)event;
It's not necessary to mention overrides in subclasses, and in general I believe
we don't.

http://codereview.chromium.org/180036/diff/9016/10006
File chrome/browser/cocoa/download_item_button.mm (right):

http://codereview.chromium.org/180036/diff/9016/10006#newcode11
chrome/browser/cocoa/download_item_button.mm:11: -
(void)setDownload:(FilePath)path {
See what I said in the header.

http://codereview.chromium.org/180036/diff/9016/10006#newcode19
chrome/browser/cocoa/download_item_button.mm:19: [self dragFile:filename
fromRect:[self bounds] slideBack:YES event:event];
I worry a bit about Cocoa's implementation of this. (In particular, can the drag
destination ask for file data? [Quite possibly?] What if the file is stored on a
slow device or somesuch? Could you check the performance of this?)

I don't have a good solution for this sort of thing, so I can live with it, I
guess.

http://codereview.chromium.org/180036/diff/9016/10007
File chrome/browser/cocoa/download_item_button_unittest.mm (right):

http://codereview.chromium.org/180036/diff/9016/10007#newcode12
chrome/browser/cocoa/download_item_button_unittest.mm:12: public:
Aaagh!

http://codereview.chromium.org/180036/diff/9016/10007#newcode15
chrome/browser/cocoa/download_item_button_unittest.mm:15: // Make sure nothing
leaks.
Could you make sure |-setWhatever:| works too (and doesn't leak)?

http://codereview.chromium.org/180036/diff/9016/10009
File chrome/browser/cocoa/download_item_controller.mm (right):

http://codereview.chromium.org/180036/diff/9016/10009#newcode165
chrome/browser/cocoa/download_item_controller.mm:165: // Set the correct popup
menu.
You seem to do a bit more than this now.

http://codereview.chromium.org/180036/diff/9016/10010
File chrome/browser/cocoa/draggable_button.h (right):

http://codereview.chromium.org/180036/diff/9016/10010#newcode8
chrome/browser/cocoa/draggable_button.h:8: // more than a given amount, this
class will call |beginDrag:| instead of
s/amount/distance

I still maintain that the '-' is an integral part of a method's name. At least
that's what Apple's docs told me, I think.

http://codereview.chromium.org/180036/diff/9016/10010#newcode23
chrome/browser/cocoa/draggable_button.h:23: // Called when a drag starts.
Subclasses should override this.
should? must?

http://codereview.chromium.org/180036/diff/9016/10011
File chrome/browser/cocoa/draggable_button.mm (right):

http://codereview.chromium.org/180036/diff/9016/10011#newcode74
chrome/browser/cocoa/draggable_button.mm:74: if ((abs(currentLocation.x -
initialMouseDownLocation_.x) >
Random rant, because I feel like it: hysteresis should be done based on distance
moved (or its square), not on a stupid box.

http://codereview.chromium.org/180036/diff/9016/10011#newcode96
chrome/browser/cocoa/draggable_button.mm:96: // Should be overridden by
subclasses.
Maybe you should have a NOTREACHED()?

http://codereview.chromium.org/180036/diff/9016/10012
File chrome/browser/cocoa/draggable_button_unittest.mm (right):

http://codereview.chromium.org/180036/diff/9016/10012#newcode1
chrome/browser/cocoa/draggable_button_unittest.mm:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Since you're adding it, 2010 is more called for. But I really don't care.

http://codereview.chromium.org/180036/diff/9016/10012#newcode12
chrome/browser/cocoa/draggable_button_unittest.mm:12: public:
Delete.

[viettrungluu, 2010-01-30 04:41:22]

Also, I lied about when I'd get the review done. But now I'm hungry.

[Nico, 2010-01-30 05:52:22]

Thanks. I hope you had a tasty meal. I helped myself to some junk food on the
way home.

http://codereview.chromium.org/180036/diff/9016/10003
File chrome/browser/cocoa/bookmark_button.mm (right):

http://codereview.chromium.org/180036/diff/9016/10003#newcode24
chrome/browser/cocoa/bookmark_button.mm:24: - (id)initWithFrame:(NSRect)frame {
On 2010/01/30 04:40:53, viettrungluu wrote:
> This whole init is now redundant, isn't it?

Done.

http://codereview.chromium.org/180036/diff/9016/10003#newcode46
chrome/browser/cocoa/bookmark_button.mm:46: scoped_nsobject<BookmarkButton>
keepSelfAlive([self retain]);
On 2010/01/30 04:40:53, viettrungluu wrote:
> I worry a bit about this change, since -release is different from
-autorelease.

Hm, good point. Reverted.

http://codereview.chromium.org/180036/diff/9016/10004
File chrome/browser/cocoa/bookmark_button_unittest.mm (right):

http://codereview.chromium.org/180036/diff/9016/10004#newcode12
chrome/browser/cocoa/bookmark_button_unittest.mm:12: public:
On 2010/01/30 04:40:53, viettrungluu wrote:
> Aagh.

I'll keep this, to save jrg some merge pains.

http://codereview.chromium.org/180036/diff/9016/10004#newcode15
chrome/browser/cocoa/bookmark_button_unittest.mm:15: // Make sure nothing leaks
On 2010/01/30 04:40:53, viettrungluu wrote:
> Sigh. This file now does so little.

I'm sure it won't stay that way.

http://codereview.chromium.org/180036/diff/9016/10005
File chrome/browser/cocoa/download_item_button.h (right):

http://codereview.chromium.org/180036/diff/9016/10005#newcode12
chrome/browser/cocoa/download_item_button.h:12: FilePath downloadPath_;
On 2010/01/30 04:40:53, viettrungluu wrote:
> @private?

Done.

http://codereview.chromium.org/180036/diff/9016/10005#newcode16
chrome/browser/cocoa/download_item_button.h:16: -
(void)setDownload:(FilePath)path;
On 2010/01/30 04:40:53, viettrungluu wrote:
> Is this a mutator for downloadPath_? Then it should have an accessor too
> (general principle). And its name should be |-setDownloadPath:|. Can you use|
> @property|?

Done.

http://codereview.chromium.org/180036/diff/9016/10006
File chrome/browser/cocoa/download_item_button.mm (right):

http://codereview.chromium.org/180036/diff/9016/10006#newcode11
chrome/browser/cocoa/download_item_button.mm:11: -
(void)setDownload:(FilePath)path {
On 2010/01/30 04:40:53, viettrungluu wrote:
> See what I said in the header.

Done.

http://codereview.chromium.org/180036/diff/9016/10006#newcode19
chrome/browser/cocoa/download_item_button.mm:19: [self dragFile:filename
fromRect:[self bounds] slideBack:YES event:event];
On 2010/01/30 04:40:53, viettrungluu wrote:
> I worry a bit about Cocoa's implementation of this. (In particular, can the
drag
> destination ask for file data? [Quite possibly?] What if the file is stored on
a
> slow device or somesuch? Could you check the performance of this?)

If the file is stored on a slow device, then saving it during downloading was
slow too. I don't have a slow device around at the moment, but I would think
that Cocoa handles this decently.

> 
> I don't have a good solution for this sort of thing, so I can live with it, I
> guess.

http://codereview.chromium.org/180036/diff/9016/10007
File chrome/browser/cocoa/download_item_button_unittest.mm (right):

http://codereview.chromium.org/180036/diff/9016/10007#newcode12
chrome/browser/cocoa/download_item_button_unittest.mm:12: public:
On 2010/01/30 04:40:53, viettrungluu wrote:
> Aaagh!

This one I removed.

http://codereview.chromium.org/180036/diff/9016/10007#newcode15
chrome/browser/cocoa/download_item_button_unittest.mm:15: // Make sure nothing
leaks.
On 2010/01/30 04:40:53, viettrungluu wrote:
> Could you make sure |-setWhatever:| works too (and doesn't leak)?

Done.

http://codereview.chromium.org/180036/diff/9016/10009
File chrome/browser/cocoa/download_item_controller.mm (right):

http://codereview.chromium.org/180036/diff/9016/10009#newcode165
chrome/browser/cocoa/download_item_controller.mm:165: // Set the correct popup
menu.
On 2010/01/30 04:40:53, viettrungluu wrote:
> You seem to do a bit more than this now.

Done.

http://codereview.chromium.org/180036/diff/9016/10010
File chrome/browser/cocoa/draggable_button.h (right):

http://codereview.chromium.org/180036/diff/9016/10010#newcode8
chrome/browser/cocoa/draggable_button.h:8: // more than a given amount, this
class will call |beginDrag:| instead of
On 2010/01/30 04:40:53, viettrungluu wrote:
> s/amount/distance

Done.

> 
> I still maintain that the '-' is an integral part of a method's name. At least
> that's what Apple's docs told me, I think.

While technically true, I maintain that readability is more important in
comments than absolute correctness. And I think it's more readable without the
-.

Changed it anyway, just for you.

http://codereview.chromium.org/180036/diff/9016/10010#newcode23
chrome/browser/cocoa/draggable_button.h:23: // Called when a drag starts.
Subclasses should override this.
On 2010/01/30 04:40:53, viettrungluu wrote:
> should? must?

Must, I guess. Done.

http://codereview.chromium.org/180036/diff/9016/10011
File chrome/browser/cocoa/draggable_button.mm (right):

http://codereview.chromium.org/180036/diff/9016/10011#newcode96
chrome/browser/cocoa/draggable_button.mm:96: // Should be overridden by
subclasses.
On 2010/01/30 04:40:53, viettrungluu wrote:
> Maybe you should have a NOTREACHED()?

Done.

http://codereview.chromium.org/180036/diff/9016/10012
File chrome/browser/cocoa/draggable_button_unittest.mm (right):

http://codereview.chromium.org/180036/diff/9016/10012#newcode1
chrome/browser/cocoa/draggable_button_unittest.mm:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
On 2010/01/30 04:40:53, viettrungluu wrote:
> Since you're adding it, 2010 is more called for. But I really don't care.

Done.

http://codereview.chromium.org/180036/diff/9016/10012#newcode12
chrome/browser/cocoa/draggable_button_unittest.mm:12: public:
On 2010/01/30 04:40:53, viettrungluu wrote:
> Delete.

Done.

[viettrungluu, 2010-01-30 06:29:36]

Ship it! I mean, LGTM.

[Nico, 2010-01-30 06:34:41]

jrg: fyi

[John Grabowski, 2010-01-30 06:37:25]

thakis: argh


On Fri, Jan 29, 2010 at 10:34 PM, <thakis@chromium.org> wrote:

> jrg: fyi
>
>
> http://codereview.chromium.org/180036
>