[Mac] Make bookmark bar a primitive drag destination.

[Mac] Make bookmark bar primitive drag destination.

Credits for the bookmark bar fix to dmac; stolen from http://codereview.chromium.org/267082 .

BUG=18289
TEST=Drag a link or bookmarklet from the web to the bookmark bar. It should be added at the end of the bar.

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

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

[Nico, 2009-10-23 04:22:01]

I started working on this, and then Ben deprioritized the bug. I think this is
useful enough to check it in, even if lots of stuff is still missing.

[John Grabowski, 2009-10-23 17:04:52]

Like it.
LGTM with "do it in IB" change.

http://codereview.chromium.org/336001/diff/4001/4003
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/336001/diff/4001/4003#newcode114
Line 114: [buttonView_ setController:self];
Hook this up in IB

http://codereview.chromium.org/336001/diff/4001/4003#newcode208
Line 208: // TODO(port): Support drops on folders etc
port --> thakis,jrg,anyone

http://codereview.chromium.org/336001/diff/4001/4004
File chrome/browser/cocoa/bookmark_bar_controller_unittest.mm (right):

http://codereview.chromium.org/336001/diff/4001/4004#newcode673
Line 673: TEST_F(BookmarkBarControllerTest, DropBookmarks) {
<3 u

http://codereview.chromium.org/336001/diff/4001/4005
File chrome/browser/cocoa/bookmark_bar_view.h (right):

http://codereview.chromium.org/336001/diff/4001/4005#newcode17
Line 17: BookmarkBarController* controller_;  // weak, owns us
IBOutlet BookmarkBarController* controller_
  and hook up in IB

http://codereview.chromium.org/336001/diff/4001/4006
File chrome/browser/cocoa/bookmark_bar_view.mm (right):

http://codereview.chromium.org/336001/diff/4001/4006#newcode63
Line 63: - (void)setController:(BookmarkBarController*)controller {
remove

http://codereview.chromium.org/336001/diff/4001/4006#newcode111
Line 111: - (void)concludeDragOperation:(id<NSDraggingInfo>)info {
Don't think you need to implement this if not used; can you check the doc to
confirm?  Or just delete and make sure you don't get a respondsToSelector error?
 Ditto for swome other empty above.

[Nico, 2009-10-23 17:31:48]

Thanks. Will submit once the try results are in for the revised patch.

http://codereview.chromium.org/336001/diff/4001/4003
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/336001/diff/4001/4003#newcode114
Line 114: [buttonView_ setController:self];
On 2009/10/23 17:04:52, John Grabowski wrote:
> Hook this up in IB

D'oh! Done.

http://codereview.chromium.org/336001/diff/4001/4003#newcode208
Line 208: // TODO(port): Support drops on folders etc
On 2009/10/23 17:04:52, John Grabowski wrote:
> port --> thakis,jrg,anyone
> 

Gave the TODO to you, but I might fix it some day.

http://codereview.chromium.org/336001/diff/4001/4005
File chrome/browser/cocoa/bookmark_bar_view.h (right):

http://codereview.chromium.org/336001/diff/4001/4005#newcode17
Line 17: BookmarkBarController* controller_;  // weak, owns us
On 2009/10/23 17:04:52, John Grabowski wrote:
> IBOutlet BookmarkBarController* controller_
>   and hook up in IB

Done.

http://codereview.chromium.org/336001/diff/4001/4006
File chrome/browser/cocoa/bookmark_bar_view.mm (right):

http://codereview.chromium.org/336001/diff/4001/4006#newcode63
Line 63: - (void)setController:(BookmarkBarController*)controller {
On 2009/10/23 17:04:52, John Grabowski wrote:
> remove

Done.

http://codereview.chromium.org/336001/diff/4001/4006#newcode111
Line 111: - (void)concludeDragOperation:(id<NSDraggingInfo>)info {
On 2009/10/23 17:04:52, John Grabowski wrote:
> Don't think you need to implement this if not used; can you check the doc to
> confirm?  Or just delete and make sure you don't get a respondsToSelector
error?
>  Ditto for swome other empty above.

http://developer.apple.com/mac/library/documentation/Cocoa/Reference/Applicat...
lists 'em all as "required".

[Nico, 2009-10-24 02:00:20]

I changed some minor things; I don't think a rereview is necessary. The try bots
now pass, so I will try to land this again.

The test failure was fixed by stealing a part of a fix by dmac
(http://codereview.chromium.org/267082 – I tried to do the same fix a few weeks
ago though :-P). Dave, I hope it's ok that I steal this.

http://codereview.chromium.org/336001/diff/4001/4006
File chrome/browser/cocoa/bookmark_bar_view.mm (right):

http://codereview.chromium.org/336001/diff/4001/4006#newcode111
Line 111: - (void)concludeDragOperation:(id<NSDraggingInfo>)info {
On 2009/10/23 17:31:48, Nico wrote:
> On 2009/10/23 17:04:52, John Grabowski wrote:
> > Don't think you need to implement this if not used; can you check the doc to
> > confirm?  Or just delete and make sure you don't get a respondsToSelector
> error?
> >  Ditto for swome other empty above.
> 
>
http://developer.apple.com/mac/library/documentation/Cocoa/Reference/Applicat...
> lists 'em all as "required".

After reading about this some more, NSView and NSWindow implement that protocol,
so I do in fact need to override only the methods I'm interested in. Done.

[pink (ping after 24hrs), 2009-10-27 15:00:43]

drive-by nits.

http://codereview.chromium.org/336001/diff/7001/8003
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/336001/diff/7001/8003#newcode83
Line 83: // Remove our view from its superview so it doesn't attempt to
reference
is this necessary with dmac's changes, or just a hack until then?

http://codereview.chromium.org/336001/diff/7001/8006
File chrome/browser/cocoa/bookmark_bar_view.mm (right):

http://codereview.chromium.org/336001/diff/7001/8006#newcode73
Line 73: // TODO(port): This should probably return |YES| and the controller
should
we've stopped using "port" for TODOs.

http://codereview.chromium.org/336001/diff/7001/8006#newcode80
Line 80: if ([[info draggingPasteboard] containsURLData])
todo for DragOperationMove when dragging a bookmark from the bar?

[Nico, 2009-10-27 15:52:42]

http://codereview.chromium.org/336001/diff/7001/8003
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/336001/diff/7001/8003#newcode83
Line 83: // Remove our view from its superview so it doesn't attempt to
reference
On 2009/10/27 15:00:43, pink wrote:
> is this necessary with dmac's changes, or just a hack until then?

See http://codereview.chromium.org/334029 .

http://codereview.chromium.org/336001/diff/7001/8006
File chrome/browser/cocoa/bookmark_bar_view.mm (right):

http://codereview.chromium.org/336001/diff/7001/8006#newcode73
Line 73: // TODO(port): This should probably return |YES| and the controller
should
On 2009/10/27 15:00:43, pink wrote:
> we've stopped using "port" for TODOs.

Will convert it when I touch this file the next time.

http://codereview.chromium.org/336001/diff/7001/8006#newcode80
Line 80: if ([[info draggingPasteboard] containsURLData])
On 2009/10/27 15:00:43, pink wrote:
> todo for DragOperationMove when dragging a bookmark from the bar?

Given that http://crbug.com/17609 is set to "started", I didn't want to add too
much stuff. Also, the bookmark dragging might happen in NSBookmarkButton instead
of here (?), so that feels like premature TODOization.