Move the panel to the root window where the mouse click occurred.

Move the panel to the root window where the mouse click occurred.

 * Change the LauncherDelegate::ItemClicked's argument from
   int flag to const ui::Event&
 * temporarily set the view's event target to RV. Sadrul
   is working on real solution in crbug.com/173235.

I tried to add interactive_ui_tests for chrome, but turns
out it's require more work as there is no interactive ui tests for
extensions. I'd rather refactor launcher code so that we can test
the same behavior in ash_unittests. I'll work on it and add more tests
once @skuhne finished new launcher (could be 26 or 27)

BUG=166195
TEST=covered by tests. Also tested on device (clicking panel icon on other display should move the panel to that display)

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

[oshima, 2013-01-31 20:08:47]

sadrul -> ui/views
stevenjs -> everything else

[sadrul, 2013-01-31 20:14:06]

ui/views/ lgtm

[stevenjb, 2013-01-31 20:23:40]

lgtm

https://codereview.chromium.org/12093075/diff/2003/ash/launcher/launcher_util.h
File ash/launcher/launcher_util.h (right):

https://codereview.chromium.org/12093075/diff/2003/ash/launcher/launcher_util...
ash/launcher/launcher_util.h:30: const ui::Event& event);
nit: I would consider either:
a) require maybe_panel be a panel and remove the if/maybe
b) make this more generic, like 'MaybeMoveToEventRoot' with a note in the
comment that currently this only affects panels.

https://codereview.chromium.org/12093075/diff/2003/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/shell_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/12093075/diff/2003/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/shell_window_launcher_item_controller.cc:127: //
gets activated.
nit: 'then gets activated later'?

[oshima, 2013-01-31 20:37:41]

https://codereview.chromium.org/12093075/diff/2003/ash/launcher/launcher_util.h
File ash/launcher/launcher_util.h (right):

https://codereview.chromium.org/12093075/diff/2003/ash/launcher/launcher_util...
ash/launcher/launcher_util.h:30: const ui::Event& event);
On 2013/01/31 20:23:40, stevenjb (chromium) wrote:
> nit: I would consider either:
> a) require maybe_panel be a panel and remove the if/maybe

I intentionally included if/panel logic so that ash_unittests can
test the logic that is used by chrome. I tried to write another test
on chrome side, but it turns out that it's more complicated than that.
I agree that this is a bit ugly, but a compromise to increase the coverage.

I'm planning to clean up this once new launcher is completed though.

> b) make this more generic, like 'MaybeMoveToEventRoot' with a note in the
> comment that currently this only affects panels.
Clicking other items (apps, browser window) shouldn't move the window,
(plus, I'll most likely remove this when I clean up), so I'd like to keep it as
is.

https://codereview.chromium.org/12093075/diff/2003/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/shell_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/12093075/diff/2003/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/shell_window_launcher_item_controller.cc:127: //
gets activated.
On 2013/01/31 20:23:40, stevenjb (chromium) wrote:
> nit: 'then gets activated later'?

changed to "then gets activated in |RestoreOrShow| before.

[oshima, 2013-01-31 20:38:21]

sky for OWNERS approval.

It touches a lot of files, but changes are small.

[sky, 2013-01-31 22:31:44]

https://codereview.chromium.org/12093075/diff/2003/ash/launcher/launcher_util.h
File ash/launcher/launcher_util.h (right):

https://codereview.chromium.org/12093075/diff/2003/ash/launcher/launcher_util...
ash/launcher/launcher_util.h:26: // Move the maybe_panel to the root window
where the |event| occured if
|maybe_panel|

https://codereview.chromium.org/12093075/diff/17001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/shell_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/12093075/diff/17001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/shell_window_launcher_item_controller.cc:124:
ash::launcher::MoveToEventRootIfPanel(first_window->GetNativeWindow(),
Is it possible to uplevel this to LauncherView?

[sky, 2013-01-31 22:56:02]

LGTM

[oshima, 2013-01-31 23:16:53]

https://codereview.chromium.org/12093075/diff/2003/ash/launcher/launcher_util.h
File ash/launcher/launcher_util.h (right):

https://codereview.chromium.org/12093075/diff/2003/ash/launcher/launcher_util...
ash/launcher/launcher_util.h:26: // Move the maybe_panel to the root window
where the |event| occured if
On 2013/01/31 22:31:45, sky wrote:
> |maybe_panel|

Done.

https://codereview.chromium.org/12093075/diff/17001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/shell_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/12093075/diff/17001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/shell_window_launcher_item_controller.cc:124:
ash::launcher::MoveToEventRootIfPanel(first_window->GetNativeWindow(),
On 2013/01/31 22:31:45, sky wrote:
> Is it possible to uplevel this to LauncherView?

Note: We discussed another approach offline, which is
to add LauncherDelegate::GetWindowByID() instead of adding event to ItemClicked.

With launcher++, there will be multiple windows associated with one launcher
item,
so we agreed to proceed with the original design.

[commit-bot: I haz the power, 2013-01-31 23:37:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/12093075/6009

[commit-bot: I haz the power, 2013-02-01 05:10:30]

Change committed as 180080