chromeos: ensure client is notified of LocalSurfaceId

The server assigns LocalSurfaceIds (each window may have a
LocalSurfaceId). LocalSurfaceIds change any time the size
changes. This means the server needs to notify the client any time the
bounds change. Normally when a client requests a change that
succeeds the server acks it, and that is it. What we've done with
bounds of roots (where LocalSurfaceIds matter), is when the
client requests the change the server responds with
OnWindowBoundsChanged(), which includes the LocalSurfaceId, and then
the server returns false. By returning false the client applies the
LocalSurfaceId that was just sent from the server.

services/ui/ws had this logic, this makes ws2 have the same logic.

This is at least part of the problem as to why painting isn't working.

BUG=837684
TEST=covered by test

Change-Id: Ia66d3e5b4120adce2a5d033563a0c387c3bd7b7e
Reviewed-on: https://chromium-review.googlesource.com/1066828
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560177}

services/ui/ws2/client_root.cc

@@ -97,11 +97,8 @@ void ClientRoot::OnWindowBoundsChanged(aura::Window* window,
   UpdatePrimarySurfaceId();
   client_surface_embedder_->UpdateSizeAndGutters();
   base::Optional<viz::LocalSurfaceId> surface_id = local_surface_id_;
-  if (window_service_client_->property_change_tracker_
-          ->IsProcessingChangeForWindow(window, ClientChangeType::kBounds)) {
-    // Do not send notifications for changes intiated by the client.
-    return;
-  }
+  // See comments in WindowServiceClient::SetWindowBoundsImpl() for details on
+  // why this always notifies the client.
   window_service_client_->window_tree_client_->OnWindowBoundsChanged(
       window_service_client_->TransportIdForWindow(window), old_bounds,
       new_bounds, std::move(surface_id));

services/ui/ws2/window_service_client.cc

@@ -663,6 +663,8 @@ bool WindowServiceClient::SetWindowBoundsImpl(
     const ClientWindowId& window_id,
     const gfx::Rect& bounds,
     const base::Optional<viz::LocalSurfaceId>& local_surface_id) {
+  // TODO: |local_surface_id| needs to be routed correctly.
+  // https://crbug.com/844789.
   aura::Window* window = GetWindowByClientId(window_id);
 
   DVLOG(3) << "SetWindowBounds window_id=" << window_id
@@ -680,6 +682,9 @@ bool WindowServiceClient::SetWindowBoundsImpl(
     return false;
   }
 
+  if (window->bounds() == bounds)
+    return true;
+
   ClientChange change(property_change_tracker_.get(), window,
                       ClientChangeType::kBounds);
   const gfx::Rect original_bounds = window->bounds();
@@ -687,7 +692,17 @@ bool WindowServiceClient::SetWindowBoundsImpl(
   if (!change.window())
     return true;  // Return value doesn't matter if window destroyed.
 
-  if (change.window()->bounds() == bounds)
+  if (IsClientRootWindow(window)) {
+    // ClientRoot always notifies the client of changes to the bounds. This is
+    // important as OnWindowBoundsChanged() includes a LocalSurfaceId, which is
+    // assigned by the WindowService (not by the client requesting the change).
+    // Returning false ensures the client applies the LocalSurfaceId assigned
+    // by ClientRoot and sent to the client in
+    // ClientRoot::OnWindowBoundsChanged().
+    return false;
+  }
+
+  if (window->bounds() == bounds)
     return true;
 
   if (window->bounds() == original_bounds)

services/ui/ws2/window_service_client_unittest.cc

@@ -153,7 +153,6 @@ TEST(WindowServiceClientTest, NewWindow) {
   ASSERT_TRUE(window);
   EXPECT_EQ("ChangeCompleted id=1 sucess=true",
             SingleChangeToDescription(*helper.changes()));
-  helper.changes()->clear();
 }
 
 TEST(WindowServiceClientTest, NewWindowWithProperties) {
@@ -167,7 +166,6 @@ TEST(WindowServiceClientTest, NewWindowWithProperties) {
   EXPECT_EQ("ChangeCompleted id=1 sucess=true",
             SingleChangeToDescription(*helper.changes()));
   EXPECT_TRUE(window->GetProperty(aura::client::kAlwaysOnTopKey));
-  helper.changes()->clear();
 }
 
 TEST(WindowServiceClientTest, NewTopLevelWindow) {
@@ -177,7 +175,6 @@ TEST(WindowServiceClientTest, NewTopLevelWindow) {
   ASSERT_TRUE(top_level);
   EXPECT_EQ("TopLevelCreated id=1 window_id=0,1 drawn=false",
             SingleChangeToDescription(*helper.changes()));
-  helper.changes()->clear();
 }
 
 TEST(WindowServiceClientTest, NewTopLevelWindowWithProperties) {
@@ -191,7 +188,6 @@ TEST(WindowServiceClientTest, NewTopLevelWindowWithProperties) {
   EXPECT_EQ("TopLevelCreated id=1 window_id=0,1 drawn=false",
             SingleChangeToDescription(*helper.changes()));
   EXPECT_TRUE(top_level->GetProperty(aura::client::kAlwaysOnTopKey));
-  helper.changes()->clear();
 }
 
 TEST(WindowServiceClientTest, SetTopLevelWindowBounds) {
@@ -202,7 +198,17 @@ TEST(WindowServiceClientTest, SetTopLevelWindowBounds) {
   const gfx::Rect bounds_from_client = gfx::Rect(1, 2, 300, 400);
   helper.helper()->SetWindowBounds(top_level, bounds_from_client, 2);
   EXPECT_EQ(bounds_from_client, top_level->bounds());
-  EXPECT_EQ("ChangeCompleted id=2 sucess=true",
+  ASSERT_EQ(2u, helper.changes()->size());
+  {
+    const Change& change = (*helper.changes())[0];
+    EXPECT_EQ(CHANGE_TYPE_NODE_BOUNDS_CHANGED, change.type);
+    EXPECT_EQ(top_level->bounds(), change.bounds2);
+    EXPECT_TRUE(change.local_surface_id);
+    helper.changes()->erase(helper.changes()->begin());
+  }
+  // See comments in WindowServiceClient::SetBoundsImpl() for why this returns
+  // false.
+  EXPECT_EQ("ChangeCompleted id=2 sucess=false",
             SingleChangeToDescription(*helper.changes()));
   helper.changes()->clear();
 
@@ -231,7 +237,6 @@ TEST(WindowServiceClientTest, SetTopLevelWindowBounds) {
   // And because the layout manager changed the bounds the result is false.
   EXPECT_EQ("ChangeCompleted id=3 sucess=false",
             ChangeToDescription((*helper.changes())[1]));
-  helper.changes()->clear();
 }
 
 // Tests the ability of the client to change properties on the server.
@@ -257,7 +262,6 @@ TEST(WindowServiceClientTest, SetTopLevelWindowProperty) {
       "PropertyChanged window=0,1 key=prop:always_on_top "
       "value=0000000000000000",
       SingleChangeToDescription(*helper.changes()));
-  helper.changes()->clear();
 }
 
 TEST(WindowServiceClientTest, WindowToWindowData) {