Support custom drag-and-drop of bookmarks in Aura

Support custom drag-and-drop of bookmarks in Aura

This follows the pattern used on other platforms to hook up the bookmark
manager to the native drag and drop system, except that on Aura we avoid the
need for the extra abstraction provided by WebDragDest/WebDragDestDelegate
since we don't have applications outside of chrome we need to interact with.

BUG=117012
TEST=drag bookmarks around in the bookmark manager, and to/from the bookmark bar


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

[Rick Byers, 2012-03-20 15:38:21]

Varun, can you please take a look at this CL for fixing bookmark DnD on Aura? 
This approach ended up being a lot simpler than I was worried it was - clearly
the better option for now.

Thanks,
   Rick

[Rick Byers, 2012-03-20 17:14:02]

+Ben for OWNERS

[Ben Goodger (Google), 2012-03-20 17:25:52]

http://codereview.chromium.org/9757001/diff/1/chrome/browser/ui/views/tab_con...
File chrome/browser/ui/views/tab_contents/native_tab_contents_view_aura.cc
(right):

http://codereview.chromium.org/9757001/diff/1/chrome/browser/ui/views/tab_con...
chrome/browser/ui/views/tab_contents/native_tab_contents_view_aura.cc:184:
bookmark_handler_.reset(new WebDragBookmarkHandlerAura());
hrm. tying the bookmark stuff into here is going to make it harder to move this
into content. How is this done on Windows and why are we different?

[Rick Byers, 2012-03-20 18:11:14]

On 2012/03/20 17:25:52, Ben Goodger (Google) wrote:
>
http://codereview.chromium.org/9757001/diff/1/chrome/browser/ui/views/tab_con...
> File chrome/browser/ui/views/tab_contents/native_tab_contents_view_aura.cc
> (right):
> 
>
http://codereview.chromium.org/9757001/diff/1/chrome/browser/ui/views/tab_con...
> chrome/browser/ui/views/tab_contents/native_tab_contents_view_aura.cc:184:
> bookmark_handler_.reset(new WebDragBookmarkHandlerAura());
> hrm. tying the bookmark stuff into here is going to make it harder to move
this
> into content. How is this done on Windows and why are we different?

NativeTabContentsViewWin also holds (and is responsible for creating) a
WebDragBookmarkHandlerWin, so in that respect this is exactly the same.

One key difference in Aura from the other platforms is that they all rely on the
WebDragDest helper object and platform-specific implementations in content, and
then rely on WebDragDestDelegate implementations in Chrome to add this custom
bookmark handling.  So there's a few extra levels of plumbing on the other OSes
that we don't have here.  Talking with Varun, he felt the WebDragDest
abstraction was unnecessary on Aura because we don't have a native OS DnD system
to integrate with and share data across.  Presumably that's only true for CrOS
though and the Aura DnD support will need to be generalized for Windows, etc. 
Perhaps by that time though we'll be able to switch the bookmark manager to
using pure html5 DnD and be able to remove all this complexity entirely.

Any suggestions for now to make this better now in 19 without blocking on a
major overhaul?  Adding the whole WebDragDest abstraction to Aura is probably
beyond the scope of what we can do in 19.

[varunjain, 2012-03-20 18:19:16]

http://codereview.chromium.org/9757001/diff/1/chrome/browser/ui/views/tab_con...
File chrome/browser/ui/views/tab_contents/native_tab_contents_view_aura.cc
(right):

http://codereview.chromium.org/9757001/diff/1/chrome/browser/ui/views/tab_con...
chrome/browser/ui/views/tab_contents/native_tab_contents_view_aura.cc:184:
bookmark_handler_.reset(new WebDragBookmarkHandlerAura());
On 2012/03/20 17:25:54, Ben Goodger (Google) wrote:
> hrm. tying the bookmark stuff into here is going to make it harder to move
this
> into content. How is this done on Windows and why are we different?

On other platforms, bookmark handling is hooked to WebDragDest{Win|Gtk|Mac}.
These listen to platform drag event directly and forward those events to the
renderer. On Aura, we get drag events here through the NativeWidgetAura, hence I
did not feel WebDragDestAura is necessary.

On other platforms NTCV{Gtk|Win} and WebDragBookmarkHandler{Gtk|Win} live in the
same chrome/browser hierarchy as here for NTCVAura and the newly added
WebDragBookmarkHandlerAura. So I am not sure how is it different for aura.

[varunjain, 2012-03-21 05:10:07]

lgtm from my side BTW.

On 2012/03/20 18:19:16, varunjain wrote:
>
http://codereview.chromium.org/9757001/diff/1/chrome/browser/ui/views/tab_con...
> File chrome/browser/ui/views/tab_contents/native_tab_contents_view_aura.cc
> (right):
> 
>
http://codereview.chromium.org/9757001/diff/1/chrome/browser/ui/views/tab_con...
> chrome/browser/ui/views/tab_contents/native_tab_contents_view_aura.cc:184:
> bookmark_handler_.reset(new WebDragBookmarkHandlerAura());
> On 2012/03/20 17:25:54, Ben Goodger (Google) wrote:
> > hrm. tying the bookmark stuff into here is going to make it harder to move
> this
> > into content. How is this done on Windows and why are we different?
> 
> On other platforms, bookmark handling is hooked to WebDragDest{Win|Gtk|Mac}.
> These listen to platform drag event directly and forward those events to the
> renderer. On Aura, we get drag events here through the NativeWidgetAura, hence
I
> did not feel WebDragDestAura is necessary.
> 
> On other platforms NTCV{Gtk|Win} and WebDragBookmarkHandler{Gtk|Win} live in
the
> same chrome/browser hierarchy as here for NTCVAura and the newly added
> WebDragBookmarkHandlerAura. So I am not sure how is it different for aura.

[Ben Goodger (Google), 2012-03-21 15:34:59]

Where can I find WebDragDest in chrome source?

I consider this kind of thing a layering violation within chrome/browser,
even though we currently don't have any DEPS etc to prevent against it.

-Ben

On Tue, Mar 20, 2012 at 11:11 AM, <rbyers@chromium.org> wrote:

> On 2012/03/20 17:25:52, Ben Goodger (Google) wrote:
>
> http://codereview.chromium.**org/9757001/diff/1/chrome/**
> browser/ui/views/tab_contents/**native_tab_contents_view_aura.**cc
>
>> File chrome/browser/ui/views/tab_**contents/native_tab_contents_**
>> view_aura.cc
>> (right):
>>
>
>
> http://codereview.chromium.**org/9757001/diff/1/chrome/**
> browser/ui/views/tab_contents/**native_tab_contents_view_aura.**
> cc#newcode184
>
>> chrome/browser/ui/views/tab_**contents/native_tab_contents_**
>> view_aura.cc:184:
>> bookmark_handler_.reset(new WebDragBookmarkHandlerAura());
>> hrm. tying the bookmark stuff into here is going to make it harder to move
>>
> this
>
>> into content. How is this done on Windows and why are we different?
>>
>
> NativeTabContentsViewWin also holds (and is responsible for creating) a
> WebDragBookmarkHandlerWin, so in that respect this is exactly the same.
>
> One key difference in Aura from the other platforms is that they all rely
> on the
> WebDragDest helper object and platform-specific implementations in
> content, and
> then rely on WebDragDestDelegate implementations in Chrome to add this
> custom
> bookmark handling.  So there's a few extra levels of plumbing on the other
> OSes
> that we don't have here.  Talking with Varun, he felt the WebDragDest
> abstraction was unnecessary on Aura because we don't have a native OS DnD
> system
> to integrate with and share data across.  Presumably that's only true for
> CrOS
> though and the Aura DnD support will need to be generalized for Windows,
> etc.
> Perhaps by that time though we'll be able to switch the bookmark manager to
> using pure html5 DnD and be able to remove all this complexity entirely.
>
> Any suggestions for now to make this better now in 19 without blocking on a
> major overhaul?  Adding the whole WebDragDest abstraction to Aura is
> probably
> beyond the scope of what we can do in 19.
>
> http://codereview.chromium.**org/9757001/
>

[Rick Byers, 2012-03-21 16:48:46]

On 2012/03/21 15:34:59, Ben Goodger (Google) wrote:
> Where can I find WebDragDest in chrome source?

content/browser/tab_contents/web_drag_dest_win.h/cc has the Windows
implementation.
content/public/browser/web_drag_dest_delegate.h defines the delegate
chrome/browser/tab_contents/web_drag_bookmark_handler_win.h has the windows
implementation of this delegate for chrome

> I consider this kind of thing a layering violation within chrome/browser,
> even though we currently don't have any DEPS etc to prevent against it.

I'm not too familiar with how you guys ultimately want to structure the code, so
let me try to clarify so that I can come up with a change that you like better: 
You're saying that NativeTabContentsView and each of the platform specific
implementations should eventually move to content and so shouldn't have any
direct dependencies on chrome features (like bookmarks), right?

If so, then each of the following existing patterns (which I was trying to model
my Aura version after) is bad also, right?

In NativeTabContentsViewWin
(chrome/browser/ui/views/tab_contents/native_tab_contents_view_win.h):
   scoped_ptr<WebDragBookmarkHandlerWin> bookmark_handler_;
In NativeTabContentsViewWin::InitNativeTabContentsView
(chrome/browser/ui/views/tab_contents/native_tab_contents_view_win.cc):
   bookmark_handler_.reset(new WebDragBookmarkHandlerWin());
Same for NativeContentsViewGtk

I'll look at this some more to see if I can figure out the right way to pull all
of this bookmark stuff out of each of the NativeContentsView* implementations,
but this code is still pretty new to me.

[Ben Goodger (Google), 2012-03-21 17:51:17]

Why can't you just implement WebDragDestAura in content? It looks like it
mostly acts as a funnel to some delegate,
content/public/browser/web_drag_dest_delegate. On the Chrome side you'd
have your bookmark handler still implement this delegate. This is how it
seems to be implemented on other platforms.

-Ben

On Wed, Mar 21, 2012 at 9:48 AM, <rbyers@chromium.org> wrote:

> On 2012/03/21 15:34:59, Ben Goodger (Google) wrote:
>
>> Where can I find WebDragDest in chrome source?
>>
>
> content/browser/tab_contents/**web_drag_dest_win.h/cc has the Windows
> implementation.
> content/public/browser/web_**drag_dest_delegate.h defines the delegate
> chrome/browser/tab_contents/**web_drag_bookmark_handler_win.**h has the
> windows
> implementation of this delegate for chrome
>
>
>  I consider this kind of thing a layering violation within chrome/browser,
>> even though we currently don't have any DEPS etc to prevent against it.
>>
>
> I'm not too familiar with how you guys ultimately want to structure the
> code, so
> let me try to clarify so that I can come up with a change that you like
> better:
> You're saying that NativeTabContentsView and each of the platform specific
> implementations should eventually move to content and so shouldn't have any
> direct dependencies on chrome features (like bookmarks), right?
>
> If so, then each of the following existing patterns (which I was trying to
> model
> my Aura version after) is bad also, right?
>
> In NativeTabContentsViewWin
> (chrome/browser/ui/views/tab_**contents/native_tab_contents_**view_win.h):
>   scoped_ptr<**WebDragBookmarkHandlerWin> bookmark_handler_;
> In NativeTabContentsViewWin::**InitNativeTabContentsView
> (chrome/browser/ui/views/tab_**contents/native_tab_contents_**
> view_win.cc):
>   bookmark_handler_.reset(new WebDragBookmarkHandlerWin());
> Same for NativeContentsViewGtk
>
> I'll look at this some more to see if I can figure out the right way to
> pull all
> of this bookmark stuff out of each of the NativeContentsView*
> implementations,
> but this code is still pretty new to me.
>
>
http://codereview.chromium.**org/9757001/<http://codereview.chromium.org/9757...
>

[Rick Byers, 2012-03-22 16:09:15]

On 2012/03/21 17:51:17, Ben Goodger (Google) wrote:
> Why can't you just implement WebDragDestAura in content? It looks like it
> mostly acts as a funnel to some delegate,
> content/public/browser/web_drag_dest_delegate. On the Chrome side you'd
> have your bookmark handler still implement this delegate. This is how it
> seems to be implemented on other platforms.

I didn't realize NativeTabContentsViewWin was deprecated and no longer in use -
that was the pattern I was following.  It looks like
ChromeWebContentsViewDelegateWin is the pattern to be following instead.

I've added all the necessary abstraction (without going further down the path of
implementing TabContentsViewAura than necessary), please take another look.

[Ben Goodger (Google), 2012-03-22 16:19:46]

This looks great LGTM.

https://chromiumcodereview.appspot.com/9757001/diff/15004/chrome/browser/tab_...
File chrome/browser/tab_contents/web_drag_bookmark_handler_aura.cc (right):

https://chromiumcodereview.appspot.com/9757001/diff/15004/chrome/browser/tab_...
chrome/browser/tab_contents/web_drag_bookmark_handler_aura.cc:66:
bookmark_drag_data_);
4-space indent

https://chromiumcodereview.appspot.com/9757001/diff/15004/chrome/browser/tab_...
chrome/browser/tab_contents/web_drag_bookmark_handler_aura.cc:84:
bookmark_drag_data_);
nit: 4-space idnent

https://chromiumcodereview.appspot.com/9757001/diff/15004/chrome/browser/tab_...
File chrome/browser/tab_contents/web_drag_bookmark_handler_aura.h (right):

https://chromiumcodereview.appspot.com/9757001/diff/15004/chrome/browser/tab_...
chrome/browser/tab_contents/web_drag_bookmark_handler_aura.h:24: virtual void
DragInitialize(content::WebContents* contents) OVERRIDE;
// Overridden from content::WebDragDestDelegate:

[Rick Byers, 2012-03-22 16:27:40]

Thanks Ben!

https://chromiumcodereview.appspot.com/9757001/diff/15004/chrome/browser/tab_...
File chrome/browser/tab_contents/web_drag_bookmark_handler_aura.cc (right):

https://chromiumcodereview.appspot.com/9757001/diff/15004/chrome/browser/tab_...
chrome/browser/tab_contents/web_drag_bookmark_handler_aura.cc:66:
bookmark_drag_data_);
On 2012/03/22 16:19:46, Ben Goodger (Google) wrote:
> 4-space indent

Done.

https://chromiumcodereview.appspot.com/9757001/diff/15004/chrome/browser/tab_...
chrome/browser/tab_contents/web_drag_bookmark_handler_aura.cc:84:
bookmark_drag_data_);
On 2012/03/22 16:19:46, Ben Goodger (Google) wrote:
> nit: 4-space idnent

Done.

https://chromiumcodereview.appspot.com/9757001/diff/15004/chrome/browser/tab_...
File chrome/browser/tab_contents/web_drag_bookmark_handler_aura.h (right):

https://chromiumcodereview.appspot.com/9757001/diff/15004/chrome/browser/tab_...
chrome/browser/tab_contents/web_drag_bookmark_handler_aura.h:24: virtual void
DragInitialize(content::WebContents* contents) OVERRIDE;
On 2012/03/22 16:19:46, Ben Goodger (Google) wrote:
> // Overridden from content::WebDragDestDelegate:

Done.

[commit-bot: I haz the power, 2012-03-22 16:28:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/9757001/21013

[commit-bot: I haz the power, 2012-03-22 18:16:26]

Change committed as 128250