Fixes bug where FocusController would stack a layer directly above a

Fixes bug where FocusController would stack a layer directly above a
window rather than above the windows last transient parent. This
resulted in the layers getting out of order and all sorts of future
problems.

BUG=173706
TEST=covered by unit test, see bug
R=ben@chromium.org

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

[stevenjb, 2013-03-22 18:28:15]

Thanks a bunch for taking this on. I think I need to get up to speed on
transient windows and their relationship to other windows.

Apologies for the review spam; these tests are very helpful for improving my
understanding, so the more comments the better, but this is partly just me
trying to understand the code better.

Cheers.

https://codereview.chromium.org/13008024/diff/1/ash/wm/workspace/workspace_ma...
File ash/wm/workspace/workspace_manager_unittest.cc (right):

https://codereview.chromium.org/13008024/diff/1/ash/wm/workspace/workspace_ma...
ash/wm/workspace/workspace_manager_unittest.cc:1498: generator.MoveMouseTo(5,
5);
Could you document why the initial MoveMouseTo?

https://codereview.chromium.org/13008024/diff/1/ash/wm/workspace/workspace_ma...
ash/wm/workspace/workspace_manager_unittest.cc:1542:
app->SetProperty(aura::client::kShowStateKey, ui::SHOW_STATE_MINIMIZED);
Test that focus does go to the browser here? I know that's not what we are
testing, but it seems like it would help document the conditions for the test
setup better than just the comment.

https://codereview.chromium.org/13008024/diff/1/ash/wm/workspace/workspace_ma...
ash/wm/workspace/workspace_manager_unittest.cc:1546:
browser->SetProperty(aura::client::kShowStateKey, ui::SHOW_STATE_MINIMIZED);
Mostly out of curiosity, where does focus go here? I assume neither window
should be focused now?

https://codereview.chromium.org/13008024/diff/1/ash/wm/workspace/workspace_ma...
ash/wm/workspace/workspace_manager_unittest.cc:1549: // Show the browser (which
should restore it).
But not focus it, correct?

https://codereview.chromium.org/13008024/diff/1/ash/wm/workspace/workspace_ma...
ash/wm/workspace/workspace_manager_unittest.cc:1554:
ash::wm::ActivateWindow(browser.get());
Until here, right?

https://codereview.chromium.org/13008024/diff/1/ash/wm/workspace/workspace_ma...
ash/wm/workspace/workspace_manager_unittest.cc:1559:
app->SetProperty(aura::client::kShowStateKey, ui::SHOW_STATE_NORMAL);
Should we change the App code to use Show() instead of Restore()? It seems like
they should be consistent. Making that change doesn't seem to affect the bug, so
it seems like it should be consistent in the test (or tested separately)?

[sky, 2013-03-22 19:30:15]

New patch uploaded

https://codereview.chromium.org/13008024/diff/1/ash/wm/workspace/workspace_ma...
File ash/wm/workspace/workspace_manager_unittest.cc (right):

https://codereview.chromium.org/13008024/diff/1/ash/wm/workspace/workspace_ma...
ash/wm/workspace/workspace_manager_unittest.cc:1498: generator.MoveMouseTo(5,
5);
On 2013/03/22 18:28:15, stevenjb (chromium) wrote:
> Could you document why the initial MoveMouseTo?

That's not necessary and left over from another test. I'll nuke it.

https://codereview.chromium.org/13008024/diff/1/ash/wm/workspace/workspace_ma...
ash/wm/workspace/workspace_manager_unittest.cc:1542:
app->SetProperty(aura::client::kShowStateKey, ui::SHOW_STATE_MINIMIZED);
On 2013/03/22 18:28:15, stevenjb (chromium) wrote:
> Test that focus does go to the browser here? I know that's not what we are
> testing, but it seems like it would help document the conditions for the test
> setup better than just the comment.

Sure.

https://codereview.chromium.org/13008024/diff/1/ash/wm/workspace/workspace_ma...
ash/wm/workspace/workspace_manager_unittest.cc:1546:
browser->SetProperty(aura::client::kShowStateKey, ui::SHOW_STATE_MINIMIZED);
On 2013/03/22 18:28:15, stevenjb (chromium) wrote:
> Mostly out of curiosity, where does focus go here? I assume neither window
> should be focused now?

I believe it goes no where, but I think ideally we want the launcher to get
focus in this case.

https://codereview.chromium.org/13008024/diff/1/ash/wm/workspace/workspace_ma...
ash/wm/workspace/workspace_manager_unittest.cc:1549: // Show the browser (which
should restore it).
On 2013/03/22 18:28:15, stevenjb (chromium) wrote:
> But not focus it, correct?

Not sure there. The focus rules may implicitly focus it. I'm not going to
assert, but will below.

https://codereview.chromium.org/13008024/diff/1/ash/wm/workspace/workspace_ma...
ash/wm/workspace/workspace_manager_unittest.cc:1559:
app->SetProperty(aura::client::kShowStateKey, ui::SHOW_STATE_NORMAL);
On 2013/03/22 18:28:15, stevenjb (chromium) wrote:
> Should we change the App code to use Show() instead of Restore()? It seems
like
> they should be consistent. Making that change doesn't seem to affect the bug,
so
> it seems like it should be consistent in the test (or tested separately)?

Yes, but it won't make a difference for this particular test. That said,
changing the app code to use show should fix 179387. I'll add a comment to that
bug.

[Ben Goodger (Google), 2013-03-22 20:12:31]

lgtm for focus controller change.

[stevenjb, 2013-03-22 20:18:51]

lgtm

[commit-bot: I haz the power, 2013-03-22 20:36:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/13008024/6001

[commit-bot: I haz the power, 2013-03-23 01:31:46]

Retried try job too often on linux_chromeos for step(s) ash_unittests,
browser_tests, interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-03-25 15:30:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/13008024/28001

[commit-bot: I haz the power, 2013-03-25 17:04:21]

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

[commit-bot: I haz the power, 2013-03-25 18:26:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/13008024/28001

[sky, 2013-03-25 19:23:51]

Committed patchset #5 manually as r190457 (presubmit successful).