First shot at implementing drag&drop for Aura

First shot at implementing drag&drop for Aura

BUG=97845
TEST=none


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

[Ben Goodger (Google), 2011-11-07 20:33:36]

In short: I see no reason why this shouldn't work on windows with aura... and
you should add some unit tests.

http://codereview.chromium.org/8450018/diff/2001/ui/aura/test/test_desktop_de...
File ui/aura/test/test_desktop_delegate.h (right):

http://codereview.chromium.org/8450018/diff/2001/ui/aura/test/test_desktop_de...
ui/aura/test/test_desktop_delegate.h:34: virtual DragDropController*
GetDragDropController() const OVERRIDE {
Can you move this to the .cc like the other methods here?

http://codereview.chromium.org/8450018/diff/2001/ui/aura_shell/shell.cc
File ui/aura_shell/shell.cc (right):

http://codereview.chromium.org/8450018/diff/2001/ui/aura_shell/shell.cc#newco...
ui/aura_shell/shell.cc:80: params.type = Widget::InitParams::TYPE_TOOLTIP;
I fear overloading types like this. It's not actually a tooltip.

http://codereview.chromium.org/8450018/diff/2001/ui/aura_shell/shell.cc#newco...
ui/aura_shell/shell.cc:219: return NULL;
why?

http://codereview.chromium.org/8450018/diff/2001/ui/aura_shell/shell.cc#newco...
ui/aura_shell/shell.cc:228: #if !defined(OS_WIN)
why?

http://codereview.chromium.org/8450018/diff/2001/ui/aura_shell/shell.cc#newco...
ui/aura_shell/shell.cc:246: #if !defined(OS_WIN)
why?

http://codereview.chromium.org/8450018/diff/2001/ui/aura_shell/shell.cc#newco...
ui/aura_shell/shell.cc:263: #if !defined(OS_WIN)
etc

http://codereview.chromium.org/8450018/diff/2001/views/views.gyp
File views/views.gyp (right):

http://codereview.chromium.org/8450018/diff/2001/views/views.gyp#newcode468
views/views.gyp:468: ['exclude', 'drag_utils_aura.cc'],
??

[varunjain, 2011-11-08 21:59:35]

http://codereview.chromium.org/8450018/diff/2001/ui/aura/test/test_desktop_de...
File ui/aura/test/test_desktop_delegate.h (right):

http://codereview.chromium.org/8450018/diff/2001/ui/aura/test/test_desktop_de...
ui/aura/test/test_desktop_delegate.h:34: virtual DragDropController*
GetDragDropController() const OVERRIDE {
On 2011/11/07 20:33:36, Ben Goodger (Google) wrote:
> Can you move this to the .cc like the other methods here?

Done.

http://codereview.chromium.org/8450018/diff/2001/ui/aura_shell/shell.cc
File ui/aura_shell/shell.cc (right):

http://codereview.chromium.org/8450018/diff/2001/ui/aura_shell/shell.cc#newco...
ui/aura_shell/shell.cc:80: params.type = Widget::InitParams::TYPE_TOOLTIP;
On 2011/11/07 20:33:36, Ben Goodger (Google) wrote:
> I fear overloading types like this. It's not actually a tooltip.

My intention was to put it in the same container as the tooltip. In terms of
z-order, it should be as on top as tooltips. Would you rather I create a new
type?

http://codereview.chromium.org/8450018/diff/2001/ui/aura_shell/shell.cc#newco...
ui/aura_shell/shell.cc:219: return NULL;
On 2011/11/07 20:33:36, Ben Goodger (Google) wrote:
> why?

Done.

http://codereview.chromium.org/8450018/diff/2001/ui/aura_shell/shell.cc#newco...
ui/aura_shell/shell.cc:228: #if !defined(OS_WIN)
On 2011/11/07 20:33:36, Ben Goodger (Google) wrote:
> why?

Done.

http://codereview.chromium.org/8450018/diff/2001/ui/aura_shell/shell.cc#newco...
ui/aura_shell/shell.cc:246: #if !defined(OS_WIN)
On 2011/11/07 20:33:36, Ben Goodger (Google) wrote:
> why?

Done.

http://codereview.chromium.org/8450018/diff/2001/ui/aura_shell/shell.cc#newco...
ui/aura_shell/shell.cc:263: #if !defined(OS_WIN)
On 2011/11/07 20:33:36, Ben Goodger (Google) wrote:
> etc

Done.

http://codereview.chromium.org/8450018/diff/2001/views/views.gyp
File views/views.gyp (right):

http://codereview.chromium.org/8450018/diff/2001/views/views.gyp#newcode468
views/views.gyp:468: ['exclude', 'drag_utils_aura.cc'],
On 2011/11/07 20:33:36, Ben Goodger (Google) wrote:
> ??

Done.

[Ben Goodger (Google), 2011-11-09 18:38:15]

Another general note and a request...

I don't think this should live on Desktop. It's not technically a desktop
function... it's a different thing.

Seems like something that becomes part of the "aura client" api I described in
my email to chrome-aura just now.

Also, can you just merge this CL with the other one? I've been reviewing them
together, and i'd like to get a better understanding of your usage.

[varunjain, 2011-11-09 19:14:06]

On 2011/11/09 18:38:15, Ben Goodger (Google) wrote:
> Another general note and a request...
> 
> I don't think this should live on Desktop. It's not technically a desktop
> function... it's a different thing.
> 
> Seems like something that becomes part of the "aura client" api I described in
> my email to chrome-aura just now.
> 
> Also, can you just merge this CL with the other one? I've been reviewing them
> together, and i'd like to get a better understanding of your usage.

Merged the two CLs... also took care of the comment you gave in the other CL
(move drag drop methods to separate delegate)

[varunjain, 2011-11-11 00:07:25]

Hi Ben.. refactored to fit the aura client design. PTAL.

[Ben Goodger (Google), 2011-11-11 17:26:53]

http://codereview.chromium.org/8450018/diff/28001/ui/aura/desktop.h
File ui/aura/desktop.h (right):

http://codereview.chromium.org/8450018/diff/28001/ui/aura/desktop.h#newcode64
ui/aura/desktop.h:64: DragDropClient* drag_drop_client() { return
drag_drop_client_.get(); }
Since it looks like Desktop isn't actually using the client for anything itself,
I think DragDropClient should have its own GetInstance method so we don't need
to add this here.

http://codereview.chromium.org/8450018/diff/28001/ui/aura/window.h
File ui/aura/window.h (right):

http://codereview.chromium.org/8450018/diff/28001/ui/aura/window.h#newcode359
ui/aura/window.h:359: WindowDragDropDelegate* drag_drop_delegate_;
I think we should not add this directly to Window. Instead, this can be a
property that gets set. See client/aura_constants.h

http://codereview.chromium.org/8450018/diff/28001/ui/aura_shell/desktop_event...
File ui/aura_shell/desktop_event_filter.cc (right):

http://codereview.chromium.org/8450018/diff/28001/ui/aura_shell/desktop_event...
ui/aura_shell/desktop_event_filter.cc:64: // Give all events to the drag/drop
controller is a drag/drop is in progress.
Move this whole section to a separate function HandleDragEvent() in an anonymous
namespace at the head of this file.

The code here then becomes:

if (HandleDragEvent(target, event))
  return true;

http://codereview.chromium.org/8450018/diff/28001/ui/aura_shell/drag_drop_con...
File ui/aura_shell/drag_drop_controller.cc (right):

http://codereview.chromium.org/8450018/diff/28001/ui/aura_shell/drag_drop_con...
ui/aura_shell/drag_drop_controller.cc:26: Widget* CreateDragWidget() {
I'd like it if the whole "view" part of the drag&drop code was a separate class,
let's call it DragImage. When created, it creates a widget, and offers an API
SetDragImage(). This way you don't need to futz around with views classes
(ImageView) in the controller (StartDragAndDrop) and can just let the DragImage
class deal with that in its implementation.

http://codereview.chromium.org/8450018/diff/28001/ui/aura_shell/drag_drop_con...
ui/aura_shell/drag_drop_controller.cc:65:
drag_widget_.reset(CreateDragWidget());
This would become something like 

drag_image_.reset(new DragImage(provider.drag_image(), location));

http://codereview.chromium.org/8450018/diff/28001/ui/aura_shell/drag_drop_con...
ui/aura_shell/drag_drop_controller.cc:114: drag_widget_->SetBounds(bounds);
And this would become drag_image_->SetPosition(Desktop...);

http://codereview.chromium.org/8450018/diff/28001/ui/aura_shell/drag_drop_con...
ui/aura_shell/drag_drop_controller.cc:146: drag_widget_->Hide();
This should all be done by drag_image_'s dtor.

http://codereview.chromium.org/8450018/diff/28001/ui/aura_shell/drag_drop_con...
File ui/aura_shell/drag_drop_controller.h (right):

http://codereview.chromium.org/8450018/diff/28001/ui/aura_shell/drag_drop_con...
ui/aura_shell/drag_drop_controller.h:59: scoped_ptr<views::Widget> drag_widget_;
This would become:

scoped_ptr<DragImage> drag_image_;

See comments in next file.

[varunjain, 2011-11-15 19:39:33]

http://codereview.chromium.org/8450018/diff/28001/ui/aura/desktop.h
File ui/aura/desktop.h (right):

http://codereview.chromium.org/8450018/diff/28001/ui/aura/desktop.h#newcode64
ui/aura/desktop.h:64: DragDropClient* drag_drop_client() { return
drag_drop_client_.get(); }
On 2011/11/11 17:26:53, Ben Goodger (Google) wrote:
> Since it looks like Desktop isn't actually using the client for anything
itself,
> I think DragDropClient should have its own GetInstance method so we don't need
> to add this here.

Done.

http://codereview.chromium.org/8450018/diff/28001/ui/aura/window.h
File ui/aura/window.h (right):

http://codereview.chromium.org/8450018/diff/28001/ui/aura/window.h#newcode359
ui/aura/window.h:359: WindowDragDropDelegate* drag_drop_delegate_;
On 2011/11/11 17:26:53, Ben Goodger (Google) wrote:
> I think we should not add this directly to Window. Instead, this can be a
> property that gets set. See client/aura_constants.h

Done.

http://codereview.chromium.org/8450018/diff/28001/ui/aura_shell/desktop_event...
File ui/aura_shell/desktop_event_filter.cc (right):

http://codereview.chromium.org/8450018/diff/28001/ui/aura_shell/desktop_event...
ui/aura_shell/desktop_event_filter.cc:64: // Give all events to the drag/drop
controller is a drag/drop is in progress.
On 2011/11/11 17:26:53, Ben Goodger (Google) wrote:
> Move this whole section to a separate function HandleDragEvent() in an
anonymous
> namespace at the head of this file.
> 
> The code here then becomes:
> 
> if (HandleDragEvent(target, event))
>   return true;

Done.

http://codereview.chromium.org/8450018/diff/28001/ui/aura_shell/drag_drop_con...
File ui/aura_shell/drag_drop_controller.cc (right):

http://codereview.chromium.org/8450018/diff/28001/ui/aura_shell/drag_drop_con...
ui/aura_shell/drag_drop_controller.cc:26: Widget* CreateDragWidget() {
On 2011/11/11 17:26:53, Ben Goodger (Google) wrote:
> I'd like it if the whole "view" part of the drag&drop code was a separate
class,
> let's call it DragImage. When created, it creates a widget, and offers an API
> SetDragImage(). This way you don't need to futz around with views classes
> (ImageView) in the controller (StartDragAndDrop) and can just let the
DragImage
> class deal with that in its implementation.

Done.

http://codereview.chromium.org/8450018/diff/28001/ui/aura_shell/drag_drop_con...
ui/aura_shell/drag_drop_controller.cc:65:
drag_widget_.reset(CreateDragWidget());
On 2011/11/11 17:26:53, Ben Goodger (Google) wrote:
> This would become something like 
> 
> drag_image_.reset(new DragImage(provider.drag_image(), location));

Done.

http://codereview.chromium.org/8450018/diff/28001/ui/aura_shell/drag_drop_con...
ui/aura_shell/drag_drop_controller.cc:114: drag_widget_->SetBounds(bounds);
On 2011/11/11 17:26:53, Ben Goodger (Google) wrote:
> And this would become drag_image_->SetPosition(Desktop...);

Done.

http://codereview.chromium.org/8450018/diff/28001/ui/aura_shell/drag_drop_con...
ui/aura_shell/drag_drop_controller.cc:146: drag_widget_->Hide();
On 2011/11/11 17:26:53, Ben Goodger (Google) wrote:
> This should all be done by drag_image_'s dtor.

Done.

http://codereview.chromium.org/8450018/diff/28001/ui/aura_shell/drag_drop_con...
File ui/aura_shell/drag_drop_controller.h (right):

http://codereview.chromium.org/8450018/diff/28001/ui/aura_shell/drag_drop_con...
ui/aura_shell/drag_drop_controller.h:59: scoped_ptr<views::Widget> drag_widget_;
On 2011/11/11 17:26:53, Ben Goodger (Google) wrote:
> This would become:
> 
> scoped_ptr<DragImage> drag_image_;
> 
> See comments in next file.

Done.

[Ben Goodger (Google), 2011-11-16 00:13:20]

Getting very close...

http://codereview.chromium.org/8450018/diff/36001/ui/aura/window_drag_drop_de...
File ui/aura/window_drag_drop_delegate.h (right):

http://codereview.chromium.org/8450018/diff/36001/ui/aura/window_drag_drop_de...
ui/aura/window_drag_drop_delegate.h:5: #ifndef
UI_AURA_WINDOW_DRAG_DROP_DELEGATE_H_
this should live in client too.

http://codereview.chromium.org/8450018/diff/36001/ui/aura_shell/desktop_event...
File ui/aura_shell/desktop_event_filter.cc (right):

http://codereview.chromium.org/8450018/diff/36001/ui/aura_shell/desktop_event...
ui/aura_shell/desktop_event_filter.cc:101: if (HandleDragDrop(target, event))
Now that we support addtl event filters... just move this code to a separate
event filter and call AddFilter on this guy.

Then you'll be automagically called by FilterMouseEvent below!

http://codereview.chromium.org/8450018/diff/36001/ui/aura_shell/drag_image_vi...
File ui/aura_shell/drag_image_view.cc (right):

http://codereview.chromium.org/8450018/diff/36001/ui/aura_shell/drag_image_vi...
ui/aura_shell/drag_image_view.cc:50: void DragImageView::SetVisible(bool
visible) {
This feels weird. In general I dislike overriding. Can we move this onto a
separate Show() method so we don't muck with the view's visibility state?

http://codereview.chromium.org/8450018/diff/36001/views/views.gyp
File views/views.gyp (right):

http://codereview.chromium.org/8450018/diff/36001/views/views.gyp#newcode471
views/views.gyp:471: ['exclude', 'drag_utils_aura.cc'],
seems like we want to build this on win too when aura is defined

[varunjain, 2011-11-16 20:30:14]

the test has started failing :(... I am looking into it.. please review the
other parts

http://codereview.chromium.org/8450018/diff/36001/ui/aura/window_drag_drop_de...
File ui/aura/window_drag_drop_delegate.h (right):

http://codereview.chromium.org/8450018/diff/36001/ui/aura/window_drag_drop_de...
ui/aura/window_drag_drop_delegate.h:5: #ifndef
UI_AURA_WINDOW_DRAG_DROP_DELEGATE_H_
On 2011/11/16 00:13:20, Ben Goodger (Google) wrote:
> this should live in client too.

Done.

http://codereview.chromium.org/8450018/diff/36001/ui/aura_shell/desktop_event...
File ui/aura_shell/desktop_event_filter.cc (right):

http://codereview.chromium.org/8450018/diff/36001/ui/aura_shell/desktop_event...
ui/aura_shell/desktop_event_filter.cc:101: if (HandleDragDrop(target, event))
On 2011/11/16 00:13:20, Ben Goodger (Google) wrote:
> Now that we support addtl event filters... just move this code to a separate
> event filter and call AddFilter on this guy.
> 
> Then you'll be automagically called by FilterMouseEvent below!

Done.

http://codereview.chromium.org/8450018/diff/36001/ui/aura_shell/drag_image_vi...
File ui/aura_shell/drag_image_view.cc (right):

http://codereview.chromium.org/8450018/diff/36001/ui/aura_shell/drag_image_vi...
ui/aura_shell/drag_image_view.cc:50: void DragImageView::SetVisible(bool
visible) {
On 2011/11/16 00:13:20, Ben Goodger (Google) wrote:
> This feels weird. In general I dislike overriding. Can we move this onto a
> separate Show() method so we don't muck with the view's visibility state?

Done.

http://codereview.chromium.org/8450018/diff/36001/views/views.gyp
File views/views.gyp (right):

http://codereview.chromium.org/8450018/diff/36001/views/views.gyp#newcode471
views/views.gyp:471: ['exclude', 'drag_utils_aura.cc'],
On 2011/11/16 00:13:20, Ben Goodger (Google) wrote:
> seems like we want to build this on win too when aura is defined

Done.

[Ben Goodger (Google), 2011-11-16 20:39:49]

OK I am generally happy with the way this looks now.

Let me know when your tests pass.

http://codereview.chromium.org/8450018/diff/39001/ui/aura/aura.gyp
File ui/aura/aura.gyp (right):

http://codereview.chromium.org/8450018/diff/39001/ui/aura/aura.gyp#newcode39
ui/aura/aura.gyp:39: 'client/drag_drop_client.h',
sort up

http://codereview.chromium.org/8450018/diff/39001/ui/aura_shell/drag_drop_con...
File ui/aura_shell/drag_drop_controller.cc (right):

http://codereview.chromium.org/8450018/diff/39001/ui/aura_shell/drag_drop_con...
ui/aura_shell/drag_drop_controller.cc:50:
static_cast<DesktopEventFilter*>(Desktop::GetInstance()->event_filter())->
Sync, and you can now just do:

Shell::GetInstance()->AddDesktopEventFilter(this);

http://codereview.chromium.org/8450018/diff/39001/ui/aura_shell/drag_drop_con...
ui/aura_shell/drag_drop_controller.cc:56: RemoveFilter(this);
ditto:

Shell::GetInstance()->RemoveDesktopEventFilter(this);

[varunjain, 2011-11-16 22:55:20]

tests pass locally... sent to trybots

http://codereview.chromium.org/8450018/diff/39001/ui/aura/aura.gyp
File ui/aura/aura.gyp (right):

http://codereview.chromium.org/8450018/diff/39001/ui/aura/aura.gyp#newcode39
ui/aura/aura.gyp:39: 'client/drag_drop_client.h',
On 2011/11/16 20:39:49, Ben Goodger (Google) wrote:
> sort up

Done.

http://codereview.chromium.org/8450018/diff/39001/ui/aura_shell/drag_drop_con...
File ui/aura_shell/drag_drop_controller.cc (right):

http://codereview.chromium.org/8450018/diff/39001/ui/aura_shell/drag_drop_con...
ui/aura_shell/drag_drop_controller.cc:50:
static_cast<DesktopEventFilter*>(Desktop::GetInstance()->event_filter())->
On 2011/11/16 20:39:49, Ben Goodger (Google) wrote:
> Sync, and you can now just do:
> 
> Shell::GetInstance()->AddDesktopEventFilter(this);

Done.

http://codereview.chromium.org/8450018/diff/39001/ui/aura_shell/drag_drop_con...
ui/aura_shell/drag_drop_controller.cc:56: RemoveFilter(this);
On 2011/11/16 20:39:49, Ben Goodger (Google) wrote:
> ditto:
> 
> Shell::GetInstance()->RemoveDesktopEventFilter(this);

Done.

[Ben Goodger (Google), 2011-11-16 23:04:03]

LGTM

[commit-bot: I haz the power, 2011-11-17 00:10:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/8450018/46002

[commit-bot: I haz the power, 2011-11-17 05:18:18]

Change committed as 110437