exo: Synchronize multi-display window positioning

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 <algorithm>
 
 #include "ash/common/frame/custom_frame_view_ash.h"
 #include "ash/common/shelf/wm_shelf.h"
@@ skipping 351 matching lines @@
     // Commit() is called.
     pending_origin_offset_ += config.origin_offset;
 
     // Set the resize direction that will be applied when Commit() is called.
     pending_resize_component_ = config.resize_component;
 
     if (config.serial == serial)
       break;
   }
 
-  if (widget_)
+  if (widget_) {
     UpdateWidgetBounds();
+    UpdateShadow();
+  }
 }
 
 void ShellSurface::SetParent(ShellSurface* parent) {
   TRACE_EVENT1("exo", "ShellSurface::SetParent", "parent",
                parent ? base::UTF16ToASCII(parent->title_) : "null");
 
   if (parent_) {
     parent_->RemoveObserver(this);
     if (widget_)
       wm::RemoveTransientChild(parent_, widget_->GetNativeWindow());
@@ skipping 238 matching lines @@
 
 void ShellSurface::SetTopInset(int height) {
   TRACE_EVENT1("exo", "ShellSurface::SetTopInset", "height", height);
 
   pending_top_inset_height_ = 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) {
   TRACE_EVENT1("exo", "ShellSurface::SetActivatable", "activatable",
                activatable);
 
   activatable_ = activatable;
 }
 
 void ShellSurface::SetContainer(int container) {
@@ skipping 26 matching lines @@
 
 ////////////////////////////////////////////////////////////////////////////////
 // SurfaceDelegate overrides:
 
 void ShellSurface::OnSurfaceCommit() {
   surface_->CheckIfSurfaceHierarchyNeedsCommitToNewSurfaces();
   surface_->CommitSurfaceHierarchy();
 
   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
   // configure requests.
   origin_offset_ += pending_origin_offset_;
   pending_origin_offset_ = gfx::Vector2d();
 
   // Update resize direction to reflect acknowledged configure requests.
   resize_component_ = pending_resize_component_;
 
@@ skipping 171 matching lines @@
     ash::wm::WindowState* window_state,
     ash::wm::WindowStateType old_type) {
   ash::wm::WindowStateType new_type = window_state->GetStateType();
   if (ash::wm::IsMaximizedOrFullscreenOrPinnedWindowStateType(old_type) ||
       ash::wm::IsMaximizedOrFullscreenOrPinnedWindowStateType(new_type)) {
     // When transitioning in/out of maximized or fullscreen mode we need to
     // make sure we have a configure callback before we allow the default
     // 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));
   }
 }
 
 void ShellSurface::OnPostWindowStateTypeChange(
     ash::wm::WindowState* window_state,
     ash::wm::WindowStateType old_type) {
   ash::wm::WindowStateType new_type = window_state->GetStateType();
   if (ash::wm::IsMaximizedOrFullscreenOrPinnedWindowStateType(old_type) ||
       ash::wm::IsMaximizedOrFullscreenOrPinnedWindowStateType(new_type)) {
@@ skipping 11 matching lines @@
   // Re-enable animations if they were disabled in pre state change handler.
   scoped_animations_disabled_.reset();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // aura::WindowObserver overrides:
 
 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()) {
     if (new_bounds.size() == old_bounds.size())
       return;
 
     // If size changed then give the client a chance to produce new contents
     // before origin on screen is changed. Retain the old origin by reverting
     // the origin delta until the next configure is acknowledged.
     gfx::Vector2d delta = new_bounds.origin() - old_bounds.origin();
@@ skipping 230 matching lines @@
       resize_component = window_state->drag_details()->window_component;
   }
 
   uint32_t serial = 0;
   if (!configure_callback_.is_null()) {
     if (widget_) {
       const views::NonClientView* non_client_view = widget_->non_client_view();
       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_);
     }
   }
 
   if (!serial) {
     pending_origin_offset_ += origin_offset;
     pending_resize_component_ = resize_component;
     return;
   }
 
   // Apply origin offset and resize component at the first Commit() after this
@@ skipping 115 matching lines @@
 gfx::Rect ShellSurface::GetVisibleBounds() const {
   // Use |geometry_| if set, otherwise use the visual bounds of the surface.
   return geometry_.IsEmpty() ? gfx::Rect(surface_->window()->layer()->size())
                              : geometry_;
 }
 
 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);
     return origin_ - position.OffsetFromOrigin();
   }
 
   gfx::Rect visible_bounds = GetVisibleBounds();
   gfx::Rect client_bounds =
       widget_->non_client_view()->frame_view()->GetBoundsForClientView();
   switch (resize_component_) {
@@ skipping 42 matching lines @@
   if (!pending_configs_.empty() || scoped_configure_)
     return;
 
   gfx::Rect visible_bounds = GetVisibleBounds();
   gfx::Rect new_widget_bounds =
       widget_->non_client_view()->GetWindowBoundsForClientBounds(
           visible_bounds);
 
   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();
       break;
     case BoundsMode::SHELL:
       // Update widget origin using the surface origin if the current location
       // of surface is being anchored to one side of the widget as a result of a
       // resize operation.
       if (resize_component_ != HTCAPTION) {
         gfx::Point widget_origin =
@@ skipping 42 matching lines @@
     if (shadow_underlay_bounds.IsEmpty())
       shadow_underlay_bounds = gfx::Rect(surface_->window()->bounds().size());
 
     if (!shadow_underlay_in_surface_) {
       shadow_content_bounds = shadow_content_bounds_;
       if (shadow_content_bounds.IsEmpty()) {
         shadow_content_bounds = window->bounds();
       }
     }
 
-    // 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.
     if (!shadow_underlay_) {
       shadow_underlay_ = new aura::Window(nullptr);
       shadow_underlay_event_handler_ =
           base::MakeUnique<ShadowUnderlayEventHandler>();
       shadow_underlay_->SetTargetHandler(shadow_underlay_event_handler_.get());
       DCHECK(shadow_underlay_->owned_by_parent());
       // Ensure the background area inside the shadow is solid black.
@@ skipping 85 matching lines @@
     // small style shadow for them.
     if (!activatable_)
       shadow->SetElevation(wm::ShadowElevation::SMALL);
     // We don't have rounded corners unless frame is enabled.
     if (!frame_enabled_)
       shadow->SetRoundedCornerRadius(0);
   }
 }
 
 }  // namespace exo