Allow updating arc widget bounds in maximize/fullscreen state.

components/exo/shell_surface.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/exo/shell_surface.h"
 
 #include "ash/aura/wm_window_aura.h"
 #include "ash/common/shell_window_ids.h"
 #include "ash/common/wm/window_resizer.h"
 #include "ash/common/wm/window_state.h"
@@ skipping 914 matching lines @@
   window->SetName("ExoShellSurface");
   window->AddChild(surface_->window());
   window->SetEventTargeter(base::WrapUnique(new CustomWindowTargeter));
   SetApplicationId(window, &application_id_);
   SetMainSurface(window, surface_);
 
   // Start tracking changes to window bounds and window state.
   window->AddObserver(this);
   ash::wm::WindowState* window_state = ash::wm::GetWindowState(window);
   window_state->AddObserver(this);
+
+  // Allow remote shell clients to update the bounds in maixmized/fullscreen

[reveman, 2016/08/12 16:17:32]

nit: maximized

[oshima, 2016/08/12 23:03:33]

Done.

+  // mode.
+  if (!application_id_.empty())

[reveman, 2016/08/12 16:17:32]

This is not a proper test for arc apps. application_id is used by xdg-shell
clients too. initial_bounds_ and activatable_ being set is unique to arc apps
today. initial_bounds_ will be set for xdg-shell popups but they are never
activatable_.

nit: please avoid referring to "remote shell" in the code as that interface will
change name in the near future. maybe we can say "absolute positioned" shell
surface in this case?

[oshima, 2016/08/12 23:03:33]

Done.

+    window_state->set_allow_set_bounds_in_maximized(true);
+
   // Notify client of initial state if different than normal.
   if (window_state->GetStateType() != ash::wm::WINDOW_STATE_TYPE_NORMAL &&
       !state_changed_callback_.is_null()) {
     state_changed_callback_.Run(ash::wm::WINDOW_STATE_TYPE_NORMAL,
                                 window_state->GetStateType());
   }
 
   // Disable movement if initial bounds were specified.
   widget_->set_movement_disabled(!initial_bounds_.IsEmpty());
   window_state->set_ignore_keyboard_bounds_change(!initial_bounds_.IsEmpty());
@@ skipping 200 matching lines @@
              visible_bounds.OffsetFromOrigin();
     default:
       NOTREACHED();
       return gfx::Point();
   }
 }
 
 void ShellSurface::UpdateWidgetBounds() {
   DCHECK(widget_);
 
-  // Return early if the shell is currently managing the bounds of the widget.
-  if (widget_->IsMaximized() || widget_->IsFullscreen() || IsResizing())

[reveman, 2016/08/12 16:17:32]

I'm pretty sure we need the IsResizing part for interactive resize of wl_shell
client surfaces. That type of interactive resize works by chrome changing the
size of the widget and that generating configure events that the client can
ignore if it doesn't like the size or provide contents of a new size if it likes
to. For this to work, client contents cannot control the widget size while a
interactive resize is taking place.

Actually, I think we need the other checks too for wl_shell clients as they are
not being sent configure events so removing those will create a race condition
that can result in these clients not producing contents correctly for maximized
or fullscreen mode.

I don't fully understand why this if-statement needs to be removed. Can you
explain more why this is needed?

[oshima, 2016/08/12 23:03:33]

I thought you wanted me to remove it. (and I looks working when I tested, both
arc and wayland terminal)
For arc++ apps, android controls the actual size of maximized/fullscreen, so his
is causing the inconsistency between the window size that chromeos have, and
what android uses, which leads to blank areas around the window in overview
mode.

[reveman, 2016/08/12 23:44:40]

Sorry for the confusion. In my initial comment I was trying to understand how
this does not break wl_shell and xdg_shell clients. Removing IsResizing
definitely does so we need to keep that for sure.

Here's the sequence of events that can be a problem for wl-shell clients:

1. client produces a new frame in non-maximized mode and starts transmitting it
to chrome
2. chrome maximizes the window before processing the new frame from the client
 2.1. chrome changes the size of the widget to the ideal maximized size
 2.2. chrome sends configure event 1 to the client with the new maximized state
and suggested size
3. chrome receives the new (non-maximized) frame from the client and updates the
widget size as a result of this CL (window is now maximized but widget size is
same as before)
4. chrome moves focus to a different window
 4.1. the move of focus results in chrome sending configure event 2 to the
client with the current size of the widget and new active state
5. client receives configure event 1 and produces a frame for the ideal
maximized size
6. client receives configure event 2 and produces a frame for the non-maximized
size even though the window is still maximized (BAD)

this might be limited to wl-shell clients and not xdg-shell clients as the
serials in configure events should help in that case.

anyhow, to remove these checks as this CL does we'd need to keep track of the
ideal maximized size so we can send that instead of the widget size in configure
events when a window is maximized. 

I think the easiest solution is to just make these checks depend on
initial_bounds_ not being set. that makes sense to me as when initial_bounds_ is
set we shouldn't really be changing widget bounds at all and the bounds should
always be controlled by the contents produced by the client. Change that and
this CL looks good.

[oshima, 2016/08/13 00:45:07]

Note that it will only happens when this flag is set. That is, SetBounds will
not update the
widget bounds in maximized/fullscreen state unless this flag is set.

I agree that checking would be safer.

-    return;
-
   // Return early if there is pending configure requests.
   if (!pending_configs_.empty() || scoped_configure_)
     return;
 
   gfx::Rect visible_bounds = GetVisibleBounds();
   gfx::Rect new_widget_bounds = visible_bounds;
 
   // Avoid changing widget origin unless initial bounds were specified and
   // widget origin is always relative to it.
   if (initial_bounds_.IsEmpty())
@@ skipping 73 matching lines @@
       shadow_overlay_->layer()->Add(shadow->layer());
       window->AddChild(shadow_overlay_);
       shadow_overlay_->Show();
     }
     shadow_overlay_->SetBounds(shadow_bounds);
     shadow->SetContentBounds(gfx::Rect(shadow_bounds.size()));
   }
 }
 
 }  // namespace exo