Fix wrong popup offset when changing popup bounds.

ui/views/widget/desktop_aura/desktop_native_widget_aura.cc

 // Copyright (c) 2012 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 "ui/views/widget/desktop_aura/desktop_native_widget_aura.h"
 
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
@@ skipping 132 matching lines @@
     }
     delete this;
   }
 
   void OnWindowBoundsChanged(aura::Window* window,
                              const gfx::Rect& old_bounds,
                              const gfx::Rect& new_bounds) override {
     // The position of the window may have changed. Hence we use SetBounds in
     // place of SetSize. We need to pass the bounds in screen coordinates to
     // the Widget::SetBounds function.
-    if (top_level_widget_ && window == child_window_)
+    if (top_level_widget_ && window == child_window_) {
       top_level_widget_->SetBounds(window->GetBoundsInScreen());

[ananta, 2016/08/09 21:24:07]

The Widget::SetBounds should make it all the way to
HWNDMessageHandler::SetBounds, which should then resize the WindowTreeHost and
with it the underlying aura::Window? Please check why that is not working.

[bokan, 2016/08/11 17:04:08]

The aura::Window *is* resized, the problem is that its offset is set relative to
where it used to be. In the case of this bug its offset is set to (0, -20) since
the popup grows and needs to be repositioned. This causes us to call back in
here and set bounds on the top level widget which sees the -20 in the origin and
moves the widget up by 20 pixels on the screen. But the aura::Window is still
positioned at (0, -20) so the aura::Window is offset by -20 within the widget.

Are you saying there's already code that's supposed to deal with this? I'm
building on Windows now so I can step through HWNDMessageHandler but on Linux it
seems like everything assumes we should be keeping the bounds' origin.

[bokan, 2016/08/11 18:14:24]

Stepping through on Win, we do get through to HWNDMessageHandler::SetBounds but
that only sets the bounds on the native window. It seems that the relationship
is the other way around, resizing/moving the aura::Window causes this callback
which resizes/moves the Widget/native window.

+
+      // The window's position is relative to the widget. If the widget
+      // position changed from the call above then it'll be "double applied".
+      // We clear the origin here to allow the widget to position the window.
+      window->SetBounds(gfx::Rect(gfx::Point(), window->bounds().size()));

[sky, 2016/08/15 15:05:48]

I'm nervous about changing the bounds of the same window that has it's bound
changing. That can easily lead to observers seeing the wrong thing.

This should be done using an aura::LayoutManager. Requests to change a windows
bounds flow through the layoutmanager, so it's the ideal place to change things
like this.

[bokan, 2016/08/18 22:51:06]

Thanks, I'll give that a try. Any advice on where a test for this might go
(sorry, I'm new in this area).

[bokan, 2016/08/19 17:57:46]

I don't think this'll work since the flow through LayoutManager happens before
the NativeWidget gets the bounds from the Window. It seems like there's a
strange circular dependency here where the aura::Window location is relative to
the Widget but the Widget's location is dependent on the Window location.

Put another way, we want to move the popup up 20px so we do this:

1. RWHVA::SetBounds called with new screen coordinates
2. Call Window::SetBounds with parent-relative coordinates y: -20px
3. Call Layer::SetBounds
4. Calls Layer::SetBoundsFromAnimation
5. Calls Window::OnWindowBoundsChanged (this is where the Window::bounds_ is
actually set)
6. Calls Window observer DesktopNativeWidgetAura::OnWindowBoundsChanged
7. Calls Widget::SetBounds passing Window::GetBoundsInScreen which because of
the -20px will move Widget up by 20px on the screen but the Window is still
-20px relative to the Widget.

The LayoutManager path is inserted between steps 2 and 3 so clearing the
location coordinates there means the Widget wouldn't be moved. If the Widget
gets its location from the Window, then changing the Widget's location should
counter move the Window to keep it in the same screen location.

Perhaps the right thing is to not move the Window at all? Can we move the Widget
from the RWHVA::SetBounds and Window::SetBounds using only the size?

+    }
   }
 
  private:
   DesktopNativeWidgetTopLevelHandler()
       : top_level_widget_(NULL),
         child_window_(NULL) {}
 
   ~DesktopNativeWidgetTopLevelHandler() override {}
 
   Widget* top_level_widget_;
@@ skipping 1031 matching lines @@
   if (cursor_reference_count_ == 0) {
     // We are the last DesktopNativeWidgetAura instance, and we are responsible
     // for cleaning up |cursor_manager_|.
     delete cursor_manager_;
     native_cursor_manager_ = NULL;
     cursor_manager_ = NULL;
   }
 }
 
 }  // namespace views