Stack panels so that part of them will always be visible.

Stack panels so that part of them will always be visible.

BUG=125102
TEST=aura_shell_unittests --gtest_filter=*Panel* --aura-panels


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134195

[dcheng, 2012-04-25 01:07:40]

Initial pass at stacking panels according to the UI mocks.

There's two things that bother me about this patch:
1) I'm pretty sure calling UpdateStacking() from the panel window event filter
is wrong. I looked around, but I couldn't find anything that's like a
"FocusObserver"; is there something I can use that's already built, or would it
be OK for me to add a FocusObserver and hook it in to FocusManager?

2) The launcher icons disappear after I call the Stack* methods. Is this a bug
in the launcher or my code?

[sky, 2012-04-25 03:45:30]

You can track the active window by listening for changes to the
kRootWindowActiveWindowKey property on the RootWindow.

As to a launcher bug, you may have been at a bad sync point. Xiyuan landed a
patch that broke some stuff, but I believe has been since fixed (>=133853).

[dcheng, 2012-04-25 23:12:15]

Thanks for the tip about WindowObserver. I've updated the code to use that, and
added several test cases to validate behavior.

[sky, 2012-04-26 04:30:31]

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager.cc
File ash/wm/panel_layout_manager.cc (right):

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager...
ash/wm/panel_layout_manager.cc:230: // We want to to stack the panels like a
deck of cards:
Be warned, we're going to make the launcher support vertical tiling soon. This
code will need to be updated then.

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager...
ash/wm/panel_layout_manager.cc:237: // It's a little hacky...
You can actually get this information from the LauncherDelegate and model. I'm
not opposed to exposing a method to get the set of windows shown from the
launcher.

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager...
ash/wm/panel_layout_manager.cc:245: if (!active_panel) {
Since this can early return, make it first.

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager...
ash/wm/panel_layout_manager.cc:248: active_panel = last_active_panel_;
Should you return if last_active_panel_ is NULL?

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager...
File ash/wm/panel_layout_manager_unittest.cc (right):

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager...
ash/wm/panel_layout_manager_unittest.cc:77: if
(!CommandLine::ForCurrentProcess()->HasSwitch(switches::kAuraPanelManager))
Doesn't this mean none of these tests are going to be run? You should force this
to be true in setup so that all your tests run.

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager...
ash/wm/panel_layout_manager_unittest.cc:85: LOG(ERROR) << "Running tests!";
remove this.

[dcheng, 2012-04-26 06:30:59]

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager.cc
File ash/wm/panel_layout_manager.cc (right):

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager...
ash/wm/panel_layout_manager.cc:230: // We want to to stack the panels like a
deck of cards:
On 2012/04/26 04:30:31, sky wrote:
> Be warned, we're going to make the launcher support vertical tiling soon. This
> code will need to be updated then.

Doh. Thanks for the heads up. We're definitely going to have to think about how
to make panels appear decent when oriented vertically... the current design
doesn't really provide for that.

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager...
ash/wm/panel_layout_manager.cc:237: // It's a little hacky...
On 2012/04/26 04:30:31, sky wrote:
> You can actually get this information from the LauncherDelegate and model. I'm
> not opposed to exposing a method to get the set of windows shown from the
> launcher.

I've updated this comment to better reflect the current state of affairs. Since
we want dragged panels to trigger this stacking even though it doesn't affect
the launcher icon positions, we actually do need to use the x coordinate.

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager...
ash/wm/panel_layout_manager.cc:245: if (!active_panel) {
On 2012/04/26 04:30:31, sky wrote:
> Since this can early return, make it first.

Done.

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager...
ash/wm/panel_layout_manager.cc:248: active_panel = last_active_panel_;
On 2012/04/26 04:30:31, sky wrote:
> Should you return if last_active_panel_ is NULL?

No, this can happen if this is the first time a panel has been focused.

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager...
File ash/wm/panel_layout_manager_unittest.cc (right):

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager...
ash/wm/panel_layout_manager_unittest.cc:77: if
(!CommandLine::ForCurrentProcess()->HasSwitch(switches::kAuraPanelManager))
On 2012/04/26 04:30:31, sky wrote:
> Doesn't this mean none of these tests are going to be run? You should force
this
> to be true in setup so that all your tests run.

I've been under the impression that this wouldn't work for some reason... but it
works.

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager...
ash/wm/panel_layout_manager_unittest.cc:85: LOG(ERROR) << "Running tests!";
On 2012/04/26 04:30:31, sky wrote:
> remove this.

Done.

[sky, 2012-04-26 16:05:10]

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager.cc
File ash/wm/panel_layout_manager.cc (right):

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager...
ash/wm/panel_layout_manager.cc:248: active_panel = last_active_panel_;
On 2012/04/26 06:30:59, dcheng wrote:
> On 2012/04/26 04:30:31, sky wrote:
> > Should you return if last_active_panel_ is NULL?
> 
> No, this can happen if this is the first time a panel has been focused.

Can't it also happen if last_active_panel_ is closed? And if it is NULL, won't
it cause problems below (269 in particular, not sure if we ignore NULL).

http://codereview.chromium.org/10173016/diff/19001/ash/wm/panel_layout_manage...
File ash/wm/panel_layout_manager_unittest.cc (right):

http://codereview.chromium.org/10173016/diff/19001/ash/wm/panel_layout_manage...
ash/wm/panel_layout_manager_unittest.cc:49: CHECK(launcher_delegate);
Don't CHECK in tests (it would make all the unit tests crash). Instead return
NULL and ASSERT on the result.

[dcheng, 2012-04-26 18:10:25]

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager.cc
File ash/wm/panel_layout_manager.cc (right):

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager...
ash/wm/panel_layout_manager.cc:248: active_panel = last_active_panel_;
On 2012/04/26 16:05:10, sky wrote:
> On 2012/04/26 06:30:59, dcheng wrote:
> > On 2012/04/26 04:30:31, sky wrote:
> > > Should you return if last_active_panel_ is NULL?
> > 
> > No, this can happen if this is the first time a panel has been focused.
> 
> Can't it also happen if last_active_panel_ is closed? And if it is NULL, won't
> it cause problems below (269 in particular, not sure if we ignore NULL).

Good point--it didn't occur to me since there's no easy way to create a panel
that's not focused by default. However, shouldn't the return on line 247 be
handling that already?

http://codereview.chromium.org/10173016/diff/19001/ash/wm/panel_layout_manage...
File ash/wm/panel_layout_manager_unittest.cc (right):

http://codereview.chromium.org/10173016/diff/19001/ash/wm/panel_layout_manage...
ash/wm/panel_layout_manager_unittest.cc:49: CHECK(launcher_delegate);
On 2012/04/26 16:05:10, sky wrote:
> Don't CHECK in tests (it would make all the unit tests crash). Instead return
> NULL and ASSERT on the result.

I feel like this makes the unit test look really clunky and verbose. Most other
test code that I saw doesn't ever assert that the result of CreateTestWindow is
non-NULL, while this test would have to read something like:
scoped_ptr<Window> w(CreatePanelWindow());
ASSERT_TRUE(w.get());
w->Foo();

Would you be OK with removing the assert for TestLauncherDelegate completely,
since that would mean something is very wrong anyway?

[sky, 2012-04-26 19:06:59]

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager.cc
File ash/wm/panel_layout_manager.cc (right):

http://codereview.chromium.org/10173016/diff/7002/ash/wm/panel_layout_manager...
ash/wm/panel_layout_manager.cc:248: active_panel = last_active_panel_;
On 2012/04/26 18:10:25, dcheng wrote:
> On 2012/04/26 16:05:10, sky wrote:
> > On 2012/04/26 06:30:59, dcheng wrote:
> > > On 2012/04/26 04:30:31, sky wrote:
> > > > Should you return if last_active_panel_ is NULL?
> > > 
> > > No, this can happen if this is the first time a panel has been focused.
> > 
> > Can't it also happen if last_active_panel_ is closed? And if it is NULL,
won't
> > it cause problems below (269 in particular, not sure if we ignore NULL).
> 
> Good point--it didn't occur to me since there's no easy way to create a panel
> that's not focused by default. However, shouldn't the return on line 247 be
> handling that already?

Of course. You're right.

http://codereview.chromium.org/10173016/diff/19001/ash/wm/panel_layout_manage...
File ash/wm/panel_layout_manager_unittest.cc (right):

http://codereview.chromium.org/10173016/diff/19001/ash/wm/panel_layout_manage...
ash/wm/panel_layout_manager_unittest.cc:49: CHECK(launcher_delegate);
On 2012/04/26 18:10:25, dcheng wrote:
> On 2012/04/26 16:05:10, sky wrote:
> > Don't CHECK in tests (it would make all the unit tests crash). Instead
return
> > NULL and ASSERT on the result.
> 
> I feel like this makes the unit test look really clunky and verbose. Most
other
> test code that I saw doesn't ever assert that the result of CreateTestWindow
is
> non-NULL, while this test would have to read something like:
> scoped_ptr<Window> w(CreatePanelWindow());
> ASSERT_TRUE(w.get());
> w->Foo();
> Would you be OK with removing the assert for TestLauncherDelegate completely,
> since that would mean something is very wrong anyway?

That's an important assert to keep. But, you can move it to SetUp.

[dcheng, 2012-04-26 23:00:59]

http://codereview.chromium.org/10173016/diff/19001/ash/wm/panel_layout_manage...
File ash/wm/panel_layout_manager_unittest.cc (right):

http://codereview.chromium.org/10173016/diff/19001/ash/wm/panel_layout_manage...
ash/wm/panel_layout_manager_unittest.cc:49: CHECK(launcher_delegate);
On 2012/04/26 19:06:59, sky wrote:
> On 2012/04/26 18:10:25, dcheng wrote:
> > On 2012/04/26 16:05:10, sky wrote:
> > > Don't CHECK in tests (it would make all the unit tests crash). Instead
> return
> > > NULL and ASSERT on the result.
> > 
> > I feel like this makes the unit test look really clunky and verbose. Most
> other
> > test code that I saw doesn't ever assert that the result of CreateTestWindow
> is
> > non-NULL, while this test would have to read something like:
> > scoped_ptr<Window> w(CreatePanelWindow());
> > ASSERT_TRUE(w.get());
> > w->Foo();
> > Would you be OK with removing the assert for TestLauncherDelegate
completely,
> > since that would mean something is very wrong anyway?
> 
> That's an important assert to keep. But, you can move it to SetUp.

Done.

[sky, 2012-04-26 23:04:32]

LGTM

[commit-bot: I haz the power, 2012-04-26 23:08:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/10173016/22001

[commit-bot: I haz the power, 2012-04-27 00:55:43]

Change committed as 134195