Mac: implement DnD of URLs onto Omnibox.

Mac: implement DnD of URLs onto Omnibox.

(DnD of text coming in another patch.)

BUG=24631
TEST=Select a URL/link/file from somewhere (a link in a browser, a URL in text, a file from the desktop, etc.) and drag it to the Omnibox in a Chromium browser window; the contents of the Omnibox should be selected to indicate that a drop would replace its contents; dropping should navigate to the appropriate location.

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

[Scott Hess, 2009-12-10 22:05:21]

http://codereview.chromium.org/481012/diff/1/4
File chrome/browser/cocoa/autocomplete_text_field_editor.h (right):

http://codereview.chromium.org/481012/diff/1/4#newcode7
chrome/browser/cocoa/autocomplete_text_field_editor.h:7: #include
"base/scoped_nsobject.h"
Suspect this is leftover from earlier prototyping.

http://codereview.chromium.org/481012/diff/1/6
File chrome/browser/cocoa/browser_window_controller.h (left):

http://codereview.chromium.org/481012/diff/1/6#oldcode192
chrome/browser/cocoa/browser_window_controller.h:192: -
(void)hideDropURLsIndicator;
Did this get lost in a merge or something?

http://codereview.chromium.org/481012/diff/1/7
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/481012/diff/1/7#newcode1404
chrome/browser/cocoa/browser_window_controller.mm:1404: //
(URLDropTargetWindowController protocol)
Overriding the standard functions to forward to the window controller which
forwards back to the view seems kind of clunky.  I wonder if you need some
refactoring - in which case, no time like the present!

[viettrungluu, 2009-12-18 18:40:20]

Okay, stuff refactored. Please re-review. My plan is to wear you down till you
concede defeat.

http://codereview.chromium.org/481012/diff/1/4
File chrome/browser/cocoa/autocomplete_text_field_editor.h (right):

http://codereview.chromium.org/481012/diff/1/4#newcode7
chrome/browser/cocoa/autocomplete_text_field_editor.h:7: #include
"base/scoped_nsobject.h"
On 2009/12/10 22:05:21, shess wrote:
> Suspect this is leftover from earlier prototyping.

Correct. Thanks for pointing that out.

http://codereview.chromium.org/481012/diff/1/6
File chrome/browser/cocoa/browser_window_controller.h (left):

http://codereview.chromium.org/481012/diff/1/6#oldcode192
chrome/browser/cocoa/browser_window_controller.h:192: -
(void)hideDropURLsIndicator;
On 2009/12/10 22:05:21, shess wrote:
> Did this get lost in a merge or something?

No, I got rid of it because it's redundant (declaring that the BWC implements
the protocol implies that these must be implemented).

[Scott Hess, 2009-12-18 21:36:48]

LGTM, with the #if 0 code removed.

http://codereview.chromium.org/481012/diff/7001/7003
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/481012/diff/7001/7003#newcode261
chrome/browser/cocoa/autocomplete_text_field.mm:261: return [windowController
toolbarController];
This is just sad.  I don't have a real alternative, though.  At least the local
code is making the decision about how to route things, though.

http://codereview.chromium.org/481012/diff/7001/7003#newcode268
chrome/browser/cocoa/autocomplete_text_field.mm:268: // TODO(viettrungluu): This
is a hack....
Please note that this doesn't restore focus when your drag exits the field.

Or just go ahead and log the bug now, before someone beats you to it :-).

http://codereview.chromium.org/481012/diff/7001/7006
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/481012/diff/7001/7006#newcode1548
chrome/browser/cocoa/browser_window_controller.mm:1548: #if 0
Is this meant to be gone-gone now?

[Presumably along with the autocomplete_text_field.h include?]

http://codereview.chromium.org/481012/diff/7001/7012
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/481012/diff/7001/7012#newcode688
chrome/browser/cocoa/toolbar_controller.mm:688: DCHECK_EQ(view,
locationBarRetainer_.get());
I see these references to locationBarRetainer_, but you never use it in the
implementation, so they are irrelevant.

That said, I'd kind of expect this code to live in the locationBar somehow,
though I can't for the life of me tell if there is any consistency in the other
platforms (some code seems to live in autocomplete/, which is probably not great
for this - unless you do convert-to-vector and pass them over there), so I guess
I shouldn't flog on that horse.

http://codereview.chromium.org/481012/diff/7001/7013
File chrome/browser/cocoa/url_drop_target.h (right):

http://codereview.chromium.org/481012/diff/7001/7013#newcode22
chrome/browser/cocoa/url_drop_target.h:22: -
(id)initWithView:(NSView<URLDropTarget>*)view;
I still don't really understand why you don't just:

- (id)initWithView:(NSView<URLDropTarget>*)view
controller:(id<URLDropTargetController>)controller;

and get rid of -urlDropController.

[viettrungluu, 2009-12-18 22:25:02]

Thanks.

http://codereview.chromium.org/481012/diff/7001/7003
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/481012/diff/7001/7003#newcode261
chrome/browser/cocoa/autocomplete_text_field.mm:261: return [windowController
toolbarController];
On 2009/12/18 21:36:48, shess wrote:
> This is just sad.  I don't have a real alternative, though.  At least the
local
> code is making the decision about how to route things, though.

Probably what we need is a |-controller| method of some sort which is always
safe.

http://codereview.chromium.org/481012/diff/7001/7003#newcode268
chrome/browser/cocoa/autocomplete_text_field.mm:268: // TODO(viettrungluu): This
is a hack....
On 2009/12/18 21:36:48, shess wrote:
> Please note that this doesn't restore focus when your drag exits the field.
> 
> Or just go ahead and log the bug now, before someone beats you to it :-).

Done.

http://codereview.chromium.org/481012/diff/7001/7006
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/481012/diff/7001/7006#newcode1548
chrome/browser/cocoa/browser_window_controller.mm:1548: #if 0
On 2009/12/18 21:36:48, shess wrote:
> Is this meant to be gone-gone now?
> 
> [Presumably along with the autocomplete_text_field.h include?]

Oops. Gone.

http://codereview.chromium.org/481012/diff/7001/7012
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/481012/diff/7001/7012#newcode688
chrome/browser/cocoa/toolbar_controller.mm:688: DCHECK_EQ(view,
locationBarRetainer_.get());
On 2009/12/18 21:36:48, shess wrote:
> I see these references to locationBarRetainer_, but you never use it in the
> implementation, so they are irrelevant.

Okay. They're gone.

> That said, I'd kind of expect this code to live in the locationBar somehow,
> though I can't for the life of me tell if there is any consistency in the
other
> platforms (some code seems to live in autocomplete/, which is probably not
great
> for this - unless you do convert-to-vector and pass them over there), so I
guess
> I shouldn't flog on that horse.

If I could find a better home for this code, I'd put it there, but the way the
code is divided is kind of awkward....

http://codereview.chromium.org/481012/diff/7001/7013
File chrome/browser/cocoa/url_drop_target.h (right):

http://codereview.chromium.org/481012/diff/7001/7013#newcode22
chrome/browser/cocoa/url_drop_target.h:22: -
(id)initWithView:(NSView<URLDropTarget>*)view;
On 2009/12/18 21:36:48, shess wrote:
> I still don't really understand why you don't just:
> 
> - (id)initWithView:(NSView<URLDropTarget>*)view
> controller:(id<URLDropTargetController>)controller;
> 
> and get rid of -urlDropController.

Probably stubbornness on my part. Or maybe eventually I'll add some sort of
-controller method to everything in sight.