exo: Synchronize multi-display window positioning

components/exo/shell_surface.cc

Index: components/exo/shell_surface.cc
diff --git a/components/exo/shell_surface.cc b/components/exo/shell_surface.cc
index 7183474b8bfffe15cf3348671224cc19c03b5422..58fb2b861678b4de4697267a7e81bdb37019148d 100644
--- a/components/exo/shell_surface.cc
+++ b/components/exo/shell_surface.cc
@@ -369,8 +369,10 @@ void ShellSurface::AcknowledgeConfigure(uint32_t serial) {
       break;
   }
 
-  if (widget_)
+  if (widget_) {
     UpdateWidgetBounds();
+    UpdateShadow();
+  }
 }
 
 void ShellSurface::SetParent(ShellSurface* parent) {
@@ -629,7 +631,25 @@ void ShellSurface::SetTopInset(int height) {
 void ShellSurface::SetOrigin(const gfx::Point& origin) {
   TRACE_EVENT1("exo", "ShellSurface::SetOrigin", "origin", origin.ToString());
 
+  const gfx::Point old_origin = origin_;

[reveman, 2017/02/18 00:32:39]

nit: exo and chromium code typically doesn't use const for local variables
unless it's compile time constant

[Dominik Laskowski, 2017/02/23 03:43:48]

Done.

   origin_ = origin;
+
+  if (bounds_mode_ != BoundsMode::CLIENT || origin == old_origin)

[reveman, 2017/02/18 00:32:39]

nit: can you move the "origin == old_origin" check above as "(origin == origin_)
return" and remove the need for the old_origin variable?

[Dominik Laskowski, 2017/02/23 03:43:48]

Done.

+    return;
+
+  // If the origin changed, give the client a chance to adjust window positions
+  // before switching to the new coordinate system. Retain the old origin by
+  // reverting the origin delta until the next configure is acknowledged.
+  const gfx::Vector2d delta = origin - old_origin;
+  origin_offset_ -= delta;
+  pending_origin_offset_accumulator_ += delta;
+
+  if (widget_) {
+    UpdateWidgetBounds();
+    UpdateShadow();
+  }
+
+  Configure();
 }
 
 void ShellSurface::SetActivatable(bool activatable) {
@@ -676,10 +696,12 @@ void ShellSurface::OnSurfaceCommit() {
 
   if (enabled() && !widget_) {
     // Defer widget creation until surface contains some contents.
-    if (surface_->content_size().IsEmpty())
+    if (surface_->content_size().IsEmpty()) {
       Configure();
-    else
-      CreateShellSurfaceWidget(ui::SHOW_STATE_NORMAL);
+      return;
+    }
+
+    CreateShellSurfaceWidget(ui::SHOW_STATE_NORMAL);
   }
 
   // Apply the accumulated pending origin offset to reflect acknowledged
@@ -871,7 +893,10 @@ void ShellSurface::OnPreWindowStateTypeChange(
     // cross-fade animations. The configure callback provides a mechanism for
     // the client to inform us that a frame has taken the state change into
     // account and without this cross-fade animations are unreliable.
-    if (configure_callback_.is_null())
+
+    // TODO(domlaskowski): For shell surfaces whose bounds are controlled by the
+    // client, the configure callback does not yet support window state changes.
+    if (configure_callback_.is_null() || bounds_mode_ == BoundsMode::CLIENT)
       scoped_animations_disabled_.reset(new ScopedAnimationsDisabled(this));
   }
 }
@@ -903,7 +928,8 @@ void ShellSurface::OnPostWindowStateTypeChange(
 void ShellSurface::OnWindowBoundsChanged(aura::Window* window,
                                          const gfx::Rect& old_bounds,
                                          const gfx::Rect& new_bounds) {
-  if (!widget_ || !surface_ || ignore_window_bounds_changes_)
+  if (bounds_mode_ == BoundsMode::CLIENT || !widget_ || !surface_ ||

[reveman, 2017/02/18 00:32:39]

nit: is this needed as part of this patch? in that case at least move
"bounds_mode_ == BoundsMode::CLIENT" into a separate check? I prefer that as I
think it's cleaner and not sure we should be doing this long term

[Dominik Laskowski, 2017/02/23 03:43:48]

Yes, but it will be removed once resizing is driven by the server. Until then,
the origin offset should not be modified when the window bounds change.

Moved into a separate check and added a comment.

+      ignore_window_bounds_changes_)
     return;
 
   if (window == widget_->GetNativeWindow()) {
@@ -1154,10 +1180,11 @@ void ShellSurface::Configure() {
       serial = configure_callback_.Run(
           non_client_view->frame_view()->GetBoundsForClientView().size(),
           ash::wm::GetWindowState(widget_->GetNativeWindow())->GetStateType(),
-          IsResizing(), widget_->IsActive());
+          IsResizing(), widget_->IsActive(), origin_);
     } else {
-      serial = configure_callback_.Run(
-          gfx::Size(), ash::wm::WINDOW_STATE_TYPE_NORMAL, false, false);
+      serial = configure_callback_.Run(gfx::Size(),
+                                       ash::wm::WINDOW_STATE_TYPE_NORMAL, false,
+                                       false, origin_);
     }
   }
 
@@ -1293,7 +1320,8 @@ gfx::Point ShellSurface::GetSurfaceOrigin() const {
   // For client-positioned shell surfaces, the surface origin corresponds to the
   // widget position relative to the origin specified by the client. Since the
   // surface is positioned relative to the widget, negate this vector to align
-  // the surface with the widget.
+  // the surface with the widget. Note that the widget position must have been

[reveman, 2017/02/18 00:32:39]

hm, I'd expect CLIENT bounds to behave just like SHELL when in TOPLEFT resize
mode. See comment below.

[Dominik Laskowski, 2017/02/23 03:43:48]

I've removed the special case below to avoid depending on widget state.

+  // adjusted by the |origin_offset_| prior to this call.
   if (bounds_mode_ != BoundsMode::SHELL) {
     gfx::Point position = widget_->GetNativeWindow()->bounds().origin();
     wm::ConvertPointToScreen(widget_->GetNativeWindow()->parent(), &position);
@@ -1356,6 +1384,9 @@ void ShellSurface::UpdateWidgetBounds() {
 
   switch (bounds_mode_) {
     case BoundsMode::CLIENT:
+      // Position is relative to the latest origin acknowledged by the client.

[reveman, 2017/02/18 00:32:39]

Can we have this behave as the SHELL when "resize_component_ != HTCAPTION"?
That's how I expected this to work with minimal extra logic for the CLIENT case.

[Dominik Laskowski, 2017/02/23 03:43:48]

I've simplified this a bit, but I don't think unifying these cases is possible
because the meaning of origin/geometry is different.

+      new_widget_bounds -= origin_offset_;
+      break;
     case BoundsMode::FIXED:
       // Position is relative to the origin.
       new_widget_bounds += origin_.OffsetFromOrigin();
@@ -1418,11 +1449,9 @@ void ShellSurface::UpdateShadow() {
       }
     }
 
-    // TODO(oshima): Adjust the coordinates from client screen to
-    // chromeos screen when multi displays are supported.
-    gfx::Point origin = window->bounds().origin();
-    gfx::Point shadow_origin = shadow_content_bounds.origin();
-    shadow_origin -= origin.OffsetFromOrigin();
+    gfx::Point shadow_origin = shadow_content_bounds.origin() - origin_offset_;
+    wm::ConvertPointFromScreen(window->parent(), &shadow_origin);
+    shadow_origin -= window->bounds().OffsetFromOrigin();
     gfx::Rect shadow_bounds(shadow_origin, shadow_content_bounds.size());
 
     // Always create and show the underlay, even in maximized/fullscreen.