Drag and drop between app list and launcher - First patch

This is the first of two patches to drag and drop items from the app list to the launcher.

Everything basically works with this patch, but two essential things are still missing:
1. The icon which gets dragged should get its own widget so that it can get visually dragged outside the app list.
2. The unit tests.

They will be send as a second patch because of:
a. The patch is already pretty big as it is.
b. I want to make get this "signed off" before continuing this route.

BUG=166429
TEST=visual, tests come with second patch

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

[Mr4D (OOO till 08-26), 2013-04-29 15:47:06]

This is the first of 2 patches. Please see the description of this patch for
more details.

Thanks!

[James Cook, 2013-04-29 17:39:42]

On 2013/04/29 15:47:06, Mr4D wrote:
> This is the first of 2 patches. Please see the description of this patch for
> more details.
> 
> Thanks!

I will get to it this afternoon

[James Cook, 2013-04-29 22:08:29]

Overall approach seems reasonable, some comments.

https://codereview.chromium.org/14533006/diff/1/ash/ash_switches.h
File ash/ash_switches.h (right):

https://codereview.chromium.org/14533006/diff/1/ash/ash_switches.h#newcode35
ash/ash_switches.h:35: ASH_EXPORT extern const char kAshDnDAppListToLauncher[];
Use something other than "DnD".  No acronyms.

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_button_...
File ash/launcher/launcher_button_host.h (right):

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_button_...
ash/launcher/launcher_button_host.h:28: DND,
DRAG or DRAG_AND_DROP

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_delegate.h
File ash/launcher/launcher_delegate.h (right):

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_delegat...
ash/launcher/launcher_delegate.h:103: // Pins an app with |app_id| to launcher.
If there is a running instance in
nice comment

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_delegat...
ash/launcher/launcher_delegate.h:108: // Unpins any app items whose id is
|app_id|.
Consider explaining why there might be more than one app pinned with the same
id.

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_view.cc...
ash/launcher/launcher_view.cc:556: // First we have to center the mouse cursor
over the item.
I don't understand this comment.  Isn't the code synthesizing a mouse press at
the center of the icon, not moving the cursor (or the icon)?

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_view.cc...
ash/launcher/launcher_view.cc:574: views::View* dnd_view = view_model_->view_at(
Do you need to check if this is NULL?  What if you're in the middle of a drag
and sync decides to delete all your apps?  Could this be a problem elsewhere
too?

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_view.cc...
ash/launcher/launcher_view.cc:1437: if
(!CommandLine::ForCurrentProcess()->HasSwitch(
Did you want to invert this test before committing?

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_view.cc...
ash/launcher/launcher_view.cc:1439:
Shell::GetInstance()->SetAppListDnDHost(this);
Can this be done once in the constructor or in Init() rather than every time the
app list is clicked?

If it has to be done every time the app list is shown, is there a way to make it
more clear that the host is being associated with a particular app list view
instance rather than the Shell in general?

A comment on multi-monitor/multi-launcher handling might be good too.

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_view.h
File ash/launcher/launcher_view.h (right):

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_view.h#...
ash/launcher/launcher_view.h:332: // True if a DND operation created the item in
the launcher and it needs to
DND -> drag and drop or just drag

https://codereview.chromium.org/14533006/diff/1/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/14533006/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:6760: +        Enables drag and drop from the
app list menu to the launcher.
nit: Consider "Allows" instead of "Enables".  We use the word "Enable" many
times in the about:flags dialog.

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/app_list_dn...
File ui/app_list/views/app_list_dnd_host.h (right):

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/app_list_dn...
ui/app_list/views/app_list_dnd_host.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
"Copyright 2013" without (c)

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/app_list_dn...
ui/app_list/views/app_list_dnd_host.h:16: class ApplicationDnDHost {
DnD -> DragAndDrop, or just Drag, or just Drop.  No acronyms.

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/app_list_dn...
ui/app_list/views/app_list_dnd_host.h:27: // returns false it requests that the
operation gets aborted since the event
nit: gets aborted -> be aborted

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/app_list_vi...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/app_list_vi...
ui/app_list/views/app_list_view.cc:125: DCHECK(app_list_main_view_);
I like asserts!

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/app_list_vi...
File ui/app_list/views/app_list_view.h (right):

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/app_list_vi...
ui/app_list/views/app_list_view.h:49: // InitAsBubble.
Comment why it must be called afterwards.

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/contents_vi...
File ui/app_list/views/contents_view.cc (right):

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/contents_vi...
ui/app_list/views/contents_view.cc:59: pagination_model);
indentation

[Mr4D (OOO till 08-26), 2013-04-30 16:59:00]

Addressed. Please have another look!

https://codereview.chromium.org/14533006/diff/1/ash/ash_switches.h
File ash/ash_switches.h (right):

https://codereview.chromium.org/14533006/diff/1/ash/ash_switches.h#newcode35
ash/ash_switches.h:35: ASH_EXPORT extern const char kAshDnDAppListToLauncher[];
On 2013/04/29 22:08:29, James Cook (Chromium) wrote:
> Use something other than "DnD".  No acronyms.

Done.

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_button_...
File ash/launcher/launcher_button_host.h (right):

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_button_...
ash/launcher/launcher_button_host.h:28: DND,
On 2013/04/29 22:08:29, James Cook (Chromium) wrote:
> DRAG or DRAG_AND_DROP

Done.

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_delegate.h
File ash/launcher/launcher_delegate.h (right):

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_delegat...
ash/launcher/launcher_delegate.h:103: // Pins an app with |app_id| to launcher.
If there is a running instance in
Okay, changed comment.

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_delegat...
ash/launcher/launcher_delegate.h:108: // Unpins any app items whose id is
|app_id|.
On 2013/04/29 22:08:29, James Cook (Chromium) wrote:
> Consider explaining why there might be more than one app pinned with the same
> id.

Done.

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_view.cc...
ash/launcher/launcher_view.cc:556: // First we have to center the mouse cursor
over the item.
On 2013/04/29 22:08:29, James Cook (Chromium) wrote:
> I don't understand this comment.  Isn't the code synthesizing a mouse press at
> the center of the icon, not moving the cursor (or the icon)?

Done.

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_view.cc...
ash/launcher/launcher_view.cc:574: views::View* dnd_view = view_model_->view_at(
Good one! I updated this!

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_view.cc...
ash/launcher/launcher_view.cc:1437: if
(!CommandLine::ForCurrentProcess()->HasSwitch(
Done. (yes in deed - for some reason flags do not work anymore on linux)

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_view.cc...
ash/launcher/launcher_view.cc:1439:
Shell::GetInstance()->SetAppListDnDHost(this);
Since the application list is created upon every click, the state is not
persistent. As such the handler needs to be set up every time. I renamed the
function to make it clear that this call will only have an impact on the current
application list. Done.

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_view.h
File ash/launcher/launcher_view.h (right):

https://codereview.chromium.org/14533006/diff/1/ash/launcher/launcher_view.h#...
ash/launcher/launcher_view.h:332: // True if a DND operation created the item in
the launcher and it needs to
On 2013/04/29 22:08:29, James Cook (Chromium) wrote:
> DND -> drag and drop or just drag

Done.

https://codereview.chromium.org/14533006/diff/1/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/14533006/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:6760: +        Enables drag and drop from the
app list menu to the launcher.
On 2013/04/29 22:08:29, James Cook (Chromium) wrote:
> nit: Consider "Allows" instead of "Enables".  We use the word "Enable" many
> times in the about:flags dialog.

Done.

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/app_list_dn...
File ui/app_list/views/app_list_dnd_host.h (right):

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/app_list_dn...
ui/app_list/views/app_list_dnd_host.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/04/29 22:08:29, James Cook (Chromium) wrote:
> "Copyright 2013" without (c)

Done.

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/app_list_dn...
ui/app_list/views/app_list_dnd_host.h:16: class ApplicationDnDHost {
On 2013/04/29 22:08:29, James Cook (Chromium) wrote:
> DnD -> DragAndDrop, or just Drag, or just Drop.  No acronyms.

Done.

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/app_list_dn...
ui/app_list/views/app_list_dnd_host.h:27: // returns false it requests that the
operation gets aborted since the event
On 2013/04/29 22:08:29, James Cook (Chromium) wrote:
> nit: gets aborted -> be aborted

Done.

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/app_list_vi...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/app_list_vi...
ui/app_list/views/app_list_view.cc:125: DCHECK(app_list_main_view_);
Okay, converted into a CHECK since this should not happen - debug or not.

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/app_list_vi...
File ui/app_list/views/app_list_view.h (right):

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/app_list_vi...
ui/app_list/views/app_list_view.h:49: // InitAsBubble.
On 2013/04/29 22:08:29, James Cook (Chromium) wrote:
> Comment why it must be called afterwards.

Done.

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/contents_vi...
File ui/app_list/views/contents_view.cc (right):

https://codereview.chromium.org/14533006/diff/1/ui/app_list/views/contents_vi...
ui/app_list/views/contents_view.cc:59: pagination_model);
On 2013/04/29 22:08:29, James Cook (Chromium) wrote:
> indentation

Done.

[James Cook, 2013-04-30 18:03:00]

LGTM with nit

https://codereview.chromium.org/14533006/diff/15007/ui/app_list/views/app_lis...
File ui/app_list/views/app_list_drag_and_drop_host.h (right):

https://codereview.chromium.org/14533006/diff/15007/ui/app_list/views/app_lis...
ui/app_list/views/app_list_drag_and_drop_host.h:10: #include "ui/gfx/point.h"
No need to fix for this CL, but you could have forward-declared gfx::Point here.

https://codereview.chromium.org/14533006/diff/15007/ui/app_list/views/apps_gr...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/14533006/diff/15007/ui/app_list/views/apps_gr...
ui/app_list/views/apps_grid_view.cc:700: event.root_location())) {
nit: indentation

[commit-bot: I haz the power, 2013-04-30 18:11:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/14533006/36001

[Mr4D (OOO till 08-26), 2013-04-30 18:22:47]

Addressed. Ben, I was moving you to the CC list since I am not sure if you come
to it. If you want to review the patch: Please go ahead and do so - I will
address then any resulting concerns in patch #2 (or even revert this one if it
does not work for your windows needs).

https://codereview.chromium.org/14533006/diff/15007/ui/app_list/views/app_lis...
File ui/app_list/views/app_list_drag_and_drop_host.h (right):

https://codereview.chromium.org/14533006/diff/15007/ui/app_list/views/app_lis...
ui/app_list/views/app_list_drag_and_drop_host.h:10: #include "ui/gfx/point.h"
Yes - right. Done.

https://codereview.chromium.org/14533006/diff/15007/ui/app_list/views/apps_gr...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/14533006/diff/15007/ui/app_list/views/apps_gr...
ui/app_list/views/apps_grid_view.cc:700: event.root_location())) {
On 2013/04/30 18:03:00, James Cook (Chromium) wrote:
> nit: indentation

Done.

[commit-bot: I haz the power, 2013-04-30 18:23:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/14533006/39001

[Mr4D (OOO till 08-26), 2013-04-30 18:28:23]

It turns out that there was a good reason to include benwells: 

You are the owner and you therefore have to review!
So - I will wait patiently for your review! Take your time!

Cheers!

[commit-bot: I haz the power, 2013-04-30 18:32:41]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[benwells, 2013-05-02 06:40:05]

I haven't looked though all the code yet, but I want to check I understand the
design. It looks like there is one thing that the app list can have apps dragged
into: the launcher, and it basically knows about the app list.

Have you investigated / thought about how this will fit into windows, where we
would want to drag into the taskbar and onto the desktop? I have not recently
(i.e. in the past few years) looked at how drag and drop works on windows, but I
think it is quite different; I'm not sure how applicable this design will be.

https://chromiumcodereview.appspot.com/14533006/diff/39001/chrome/browser/ui/...
File chrome/browser/ui/app_list/extension_app_item.cc (right):

https://chromiumcodereview.appspot.com/14533006/diff/39001/chrome/browser/ui/...
chrome/browser/ui/app_list/extension_app_item.cc:213: SetAppId(extension->id());
You should call SetAppId(extension_id) before the if block, so this will allow
pinning of apps that are currently installing.

https://chromiumcodereview.appspot.com/14533006/diff/39001/ui/app_list/app_li...
File ui/app_list/app_list_item_model.h (right):

https://chromiumcodereview.appspot.com/14533006/diff/39001/ui/app_list/app_li...
ui/app_list/app_list_item_model.h:46: void SetAppId(const std::string& app_id);
This should just be set_app_id (and inline) as there are no side effects to
setting it.

[Mr4D (OOO till 08-26), 2013-05-02 21:15:23]

Hey Ben,

since this might not be suitable for Windows & it is not ready for M28
anyways & behind a flag & only P2 I have decided to work today and tomorrow
quickly on some P1's. I will look into this tomorrow afternoon and try to
do some better comments.

But one thing first:
I programmed on Windows from 1991+ (2.0) till about a years ago. And I know
a lot of internals, ..
That said: I did never compile Chrome on Windows. (have my box finally here
though). So I am not sure how our windows framework looks like.

Either way - will look into it after I got rid of my P1's.

Maybe - lets chat on Monday (Tuesday for you)?

Cheers
-Stefan

On Wed, May 1, 2013 at 11:40 PM, <benwells@chromium.org> wrote:

> I haven't looked though all the code yet, but I want to check I understand
> the
> design. It looks like there is one thing that the app list can have apps
> dragged
> into: the launcher, and it basically knows about the app list.
>
> Have you investigated / thought about how this will fit into windows,
> where we
> would want to drag into the taskbar and onto the desktop? I have not
> recently
> (i.e. in the past few years) looked at how drag and drop works on windows,
> but I
> think it is quite different; I'm not sure how applicable this design will
> be.
>
>
> https://chromiumcodereview.**appspot.com/14533006/diff/**
>
39001/chrome/browser/ui/app_**list/extension_app_item.cc<https://chromiumcodereview.appspot.com/14533006/diff/39001/chrome/browser/ui/app_list/extension_app_item.cc>
> File chrome/browser/ui/app_list/**extension_app_item.cc (right):
>
> https://chromiumcodereview.**appspot.com/14533006/diff/**
>
39001/chrome/browser/ui/app_**list/extension_app_item.cc#**newcode213<https://chromiumcodereview.appspot.com/14533006/diff/39001/chrome/browser/ui/app_list/extension_app_item.cc#newcode213>
> chrome/browser/ui/app_list/**extension_app_item.cc:213:
> SetAppId(extension->id());
> You should call SetAppId(extension_id) before the if block, so this will
> allow pinning of apps that are currently installing.
>
> https://chromiumcodereview.**appspot.com/14533006/diff/**
>
39001/ui/app_list/app_list_**item_model.h<https://chromiumcodereview.appspot.com/14533006/diff/39001/ui/app_list/app_list_item_model.h>
> File ui/app_list/app_list_item_**model.h (right):
>
> https://chromiumcodereview.**appspot.com/14533006/diff/**
>
39001/ui/app_list/app_list_**item_model.h#newcode46<https://chromiumcodereview.appspot.com/14533006/diff/39001/ui/app_list/app_list_item_model.h#newcode46>
> ui/app_list/app_list_item_**model.h:46: void SetAppId(const std::string&
> app_id);
> This should just be set_app_id (and inline) as there are no side effects
> to setting it.
>
>
https://chromiumcodereview.**appspot.com/14533006/<https://chromiumcodereview...
>

[Mr4D (OOO till 08-26), 2013-05-02 21:16:16]

On Wed, May 1, 2013 at 11:40 PM, <benwells@chromium.org> wrote:

> I haven't looked though all the code yet, but I want to check I understand
> the
> design. It looks like there is one thing that the app list can have apps
> dragged
> into: the launcher, and it basically knows about the app list.
>
> Have you investigated / thought about how this will fit into windows,
> where we
> would want to drag into the taskbar and onto the desktop? I have not
> recently
> (i.e. in the past few years) looked at how drag and drop works on windows,
> but I
> think it is quite different; I'm not sure how applicable this design will
> be.
>
>
> https://chromiumcodereview.**appspot.com/14533006/diff/**
>
39001/chrome/browser/ui/app_**list/extension_app_item.cc<https://chromiumcodereview.appspot.com/14533006/diff/39001/chrome/browser/ui/app_list/extension_app_item.cc>
> File chrome/browser/ui/app_list/**extension_app_item.cc (right):
>
> https://chromiumcodereview.**appspot.com/14533006/diff/**
>
39001/chrome/browser/ui/app_**list/extension_app_item.cc#**newcode213<https://chromiumcodereview.appspot.com/14533006/diff/39001/chrome/browser/ui/app_list/extension_app_item.cc#newcode213>
> chrome/browser/ui/app_list/**extension_app_item.cc:213:
> SetAppId(extension->id());
> You should call SetAppId(extension_id) before the if block, so this will
> allow pinning of apps that are currently installing.
>
> https://chromiumcodereview.**appspot.com/14533006/diff/**
>
39001/ui/app_list/app_list_**item_model.h<https://chromiumcodereview.appspot.com/14533006/diff/39001/ui/app_list/app_list_item_model.h>
> File ui/app_list/app_list_item_**model.h (right):
>
> https://chromiumcodereview.**appspot.com/14533006/diff/**
>
39001/ui/app_list/app_list_**item_model.h#newcode46<https://chromiumcodereview.appspot.com/14533006/diff/39001/ui/app_list/app_list_item_model.h#newcode46>
> ui/app_list/app_list_item_**model.h:46: void SetAppId(const std::string&
> app_id);
> This should just be set_app_id (and inline) as there are no side effects
> to setting it.
>
>
https://chromiumcodereview.**appspot.com/14533006/<https://chromiumcodereview...
>

[Mr4D (OOO till 08-26), 2013-05-02 21:18:33]

That is great. In the past gMail was - when clicking "Reply" only sending to THE
person. Now "Reply" reply's to all. I seriously wonder why there is a "reply
all" button.

So except for benwells: please ignore the last eMails.

[Mr4D (OOO till 08-26), 2013-05-03 00:23:17]

Uploaded - but lets discuss!

https://codereview.chromium.org/14533006/diff/39001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/extension_app_item.cc (right):

https://codereview.chromium.org/14533006/diff/39001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/extension_app_item.cc:213: SetAppId(extension->id());
On 2013/05/02 06:40:05, benwells wrote:
> You should call SetAppId(extension_id) before the if block, so this will allow
> pinning of apps that are currently installing.

Done.

https://codereview.chromium.org/14533006/diff/39001/ui/app_list/app_list_item...
File ui/app_list/app_list_item_model.h (right):

https://codereview.chromium.org/14533006/diff/39001/ui/app_list/app_list_item...
ui/app_list/app_list_item_model.h:46: void SetAppId(const std::string& app_id);
On 2013/05/02 06:40:05, benwells wrote:
> This should just be set_app_id (and inline) as there are no side effects to
> setting it.

Done.

[koz (OOO until 15th September), 2013-05-03 01:19:51]

drive-by

https://codereview.chromium.org/14533006/diff/51001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/extension_app_item.cc (right):

https://codereview.chromium.org/14533006/diff/51001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/extension_app_item.cc:208:
set_app_id(extension->id());
This should be set_app_id(extension_id_) as extension may be NULL here. ie: when
the app is installing there is no associated Extension* yet, but we do know its
id.

[Mr4D (OOO till 08-26), 2013-05-07 04:31:01]

Addressed (and merged)! Please have another look!

https://codereview.chromium.org/14533006/diff/51001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/extension_app_item.cc (right):

https://codereview.chromium.org/14533006/diff/51001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/extension_app_item.cc:208:
set_app_id(extension->id());
On 2013/05/03 01:19:51, koz wrote:
> This should be set_app_id(extension_id_) as extension may be NULL here. ie:
when
> the app is installing there is no associated Extension* yet, but we do know
its
> id.

Done.

[koz (OOO until 15th September), 2013-05-08 02:00:38]

https://codereview.chromium.org/14533006/diff/58001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/14533006/diff/58001/ash/ash_switches.cc#newco...
ash/ash_switches.cc:144: 
nit: stray newline?

https://codereview.chromium.org/14533006/diff/58001/ash/launcher/launcher_vie...
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/14533006/diff/58001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:531: // Bail if an operation is already going on -
or the cursor is not inside.
Under what circumstances can a second drag start before the first one ends?
Could you mention them here?

https://codereview.chromium.org/14533006/diff/58001/ash/wm/app_list_controller.h
File ash/wm/app_list_controller.h (right):

https://codereview.chromium.org/14533006/diff/58001/ash/wm/app_list_controlle...
ash/wm/app_list_controller.h:67: void SetDragAndDropHostOfCurrentAppList(
Could you rename this to SetDragAndDropHost()? I think the "OfCurrentAppList"
bit is implied, eg: SetVisible() is implicitly talking about the current app
list.

https://codereview.chromium.org/14533006/diff/58001/ui/app_list/views/app_lis...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/14533006/diff/58001/ui/app_list/views/app_lis...
ui/app_list/views/app_list_view.cc:143: app_list_main_view_ = NULL;
I don't think we want to set this to NULL because on Windows we don't create the
view each time we display the app list, rather we hold on to the first view and
show/hide it as needed.

https://codereview.chromium.org/14533006/diff/58001/ui/app_list/views/apps_gr...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/14533006/diff/58001/ui/app_list/views/apps_gr...
ui/app_list/views/apps_grid_view.h:223: bool
drag_and_drop_host_dispatched_drag_;
Maybe rename this to forward_events_to_drag_and_drop_host_?

[Mr4D (OOO till 08-26), 2013-05-08 02:53:34]

Please have another look!

https://codereview.chromium.org/14533006/diff/58001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/14533006/diff/58001/ash/ash_switches.cc#newco...
ash/ash_switches.cc:144: 
On 2013/05/08 02:00:39, koz wrote:
> nit: stray newline?

Done.

https://codereview.chromium.org/14533006/diff/58001/ash/launcher/launcher_vie...
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/14533006/diff/58001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:531: // Bail if an operation is already going on -
or the cursor is not inside.
On 2013/05/08 02:00:39, koz wrote:
> Under what circumstances can a second drag start before the first one ends?
> Could you mention them here?

Done.

https://codereview.chromium.org/14533006/diff/58001/ash/wm/app_list_controller.h
File ash/wm/app_list_controller.h (right):

https://codereview.chromium.org/14533006/diff/58001/ash/wm/app_list_controlle...
ash/wm/app_list_controller.h:67: void SetDragAndDropHostOfCurrentAppList(
I did this because of my previous reviewer asked me to. His reasoning is that it
is clear that the association has to be set to the current object (every time)
since the object might go away AND the object needs first to be created before
it can be set.

https://codereview.chromium.org/14533006/diff/58001/ui/app_list/views/app_lis...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/14533006/diff/58001/ui/app_list/views/app_lis...
ui/app_list/views/app_list_view.cc:143: app_list_main_view_ = NULL;
On 2013/05/08 02:00:39, koz wrote:
> I don't think we want to set this to NULL because on Windows we don't create
the
> view each time we display the app list, rather we hold on to the first view
and
> show/hide it as needed.

Done.

https://codereview.chromium.org/14533006/diff/58001/ui/app_list/views/apps_gr...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/14533006/diff/58001/ui/app_list/views/apps_gr...
ui/app_list/views/apps_grid_view.h:223: bool
drag_and_drop_host_dispatched_drag_;
On 2013/05/08 02:00:39, koz wrote:
> Maybe rename this to forward_events_to_drag_and_drop_host_?

Done.

[koz (OOO until 15th September), 2013-05-08 03:16:36]

lgtm

[benwells, 2013-05-08 04:03:35]

On 2013/05/08 03:16:36, koz wrote:
> lgtm

stampy lgtm

[commit-bot: I haz the power, 2013-05-08 14:15:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/14533006/71001

[xiyuan, 2013-05-08 16:43:47]

https://codereview.chromium.org/14533006/diff/71001/ash/launcher/launcher_vie...
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/14533006/diff/71001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1445:
Shell::GetInstance()->SetDragAndDropHostOfCurrentAppList(this);
What happens if app list is invoked via shortcut key?

[commit-bot: I haz the power, 2013-05-08 20:58:54]

List of reviewers changed. xiyuan@chromium.org did a drive-by without LGTM'ing!

[xiyuan, 2013-05-08 21:06:35]

LGTM per offline discussion. The issue will be addressed separately.

[commit-bot: I haz the power, 2013-05-08 21:27:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/14533006/71001

[commit-bot: I haz the power, 2013-05-08 21:40:09]

Change committed as 199016