vk: Only send visibility / bounds changes if there are real changes.

Whenever the VK visibility / bounds changes, it notifies listeners such
as the WorkspaceLayoutManager. For historical reasons, we always notify
the first time, even if there was actually no change.

For example, the VK starts off with empty "displaced" bounds. If the VK
is opened, the "displaced" bounds can still be empty (as expected).
However, we still notify listeners that the bounds have "changed" to
empty, even though it was already empty.

This can be misleading. It makes more sense to only notify when there is
actually a change in the bounds value.

In fact, [1] relies on this behaviour. Whenever it receives a zero bounds, it
assumes that the VK went from shown -> hidden. The assumption only holds if we
always send real bounds changes.

[1] https://source.chromium.org/chromium/chromium/src/+/master:ash/wm/workspace/workspace_layout_manager.cc;l=249;drc=e3687581e4b1a85562130e9cc1a8765fc5b9022e

Bug: 1030481
Change-Id: I0cdd91e2b334caefd3f10b818d3d4b6b77a2ee93
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2434410Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Auto-Submit: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812235}

ash/keyboard/ui/notification_manager.cc

@@ -9,14 +9,14 @@
 
 namespace keyboard {
 
+template <typename T>
+ValueNotificationConsolidator<T>::ValueNotificationConsolidator(
+    const T& initial_value)
+    : value_(initial_value) {}
+
 template <typename T>
 bool ValueNotificationConsolidator<T>::ShouldSendNotification(
-    const T new_value) {
-  if (never_sent_) {
-    value_ = new_value;
-    never_sent_ = false;
-    return true;
-  }
+    const T& new_value) {
   const bool value_changed = new_value != value_;
   if (value_changed) {
     value_ = new_value;
@@ -24,7 +24,11 @@ bool ValueNotificationConsolidator<T>::ShouldSendNotification(
   return value_changed;
 }
 
-NotificationManager::NotificationManager() = default;
+NotificationManager::NotificationManager()
+    : visibility_(false),
+      visual_bounds_(gfx::Rect()),
+      occluded_bounds_(gfx::Rect()),
+      workspace_displaced_bounds_(gfx::Rect()) {}
 
 void NotificationManager::SendNotifications(
     bool does_occluded_bounds_affect_layout,

ash/keyboard/ui/notification_manager.h

@@ -18,12 +18,11 @@ namespace keyboard {
 template <typename T>
 class ValueNotificationConsolidator {
  public:
-  ValueNotificationConsolidator() {}
+  explicit ValueNotificationConsolidator(const T& initial_value);
 
-  bool ShouldSendNotification(const T new_value);
+  bool ShouldSendNotification(const T& new_value);
 
  private:
-  bool never_sent_ = true;
   T value_;
 };
 

ash/keyboard/ui/notification_manager_unittest.cc

@@ -7,45 +7,52 @@
 
 namespace keyboard {
 
-TEST(NotificationManagerTest, DoesItConsolidate) {
+TEST(NotificationManagerTest, DoesNotSendIfSameAsInitialState) {
   NotificationManager manager;
 
-  ASSERT_TRUE(manager.ShouldSendVisibilityNotification(false));
-  ASSERT_FALSE(manager.ShouldSendVisibilityNotification(false));
-  ASSERT_TRUE(manager.ShouldSendVisibilityNotification(true));
-  ASSERT_TRUE(manager.ShouldSendVisibilityNotification(false));
-  ASSERT_FALSE(manager.ShouldSendVisibilityNotification(false));
+  EXPECT_FALSE(manager.ShouldSendVisibilityNotification(false));
+  EXPECT_FALSE(manager.ShouldSendVisualBoundsNotification(gfx::Rect()));
+  EXPECT_FALSE(manager.ShouldSendOccludedBoundsNotification(gfx::Rect()));
+  EXPECT_FALSE(
+      manager.ShouldSendWorkspaceDisplacementBoundsNotification(gfx::Rect()));
+}
+
+TEST(NotificationManagerTest, ConsolidatesVisibilityChanges) {
+  NotificationManager manager;
+
+  EXPECT_TRUE(manager.ShouldSendVisibilityNotification(true));
+  EXPECT_FALSE(manager.ShouldSendVisibilityNotification(true));
+  EXPECT_TRUE(manager.ShouldSendVisibilityNotification(false));
+}
+
+TEST(NotificationManagerTest, ConsolidatesVisualBoundsChanges) {
+  NotificationManager manager;
 
-  ASSERT_TRUE(
+  EXPECT_TRUE(
       manager.ShouldSendVisualBoundsNotification(gfx::Rect(10, 10, 10, 10)));
-  ASSERT_FALSE(
+  EXPECT_FALSE(
       manager.ShouldSendVisualBoundsNotification(gfx::Rect(10, 10, 10, 10)));
-  ASSERT_TRUE(
+  EXPECT_TRUE(
       manager.ShouldSendVisualBoundsNotification(gfx::Rect(10, 10, 20, 20)));
-
   // This is technically empty
-  ASSERT_TRUE(
+  EXPECT_TRUE(
       manager.ShouldSendVisualBoundsNotification(gfx::Rect(0, 0, 0, 100)));
-
   // This is still empty
-  ASSERT_FALSE(
+  EXPECT_FALSE(
       manager.ShouldSendVisualBoundsNotification(gfx::Rect(0, 0, 100, 0)));
+}
 
-  // Occluded bounds...
-
-  // Start the field with an empty value
-  ASSERT_TRUE(
-      manager.ShouldSendOccludedBoundsNotification(gfx::Rect(0, 0, 0, 0)));
+TEST(NotificationManagerTest, ConsolidatesOccludedBoundsChanges) {
+  NotificationManager manager;
 
   // Still empty
-  ASSERT_FALSE(
+  EXPECT_FALSE(
       manager.ShouldSendOccludedBoundsNotification(gfx::Rect(0, 0, 10, 0)));
 
-  ASSERT_TRUE(
+  EXPECT_TRUE(
       manager.ShouldSendOccludedBoundsNotification(gfx::Rect(0, 0, 10, 10)));
-
   // Different bounds, same size
-  ASSERT_TRUE(
+  EXPECT_TRUE(
       manager.ShouldSendOccludedBoundsNotification(gfx::Rect(30, 30, 10, 10)));
 }
 

ash/wm/always_on_top_controller_unittest.cc

@@ -41,19 +41,22 @@ class AlwaysOnTopControllerTest : public AshTestBase {
 class TestLayoutManager : public WorkspaceLayoutManager {
  public:
   explicit TestLayoutManager(aura::Window* window)
-      : WorkspaceLayoutManager(window), keyboard_bounds_changed_(false) {}
+      : WorkspaceLayoutManager(window),
+        keyboard_displacing_bounds_changed_(false) {}
 
   ~TestLayoutManager() override = default;
 
   void OnKeyboardDisplacingBoundsChanged(const gfx::Rect& bounds) override {
-    keyboard_bounds_changed_ = true;
+    keyboard_displacing_bounds_changed_ = true;
     WorkspaceLayoutManager::OnKeyboardDisplacingBoundsChanged(bounds);
   }
 
-  bool keyboard_bounds_changed() const { return keyboard_bounds_changed_; }
+  bool keyboard_displacing_bounds_changed() const {
+    return keyboard_displacing_bounds_changed_;
+  }
 
  private:
-  bool keyboard_bounds_changed_;
+  bool keyboard_displacing_bounds_changed_;
   DISALLOW_COPY_AND_ASSIGN(TestLayoutManager);
 };
 
@@ -70,13 +73,16 @@ TEST_F(AlwaysOnTopControllerTest, NotifyKeyboardBoundsChanging) {
       controller->always_on_top_controller();
   always_on_top_controller->SetLayoutManagerForTest(base::WrapUnique(manager));
 
-  // Show the keyboard.
+  // Show the keyboard to change the displacing bounds.
   auto* keyboard_controller = keyboard::KeyboardUIController::Get();
-  keyboard_controller->ShowKeyboard(false /* locked */);
+  keyboard_controller->SetKeyboardWindowBounds(gfx::Rect(0, 0, 100, 100));
+  EXPECT_FALSE(manager->keyboard_displacing_bounds_changed());
+
+  keyboard_controller->ShowKeyboard(true /* locked */);
   ASSERT_TRUE(keyboard::WaitUntilShown());
 
   // Verify that test manager was notified of bounds change.
-  ASSERT_TRUE(manager->keyboard_bounds_changed());
+  EXPECT_TRUE(manager->keyboard_displacing_bounds_changed());
 }
 
 TEST_F(AlwaysOnTopControllerTest,