Snap RWHVA and resize the legacy window on Windows whenever the ancestor window's bounds change.

content/browser/renderer_host/render_widget_host_view_aura.cc

Index: content/browser/renderer_host/render_widget_host_view_aura.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_aura.cc b/content/browser/renderer_host/render_widget_host_view_aura.cc
index 146355707f08f247446929d5c6c0d22d7b5e11e2..caa49977841b9e4ccc2ebaac80d1a72cfcaf165f 100644
--- a/content/browser/renderer_host/render_widget_host_view_aura.cc
+++ b/content/browser/renderer_host/render_widget_host_view_aura.cc
@@ -4,6 +4,8 @@
 
 #include "content/browser/renderer_host/render_widget_host_view_aura.h"
 
+#include <set>
+
 #include "base/auto_reset.h"
 #include "base/basictypes.h"
 #include "base/bind.h"
@@ -352,12 +354,68 @@ class RenderWidgetHostViewAura::WindowObserver : public aura::WindowObserver {
       view_->RemovingFromRootWindow();
   }
 
+  void OnWindowHierarchyChanged(const HierarchyChangeParams& params) override {
+    view_->ParentHierarchyChanged();
+  }
+
  private:
   RenderWidgetHostViewAura* view_;
 
   DISALLOW_COPY_AND_ASSIGN(WindowObserver);
 };
 
+// This class provides functionality to observe the ancestors of the RWHVA

[sky, 2015/04/10 15:20:21]

This comment is misleading. Isn't the issue that we need to position on pixel
pixel boundaries, which may change when the bounds relative to the root changes?

[ananta, 2015/04/10 17:51:22]

Reworded

+// window for bounds changes and snap the RWHVA instance to pixel boundaries.
+// Reason for doing this is there are cases like the fast resize code path for
+// bookmarks where in the parent of RWHVA which is WCV has its bounds changed
+// before the bookmark is hidden. This results in it reporting the old bounds
+// which includes the bookmark. Eventually when it does get the correct bounds
+// after the bar is hidden, the layer ignores the bounds changed call as the
+// bounds are the same. To work around this we observe the
+// ancestors of the RWHVA window for bounds changed notifications and snap when
+// we receive them.
+class RenderWidgetHostViewAura::WindowAncestorObserver
+    : public aura::WindowObserver {
+ public:
+  explicit WindowAncestorObserver(RenderWidgetHostViewAura* view)
+      : view_(view) {
+    aura::Window* parent = view_->window_->parent();

[sky, 2015/04/10 15:20:21]

Isn't this a lot simpler!

[ananta, 2015/04/10 17:51:21]

Yes. Thanks

+    while (parent) {

[sky, 2015/04/10 15:20:21]

Who handles positioning when the bounds of view_->window_ changes?

[ananta, 2015/04/10 17:51:22]

RWHVA in its OnBoundsChanged function via the Window delegate.

+      parent->AddObserver(this);
+      ancestor_observer_tracker_.insert(parent);
+      parent = parent->parent();
+    }
+  }
+
+  ~WindowAncestorObserver() override {
+    RemoveAncestorObservers();
+  }
+
+  void OnWindowBoundsChanged(aura::Window* window,
+                             const gfx::Rect& old_bounds,
+                             const gfx::Rect& new_bounds) override {
+    if (ancestor_observer_tracker_.find(window) !=

[sky, 2015/04/10 15:20:21]

Shouldn't this be a DCHECK?

[ananta, 2015/04/10 17:51:21]

There is a DCHECK in the else. Were you indicating something else?

[sky, 2015/04/10 19:36:25]

I'm saying you should convert the if to a DCHECK and remove the DCHECK on 402.

+        ancestor_observer_tracker_.end()) {
+      if (new_bounds.origin() != old_bounds.origin())
+        view_->HandleParentBoundsChanged();
+    } else {
+      DCHECK(false);
+    }
+  }
+
+ private:
+  void RemoveAncestorObservers() {
+    for (auto ancestor : ancestor_observer_tracker_)
+      ancestor->RemoveObserver(this);
+    ancestor_observer_tracker_.clear();
+  }
+
+  RenderWidgetHostViewAura* view_;
+  std::set<aura::Window*> ancestor_observer_tracker_;

[sky, 2015/04/10 15:20:21]

Why the observer_tracker_? How about just ancestors_?

[ananta, 2015/04/10 17:51:21]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(WindowAncestorObserver);
+};
+
 ////////////////////////////////////////////////////////////////////////////////
 // RenderWidgetHostViewAura, public:
 
@@ -395,6 +453,7 @@ RenderWidgetHostViewAura::RenderWidgetHostViewAura(RenderWidgetHost* host,
     host_->SetView(this);
 
   window_observer_.reset(new WindowObserver(this));
+
   aura::client::SetTooltipText(window_, &tooltip_);
   aura::client::SetActivationDelegate(window_, this);
   aura::client::SetFocusChangeObserver(window_, this);
@@ -734,6 +793,22 @@ bool RenderWidgetHostViewAura::CanRendererHandleEvent(
   return true;
 }
 
+void RenderWidgetHostViewAura::HandleParentBoundsChanged() {
+  SnapToPhysicalPixelBoundary();
+#if defined(OS_WIN)
+  if (legacy_render_widget_host_HWND_) {
+    legacy_render_widget_host_HWND_->SetBounds(
+        window_->GetBoundsInRootWindow());
+  }
+#endif
+}
+
+void RenderWidgetHostViewAura::ParentHierarchyChanged() {
+  ancestor_window_observer_.reset(new WindowAncestorObserver(this));
+  // Snap when we receive a hierarchy changed. http://crbug.com/388908.

[sky, 2015/04/10 15:20:21]

I shouldn't have to look at a bug to see why we need to snap when hierarchy
changes. Document why.

+  HandleParentBoundsChanged();
+}
+
 void RenderWidgetHostViewAura::MovePluginWindows(
     const std::vector<WebPluginGeometry>& plugin_window_moves) {
 #if defined(OS_WIN)