[Win] App launcher drag/drop.

[Win] App launcher drag/drop.

This allows icons to be dragged from the windows App Launcher to the taskbar.

BUG=153335
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207608

[koz (OOO until 15th September), 2013-06-19 03:50:28]

Hi Xiyuan,

I originally had hoped to change the ApplicationDragAndDropHost interface to be
generic and support both win and ash drag/drop, but the two APIs are so
different and are subject to such subtle differences (eg: the need to
asynchronously create a shortcut in the win drag case) that I don't think it
would be a good idea to unify the interfaces like that.

[xiyuan, 2013-06-19 05:41:37]

Nice work. It's cleaner than I would expect.

https://codereview.chromium.org/17370003/diff/3001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/17370003/diff/3001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_view_delegate.h:11: #include
"base/callback.h"
nit: #include "base/callback_forward.h" and include callback.h in cc

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/app_list_view_...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/app_list_view_...
ui/app_list/app_list_view_delegate.h:8: #include "base/callback.h"
nit: callback_forward.h here and callback.h in cc

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/apps_grid_view...
File ui/app_list/apps_grid_view_delegate.h (right):

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/apps_grid_view...
ui/app_list/apps_grid_view_delegate.h:8: #include "base/callback.h"
nit: callback_forward.h here and callback.h in cc

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/app_list...
File ui/app_list/views/app_list_item_view.cc (left):

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/app_list...
ui/app_list/views/app_list_item_view.cc:344: void
AppListItemView::OnMouseCaptureLost() {
Why don't we need this?

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/app_list...
File ui/app_list/views/app_list_item_view.cc (right):

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/app_list...
ui/app_list/views/app_list_item_view.cc:179: gfx::Rect bounds(0, 0,
size().width(), size().height());
nit: gfx::Rect bounds(size);

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/app_list...
ui/app_list/views/app_list_item_view.cc:180: LOG(ERROR) << bounds.ToString();
Get rid of debugging log.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/app_list...
ui/app_list/views/app_list_item_view.cc:182: SkXfermode::kSrc_Mode);
nit: canvas.DrawColor(SK_ColorTRANSPARENT);

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:11: #include "base/win/shortcut.h"
Would this need to be wrapped in a #if defined(OS_WIN)?

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:21: #include
"ui/base/dragdrop/drop_target_win.h"
and this?

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:23: #include
"ui/base/dragdrop/os_exchange_data_provider_win.h"
and this?

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:156: gfx::Size drag_view_size =
drag_view_->size();
nit: const gfx::Size

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:187: 
nit: nuke empty line

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:226: bool canceled_;
nit: DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:230: 
nit: nuke empty line

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:433:
drag_and_drop_host_->UpdateDragIconProxy(point);
This probably breaks cros because event.root_location() is in RootWindow's
coordinates. It is complicated to convert from a view's coordinates back. Either
convert view's coordinates to screen coordinates then using ScreenPositionClient
to convert it to a RootWindow or convert it to widget's coordinates then find
out which RootWindow the widget is on then convert it. It might easier just
passing down the root location or original LocatedEvent.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:517: return true;
Should this be false since we don't support dropping on app list?

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.h:15: #include
"ui/base/dragdrop/drag_source_win.h"
Shouldn't this be wrapped in a #if defined(OS_WIN)?

[koz (OOO until 15th September), 2013-06-19 07:55:08]

Thanks, it took a lot of work to get it to be this non-intrusive :-)

+sky for ash/shell/app_list.cc (and anything else you want to have a look at)
+benwells for chrome/browser/web_applications/

https://codereview.chromium.org/17370003/diff/3001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/17370003/diff/3001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_view_delegate.h:11: #include
"base/callback.h"
On 2013/06/19 05:41:38, xiyuan wrote:
> nit: #include "base/callback_forward.h" and include callback.h in cc

Done.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/app_list_view_...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/app_list_view_...
ui/app_list/app_list_view_delegate.h:8: #include "base/callback.h"
On 2013/06/19 05:41:38, xiyuan wrote:
> nit: callback_forward.h here and callback.h in cc

Done.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/apps_grid_view...
File ui/app_list/apps_grid_view_delegate.h (right):

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/apps_grid_view...
ui/app_list/apps_grid_view_delegate.h:8: #include "base/callback.h"
On 2013/06/19 05:41:38, xiyuan wrote:
> nit: callback_forward.h here and callback.h in cc

Done.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/app_list...
File ui/app_list/views/app_list_item_view.cc (left):

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/app_list...
ui/app_list/views/app_list_item_view.cc:344: void
AppListItemView::OnMouseCaptureLost() {
On 2013/06/19 05:41:38, xiyuan wrote:
> Why don't we need this?

Ah, we do on CROS, but it causes problems on Windows. I've added a comment and
an #ifdef here.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/app_list...
File ui/app_list/views/app_list_item_view.cc (right):

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/app_list...
ui/app_list/views/app_list_item_view.cc:179: gfx::Rect bounds(0, 0,
size().width(), size().height());
On 2013/06/19 05:41:38, xiyuan wrote:
> nit: gfx::Rect bounds(size);

Done.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/app_list...
ui/app_list/views/app_list_item_view.cc:180: LOG(ERROR) << bounds.ToString();
On 2013/06/19 05:41:38, xiyuan wrote:
> Get rid of debugging log.

Done.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/app_list...
ui/app_list/views/app_list_item_view.cc:182: SkXfermode::kSrc_Mode);
On 2013/06/19 05:41:38, xiyuan wrote:
> nit: canvas.DrawColor(SK_ColorTRANSPARENT);

Done.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:11: #include "base/win/shortcut.h"
On 2013/06/19 05:41:38, xiyuan wrote:
> Would this need to be wrapped in a #if defined(OS_WIN)?

Done.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:21: #include
"ui/base/dragdrop/drop_target_win.h"
On 2013/06/19 05:41:38, xiyuan wrote:
> and this?

Done.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:23: #include
"ui/base/dragdrop/os_exchange_data_provider_win.h"
On 2013/06/19 05:41:38, xiyuan wrote:
> and this?

Done.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:156: gfx::Size drag_view_size =
drag_view_->size();
On 2013/06/19 05:41:38, xiyuan wrote:
> nit: const gfx::Size

Done.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:187: 
On 2013/06/19 05:41:38, xiyuan wrote:
> nit: nuke empty line

Done.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:226: bool canceled_;
On 2013/06/19 05:41:38, xiyuan wrote:
> nit: DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:230: 
On 2013/06/19 05:41:38, xiyuan wrote:
> nit: nuke empty line

Done.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:433:
drag_and_drop_host_->UpdateDragIconProxy(point);
On 2013/06/19 05:41:38, xiyuan wrote:
> This probably breaks cros because event.root_location() is in RootWindow's
> coordinates. It is complicated to convert from a view's coordinates back.
Either
> convert view's coordinates to screen coordinates then using
ScreenPositionClient
> to convert it to a RootWindow or convert it to widget's coordinates then find
> out which RootWindow the widget is on then convert it. It might easier just
> passing down the root location or original LocatedEvent. 

I see, thanks for catching this. I've rearranged it to get the root location to
the drag_and_drop_host_. Is there a concise explanation for what a root_location
is and why it differs from platform to platform (am I right that on Windows it
is the coordinates in terms of the HWND?)

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:517: return true;
On 2013/06/19 05:41:38, xiyuan wrote:
> Should this be false since we don't support dropping on app list?

This being true puts the "moved" icon next to the cursor while we're dragging
app items around the app list, the same one you get when dragging blocks of text
around in a text editor.

It does mean that the app list will show that for any dragged object that has a
filename, which is why I've got a TODO for myself above. I think it's probably
hard to drag a file onto the app launcher anyway, so not too important to fix.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.h:15: #include
"ui/base/dragdrop/drag_source_win.h"
On 2013/06/19 05:41:38, xiyuan wrote:
> Shouldn't this be wrapped in a #if defined(OS_WIN)?

Done.

[benwells, 2013-06-19 09:30:01]

https://codereview.chromium.org/17370003/diff/13001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/17370003/diff/13001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app.h:104: base::FilePath
CreateShortcutInWebAppDir(
Mac does something very similar to this, but in a completely different way. You
need two things:
1. create it in the web app dir
2. find out the path of the created shortcut.

For 1. I'd like to see something added to ShortcutLocations. You can leave a
TODO on jackhou@ to make Mac use that.

For 2. ideally we would make a function like mac has to get the shim location
available on all platforms.

Maybe that is too much to ask for this change. At the very least please move
this and CheckAndSaveIcon into a new file web_app_win.h.

[benwells, 2013-06-19 09:30:31]

On 2013/06/19 09:30:01, benwells wrote:
>
https://codereview.chromium.org/17370003/diff/13001/chrome/browser/web_applic...
> File chrome/browser/web_applications/web_app.h (right):
> 
>
https://codereview.chromium.org/17370003/diff/13001/chrome/browser/web_applic...
> chrome/browser/web_applications/web_app.h:104: base::FilePath
> CreateShortcutInWebAppDir(
> Mac does something very similar to this, but in a completely different way.
You
> need two things:
> 1. create it in the web app dir
> 2. find out the path of the created shortcut.
> 
> For 1. I'd like to see something added to ShortcutLocations. You can leave a
> TODO on jackhou@ to make Mac use that.
> 
> For 2. ideally we would make a function like mac has to get the shim location
> available on all platforms.
> 
> Maybe that is too much to ask for this change. At the very least please move
> this and CheckAndSaveIcon into a new file web_app_win.h.

Oh, forgot to state the obvious: this change is awesome!

[sky, 2013-06-19 14:07:40]

LGTM

https://codereview.chromium.org/17370003/diff/13001/ui/app_list/apps_grid_vie...
File ui/app_list/apps_grid_view_delegate.h (right):

https://codereview.chromium.org/17370003/diff/13001/ui/app_list/apps_grid_vie...
ui/app_list/apps_grid_view_delegate.h:25: // Gets the path to a shortcut for the
app represented by |item|.
Document that |callback| may be run immediately.

https://codereview.chromium.org/17370003/diff/13001/ui/app_list/apps_grid_vie...
ui/app_list/apps_grid_view_delegate.h:28: base::Callback<void(const
base::FilePath&)> callback) = 0;
const Callback& ?

[xiyuan, 2013-06-19 16:21:38]

LGTM 

Please fix unit test compile and address ben and sky's comments.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:433:
drag_and_drop_host_->UpdateDragIconProxy(point);
On 2013/06/19 07:55:08, koz wrote:
> On 2013/06/19 05:41:38, xiyuan wrote:
> > This probably breaks cros because event.root_location() is in RootWindow's
> > coordinates. It is complicated to convert from a view's coordinates back.
> Either
> > convert view's coordinates to screen coordinates then using
> ScreenPositionClient
> > to convert it to a RootWindow or convert it to widget's coordinates then
find
> > out which RootWindow the widget is on then convert it. It might easier just
> > passing down the root location or original LocatedEvent. 
> 
> I see, thanks for catching this. I've rearranged it to get the root location
to
> the drag_and_drop_host_. Is there a concise explanation for what a
root_location
> is and why it differs from platform to platform (am I right that on Windows it
> is the coordinates in terms of the HWND?)

oshima has a ddoc for the coordinates used for ChromeOS and I think part of it
(HWC and RWC) probably applies to all aura builds (win aura and linux aura). For
non-aura Windows, think you are right that root_location would be the same as
location.

https://docs.google.com/a/google.com/document/d/1ZFVWOZLCQygYVMv05y6AJ25nUzM5...

https://codereview.chromium.org/17370003/diff/13001/ui/app_list/app_list_view...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/17370003/diff/13001/ui/app_list/app_list_view...
ui/app_list/app_list_view_delegate.h:43: base::Callback<void(const
base::FilePath&)> callback) = 0;
const Callback& here too.

[koz (OOO until 15th September), 2013-06-20 13:52:23]

I've disabled the new drag stuff on win/aura. It shouldn't be too hard to get it
working but I'll do that in a follow-up CL as I'm about to go on leave.

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/17370003/diff/3001/ui/app_list/views/apps_gri...
ui/app_list/views/apps_grid_view.cc:433:
drag_and_drop_host_->UpdateDragIconProxy(point);
On 2013/06/19 16:21:38, xiyuan wrote:
> On 2013/06/19 07:55:08, koz wrote:
> > On 2013/06/19 05:41:38, xiyuan wrote:
> > > This probably breaks cros because event.root_location() is in RootWindow's
> > > coordinates. It is complicated to convert from a view's coordinates back.
> > Either
> > > convert view's coordinates to screen coordinates then using
> > ScreenPositionClient
> > > to convert it to a RootWindow or convert it to widget's coordinates then
> find
> > > out which RootWindow the widget is on then convert it. It might easier
just
> > > passing down the root location or original LocatedEvent. 
> > 
> > I see, thanks for catching this. I've rearranged it to get the root location
> to
> > the drag_and_drop_host_. Is there a concise explanation for what a
> root_location
> > is and why it differs from platform to platform (am I right that on Windows
it
> > is the coordinates in terms of the HWND?)
> 
> oshima has a ddoc for the coordinates used for ChromeOS and I think part of it
> (HWC and RWC) probably applies to all aura builds (win aura and linux aura).
For
> non-aura Windows, think you are right that root_location would be the same as
> location.
> 
>
https://docs.google.com/a/google.com/document/d/1ZFVWOZLCQygYVMv05y6AJ25nUzM5...

Cool, thanks for the link!

https://codereview.chromium.org/17370003/diff/13001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/17370003/diff/13001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app.h:104: base::FilePath
CreateShortcutInWebAppDir(
On 2013/06/19 09:30:01, benwells wrote:
> Mac does something very similar to this, but in a completely different way.
You
> need two things:
> 1. create it in the web app dir
> 2. find out the path of the created shortcut.
> 
> For 1. I'd like to see something added to ShortcutLocations. You can leave a
> TODO on jackhou@ to make Mac use that.
> 
> For 2. ideally we would make a function like mac has to get the shim location
> available on all platforms.
> 
> Maybe that is too much to ask for this change. At the very least please move
> this and CheckAndSaveIcon into a new file web_app_win.h.

OK, I moved the functions.

https://codereview.chromium.org/17370003/diff/13001/ui/app_list/app_list_view...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/17370003/diff/13001/ui/app_list/app_list_view...
ui/app_list/app_list_view_delegate.h:43: base::Callback<void(const
base::FilePath&)> callback) = 0;
On 2013/06/19 16:21:38, xiyuan wrote:
> const Callback& here too.

Done.

https://codereview.chromium.org/17370003/diff/13001/ui/app_list/apps_grid_vie...
File ui/app_list/apps_grid_view_delegate.h (right):

https://codereview.chromium.org/17370003/diff/13001/ui/app_list/apps_grid_vie...
ui/app_list/apps_grid_view_delegate.h:25: // Gets the path to a shortcut for the
app represented by |item|.
On 2013/06/19 14:07:40, sky wrote:
> Document that |callback| may be run immediately.

Done.

https://codereview.chromium.org/17370003/diff/13001/ui/app_list/apps_grid_vie...
ui/app_list/apps_grid_view_delegate.h:28: base::Callback<void(const
base::FilePath&)> callback) = 0;
On 2013/06/19 14:07:40, sky wrote:
> const Callback& ?

Done.

[commit-bot: I haz the power, 2013-06-20 13:52:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/17370003/41001

[commit-bot: I haz the power, 2013-06-20 22:21:37]

Change committed as 207608