Allow dragging and dropping of URLs to any portion of the toolbar view....

Allow dragging and dropping of URLs to any portion of the toolbar view.

BUG=9181
TEST=Drag a URL or ay text to the toolbar area, including back/forward/reload
buttons, and the browser action/menu area.  This should not interfere with
dragging and dropping of browser actions.  Test that browser actions behave
as they should (i.e. should be able to change their order in the toolbar).


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

[Peter Kasting, 2011-02-08 22:34:04]

Is any of this duplicated elsewhere, e.g. for handling drops on the tab or
omnibox?  If so, can we unify to avoid copy-and-pasting?

sky might be a better reviewer for this (at least I think it was him who did the
original DnD support here, maybe it was tony though).

[Roger Tawa OOO till Jul 10th, 2011-02-10 18:43:47]

Hi Scott,

Changing reviewer to you as requested by Peter.

I have not tested this yet on platforms other than windows, but if you could
take a look to let me know if the windows implementation is OK that would be
great.  Thanks.

[sky, 2011-02-11 01:06:07]

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:862: const
ui::OSExchangeData& data = event.data();
What if the drag originated from this view?

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_views.cc (right):

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:567: return
ui::DragDropTypes::DRAG_NONE;
indent by 2, also add NOTIMPLEMENTED.

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:952: return
event.source_operations();
I believe you need to return one type here, not all. Same with 969

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar_view.cc (right):

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar_view.cc:617: return
ui::DragDropTypes::DRAG_COPY;
I think you should allow move or link too. But, you shouldn't allow move if the
drag originated from the edit.

[Roger Tawa OOO till Jul 10th, 2011-02-11 15:20:17]

Thanks Scott.  Please take another look.

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:862: const
ui::OSExchangeData& data = event.data();
On 2011/02/11 01:06:07, sky wrote:
> What if the drag originated from this view?

Good point.  This code should be equivalent to the windows implementation.  As I
mentioned in the comment, the non-windows part has not been fully tested yet,
but I'm working on it now.  More changes to come.

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_views.cc (right):

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:567: return
ui::DragDropTypes::DRAG_NONE;
On 2011/02/11 01:06:07, sky wrote:
> indent by 2, also add NOTIMPLEMENTED.

Done.

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:952: return
event.source_operations();
On 2011/02/11 01:06:07, sky wrote:
> I believe you need to return one type here, not all. Same with 969

At line 183, effect is guaranteed to have only one type.  effect is then used to
build the event object at line 186, which therefore also has only one type.  I
can add a comment to point this out.

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar_view.cc (right):

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar_view.cc:617: return
ui::DragDropTypes::DRAG_COPY;
On 2011/02/11 01:06:07, sky wrote:
> I think you should allow move or link too. But, you shouldn't allow move if
the
> drag originated from the edit.

Changed to support either copy or link, just like edit.  However, I don't think
move makes sense for any drag source.

[sky, 2011-02-11 17:14:53]

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:952: return
event.source_operations();
On 2011/02/11 15:20:17, Roger Tawa wrote:
> On 2011/02/11 01:06:07, sky wrote:
> > I believe you need to return one type here, not all. Same with 969
> 
> At line 183, effect is guaranteed to have only one type.  effect is then used
to
> build the event object at line 186, which therefore also has only one type.  I
> can add a comment to point this out.

But isn't OnPerformDrop also forwarded from the toolbar, which may have more
types?

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar_view.cc (right):

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar_view.cc:617: return
ui::DragDropTypes::DRAG_COPY;
On 2011/02/11 15:20:17, Roger Tawa wrote:
> On 2011/02/11 01:06:07, sky wrote:
> > I think you should allow move or link too. But, you shouldn't allow move if
> the
> > drag originated from the edit.
> 
> Changed to support either copy or link, just like edit.  However, I don't
think
> move makes sense for any drag source.

Fair enough, but shouldn't we ignore the drop entirely if it's drop the edit?

[Roger Tawa OOO till Jul 10th, 2011-02-11 21:35:26]

Thanks Scott.  Changes uploaded.

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:952: return
event.source_operations();
On 2011/02/11 17:14:53, sky wrote:
> On 2011/02/11 15:20:17, Roger Tawa wrote:
> > On 2011/02/11 01:06:07, sky wrote:
> > > I believe you need to return one type here, not all. Same with 969
> > 
> > At line 183, effect is guaranteed to have only one type.  effect is then
used
> to
> > build the event object at line 186, which therefore also has only one type. 
I
> > can add a comment to point this out.
> 
> But isn't OnPerformDrop also forwarded from the toolbar, which may have more
> types?

You are correct, I missed that.  Fixed.

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar_view.cc (right):

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar_view.cc:617: return
ui::DragDropTypes::DRAG_COPY;
On 2011/02/11 17:14:53, sky wrote:
> On 2011/02/11 15:20:17, Roger Tawa wrote:
> > On 2011/02/11 01:06:07, sky wrote:
> > > I think you should allow move or link too. But, you shouldn't allow move
if
> > the
> > > drag originated from the edit.
> > 
> > Changed to support either copy or link, just like edit.  However, I don't
> think
> > move makes sense for any drag source.
> 
> Fair enough, but shouldn't we ignore the drop entirely if it's drop the edit?

If the drop is on the edit itself, it uses its own handler for the drop.  This
code path does not handle that case.

[sky, 2011-02-11 23:18:50]

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar_view.cc (right):

http://codereview.chromium.org/6462009/diff/14022/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar_view.cc:617: return
ui::DragDropTypes::DRAG_COPY;
On 2011/02/11 21:35:26, Roger Tawa wrote:
> On 2011/02/11 17:14:53, sky wrote:
> > On 2011/02/11 15:20:17, Roger Tawa wrote:
> > > On 2011/02/11 01:06:07, sky wrote:
> > > > I think you should allow move or link too. But, you shouldn't allow move
> if
> > > the
> > > > drag originated from the edit.
> > > 
> > > Changed to support either copy or link, just like edit.  However, I don't
> > think
> > > move makes sense for any drag source.
> > 
> > Fair enough, but shouldn't we ignore the drop entirely if it's drop the
edit?
> 
> If the drop is on the edit itself, it uses its own handler for the drop.  This
> code path does not handle that case.

If you drag the url from the omnibox it won't allow you to drop it on the
omnibox. But it seems like this change would end up showing the drop cursor when
you drag from the omnibox to the toolbar, but not do anything. We should be
consistent and either always allow the drop or always disallow the drop in such
a situation.

[Roger Tawa OOO till Jul 10th, 2011-02-12 00:19:04]

Hi Scott,

As is, this change allows you to drop any string selection from the
omnibox to the toolbar, and it does do something.  For example, if the
url in the omnibox is:

http://desktop.google.com/en/linux/features.html

then you can select:

http://desktop.google.com
desktop.google.com
http://desktop.google.com/en/linux
linux/features

or whatever, and it gets dropped correctly.  True if you select the
entire URL it gets dropped too, causing a refresh of the page.  So we
definitely want to allow the drop from the omnibox to the toolbar.
I'll see if its easy to detect the case where the URL is exactly the
same and prevent that, but not sure how much of a bonus that is.

So this change, as is, works on windows and linux_view builds.  It
should work on chromeos builds since it also uses views, but I have
not tested it there yet.  However, this change does not affect the mac
and the linux builds.  Can you point me to the drag and drop code for
these two builds?

Thanks,
Roger

-



On Fri, Feb 11, 2011 at 18:18,  <sky@chromium.org> wrote:
>
>
http://codereview.chromium.org/6462009/diff/14022/chrome/browser/ui/views/too...
> File chrome/browser/ui/views/toolbar_view.cc (right):
>
>
http://codereview.chromium.org/6462009/diff/14022/chrome/browser/ui/views/too...
> chrome/browser/ui/views/toolbar_view.cc:617: return
> ui::DragDropTypes::DRAG_COPY;
> On 2011/02/11 21:35:26, Roger Tawa wrote:
>>
>> On 2011/02/11 17:14:53, sky wrote:
>> > On 2011/02/11 15:20:17, Roger Tawa wrote:
>> > > On 2011/02/11 01:06:07, sky wrote:
>> > > > I think you should allow move or link too. But, you shouldn't
>
> allow move
>>
>> if
>> > > the
>> > > > drag originated from the edit.
>> > >
>> > > Changed to support either copy or link, just like edit.  However,
>
> I don't
>>
>> > think
>> > > move makes sense for any drag source.
>> >
>> > Fair enough, but shouldn't we ignore the drop entirely if it's drop
>
> the edit?
>
>> If the drop is on the edit itself, it uses its own handler for the
>
> drop.  This
>>
>> code path does not handle that case.
>
> If you drag the url from the omnibox it won't allow you to drop it on
> the omnibox. But it seems like this change would end up showing the drop
> cursor when you drag from the omnibox to the toolbar, but not do
> anything. We should be consistent and either always allow the drop or
> always disallow the drop in such a situation.
>
> http://codereview.chromium.org/6462009/
>

[sky, 2011-02-12 00:48:15]

On Fri, Feb 11, 2011 at 4:18 PM, Roger Tawa <rogerta@chromium.org> wrote:
> Hi Scott,
>
> As is, this change allows you to drop any string selection from the
> omnibox to the toolbar, and it does do something.  For example, if the
> url in the omnibox is:
>
> http://desktop.google.com/en/linux/features.html
>
> then you can select:
>
> http://desktop.google.com
> desktop.google.com
> http://desktop.google.com/en/linux
> linux/features
>
> or whatever, and it gets dropped correctly.  True if you select the
> entire URL it gets dropped too, causing a refresh of the page.  So we
> definitely want to allow the drop from the omnibox to the toolbar.
> I'll see if its easy to detect the case where the URL is exactly the
> same and prevent that, but not sure how much of a bonus that is.

Currently we disallow dropping on the omnibox if the omnibox
originated the URL and drag is for the complete URL. We should be
consistent, either always disallow or always allow. I like disallowing
as it seems this situation is more of an accidental drop rather than
something you want to have happen.

> So this change, as is, works on windows and linux_view builds.  It
> should work on chromeos builds since it also uses views, but I have
> not tested it there yet.  However, this change does not affect the mac
> and the linux builds.  Can you point me to the drag and drop code for
> these two builds?

I think gtk is in autocomplete_edit_view_gtk, not sure on the mac
side. Did you want to keep this patch for only views?

  -Scott
>
> Thanks,
> Roger
>
> -
>
>
>
> On Fri, Feb 11, 2011 at 18:18,  <sky@chromium.org> wrote:
>>
>>
http://codereview.chromium.org/6462009/diff/14022/chrome/browser/ui/views/too...
>> File chrome/browser/ui/views/toolbar_view.cc (right):
>>
>>
http://codereview.chromium.org/6462009/diff/14022/chrome/browser/ui/views/too...
>> chrome/browser/ui/views/toolbar_view.cc:617: return
>> ui::DragDropTypes::DRAG_COPY;
>> On 2011/02/11 21:35:26, Roger Tawa wrote:
>>>
>>> On 2011/02/11 17:14:53, sky wrote:
>>> > On 2011/02/11 15:20:17, Roger Tawa wrote:
>>> > > On 2011/02/11 01:06:07, sky wrote:
>>> > > > I think you should allow move or link too. But, you shouldn't
>>
>> allow move
>>>
>>> if
>>> > > the
>>> > > > drag originated from the edit.
>>> > >
>>> > > Changed to support either copy or link, just like edit.  However,
>>
>> I don't
>>>
>>> > think
>>> > > move makes sense for any drag source.
>>> >
>>> > Fair enough, but shouldn't we ignore the drop entirely if it's drop
>>
>> the edit?
>>
>>> If the drop is on the edit itself, it uses its own handler for the
>>
>> drop.  This
>>>
>>> code path does not handle that case.
>>
>> If you drag the url from the omnibox it won't allow you to drop it on
>> the omnibox. But it seems like this change would end up showing the drop
>> cursor when you drag from the omnibox to the toolbar, but not do
>> anything. We should be consistent and either always allow the drop or
>> always disallow the drop in such a situation.
>>
>> http://codereview.chromium.org/6462009/
>>
>

[Roger Tawa OOO till Jul 10th, 2011-02-15 15:02:12]

Thanks Scott.  All your comments have been addressed.  Please take another look.

[sky, 2011-02-15 19:05:50]

I thought of a different approach that doesn't involve more methods on the
edit_view. Can you remove the drop target from the editor entirely, and override
the view methods in toolbar_bar? This way we only have one drop target.

http://codereview.chromium.org/6462009/diff/51003/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/6462009/diff/51003/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:104: bool IsDrag(const
POINT& origin, const POINT& current) {
Use views::View::ExeceededDragThreshold instead

http://codereview.chromium.org/6462009/diff/51003/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:228: // OnDragOver()
always returns only one effect type.  We create the event
Nuke this comment, it isn't useful.

[sky, 2011-02-15 19:35:44]

One comment. I like the approach of centralizing the handling of the drop in the
toolbar so we don't have two places dealing with it, but that assumes we don't
need to share state between the two places. If we end up needing to expose more
method on the edit so the toolbar can figure out state, then forget it.

  -Scott

On 2011/02/15 19:05:50, sky wrote:
> I thought of a different approach that doesn't involve more methods on the
> edit_view. Can you remove the drop target from the editor entirely, and
override
> the view methods in toolbar_bar? This way we only have one drop target.
> 
>
http://codereview.chromium.org/6462009/diff/51003/chrome/browser/autocomplete...
> File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):
> 
>
http://codereview.chromium.org/6462009/diff/51003/chrome/browser/autocomplete...
> chrome/browser/autocomplete/autocomplete_edit_view_win.cc:104: bool
IsDrag(const
> POINT& origin, const POINT& current) {
> Use views::View::ExeceededDragThreshold instead
> 
>
http://codereview.chromium.org/6462009/diff/51003/chrome/browser/autocomplete...
> chrome/browser/autocomplete/autocomplete_edit_view_win.cc:228: // OnDragOver()
> always returns only one effect type.  We create the event
> Nuke this comment, it isn't useful.

[Roger Tawa OOO till Jul 10th, 2011-02-15 19:50:07]

Hi Scott,

I don't believe its possible to remove the drop target from the edit view. 
Doing so would break dropping partially selected text within the omnibox itself.
 Also, search autocomplete_edit_view_win.cc for calls to in_drag() to see where
other things would break.

Even if it were possible to remove the edit view drop target, I don't believe
this would remove the need for a new method in AutocompleteEditView.  The
implementation of AutocompleteEditView::OnPerformDrop() is not platform
agnostic.

http://codereview.chromium.org/6462009/diff/51003/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/6462009/diff/51003/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:104: bool IsDrag(const
POINT& origin, const POINT& current) {
On 2011/02/15 19:05:50, sky wrote:
> Use views::View::ExeceededDragThreshold instead

Done.

http://codereview.chromium.org/6462009/diff/51003/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:228: // OnDragOver()
always returns only one effect type.  We create the event
On 2011/02/15 19:05:50, sky wrote:
> Nuke this comment, it isn't useful.

Done.

[sky, 2011-02-15 23:02:25]

Ya, I think you're right.

LGTM

On Tue, Feb 15, 2011 at 11:50 AM,  <rogerta@chromium.org> wrote:
> Hi Scott,
>
> I don't believe its possible to remove the drop target from the edit view.
> Doing so would break dropping partially selected text within the omnibox
> itself.
>  Also, search autocomplete_edit_view_win.cc for calls to in_drag() to see
> where
> other things would break.
>
> Even if it were possible to remove the edit view drop target, I don't
> believe
> this would remove the need for a new method in AutocompleteEditView.  The
> implementation of AutocompleteEditView::OnPerformDrop() is not platform
> agnostic.
>
>
>
>
http://codereview.chromium.org/6462009/diff/51003/chrome/browser/autocomplete...
> File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):
>
>
http://codereview.chromium.org/6462009/diff/51003/chrome/browser/autocomplete...
> chrome/browser/autocomplete/autocomplete_edit_view_win.cc:104: bool
> IsDrag(const POINT& origin, const POINT& current) {
> On 2011/02/15 19:05:50, sky wrote:
>>
>> Use views::View::ExeceededDragThreshold instead
>
> Done.
>
>
http://codereview.chromium.org/6462009/diff/51003/chrome/browser/autocomplete...
> chrome/browser/autocomplete/autocomplete_edit_view_win.cc:228: //
> OnDragOver() always returns only one effect type.  We create the event
> On 2011/02/15 19:05:50, sky wrote:
>>
>> Nuke this comment, it isn't useful.
>
> Done.
>
> http://codereview.chromium.org/6462009/
>