Do not show auto-hidden shelf when dragging a window

Do not show auto-hidden shelf when dragging a window

BUG=292238
TEST=ash_unittests --gtest_filter=*ShelfLayoutManagerTest*

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

[varkha, 2013-09-16 15:21:09]

The shelf is getting exposed when dragging panels or docked windows after the CL
https://chromiumcodereview.appspot.com/22121002. The root cause is probably
MruWindowTracker::BuildWindowList not returning any windows that are not in
default or top-most containers such as panels or windows during the drag that
get reparented.
I guess with overview mode work the panels and docked windows will get exposed
to the MruWindowTracker and that could eliminate the need in this patch.

[pkotwicz, 2013-09-16 20:39:20]

LGTM with an improved comment

[flackr, 2013-09-16 20:55:06]

https://codereview.chromium.org/23591049/diff/2001/ash/shelf/shelf_layout_man...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/23591049/diff/2001/ash/shelf/shelf_layout_man...
ash/shelf/shelf_layout_manager.cc:1003: return SHELF_AUTO_HIDE_HIDDEN;
Won't this only work for mouse drags and not touch? I'm also concerned that
hiding when in a mouse drag may be the wrong thing to do if there are actually
no windows. I.e. wouldn't it hide the shelf for the duration of the drag when
starting a mouse drag with no windows?

[flackr, 2013-09-16 20:57:49]

This should have a test.

[varkha, 2013-09-17 04:36:43]

Would it be wiser to wait till the docked container is used in MruWindowTracker?
This seems to be the only solution that would not have side effects.

https://codereview.chromium.org/23591049/diff/2001/ash/shelf/shelf_layout_man...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/23591049/diff/2001/ash/shelf/shelf_layout_man...
ash/shelf/shelf_layout_manager.cc:1003: return SHELF_AUTO_HIDE_HIDDEN;
On 2013/09/16 20:55:06, flackr wrote:
> Won't this only work for mouse drags and not touch? I'm also concerned that
> hiding when in a mouse drag may be the wrong thing to do if there are actually
> no windows. I.e. wouldn't it hide the shelf for the duration of the drag when
> starting a mouse drag with no windows?

Correct on both accounts. Dragging on empty desktop only hides when touching the
screen edge.

[flackr, 2013-09-17 16:03:44]

On 2013/09/17 04:36:43, varkha wrote:
> Would it be wiser to wait till the docked container is used in
MruWindowTracker?
> This seems to be the only solution that would not have side effects.

Not the only solution, but certainly the most correct. This would probably be a
wise call.
 
>
https://codereview.chromium.org/23591049/diff/2001/ash/shelf/shelf_layout_man...
> File ash/shelf/shelf_layout_manager.cc (right):
> 
>
https://codereview.chromium.org/23591049/diff/2001/ash/shelf/shelf_layout_man...
> ash/shelf/shelf_layout_manager.cc:1003: return SHELF_AUTO_HIDE_HIDDEN;
> On 2013/09/16 20:55:06, flackr wrote:
> > Won't this only work for mouse drags and not touch? I'm also concerned that
> > hiding when in a mouse drag may be the wrong thing to do if there are
actually
> > no windows. I.e. wouldn't it hide the shelf for the duration of the drag
when
> > starting a mouse drag with no windows?
> 
> Correct on both accounts. Dragging on empty desktop only hides when touching
the
> screen edge.

[varkha, 2014-01-03 21:45:07]

flackr@, Can you please comment on this? I have tried just adding the docked
windows to the MRU tracker, but this causes interesting side effects with the
overview mode. I think this is enough to address the bug while also allowing
overview mode to function while dragging a window.

[varkha, 2014-01-06 21:10:45]

oshima@, Please take a look (flackr@ is away for a bit). This could be improved
in future by allowing access to docked windows while in overview mode but
addresses the immediate bug. Thanks!

[oshima, 2014-01-08 01:33:02]

change lg. can you add a test case?

https://codereview.chromium.org/23591049/diff/12001/ash/wm/mru_window_tracker.cc
File ash/wm/mru_window_tracker.cc (right):

https://codereview.chromium.org/23591049/diff/12001/ash/wm/mru_window_tracker...
ash/wm/mru_window_tracker.cc:36: // drag to |windows|.
// Adds windows being dragged in the docked containers to |windows| list.

https://codereview.chromium.org/23591049/diff/12001/ash/wm/mru_window_tracker...
ash/wm/mru_window_tracker.cc:41: const MruWindowTracker::WindowList&
children(container->children());
nit: I prefer = instead of ()

[varkha, 2014-01-09 05:14:20]

PTAL. I have verified that the test would fail without the change to the
MruWindowTracker (when invoked with docking enabled).

https://codereview.chromium.org/23591049/diff/12001/ash/wm/mru_window_tracker.cc
File ash/wm/mru_window_tracker.cc (right):

https://codereview.chromium.org/23591049/diff/12001/ash/wm/mru_window_tracker...
ash/wm/mru_window_tracker.cc:36: // drag to |windows|.
On 2014/01/08 01:33:03, oshima wrote:
> // Adds windows being dragged in the docked containers to |windows| list.

Done.

https://codereview.chromium.org/23591049/diff/12001/ash/wm/mru_window_tracker...
ash/wm/mru_window_tracker.cc:41: const MruWindowTracker::WindowList&
children(container->children());
On 2014/01/08 01:33:03, oshima wrote:
> nit: I prefer = instead of ()

Done.

[oshima, 2014-01-09 18:10:16]

lgtm

[commit-bot: I haz the power, 2014-01-09 18:50:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/23591049/92001

[commit-bot: I haz the power, 2014-01-09 20:24:25]

Change committed as 243964