Mac: implement drag-and-drop of URLs to tabs/tab strip.

Mac: implement drag-and-drop of URLs to tabs/tab strip.

Not yet implemented: visual feedback, dropping multiple URLs/files, perhaps key modifiers (?).

Doing the visual feedback right (making the tabs move around, etc.) will require a fairly hefty work on the tabs code, so I'm splitting up the work.

BUG=25405
TEST=Test dragging and dropping URLs/links, files, plain text (with a possibly-poorly-formed URL), etc. on the tab and on the empty part of the tab strip; web page should load in tab or in new tab, and that tab should be selected.

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

[Scott Hess - ex-Googler, 2009-12-04 21:45:08]

What happens if you drop in the titlebar above the tabs?  What should happen?

http://codereview.chromium.org/337055/diff/8001/9004
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/337055/diff/8001/9004#newcode45
chrome/browser/cocoa/tab_strip_controller.mm:45: namespace {
I'd prefer you put the namespace/whitespace/constant fixup in a separate fixup
CL, rather than mixing it in with this.  Then when I later am looking at the
file's history, I don't wonder why an url-drop change broke tab-overlap, or
whatever.

[Feel free to send the CL to me.]

http://codereview.chromium.org/337055/diff/8001/9004#newcode1429
chrome/browser/cocoa/tab_strip_controller.mm:1429: if ([tabArray_ count]) {
Is this test useful?  If the array is empty, I'd expect the for() to be
efficient.

http://codereview.chromium.org/337055/diff/8001/9004#newcode1475
chrome/browser/cocoa/tab_strip_controller.mm:1475: if ([urls count] >= 1) {
Pull the <1 case out and unnest the rest.  Even though that's longer (because of
the return;), right now the NOTREACHED() is 20 lines away...

http://codereview.chromium.org/337055/diff/8001/9004#newcode1482
chrome/browser/cocoa/tab_strip_controller.mm:1482: std::string([[urls
objectAtIndex:0] UTF8String]), std::string()));
Suggest SysNSStringToUTF8(), for consistency.

http://codereview.chromium.org/337055/diff/8001/9007
File chrome/browser/cocoa/url_drop_target.h (right):

http://codereview.chromium.org/337055/diff/8001/9007#newcode10
chrome/browser/cocoa/url_drop_target.h:10: #include "base/scoped_nsobject.h"
You don't seem to use this here.

http://codereview.chromium.org/337055/diff/8001/9007#newcode37
chrome/browser/cocoa/url_drop_target.h:37: at:(NSPoint)location;
One line (here and elsewhere, I'm guessing the signature changed at some
point?).

http://codereview.chromium.org/337055/diff/8001/9008
File chrome/browser/cocoa/url_drop_target.mm (right):

http://codereview.chromium.org/337055/diff/8001/9008#newcode52
chrome/browser/cocoa/url_drop_target.mm:52: indicateDropURLsAt:location];
This casting is kinda clunky.  I don't know if static_cast<>() is appropriate
for protocols, but it might be cleaner to use the pattern from elsewhere of
defining controller as id<URLDropTargetWindowController> in the first place,
then DCHECK that it conforms.

http://codereview.chromium.org/337055/diff/8001/9008#newcode78
chrome/browser/cocoa/url_drop_target.mm:78: at:location];
And now that you've done all this twice, maybe provide an accessor method to
wrap the cast and DCHECK up?

http://codereview.chromium.org/337055/diff/8001/9008#newcode95
chrome/browser/cocoa/url_drop_target.mm:95: return NSDragOperationNone;
Why not:
  // Only allow copy operation.
  return (sourceDragMask&NSDragOperationCopy);

[viettrungluu, 2009-12-04 22:30:07]

Thanks, Scott.

If you drag stuff above, then it'll be as if you were over the tabs. This
behaviour will be much clearer once I've added visual feedback.

(I had an arrow working, but we have less room for an arrow than Win/Chrome so
it was too small. Making a floating arrow -- which might also float over the
menu bar -- seems like as much trouble as making the tabs move around.)

http://codereview.chromium.org/337055/diff/8001/9004
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/337055/diff/8001/9004#newcode45
chrome/browser/cocoa/tab_strip_controller.mm:45: namespace {
On 2009/12/04 21:45:08, shess wrote:
> I'd prefer you put the namespace/whitespace/constant fixup in a separate fixup
> CL, rather than mixing it in with this.  Then when I later am looking at the
> file's history, I don't wonder why an url-drop change broke tab-overlap, or
> whatever.
> 
> [Feel free to send the CL to me.]

It wasn't my intention, but I had to make kTabOverlap global ... at which point
it became a drive-by. I can still split it out if you insist, but I'd rather
not....

http://codereview.chromium.org/337055/diff/8001/9004#newcode1429
chrome/browser/cocoa/tab_strip_controller.mm:1429: if ([tabArray_ count]) {
On 2009/12/04 21:45:08, shess wrote:
> Is this test useful?  If the array is empty, I'd expect the for() to be
> efficient.

Probably not useful. Eliminated.

http://codereview.chromium.org/337055/diff/8001/9004#newcode1475
chrome/browser/cocoa/tab_strip_controller.mm:1475: if ([urls count] >= 1) {
On 2009/12/04 21:45:08, shess wrote:
> Pull the <1 case out and unnest the rest.  Even though that's longer (because
of
> the return;), right now the NOTREACHED() is 20 lines away...

Done.

http://codereview.chromium.org/337055/diff/8001/9004#newcode1482
chrome/browser/cocoa/tab_strip_controller.mm:1482: std::string([[urls
objectAtIndex:0] UTF8String]), std::string()));
On 2009/12/04 21:45:08, shess wrote:
> Suggest SysNSStringToUTF8(), for consistency.

Done.

http://codereview.chromium.org/337055/diff/8001/9007
File chrome/browser/cocoa/url_drop_target.h (right):

http://codereview.chromium.org/337055/diff/8001/9007#newcode10
chrome/browser/cocoa/url_drop_target.h:10: #include "base/scoped_nsobject.h"
On 2009/12/04 21:45:08, shess wrote:
> You don't seem to use this here.

Oops. Left over from a previous version.

http://codereview.chromium.org/337055/diff/8001/9007#newcode37
chrome/browser/cocoa/url_drop_target.h:37: at:(NSPoint)location;
On 2009/12/04 21:45:08, shess wrote:
> One line (here and elsewhere, I'm guessing the signature changed at some
> point?).

Done. (Maybe it did, but sometimes I just automatically break things up and
align along the :.)

http://codereview.chromium.org/337055/diff/8001/9008
File chrome/browser/cocoa/url_drop_target.mm (right):

http://codereview.chromium.org/337055/diff/8001/9008#newcode52
chrome/browser/cocoa/url_drop_target.mm:52: indicateDropURLsAt:location];
On 2009/12/04 21:45:08, shess wrote:
> This casting is kinda clunky.  I don't know if static_cast<>() is appropriate
> for protocols, but it might be cleaner to use the pattern from elsewhere of
> defining controller as id<URLDropTargetWindowController> in the first place,
> then DCHECK that it conforms.

Changed (to defining the controller as id<...> in a private method; I find
static_cast<>(...) even clunkier in most situations). I'm not sure what I was
thinking.

http://codereview.chromium.org/337055/diff/8001/9008#newcode78
chrome/browser/cocoa/url_drop_target.mm:78: at:location];
On 2009/12/04 21:45:08, shess wrote:
> And now that you've done all this twice, maybe provide an accessor method to
> wrap the cast and DCHECK up?

Done.

http://codereview.chromium.org/337055/diff/8001/9008#newcode95
chrome/browser/cocoa/url_drop_target.mm:95: return NSDragOperationNone;
On 2009/12/04 21:45:08, shess wrote:
> Why not:
>   // Only allow copy operation.
>   return (sourceDragMask&NSDragOperationCopy);

Done.

http://codereview.chromium.org/337055/diff/8001/9008#newcode102
chrome/browser/cocoa/url_drop_target.mm:102:
[(id<URLDropTargetWindowController>)controller hideDropURLsIndicator];
Thrice!

[Scott Hess - ex-Googler, 2009-12-04 23:17:21]

lgtm if you give back my wasted lines.

http://codereview.chromium.org/337055/diff/6023/6024
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/337055/diff/6023/6024#newcode191
chrome/browser/cocoa/browser_window_controller.h:191: at:(NSPoint)location;
One line!  I'm going to charge you for the wasted pixels.

http://codereview.chromium.org/337055/diff/6023/6025
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/337055/diff/6023/6025#newcode1406
chrome/browser/cocoa/browser_window_controller.mm:1406: at:(NSPoint)location {
...

http://codereview.chromium.org/337055/diff/6023/6026
File chrome/browser/cocoa/tab_strip_controller.h (right):

http://codereview.chromium.org/337055/diff/6023/6026#newcode184
chrome/browser/cocoa/tab_strip_controller.h:184: at:(NSPoint)location;
...

http://codereview.chromium.org/337055/diff/6023/6027
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/337055/diff/6023/6027#newcode1472
chrome/browser/cocoa/tab_strip_controller.mm:1472: at:(NSPoint)location {
...

http://codereview.chromium.org/337055/diff/6023/6031
File chrome/browser/cocoa/url_drop_target.mm (right):

http://codereview.chromium.org/337055/diff/6023/6031#newcode67
chrome/browser/cocoa/url_drop_target.mm:67: andTitles:&titles];
One line!

[viettrungluu, 2009-12-04 23:46:28]

At 5 cents per wasted line, I think I could just pay the fine (can I expense
it?).

On 2009/12/04 23:17:21, shess wrote:
> lgtm if you give back my wasted lines.
> 
> http://codereview.chromium.org/337055/diff/6023/6024
> File chrome/browser/cocoa/browser_window_controller.h (right):
> 
> http://codereview.chromium.org/337055/diff/6023/6024#newcode191
> chrome/browser/cocoa/browser_window_controller.h:191: at:(NSPoint)location;
> One line!  I'm going to charge you for the wasted pixels.
> 
> http://codereview.chromium.org/337055/diff/6023/6025
> File chrome/browser/cocoa/browser_window_controller.mm (right):
> 
> http://codereview.chromium.org/337055/diff/6023/6025#newcode1406
> chrome/browser/cocoa/browser_window_controller.mm:1406: at:(NSPoint)location {
> ...
> 
> http://codereview.chromium.org/337055/diff/6023/6026
> File chrome/browser/cocoa/tab_strip_controller.h (right):
> 
> http://codereview.chromium.org/337055/diff/6023/6026#newcode184
> chrome/browser/cocoa/tab_strip_controller.h:184: at:(NSPoint)location;
> ...
> 
> http://codereview.chromium.org/337055/diff/6023/6027
> File chrome/browser/cocoa/tab_strip_controller.mm (right):
> 
> http://codereview.chromium.org/337055/diff/6023/6027#newcode1472
> chrome/browser/cocoa/tab_strip_controller.mm:1472: at:(NSPoint)location {
> ...
> 
> http://codereview.chromium.org/337055/diff/6023/6031
> File chrome/browser/cocoa/url_drop_target.mm (right):
> 
> http://codereview.chromium.org/337055/diff/6023/6031#newcode67
> chrome/browser/cocoa/url_drop_target.mm:67: andTitles:&titles];
> One line!

[pink (ping after 24hrs), 2009-12-07 15:42:01]

http://codereview.chromium.org/337055/diff/11006/11007
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/337055/diff/11006/11007#newcode52
chrome/browser/cocoa/browser_window_controller.h:52:
URLDropTargetWindowController> {
crud, i wish i had seen this before you checked it in. The way this should be
done is not with yet another protocol, but by dispatching the call through the
responder chain and catching it at the BWC. That'll prevent us from having to
plumb it all the way down the line when nothing in the middle cares.

That's how I was going to do it before I got side-tracked to other things. We
should probably do with with loading of urls clicked on the bookmark bar as well
(it's basically the same issue).

[viettrungluu, 2009-12-07 15:52:21]

http://codereview.chromium.org/337055/diff/11006/11007
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/337055/diff/11006/11007#newcode52
chrome/browser/cocoa/browser_window_controller.h:52:
URLDropTargetWindowController> {
On 2009/12/07 15:42:02, pink wrote:
> crud, i wish i had seen this before you checked it in. The way this should be
> done is not with yet another protocol, but by dispatching the call through the
> responder chain and catching it at the BWC. That'll prevent us from having to
> plumb it all the way down the line when nothing in the middle cares.
> 
> That's how I was going to do it before I got side-tracked to other things. We
> should probably do with with loading of urls clicked on the bookmark bar as
well
> (it's basically the same issue).

I had a previous version which passed it down the responder chain -- it didn't
work so well, at least not without taking fairly heroic efforts. The problem is
that you often drag to an inactive window, which isn't on the responder chain at
all. So you'd have to catch it in the app controller (in addition to in the BWC,
since the window *might* be active), which then has to figure out which window
to send it to. So in the end the app controller would have to look up the
browser window controller anyway.

(If it's any consolation, I don't add yet another delegate to go stale; I just
look up the controller from its window....)

[pink (ping after 24hrs), 2009-12-07 15:59:33]

Really? It surprises me that each window doesn't have its own
responder chain. That kinda sucks.

On Mon, Dec 7, 2009 at 10:52 AM,  <viettrungluu@chromium.org> wrote:
>
> http://codereview.chromium.org/337055/diff/11006/11007
> File chrome/browser/cocoa/browser_window_controller.h (right):
>
> http://codereview.chromium.org/337055/diff/11006/11007#newcode52
> chrome/browser/cocoa/browser_window_controller.h:52:
> URLDropTargetWindowController> {
> On 2009/12/07 15:42:02, pink wrote:
>>
>> crud, i wish i had seen this before you checked it in. The way this
>
> should be
>>
>> done is not with yet another protocol, but by dispatching the call
>
> through the
>>
>> responder chain and catching it at the BWC. That'll prevent us from
>
> having to
>>
>> plumb it all the way down the line when nothing in the middle cares.
>
>> That's how I was going to do it before I got side-tracked to other
>
> things. We
>>
>> should probably do with with loading of urls clicked on the bookmark
>
> bar as well
>>
>> (it's basically the same issue).
>
> I had a previous version which passed it down the responder chain -- it
> didn't work so well, at least not without taking fairly heroic efforts.
> The problem is that you often drag to an inactive window, which isn't on
> the responder chain at all. So you'd have to catch it in the app
> controller (in addition to in the BWC, since the window *might* be
> active), which then has to figure out which window to send it to. So in
> the end the app controller would have to look up the browser window
> controller anyway.
>
> (If it's any consolation, I don't add yet another delegate to go stale;
> I just look up the controller from its window....)
>
> http://codereview.chromium.org/337055
>



-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com