Ignore fullscreen windows which are behind other windows for fullscreen mode.

Ignore non-active fullscreen windows for shelf state.

BUG=319889
TEST=ShelfLayoutManagerTest.FullscreenWindowInFrontHidesShelf, ShelfLayoutManagerTest.FullscreenWindowOnSecondDisplay,WorkspaceLayoutManagerTest.NotifyFullscreenChanges
TEST=Navigate to a site using HTML5 fullscreen API, alt tab away to another window and back. Window remains fullscreen but shelf shows while another window is in front.

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

[flackr, 2013-12-03 15:29:26]

Hi,

This is the change I talked to you about when I was there. By having the shelf
ignore non-active full-screen windows we can preserve their state when you
switch from and back to them. Testing this, it feels like a much less surprising
user experience.

Please take a look, thanks.

[oshima, 2013-12-03 19:04:56]

https://codereview.chromium.org/100903002/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_controller.cc (left):

https://codereview.chromium.org/100903002/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_controller.cc:77: // it should exit fullscreen
mode.
is it difficult to keep it fullscreen? It'd be nice if I can pick non fullscreen
window while I have fullscreen on the back.

[flackr, 2013-12-03 19:08:25]

https://codereview.chromium.org/100903002/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_controller.cc (left):

https://codereview.chromium.org/100903002/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_controller.cc:77: // it should exit fullscreen
mode.
On 2013/12/03 19:04:57, oshima wrote:
> is it difficult to keep it fullscreen? It'd be nice if I can pick non
fullscreen
> window while I have fullscreen on the back.

That's what the code will do after this change. The window will still be
fullscreen but behind the newly activated window.

[oshima, 2013-12-03 19:11:26]

On 2013/12/03 19:08:25, flackr wrote:
>
https://codereview.chromium.org/100903002/diff/20001/ash/wm/overview/window_s...
> File ash/wm/overview/window_selector_controller.cc (left):
> 
>
https://codereview.chromium.org/100903002/diff/20001/ash/wm/overview/window_s...
> ash/wm/overview/window_selector_controller.cc:77: // it should exit fullscreen
> mode.
> On 2013/12/03 19:04:57, oshima wrote:
> > is it difficult to keep it fullscreen? It'd be nice if I can pick non
> fullscreen
> > window while I have fullscreen on the back.
> 
> That's what the code will do after this change. The window will still be
> fullscreen but behind the newly activated window.

Sorry, this is second time I mixed up the deletion and addition today.
Maybe I had too good thanksgiving.

[oshima, 2013-12-03 19:17:51]

https://codereview.chromium.org/100903002/diff/20001/ash/wm/workspace_control...
File ash/wm/workspace_controller.cc (right):

https://codereview.chromium.org/100903002/diff/20001/ash/wm/workspace_control...
ash/wm/workspace_controller.cc:77: } else if (window_state->IsFullscreen() &&
window_state->IsActive()) {
Don't we have to check if this is top most?

Window may be top most but not active in multi display environment.

[flackr, 2013-12-03 21:32:57]

https://codereview.chromium.org/100903002/diff/20001/ash/wm/workspace_control...
File ash/wm/workspace_controller.cc (right):

https://codereview.chromium.org/100903002/diff/20001/ash/wm/workspace_control...
ash/wm/workspace_controller.cc:77: } else if (window_state->IsFullscreen() &&
window_state->IsActive()) {
On 2013/12/03 19:17:52, oshima wrote:
> Don't we have to check if this is top most?
> 
> Window may be top most but not active in multi display environment.

Good point, done. This also no longer requires activating fullscreen windows in
the tests :-).

[flackr, 2013-12-03 21:54:06]

https://codereview.chromium.org/100903002/diff/50001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/100903002/diff/50001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1156:
window_->SetBounds(rect);
In my first CL an activation change in some of the browser tests was causing a
bounds update which infinitely recurses because when the window bounds update we
end up calling this method which then updates the window bounds.

It's probably not necessary anymore but this infinite recursion seems like a
failure just waiting to happen the next time we have a fullscreen window which
indirectly has a bounds update to the same thing.

[flackr, 2013-12-03 22:05:54]

https://codereview.chromium.org/100903002/diff/50001/ash/wm/workspace_control...
File ash/wm/workspace_controller.cc (right):

https://codereview.chromium.org/100903002/diff/50001/ash/wm/workspace_control...
ash/wm/workspace_controller.cc:77: } else if (*i == windows.back() &&
window_state->IsFullscreen()) {
Hang on, this check for topmost doesn't seem to be working in practice. I'll
ping you when I address this.

[flackr, 2013-12-04 19:09:57]

Okay, please take a look. The problem was that the popup window telling you that
you are in fullscreen mode was above the fullscreen window. I've addressed this
by stepping through the windows in reverse order and remembering if any tracked
windows have been seen yet. This also means we can exit early when there is a
fullscreen window as it's the first one in the loop.

[pkotwicz, 2013-12-04 20:29:44]

A cool change! I have tried applying the CL locally and I have some comments:

The code which uses:
RootWindowController::GetTopmostFullscreenWindow()
chrome::IsFullscreenMode()
ShellObserver::OnFullscreenStateChanged()
needs to be updated

With the CL applied, even though the shelf becomes visible when the
non-fullscreen window is activated,
- Notifications are still blocked
- It is not possible to auto hide the shelf by right clicking on the shelf
- It is not possible to hide the shelf via a gesture
- The dock stays hidden

The shelf also unexpectedly flares up when I open a bubble such as the bookmark
editor. (This is not the case without the CL)
It seems as if the shelf already unexpectedly flared up with javascript alert
boxes (This is the case without the CL)

[flackr, 2013-12-04 21:58:44]

Thanks for the great points! I've unified the fullscreen window detection so
that blocking of notifications will match the shelf hidden state.

On 2013/12/04 20:29:44, pkotwicz wrote:
> A cool change! I have tried applying the CL locally and I have some comments:
> 
> The code which uses:
> RootWindowController::GetTopmostFullscreenWindow()
> chrome::IsFullscreenMode()
> ShellObserver::OnFullscreenStateChanged()
> needs to be updated
> 
> With the CL applied, even though the shelf becomes visible when the
> non-fullscreen window is activated,
> - Notifications are still blocked

Fixed.

> - It is not possible to auto hide the shelf by right clicking on the shelf

Fixed.

> - It is not possible to hide the shelf via a gesture

Fixed.

> - The dock stays hidden

Still broken, investigating.

> 
> The shelf also unexpectedly flares up when I open a bubble such as the
bookmark
> editor. (This is not the case without the CL)

Fixed.

> It seems as if the shelf already unexpectedly flared up with javascript alert
> boxes (This is the case without the CL)

Fixed.

[flackr, 2013-12-04 22:41:18]

Okay, I've fixed the notifications when the fullscreen state changes and added a
test for this. The dock now correctly hides and shows when you switch to/from a
fullscreen window. Can you take another look?

[pkotwicz, 2013-12-05 02:46:28]

Looks good!

Btw, the bug wrt to javascript alerts is still there but it is unrelated.
javascript alerts happen to be system modal and that screws up
ash::MruWindowTracker::BuildWindowList()

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

https://codereview.chromium.org/100903002/diff/140001/ash/root_window_control...
ash/root_window_controller.cc:521: const aura::Window*
RootWindowController::GetTopmostFullscreenWindow() const {
Would GetWindowForFullscreenMode() / GetFullscreenModeWindow() be clearer?

https://codereview.chromium.org/100903002/diff/140001/ash/root_window_control...
ash/root_window_controller.cc:524: const aura::Window* topmost_fullscreen_window
= NULL;
Rename to |topmost_window|.

https://codereview.chromium.org/100903002/diff/140001/ash/root_window_control...
ash/root_window_controller.cc:529: (*iter)->layer()->GetTargetOpacity() != 0.0f)
{
I wonder whether the target opacity check is still necessary or whether it is
left over from when we had workspaces

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

https://codereview.chromium.org/100903002/diff/140001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:162: UpdateDesktopVisibility();
Can you please rename UpdateDesktopVisibility() to UpdateShelfVisibility()

https://codereview.chromium.org/100903002/diff/140001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:309: bool is_fullscreen =
static_cast<bool>(GetRootWindowController(
Can you explicity check for NULL instead of the static cast?

https://codereview.chromium.org/100903002/diff/140001/ash/wm/workspace_contro...
File ash/wm/workspace_controller.cc (right):

https://codereview.chromium.org/100903002/diff/140001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:83: has_maximized_window = true;
We can return WORKSPACE_WINDOW_STATE_MAXIMIZED immediately here and get rid of
|has_maximized_window|.

[flackr, 2013-12-05 16:48:54]

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

https://codereview.chromium.org/100903002/diff/140001/ash/root_window_control...
ash/root_window_controller.cc:521: const aura::Window*
RootWindowController::GetTopmostFullscreenWindow() const {
On 2013/12/05 02:46:29, pkotwicz wrote:
> Would GetWindowForFullscreenMode() / GetFullscreenModeWindow() be clearer?

Sounds like a good name to me, done.

https://codereview.chromium.org/100903002/diff/140001/ash/root_window_control...
ash/root_window_controller.cc:524: const aura::Window* topmost_fullscreen_window
= NULL;
On 2013/12/05 02:46:29, pkotwicz wrote:
> Rename to |topmost_window|.

Done.

https://codereview.chromium.org/100903002/diff/140001/ash/root_window_control...
ash/root_window_controller.cc:529: (*iter)->layer()->GetTargetOpacity() != 0.0f)
{
On 2013/12/05 02:46:29, pkotwicz wrote:
> I wonder whether the target opacity check is still necessary or whether it is
> left over from when we had workspaces

Looks like this has been the case since the maximized and overlaps states were
implemented (which was when we had workspaces),
https://chromiumcodereview.appspot.com/9764008/diff/2001/ash/wm/workspace/wor....


+Scott, I've played around with this with the check removed and things seem
fine. Are there circumstances where we make things 0 opacity but don't change
the target visibility?

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

https://codereview.chromium.org/100903002/diff/140001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:162: UpdateDesktopVisibility();
On 2013/12/05 02:46:29, pkotwicz wrote:
> Can you please rename UpdateDesktopVisibility() to UpdateShelfVisibility()

Done.

https://codereview.chromium.org/100903002/diff/140001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:309: bool is_fullscreen =
static_cast<bool>(GetRootWindowController(
On 2013/12/05 02:46:29, pkotwicz wrote:
> Can you explicity check for NULL instead of the static cast?

Done.

https://codereview.chromium.org/100903002/diff/140001/ash/wm/workspace_contro...
File ash/wm/workspace_controller.cc (right):

https://codereview.chromium.org/100903002/diff/140001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:83: has_maximized_window = true;
On 2013/12/05 02:46:29, pkotwicz wrote:
> We can return WORKSPACE_WINDOW_STATE_MAXIMIZED immediately here and get rid of
> |has_maximized_window|.

Nice, the code is getting cleaner and cleaner :-).

[oshima, 2013-12-05 19:08:58]

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

https://codereview.chromium.org/100903002/diff/200001/ash/root_window_control...
ash/root_window_controller.cc:527: if ((*iter)->type() !=
aura::client::WINDOW_TYPE_POPUP &&
can this be == TYPE_NORMAL?

https://codereview.chromium.org/100903002/diff/200001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/100903002/diff/200001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:68: int call_count() {
const

https://codereview.chromium.org/100903002/diff/200001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:72: bool is_fullscreen() {
ditto

https://codereview.chromium.org/100903002/diff/200001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:79: };
DISALLOW_COPY_AND_ASSIGN

[flackr, 2013-12-05 19:30:21]

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

https://codereview.chromium.org/100903002/diff/200001/ash/root_window_control...
ash/root_window_controller.cc:527: if ((*iter)->type() !=
aura::client::WINDOW_TYPE_POPUP &&
On 2013/12/05 19:08:58, oshima wrote:
> can this be == TYPE_NORMAL? 

== TYPE_NORMAL || == TYPE_PANEL
Done.

https://codereview.chromium.org/100903002/diff/200001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/100903002/diff/200001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:68: int call_count() {
On 2013/12/05 19:08:58, oshima wrote:
> const

Done.

https://codereview.chromium.org/100903002/diff/200001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:72: bool is_fullscreen() {
On 2013/12/05 19:08:58, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/100903002/diff/200001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:79: };
On 2013/12/05 19:08:58, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[oshima, 2013-12-05 19:35:35]

lgtm

Thanks! fullscreen will be much more useful with this.

[pkotwicz, 2013-12-05 20:50:09]

LGTM with comments

https://codereview.chromium.org/100903002/diff/220001/ash/root_window_control...
File ash/root_window_controller.h (right):

https://codereview.chromium.org/100903002/diff/220001/ash/root_window_control...
ash/root_window_controller.h:227: aura::Window* GetWindowForFullscreenMode();
WindowSelectorController seems to be the only user of the non-const version. Can
we get rid of it?

https://codereview.chromium.org/100903002/diff/220001/ash/root_window_control...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/100903002/diff/220001/ash/root_window_control...
ash/root_window_controller_unittest.cc:480: TEST_F(RootWindowControllerTest,
MultipleFullscreenWindows) {
Nit: Can we remove this test? It does not test anything that the test above is
not testing.
However, it would be nice to test that we return the proper thing when there is
just one window in the workspace, that window is fullscreen, and it is
minimized.

https://codereview.chromium.org/100903002/diff/220001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/100903002/diff/220001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:541: bool
ShelfLayoutManager::FullscreenWithHiddenShelf() const {
This function has just one caller. I think that it is simple enough that it can
be merged into ShelfLayoutManager::UpdateVisibilityState().
I think that you can drop the check for whether the RootWindowController exists.
In particular, there is no such check in WorkspaceController::GetWindowState().

https://codereview.chromium.org/100903002/diff/220001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.h (right):

https://codereview.chromium.org/100903002/diff/220001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.h:81: void UpdateFullscreenState();
Methods without comments are evil

https://codereview.chromium.org/100903002/diff/220001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/100903002/diff/220001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:469:
EXPECT_FALSE(observer.is_fullscreen());
For the sake of completeness, you should also test that the fullscreen state is
properly updated when activation is changed as a result of a window getting
closed.

https://codereview.chromium.org/100903002/diff/220001/ash/wm/workspace_contro...
File ash/wm/workspace_controller.cc (right):

https://codereview.chromium.org/100903002/diff/220001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:63: if (topmost_fullscreen_window &&
Hmm. This is problematic. Would it be possible to get rid of ignored_by_shelf()
in a seprate CL?
For now, we should add:
DCHECK(!topmost_fullscreen_window->ignored_by_shelf());

[flackr, 2013-12-05 21:39:15]

https://codereview.chromium.org/100903002/diff/220001/ash/root_window_control...
File ash/root_window_controller.h (right):

https://codereview.chromium.org/100903002/diff/220001/ash/root_window_control...
ash/root_window_controller.h:227: aura::Window* GetWindowForFullscreenMode();
On 2013/12/05 20:50:10, pkotwicz wrote:
> WindowSelectorController seems to be the only user of the non-const version.
Can
> we get rid of it?

Done.

https://codereview.chromium.org/100903002/diff/220001/ash/root_window_control...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/100903002/diff/220001/ash/root_window_control...
ash/root_window_controller_unittest.cc:480: TEST_F(RootWindowControllerTest,
MultipleFullscreenWindows) {
On 2013/12/05 20:50:10, pkotwicz wrote:
> Nit: Can we remove this test? It does not test anything that the test above is
> not testing.

It tests that the topmost of multiple fullscreen windows is returned but since
we only ever look at the topmost it does seem like the same thing. Removed.

> However, it would be nice to test that we return the proper thing when there
is
> just one window in the workspace, that window is fullscreen, and it is
> minimized.

Sure, added to GetWindowForFullscreenMode.

https://codereview.chromium.org/100903002/diff/220001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/100903002/diff/220001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:541: bool
ShelfLayoutManager::FullscreenWithHiddenShelf() const {
On 2013/12/05 20:50:10, pkotwicz wrote:
> This function has just one caller. I think that it is simple enough that it
can
> be merged into ShelfLayoutManager::UpdateVisibilityState().
> I think that you can drop the check for whether the RootWindowController
exists.
> In particular, there is no such check in
WorkspaceController::GetWindowState().

Done.

https://codereview.chromium.org/100903002/diff/220001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.h (right):

https://codereview.chromium.org/100903002/diff/220001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.h:81: void UpdateFullscreenState();
On 2013/12/05 20:50:10, pkotwicz wrote:
> Methods without comments are evil

Done.

https://codereview.chromium.org/100903002/diff/220001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/100903002/diff/220001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:469:
EXPECT_FALSE(observer.is_fullscreen());
On 2013/12/05 20:50:10, pkotwicz wrote:
> For the sake of completeness, you should also test that the fullscreen state
is
> properly updated when activation is changed as a result of a window getting
> closed.

There was actually nothing fired on window removed, at least not immediately.
Done.

https://codereview.chromium.org/100903002/diff/220001/ash/wm/workspace_contro...
File ash/wm/workspace_controller.cc (right):

https://codereview.chromium.org/100903002/diff/220001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:63: if (topmost_fullscreen_window &&
On 2013/12/05 20:50:10, pkotwicz wrote:
> Hmm. This is problematic. Would it be possible to get rid of
ignored_by_shelf()
> in a seprate CL?
> For now, we should add:
> DCHECK(!topmost_fullscreen_window->ignored_by_shelf());

ignored_by_shelf is used by overview mode to show the shelf on entering overview
even if the fullscreen window is the currently active (frontmost) window. I'm
not sure I understand why it's problematic, could you elaborate?

[commit-bot: I haz the power, 2013-12-06 18:27:57]

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

[commit-bot: I haz the power, 2013-12-06 19:08:52]

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

[flackr, 2013-12-06 19:11:54]

+sky for chrome/browser and content/browser OWNERS, PTAL, thanks!

[flackr, 2013-12-10 14:43:38]

Scott, ping.

[sky, 2013-12-10 16:42:46]

LGTM

[commit-bot: I haz the power, 2013-12-10 16:55:58]

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

[commit-bot: I haz the power, 2013-12-10 21:55:13]

Change committed as 239859