mus: Use the new drag API to implement tab dragging in chrome.

mus: Use the new drag API to implement tab dragging in chrome.

BUG=622900, 622575
Committed: https://crrev.com/6ed29f61913c5429093ff5b22667a15c425914b4
Cr-Commit-Position: refs/heads/master@{#404473}

[sky, 2016-06-24 19:42:01]

sky@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2016-06-24 19:42:02]

I started on this and then realized I have questions about the other view. So, I
stopped part way through.

https://codereview.chromium.org/2099513003/diff/20001/ash/mus/frame/move_even...
File ash/mus/frame/move_event_handler.cc (right):

https://codereview.chromium.org/2099513003/diff/20001/ash/mus/frame/move_even...
ash/mus/frame/move_event_handler.cc:20: base::LazyInstance<std::map<WmWindow*,
MoveEventHandler*>> g_live_handlers =
Use a local property on the window to track the MoveEventHandler.

https://codereview.chromium.org/2099513003/diff/20001/ash/mus/frame/move_even...
ash/mus/frame/move_event_handler.cc:87: aura::client::WINDOW_MOVE_SOURCE_MOUSE,
Might the source be touch?

https://codereview.chromium.org/2099513003/diff/20001/ash/mus/frame/move_even...
File ash/mus/frame/move_event_handler.h (right):

https://codereview.chromium.org/2099513003/diff/20001/ash/mus/frame/move_even...
ash/mus/frame/move_event_handler.h:45: // Attempts to start a drag if one is
already not in progress. This passes
nit: 'already not' -> 'not alreay'

https://codereview.chromium.org/2099513003/diff/20001/ash/mus/frame/move_even...
ash/mus/frame/move_event_handler.h:46: // the call to the underlying
WmToplevelWindowEventHandler.
document what end_closure is.

[Elliot Glaysher, 2016-06-29 20:45:10]

Description was changed from

==========
mus: Use the new drag API to implement tab dragging in chrome.

BUG=622900
==========

to

==========
mus: Use the new drag API to implement tab dragging in chrome.

BUG=622900,622575
==========

[Elliot Glaysher, 2016-06-29 22:41:57]

https://codereview.chromium.org/2099513003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder_mus.cc (right):

https://codereview.chromium.org/2099513003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder_mus.cc:20: // TODO(erg): Needs to
deal with stacking order here.
This is where I'm a little bit lost. While mus::Window has methods to reorder
itself, there doesn't appear to be a way to sort mus::Windows by screen
ordering.

[sky, 2016-06-30 00:18:32]

https://codereview.chromium.org/2099513003/diff/100001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2099513003/diff/100001/ash/mus/window_manager...
ash/mus/window_manager.cc:239: WmWindowMus* child_window =
WmWindowMus::Get(window);
DCHECK there isn't a drag in progress?

https://codereview.chromium.org/2099513003/diff/100001/ash/mus/window_manager...
ash/mus/window_manager.cc:240: MoveEventHandler* handler =
MoveEventHandler::GetForWindow(child_window);
Fail if there is no handler. This would happen if the wm is told to drag a
window it hasn't installed a frame for, which is possible.

https://codereview.chromium.org/2099513003/diff/100001/ash/mus/window_manager...
ash/mus/window_manager.cc:250: MoveEventHandler* handler =
MoveEventHandler::GetForWindow(child_window);
Same comment here about checking handler.

https://codereview.chromium.org/2099513003/diff/100001/ash/mus/window_manager...
ash/mus/window_manager.cc:251: handler->RevertDrag();
What happens if RevertDrag is called and there isn't a drag in progress? I want
to make sure we don't crash.

https://codereview.chromium.org/2099513003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder_mus.cc (right):

https://codereview.chromium.org/2099513003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder_mus.cc:20: // TODO(erg): Needs to
deal with stacking order here.
On 2016/06/29 22:41:57, Elliot Glaysher wrote:
> This is where I'm a little bit lost. While mus::Window has methods to reorder
> itself, there doesn't appear to be a way to sort mus::Windows by screen
> ordering.

The other TODO you should add is that this only handles windows created by
chrome. We'll have to decide what the right path is going forward for this, by
which I mean I'm not quite sure yet. I see two options:
. have a way for chrome to see all windows.
. add an api to mus for querying what this code needs to do.
Have to ponder what the right thing is.

https://codereview.chromium.org/2099513003/diff/100001/ui/views/mus/native_wi...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2099513003/diff/100001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus.cc:453: Widget* widget =
NativeWidgetMus::GetWidgetForWindow(window);
How come NativeWidgetMus::OnBoundsChanged doesn't cover this?

[Elliot Glaysher, 2016-06-30 19:48:25]

https://codereview.chromium.org/2099513003/diff/100001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2099513003/diff/100001/ash/mus/window_manager...
ash/mus/window_manager.cc:251: handler->RevertDrag();
On 2016/06/30 00:18:31, sky wrote:
> What happens if RevertDrag is called and there isn't a drag in progress? I
want
> to make sure we don't crash.

It doesn't. WmToplevelWindowEventHandler::CompleteDrag() just returns false if
there isn't a drag in progress.

https://codereview.chromium.org/2099513003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder_mus.cc (right):

https://codereview.chromium.org/2099513003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder_mus.cc:20: // TODO(erg): Needs to
deal with stacking order here.
On 2016/06/30 00:18:31, sky wrote:
> On 2016/06/29 22:41:57, Elliot Glaysher wrote:
> > This is where I'm a little bit lost. While mus::Window has methods to
reorder
> > itself, there doesn't appear to be a way to sort mus::Windows by screen
> > ordering.
> 
> The other TODO you should add is that this only handles windows created by
> chrome. We'll have to decide what the right path is going forward for this, by
> which I mean I'm not quite sure yet. I see two options:
> . have a way for chrome to see all windows.
> . add an api to mus for querying what this code needs to do.
> Have to ponder what the right thing is.

I don't believe that's an issue.

WindowFinder::GetLocalProcessWindowAtPoint() has one caller, and that's in the
tab dragging code, looking specifically for chrome windows of the current
process that you can drag a tab into (and ignoring the windows |ignore| which
are the window being dragged).

https://codereview.chromium.org/2099513003/diff/100001/ui/views/mus/native_wi...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2099513003/diff/100001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus.cc:453: Widget* widget =
NativeWidgetMus::GetWidgetForWindow(window);
On 2016/06/30 00:18:31, sky wrote:
> How come NativeWidgetMus::OnBoundsChanged doesn't cover this?

Because it doesn't get called in response to this. (And shouldn't.)

During MusWindowObserver::OnWindowBoundsChanging(), we tell
WindowTreeHostPlatform::SetBounds() the new bounds. That goes to
StubWindow::SetBounds(). That goes to WindowTreeHostPlatform::OnBoundsChanged(),
which dispatchs to aura::WindowTreeHostObserver.

However, pulling on this thread, this chunk is still wrong. I've replaced this
with making NWMus implement aura::WTHO. Looking at DesktopNativeWidgetAura, we
should have a OnHostMoved() method calling Widget::OnNativeWidgetMove() instead.

[sky, 2016-06-30 23:23:54]

https://codereview.chromium.org/2099513003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder_mus.cc (right):

https://codereview.chromium.org/2099513003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder_mus.cc:20: // TODO(erg): Needs to
deal with stacking order here.
On 2016/06/30 19:48:25, Elliot Glaysher wrote:
> On 2016/06/30 00:18:31, sky wrote:
> > On 2016/06/29 22:41:57, Elliot Glaysher wrote:
> > > This is where I'm a little bit lost. While mus::Window has methods to
> reorder
> > > itself, there doesn't appear to be a way to sort mus::Windows by screen
> > > ordering.
> > 
> > The other TODO you should add is that this only handles windows created by
> > chrome. We'll have to decide what the right path is going forward for this,
by
> > which I mean I'm not quite sure yet. I see two options:
> > . have a way for chrome to see all windows.
> > . add an api to mus for querying what this code needs to do.
> > Have to ponder what the right thing is.
> 
> I don't believe that's an issue.
> 
> WindowFinder::GetLocalProcessWindowAtPoint() has one caller, and that's in the
> tab dragging code, looking specifically for chrome windows of the current
> process that you can drag a tab into (and ignoring the windows |ignore| which
> are the window being dragged).

It would be nice if WindowFinder::GetLocalProcessWindowAtPoint had some docs,
eh? The expectation is that GetLocalProcessWindowAtPoint() only returns a local
window if that window is not obscured by another window. TO do otherwise means
chrome will try to attach to a window that isn't necessarily visible.

https://codereview.chromium.org/2099513003/diff/120001/ui/views/mus/native_wi...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2099513003/diff/120001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus.cc:1376: void
NativeWidgetMus::OnHostResized(const aura::WindowTreeHost* host) {
How come both this and NativeWidgetMus::OnBoundsChanged() call to
OnNativeWidgetSize/Move? Shouldn't only one be necessary?

[Elliot Glaysher, 2016-07-01 00:09:58]

https://codereview.chromium.org/2099513003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder_mus.cc (right):

https://codereview.chromium.org/2099513003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder_mus.cc:20: // TODO(erg): Needs to
deal with stacking order here.
On 2016/06/30 23:23:54, sky wrote:
> On 2016/06/30 19:48:25, Elliot Glaysher wrote:
> > On 2016/06/30 00:18:31, sky wrote:
> > > On 2016/06/29 22:41:57, Elliot Glaysher wrote:
> > > > This is where I'm a little bit lost. While mus::Window has methods to
> > reorder
> > > > itself, there doesn't appear to be a way to sort mus::Windows by screen
> > > > ordering.
> > > 
> > > The other TODO you should add is that this only handles windows created by
> > > chrome. We'll have to decide what the right path is going forward for
this,
> by
> > > which I mean I'm not quite sure yet. I see two options:
> > > . have a way for chrome to see all windows.
> > > . add an api to mus for querying what this code needs to do.
> > > Have to ponder what the right thing is.
> > 
> > I don't believe that's an issue.
> > 
> > WindowFinder::GetLocalProcessWindowAtPoint() has one caller, and that's in
the
> > tab dragging code, looking specifically for chrome windows of the current
> > process that you can drag a tab into (and ignoring the windows |ignore|
which
> > are the window being dragged).
> 
> It would be nice if WindowFinder::GetLocalProcessWindowAtPoint had some docs,
> eh? The expectation is that GetLocalProcessWindowAtPoint() only returns a
local
> window if that window is not obscured by another window. TO do otherwise means
> chrome will try to attach to a window that isn't necessarily visible.

Added what I've learned by reverse engineering to the toplevel, previously
undocumented, window_finder.h.

https://codereview.chromium.org/2099513003/diff/120001/ui/views/mus/native_wi...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2099513003/diff/120001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus.cc:1376: void
NativeWidgetMus::OnHostResized(const aura::WindowTreeHost* host) {
On 2016/06/30 23:23:54, sky wrote:
> How come both this and NativeWidgetMus::OnBoundsChanged() call to
> OnNativeWidgetSize/Move? Shouldn't only one be necessary?

I changed it, didn't notice any problems locally and I'm throwing it at the
trybots to see if it breaks anything.

(I'll also note that NativeWidgetMus used to similar like NativeWidgetAura,
which didn't implement much of the WindowTreeObserver interface and instead used
OnBoundsChanged() and friends for messaging. It now is more like
DesktopNativeWidgetAura, which uses OnHost{Resized,Moved}() and has an empty
OnBOundsChanged() implementation.)

[sky, 2016-07-01 16:01:15]

sky@chromium.org changed reviewers:
+ sadrul@chromium.org

[sky, 2016-07-01 16:01:16]

+sadrul for the changes to NativeWidgetMus::OnBoundsChanged() as I feel we've
circled around a bit on this particular code and want to make sure he doesn't
see any problems.

LGTM

https://codereview.chromium.org/2099513003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder.h (right):

https://codereview.chromium.org/2099513003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder.h:28: // |ignore|, which contain
windows such as the tab being dragged right now.
Can you also add the point you weren't aware of? Perhaps something like:
If a window from another app is the topmost window at |screen_point|, null is
returned.

https://codereview.chromium.org/2099513003/diff/160001/ui/views/mus/native_wi...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2099513003/diff/160001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus.cc:1261: const gfx::Rect& new_bounds) {}
I know if I saw this I would wonder why there is no implementation. Please add a
comment as to why this does nothing (something like OnHostResized/OnHostMoved
handle the logic).

[sadrul, 2016-07-04 20:11:38]

https://codereview.chromium.org/2099513003/diff/180001/ui/views/mus/native_wi...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2099513003/diff/180001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus.cc:1368: content_->SetBounds(new_bounds);
Do you need to SetBounds() here? It should already have happened (although
ContentWindowLayoutManager::OnWindowResized() above probably needs to reset the
origin). Maybe leave a DCHECK_EQ instead?

[Elliot Glaysher, 2016-07-07 19:12:06]

https://codereview.chromium.org/2099513003/diff/180001/ui/views/mus/native_wi...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2099513003/diff/180001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus.cc:1368: content_->SetBounds(new_bounds);
On 2016/07/04 20:11:38, sadrul wrote:
> Do you need to SetBounds() here? It should already have happened (although
> ContentWindowLayoutManager::OnWindowResized() above probably needs to reset
the
> origin). Maybe leave a DCHECK_EQ instead?

You're right that it isn't needed; I removed the call to SetBounds() and it kept
working. All tests pass.

(I'm unsure what you want in the DCHECK_EQ.)

[sadrul, 2016-07-08 00:23:07]

lgtm

[sadrul, 2016-07-08 00:24:05]

One small nit

https://codereview.chromium.org/2099513003/diff/200001/ui/views/mus/native_wi...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2099513003/diff/200001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus.cc:1369:
native_widget_delegate_->OnNativeWidgetSizeChanged(new_bounds.size());
No reason to create |new_bounds| anymore.

[Elliot Glaysher, 2016-07-08 17:29:38]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2016-07-08 17:29:39]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2099513003/#ps220001
(title: "sadrul nit")

[commit-bot: I haz the power, 2016-07-08 17:30:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 18:58:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-08 18:58:26]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Elliot Glaysher, 2016-07-08 19:10:22]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2016-07-08 19:10:23]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2099513003/#ps240001
(title: "Rebase to tot")

[commit-bot: I haz the power, 2016-07-08 19:11:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 19:16:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-08 19:16:18]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elliot Glaysher, 2016-07-08 19:27:54]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2016-07-08 19:28:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 20:10:52]

Description was changed from

==========
mus: Use the new drag API to implement tab dragging in chrome.

BUG=622900,622575
==========

to

==========
mus: Use the new drag API to implement tab dragging in chrome.

BUG=622900,622575
==========

[commit-bot: I haz the power, 2016-07-08 20:10:53]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-07-08 20:11:07]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 20:12:00]

Description was changed from

==========
mus: Use the new drag API to implement tab dragging in chrome.

BUG=622900,622575
==========

to

==========
mus: Use the new drag API to implement tab dragging in chrome.

BUG=622900,622575

Committed: https://crrev.com/6ed29f61913c5429093ff5b22667a15c425914b4
Cr-Commit-Position: refs/heads/master@{#404473}
==========

[commit-bot: I haz the power, 2016-07-08 20:12:02]

Patchset 13 (id:??) landed as
https://crrev.com/6ed29f61913c5429093ff5b22667a15c425914b4
Cr-Commit-Position: refs/heads/master@{#404473}