exo: Fix dragging edge cases

components/exo/shell_surface.h

Index: components/exo/shell_surface.h
diff --git a/components/exo/shell_surface.h b/components/exo/shell_surface.h
index 54a5081659e42fe0a08aeb5144751a65fc1ffc01..344e8ce4ff598ef64ecfedb7a42be115df9225ca 100644
--- a/components/exo/shell_surface.h
+++ b/components/exo/shell_surface.h
@@ -34,6 +34,7 @@ class TracedValue;
 }
 
 namespace exo {
+class Display;
 class Surface;
 
 // This class provides functions for treating a surfaces like toplevel,
@@ -46,14 +47,16 @@ class ShellSurface : public SurfaceDelegate,
                      public ash::AccessibilityObserver,
                      public ash::wm::WindowStateObserver,
                      public aura::WindowObserver,
-                     public WMHelper::ActivationObserver {
+                     public WMHelper::ActivationObserver,
+                     public WMHelper::ShellObserver {
  public:
-  ShellSurface(Surface* surface,
+  ShellSurface(const Display& display,
+               Surface* surface,
                ShellSurface* parent,
                const gfx::Rect& initial_bounds,
                bool activatable,
                int container);
-  explicit ShellSurface(Surface* surface);
+  ShellSurface(const Display& display, Surface* surface);
   ~ShellSurface() override;
 
   // Set the callback to run when the user wants the shell surface to be closed.
@@ -111,6 +114,10 @@ class ShellSurface : public SurfaceDelegate,
   // Restore the shell surface.
   void Restore();
 
+  // Start and stop a client-driven move of the shell surface.
+  void SetMoving();
+  void UnsetMoving(bool revert);
+
   // Set fullscreen state for shell surface.
   void SetFullscreen(bool fullscreen);
 
@@ -217,6 +224,10 @@ class ShellSurface : public SurfaceDelegate,
       aura::Window* gained_active,
       aura::Window* lost_active) override;
 
+  // Overridden from WMHelper::ShellObserver:
+  void OnOverviewModeStarted() override;
+  void OnOverviewModeEnded() override;
+
   // Overridden from ui::EventHandler:
   void OnKeyEvent(ui::KeyEvent* event) override;
   void OnMouseEvent(ui::MouseEvent* event) override;
@@ -251,6 +262,9 @@ class ShellSurface : public SurfaceDelegate,
   // End current drag operation.
   void EndDrag(bool revert);
 
+  // End current drag or client-driven move.
+  void EndDragOrMove(bool revert);
+
   // Returns true if surface is currently being resized.
   bool IsResizing() const;
 
@@ -264,10 +278,17 @@ class ShellSurface : public SurfaceDelegate,
   // Updates the bounds of widget to match the current surface bounds.
   void UpdateWidgetBounds();
 
+  // Updates the bounds of surface to match the current widget bounds.
+  void UpdateSurfaceBounds();
+
   // Creates, deletes and update the shadow bounds based on
   // |pending_shadow_content_bounds_|.
   void UpdateShadow();
 
+  // In the coordinate system of the parent root window.
+  gfx::Point GetMouseLocation() const;
+
+  const Display& display_;

[oshima, 2016/10/12 01:56:54]

can you use const Display* ?

[Dominik Laskowski, 2016/10/13 03:21:17]

It should never be null, and the reference makes that clear.

[oshima, 2016/10/17 17:22:58]

It doesn't prevent from passing null reference. assertion (DCHECK/DCHECK) is
better choice IMO, but I'll leave this to the owner.

[Dominik Laskowski, 2016/10/18 21:22:56]

The caller would have to dereference a null pointer though. I don't mind
changing it to a pointer, but I feel this is self-documenting.

reveman: let me know what style you prefer.

   views::Widget* widget_ = nullptr;
   Surface* surface_;
   aura::Window* parent_;
@@ -287,6 +308,7 @@ class ShellSurface : public SurfaceDelegate,
   StateChangedCallback state_changed_callback_;
   ConfigureCallback configure_callback_;
   ScopedConfigure* scoped_configure_ = nullptr;
+  bool ignore_widget_bounds_changes_ = false;
   bool ignore_window_bounds_changes_ = false;
   gfx::Point origin_;
   gfx::Vector2d pending_origin_offset_;
@@ -299,6 +321,7 @@ class ShellSurface : public SurfaceDelegate,
   gfx::Rect shadow_content_bounds_;
   std::deque<Config> pending_configs_;
   std::unique_ptr<ash::WindowResizer> resizer_;
+  std::unique_ptr<ash::WindowResizer> move_resizer_;

[oshima, 2016/10/12 01:56:54]

the original resizer_ is also used to drag. How about

phantom_window_resizer?

I also wonder if we really need two resizer. If we just want to create a phantom
window, you may not need resizer.

[Dominik Laskowski, 2016/10/13 03:21:17]

The pointer is also used to check whether a client-driven move is in progress,
so that name would be confusing.

We need all the logic in DragWindowResizer except resizer chaining. We could
factor it out into another class, but chaining a dummy resizer is simpler.

[oshima, 2016/10/17 17:22:58]

Hmm, then I may be reading this wrong. Can you explain why we need two resizer?

[Dominik Laskowski, 2016/10/18 21:22:56]

There are three states we want to detect:

         No drag: !resizer_ && !move_resizer_
Client-side drag: !resizer_ &&  move_resizer_
Server-side drag:  resizer_ && !move_resizer_

[Dominik Laskowski, 2016/10/21 03:05:19]

After an offline chat with oshima, the suggestion was to use a single resizer,
and detect the above states using other member variables. However, this would
complicate the code, because the only way to distinguish a remote shell surface
is:

!initial_bounds_.IsEmpty() && !parent_

reveman: do you prefer the two resizers? If not, could we introduce explicit
state to tell shell surfaces apart, e.g. an enum for regular/pop-up/remote? That
would be contradictory with the idea of moving concepts specific to the remote
shell (e.g. virtual coordinates) out of ShellSurface. On the other hand, it
would simplify a lot of conditions that implicitly depend on otherwise unused
initial state, e.g. gfx::Size(1, 1).

   std::unique_ptr<ScopedAnimationsDisabled> scoped_animations_disabled_;
   int top_inset_height_ = 0;
   int pending_top_inset_height_ = 0;