Use active window if on current workspace for fullscreen mode.

Use active window if on current workspace for fullscreen mode.

BUG=330566, 340647
TEST=WindowSelectorTest.FullscreenWindow,RootWindowControllerTest.GetWindowForFullscreenMode,RootWindowControllerTest.MultipleDisplaysGetWindowForFullscreenMode,PanelWindowResizerTest.AttachToSecondFullscreenDisplay
TEST=Open a panel and navigate a browser to an html5 fullscreen page, and fullscreen it. Alt+tab to the panel, should be able to see the panel.

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

[flackr, 2014-02-07 20:48:34]

PTAL, I considered a few approaches for this:
- using the active window or topmost if active was not on the current root
window which felt hacky and meant that activating a window on another root could
hide your panel if in front of a fullscreen window.
- tracking the last active window per root window controller which didn't work
well when that last active window changed root windows (i.e. dragged across
displays).

I couldn't just use topmost window because panels are always "above" regular
windows and so if panels were included in the search for the topmost window you
could never get into fullscreen mode if you had an open panel.

[oshima, 2014-02-10 19:21:06]

lgtm with nits

https://codereview.chromium.org/149493008/diff/1/ash/root_window_controller.h
File ash/root_window_controller.h (right):

https://codereview.chromium.org/149493008/diff/1/ash/root_window_controller.h...
ash/root_window_controller.h:118: }
can you group by method name instead?

https://codereview.chromium.org/149493008/diff/1/ash/root_window_controller_u...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/149493008/diff/1/ash/root_window_controller_u...
ash/root_window_controller_unittest.cc:491:
controllers[0]->GetWindowForFullscreenMode());
indent

https://codereview.chromium.org/149493008/diff/1/ash/root_window_controller_u...
ash/root_window_controller_unittest.cc:492: EXPECT_EQ(NULL,
controllers[1]->GetWindowForFullscreenMode());
can you also add scenario where w1 is activated?

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

https://codereview.chromium.org/149493008/diff/1/ash/wm/mru_window_tracker.cc...
ash/wm/mru_window_tracker.cc:143: return item->GetRootWindow() == root_window;
I'd prefer just looping for simple pointer comparison like like because it's
shorter and you can see everything in one place.

[oshima, 2014-02-10 19:21:08]

lgtm with nits

[flackr, 2014-02-10 21:45:27]

https://codereview.chromium.org/149493008/diff/1/ash/root_window_controller.h
File ash/root_window_controller.h (right):

https://codereview.chromium.org/149493008/diff/1/ash/root_window_controller.h...
ash/root_window_controller.h:118: }
On 2014/02/10 19:21:06, oshima wrote:
> can you group by method name instead? 

Done.

https://codereview.chromium.org/149493008/diff/1/ash/root_window_controller_u...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/149493008/diff/1/ash/root_window_controller_u...
ash/root_window_controller_unittest.cc:491:
controllers[0]->GetWindowForFullscreenMode());
On 2014/02/10 19:21:06, oshima wrote:
> indent

Done.

https://codereview.chromium.org/149493008/diff/1/ash/root_window_controller_u...
ash/root_window_controller_unittest.cc:492: EXPECT_EQ(NULL,
controllers[1]->GetWindowForFullscreenMode());
On 2014/02/10 19:21:06, oshima wrote:
> can you also add scenario where w1 is activated?

Done.

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

https://codereview.chromium.org/149493008/diff/1/ash/wm/mru_window_tracker.cc...
ash/wm/mru_window_tracker.cc:143: return item->GetRootWindow() == root_window;
On 2014/02/10 19:21:06, oshima wrote:
> I'd prefer just looping for simple pointer comparison like like because it's
> shorter and you can see everything in one place.

Done.

[flackr, 2014-02-10 22:08:33]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-02-10 22:10:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/149493008/180001

[commit-bot: I haz the power, 2014-02-10 22:54:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/149493008/180001

[commit-bot: I haz the power, 2014-02-10 23:49:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/149493008/180001

[commit-bot: I haz the power, 2014-02-11 01:47:04]

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

[commit-bot: I haz the power, 2014-02-11 01:47:05]

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

[flackr, 2014-02-13 22:22:31]

Thankfully the tests pointed out to me that we currently do not expect windows
like the app launcher to show the shelf in fullscreen mode, hurray for tests!

I've updated the code to only consider windows in the switchable containers and
fixed an issue where because window activation notifications are sent to
multiple workspace layout managers a test was getting too many notifications
about the change to fullscreen state but I left a TODO that it doesn't really
feel correct to track the fullscreen state in a layout manager which we have 3
of per root window. Can you take another look and let me know what you think?
Thanks!

[oshima, 2014-02-14 00:56:23]

On 2014/02/13 22:22:31, flackr wrote:
> Thankfully the tests pointed out to me that we currently do not expect windows
> like the app launcher to show the shelf in fullscreen mode, hurray for tests!
> 
> I've updated the code to only consider windows in the switchable containers
and
> fixed an issue where because window activation notifications are sent to
> multiple workspace layout managers a test was getting too many notifications
> about the change to fullscreen state but I left a TODO that it doesn't really
> feel correct to track the fullscreen state in a layout manager which we have 3
> of per root window. Can you take another look and let me know what you think?
> Thanks!

I'll review it either tonight or early morning.

[oshima, 2014-02-14 00:56:24]

On 2014/02/13 22:22:31, flackr wrote:
> Thankfully the tests pointed out to me that we currently do not expect windows
> like the app launcher to show the shelf in fullscreen mode, hurray for tests!
> 
> I've updated the code to only consider windows in the switchable containers
and
> fixed an issue where because window activation notifications are sent to
> multiple workspace layout managers a test was getting too many notifications
> about the change to fullscreen state but I left a TODO that it doesn't really
> feel correct to track the fullscreen state in a layout manager which we have 3
> of per root window. Can you take another look and let me know what you think?
> Thanks!

I'll review it either tonight or early morning.

[oshima, 2014-02-14 17:24:25]

https://codereview.chromium.org/149493008/diff/530001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/149493008/diff/530001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:1373:
wm::GetWindowState(window1)->Activate();
Does this mean that when we create a single window without activation, it's not
active?

https://codereview.chromium.org/149493008/diff/530001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/149493008/diff/530001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:242: aura::Window* lost_active) {
can you fix indent?

https://codereview.chromium.org/149493008/diff/530001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:249: UpdateFullscreenState();
All layout managers are notified for activation change, aren't they?

I think this is one thing I overlooked when I merged BaseLayoutManager and right
fix is to observe only events that needs. Better short term fix may be

if (!shelf_)
  return;

in UpdateFullscreenState(). WDYT?

[flackr, 2014-02-14 20:00:01]

https://codereview.chromium.org/149493008/diff/530001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/149493008/diff/530001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:1373:
wm::GetWindowState(window1)->Activate();
On 2014/02/14 17:24:26, oshima wrote:
> Does this mean that when we create a single window without activation, it's
not
> active? 

Correct. This may be to prevent stealing input focus in case you are midway
through typing a password or something, but I can verify.

https://codereview.chromium.org/149493008/diff/530001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/149493008/diff/530001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:242: aura::Window* lost_active) {
On 2014/02/14 17:24:26, oshima wrote:
> can you fix indent?

Done.

https://codereview.chromium.org/149493008/diff/530001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:249: UpdateFullscreenState();
On 2014/02/14 17:24:26, oshima wrote:
> All layout managers are notified for activation change, aren't they?

Yes, since it's a ShellObserver notification, that's why I had to make the
change in UpdateFullscreenState.

> 
> I think this is one thing I overlooked when I merged BaseLayoutManager and
right
> fix is to observe only events that needs. Better short term fix may be
> 
> if (!shelf_)
>   return;
> 
> in UpdateFullscreenState(). WDYT?

That sounds nicer in the meantime, done. Thanks!

[oshima, 2014-02-14 20:09:30]

https://codereview.chromium.org/149493008/diff/530001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/149493008/diff/530001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:1373:
wm::GetWindowState(window1)->Activate();
On 2014/02/14 20:00:02, flackr wrote:
> On 2014/02/14 17:24:26, oshima wrote:
> > Does this mean that when we create a single window without activation, it's
> not
> > active? 
> 
> Correct. This may be to prevent stealing input focus in case you are midway
> through typing a password or something, but I can verify.

We probably should investigate what's the ideal behavior for this case. (outside
of the scope of this CL)

https://codereview.chromium.org/149493008/diff/530001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (left):

https://codereview.chromium.org/149493008/diff/530001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:221: UpdateFullscreenState();
I think this one should stay here. Technically it's possible to move the
fullscreen window to topmost on another display without changing activation ,
right?

[flackr, 2014-02-14 20:15:01]

https://codereview.chromium.org/149493008/diff/530001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (left):

https://codereview.chromium.org/149493008/diff/530001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:221: UpdateFullscreenState();
On 2014/02/14 20:09:30, oshima wrote:
> I think this one should stay here. Technically it's possible to move the
> fullscreen window to topmost on another display without changing activation ,
> right?

Correct, but this calls OnWindowRemovedFromLayout on the current layout manager
and OnWindowAddedToLayout on the new layout manager.

[flackr, 2014-02-20 20:14:25]

As discussed, I've updated this to consider the active window if it's on the
current display, and otherwise use the topmost window.

Sorry this took so long to get up, I had a hard time finding the right place for
getting it to track a panel dragged onto a fullscreen display. Since the
WorkspaceLayoutManager observes the rootwindow it can get the HierarchyChanged
event for the window moving to the current display.

PTAL, thanks.

[oshima, 2014-02-21 19:49:46]

https://codereview.chromium.org/149493008/diff/690001/ash/root_window_control...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/149493008/diff/690001/ash/root_window_control...
ash/root_window_controller.cc:575: const aura::Window* active_window =
wm::GetActiveWindow();
can you add comment why these two path are necessary?

https://codereview.chromium.org/149493008/diff/690001/ash/wm/panels/panel_win...
File ash/wm/panels/panel_window_resizer_unittest.cc (right):

https://codereview.chromium.org/149493008/diff/690001/ash/wm/panels/panel_win...
ash/wm/panels/panel_window_resizer_unittest.cc:67: }
Looks a bit overkill. Can you just do

CreateTestWindowInShellWithBounds(gfx::Rect(600, 0, 101, 101))

?

https://codereview.chromium.org/149493008/diff/690001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/149493008/diff/690001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:160: const
WindowObserver::HierarchyChangeParams& params) {
A display may become fullscreen when the fullscreen became topmost as a result
of closing another window. Are we handling this case?

[flackr, 2014-02-24 16:21:09]

https://codereview.chromium.org/149493008/diff/690001/ash/root_window_control...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/149493008/diff/690001/ash/root_window_control...
ash/root_window_controller.cc:575: const aura::Window* active_window =
wm::GetActiveWindow();
On 2014/02/21 19:49:46, oshima wrote:
> can you add comment why these two path are necessary?

Done.

https://codereview.chromium.org/149493008/diff/690001/ash/wm/panels/panel_win...
File ash/wm/panels/panel_window_resizer_unittest.cc (right):

https://codereview.chromium.org/149493008/diff/690001/ash/wm/panels/panel_win...
ash/wm/panels/panel_window_resizer_unittest.cc:67: }
On 2014/02/21 19:49:46, oshima wrote:
> Looks a bit overkill. Can you just do
> 
> CreateTestWindowInShellWithBounds(gfx::Rect(600, 0, 101, 101))
> 
> ?
> 

Done.

https://codereview.chromium.org/149493008/diff/690001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/149493008/diff/690001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:160: const
WindowObserver::HierarchyChangeParams& params) {
On 2014/02/21 19:49:46, oshima wrote:
> A display may become fullscreen when the fullscreen became topmost as a result
> of closing another window. Are we handling this case?

Yes, if an active window (i.e. panel) is preventing fullscreen and it closes it
will cause an activation change (possibly to NULL) which will call
UpdateFullscreenState. If a window in the container (active or not) is
preventing fullscreen (i.e. it is above the fullscreen window), it will call
UpdateFullscreen in OnWindowRemovedFromLayout.

[oshima, 2014-02-24 16:39:03]

lgtm with a nit.

https://codereview.chromium.org/149493008/diff/780001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/149493008/diff/780001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:101: UpdateFullscreenState();
I think this should be outside of if because hiding normal window can change the
fullscreen state.

[flackr, 2014-02-24 16:52:35]

https://codereview.chromium.org/149493008/diff/780001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/149493008/diff/780001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:101: UpdateFullscreenState();
On 2014/02/24 16:39:04, oshima wrote:
> I think this should be outside of if because hiding normal window can change
the
> fullscreen state.

Done.

[flackr, 2014-02-24 17:01:07]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-02-24 17:01:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/149493008/830001

[commit-bot: I haz the power, 2014-02-24 18:09:07]

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

[commit-bot: I haz the power, 2014-02-24 18:09:07]

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

[flackr, 2014-02-24 18:24:09]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-02-24 18:27:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/149493008/830001

[commit-bot: I haz the power, 2014-02-24 20:25:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/149493008/830001

[commit-bot: I haz the power, 2014-02-24 21:09:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/149493008/830001

[commit-bot: I haz the power, 2014-02-24 22:43:33]

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

[commit-bot: I haz the power, 2014-02-24 22:43:33]

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

[flackr, 2014-02-25 14:52:49]

Sorry, one more update. In
ShelfLayoutManagerTest.OpenAppListWithShelfAutoHideState we expect the shelf to
remain hidden when bringing up the app list from a fullscreen window. I can
maintain this (as I've done in the latest patchset) or update the test if we
want to unhide the shelf when the app list is showing. WDYT?

[oshima, 2014-02-25 18:03:14]

lgtm

[flackr, 2014-02-25 18:03:56]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-02-25 18:05:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/149493008/1120001

[commit-bot: I haz the power, 2014-02-25 21:49:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/149493008/1120001

[commit-bot: I haz the power, 2014-02-25 22:14:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/149493008/1120001

[commit-bot: I haz the power, 2014-02-25 23:17:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/149493008/1120001

[commit-bot: I haz the power, 2014-02-26 00:04:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/149493008/1120001

[Paweł Hajdan Jr., 2014-02-26 05:34:14]

The CQ bit was unchecked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-26 05:34:52]

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

[Paweł Hajdan Jr., 2014-02-26 05:42:34]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-26 05:51:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/149493008/1120001

[Paweł Hajdan Jr., 2014-02-26 22:52:09]

The CQ bit was unchecked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2014-02-26 22:52:57]

On 2014/02/26 22:52:09, Paweł Hajdan Jr. wrote:
> The CQ bit was unchecked by mailto:phajdan.jr@chromium.org

Sorry, this CL got stuck in CQ.

I recommend uploading a new patchset and trying again as a workaround.

The tracking bug for this is
https://code.google.com/p/chromium/issues/detail?id=346959

[flackr, 2014-02-27 01:53:34]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-02-27 01:55:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/149493008/1140001

[commit-bot: I haz the power, 2014-02-27 04:54:58]

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

[commit-bot: I haz the power, 2014-02-27 04:54:59]

Try jobs failed on following builders: linux_chromium_rel

[flackr, 2014-02-27 05:01:32]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-02-27 05:01:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/149493008/1140001

[commit-bot: I haz the power, 2014-02-27 13:58:16]

Change committed as 253794