[X11] Fix GeometryCache not notifying the observer in some cases

Previously, it was possible for geometry changes to not notify
observers during reparent events.  This occurred because geometry
updates were not sent while the parent cache was rebuilt.

This CL fixes the issue by adding an explicit last_notified_bounds_
and simplifies the logic of OnParentChanged and OnGeometryChanged.

R=sky

Change-Id: Ic93d982a1604b15375e0cc3d2d3d7ce6b13e0137
Fixed: 1510426
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5124460
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Auto-Submit: Thomas Anderson <thomasanderson@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1238174}

ui/gfx/x/geometry_cache.cc

@@ -63,10 +63,6 @@ void GeometryCache::OnGetGeometryResponse(GetGeometryResponse response) {
 }
 
 void GeometryCache::OnParentChanged(Window parent, const gfx::Point& position) {
-  const bool was_ready = Ready();
-  bool parent_changed = true;
-  const gfx::Rect old_geometry = geometry_;
-
   have_parent_ = true;
   if (parent == Window::None) {
     parent_.reset();
@@ -75,32 +71,18 @@ void GeometryCache::OnParentChanged(Window parent, const gfx::Point& position) {
         connection_, parent,
         base::BindRepeating(&GeometryCache::OnParentGeometryChanged,
                             weak_ptr_factory_.GetWeakPtr()));
-  } else {
-    parent_changed = false;
   }
 
-  const bool position_changed = position != geometry_.origin();
   geometry_.set_origin(position);
 
-  if (Ready() && (!was_ready || parent_changed || position_changed)) {
-    bounds_changed_callback_.Run(old_geometry, geometry_);
-  }
+  NotifyGeometryChanged();
 }
 
 void GeometryCache::OnGeometryChanged(const gfx::Rect& geometry) {
-  const bool was_ready = Ready();
-  const bool geometry_changed = geometry_ != geometry;
-  const gfx::Rect old_geometry = geometry_;
-
   have_geometry_ = true;
   geometry_ = geometry;
 
-  if (Ready() && (!was_ready || geometry_changed)) {
-    auto parent_offset =
-        parent_ ? parent_->GetBoundsPx().OffsetFromOrigin() : gfx::Vector2d();
-    bounds_changed_callback_.Run(old_geometry + parent_offset,
-                                 geometry_ + parent_offset);
-  }
+  NotifyGeometryChanged();
 }
 
 bool GeometryCache::Ready() const {
@@ -108,13 +90,24 @@ bool GeometryCache::Ready() const {
 }
 
 void GeometryCache::OnParentGeometryChanged(
-    const gfx::Rect& old_parent_bounds,
+    const absl::optional<gfx::Rect>& old_parent_bounds,
     const gfx::Rect& new_parent_bounds) {
-  if (have_geometry_) {
-    bounds_changed_callback_.Run(
-        geometry_ + old_parent_bounds.OffsetFromOrigin(),
-        geometry_ + new_parent_bounds.OffsetFromOrigin());
+  NotifyGeometryChanged();
+}
+
+void GeometryCache::NotifyGeometryChanged() {
+  if (!Ready()) {
+    return;
   }
+
+  auto geometry = GetBoundsPx();
+  if (last_notified_geometry_ == geometry) {
+    return;
+  }
+
+  auto old_geometry = last_notified_geometry_;
+  last_notified_geometry_ = geometry;
+  bounds_changed_callback_.Run(old_geometry, geometry);
 }
 
 void GeometryCache::OnEvent(const Event& xevent) {

ui/gfx/x/geometry_cache.h

@@ -22,7 +22,8 @@ namespace x11 {
 class COMPONENT_EXPORT(X11) GeometryCache final : public EventObserver {
  public:
   using BoundsChangedCallback =
-      base::RepeatingCallback<void(const gfx::Rect&, const gfx::Rect&)>;
+      base::RepeatingCallback<void(const absl::optional<gfx::Rect>&,
+                                   const gfx::Rect&)>;
 
   GeometryCache(Connection* connection,
                 Window window,
@@ -44,8 +45,11 @@ class COMPONENT_EXPORT(X11) GeometryCache final : public EventObserver {
 
   bool Ready() const;
 
-  void OnParentGeometryChanged(const gfx::Rect& old_parent_bounds,
-                               const gfx::Rect& new_parent_bounds);
+  void OnParentGeometryChanged(
+      const absl::optional<gfx::Rect>& old_parent_bounds,
+      const gfx::Rect& new_parent_bounds);
+
+  void NotifyGeometryChanged();
 
   // EventObserver:
   void OnEvent(const Event& xevent) override;
@@ -60,6 +64,7 @@ class COMPONENT_EXPORT(X11) GeometryCache final : public EventObserver {
   bool have_geometry_ = false;
   std::unique_ptr<GeometryCache> parent_;
   gfx::Rect geometry_;
+  absl::optional<gfx::Rect> last_notified_geometry_ = absl::nullopt;
 
   ScopedEventSelector window_events_;
 

ui/gfx/x/geometry_cache_unittest.cc

@@ -49,8 +49,8 @@ TEST(GeometryCacheTest, ConstructAndDestruct) {
                       gfx::Rect(12, 34, 56, 78));
   GeometryCache geometry_cache(
       connection, window.id(),
-      base::BindRepeating(
-          [](const gfx::Rect& old_bounds, const gfx::Rect& new_bounds) {}));
+      base::BindRepeating([](const absl::optional<gfx::Rect>& old_bounds,
+                             const gfx::Rect& new_bounds) {}));
 }
 
 TEST(GeometryCacheTest, GetBounds) {
@@ -59,8 +59,8 @@ TEST(GeometryCacheTest, GetBounds) {
   ScopedWindow window(connection, connection->default_root(), bounds);
   GeometryCache geometry_cache(
       connection, window.id(),
-      base::BindRepeating(
-          [](const gfx::Rect& old_bounds, const gfx::Rect& new_bounds) {}));
+      base::BindRepeating([](const absl::optional<gfx::Rect>& old_bounds,
+                             const gfx::Rect& new_bounds) {}));
 
   // Calling GetBoundsPx() should return the initial bounds of the window.
   EXPECT_EQ(geometry_cache.GetBoundsPx(), bounds);
@@ -87,9 +87,11 @@ TEST(GeometryCacheTest, BoundsChangedCallback) {
   ScopedWindow window(connection, connection->default_root(), bounds);
 
   absl::optional<gfx::Rect> last_bounds;
-  auto bounds_changed_callback =
-      [](absl::optional<gfx::Rect>* last_bounds, const gfx::Rect& old_bounds,
-         const gfx::Rect& new_bounds) { *last_bounds = new_bounds; };
+  auto bounds_changed_callback = [](absl::optional<gfx::Rect>* last_bounds,
+                                    const absl::optional<gfx::Rect>& old_bounds,
+                                    const gfx::Rect& new_bounds) {
+    *last_bounds = new_bounds;
+  };
 
   GeometryCache geometry_cache(
       connection, window.id(),
@@ -133,8 +135,8 @@ TEST(GeometryCacheTest, NestedWindows) {
   // Set up the GeometryCache for the child window.
   GeometryCache child_geometry_cache(
       connection, child_window.id(),
-      base::BindRepeating(
-          [](const gfx::Rect& old_bounds, const gfx::Rect& new_bounds) {}));
+      base::BindRepeating([](const absl::optional<gfx::Rect>& old_bounds,
+                             const gfx::Rect& new_bounds) {}));
 
   // Initial bounds should be the sum of child window position and parent window
   // position.

ui/ozone/platform/x11/x11_window.cc

@@ -2624,10 +2624,12 @@ void X11Window::OnWmSynced() {
   OnBoundsChanged(last_set_bounds_px_, GetBoundsInPixels());
 }
 
-void X11Window::OnBoundsChanged(const gfx::Rect& old_bounds_px,
+void X11Window::OnBoundsChanged(const absl::optional<gfx::Rect>& old_bounds_px,
                                 const gfx::Rect& new_bounds_px) {
-  const bool size_changed = old_bounds_px.size() != new_bounds_px.size();
-  const bool origin_changed = old_bounds_px.origin() != new_bounds_px.origin();
+  const bool size_changed = !old_bounds_px.has_value() ||
+                            old_bounds_px->size() != new_bounds_px.size();
+  const bool origin_changed = !old_bounds_px.has_value() ||
+                              old_bounds_px->origin() != new_bounds_px.origin();
 
   if (size_changed) {
     DispatchResize(origin_changed);

ui/ozone/platform/x11/x11_window.h

@@ -328,7 +328,7 @@ class X11Window : public PlatformWindow,
 
   void OnWmSynced();
 
-  void OnBoundsChanged(const gfx::Rect& old_bounds_px,
+  void OnBoundsChanged(const absl::optional<gfx::Rect>& old_bounds_px,
                        const gfx::Rect& new_bounds_px);
 
   // Stores current state of this window.