chromeos: makes SetWindowBounds() check LocalSurfaceId too

If the bounds don't change, but the LocalSurfaceId does, then the request needs
to be processed. This is important for windows that have LocalSurfaceId, such as
at the embedding.

This bug resulted in the ksv not drawing correctly initially.

BUG=837684
TEST=covered by test

Change-Id: I0bf67a15bc2af0ae42df07378908d3c1a8d90885
Reviewed-on: https://chromium-review.googlesource.com/1070689
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561328}

services/ui/ws2/window_service_client.cc

@@ -411,6 +411,14 @@ void WindowServiceClient::OnWillBecomeClientRootWindow(aura::Window* window) {
     window->RemoveChild(window->children().front());
 }
 
+base::Optional<viz::LocalSurfaceId> WindowServiceClient::GetLocalSurfaceId(
+    aura::Window* window) {
+  auto iter = FindClientRootWithRoot(window);
+  if (iter == client_roots_.end())
+    return base::nullopt;
+  return iter->get()->GetLocalSurfaceId();
+}
+
 bool WindowServiceClient::NewWindowImpl(
     const ClientWindowId& client_window_id,
     const std::map<std::string, std::vector<uint8_t>>& properties) {
@@ -687,8 +695,10 @@ bool WindowServiceClient::SetWindowBoundsImpl(
     return false;
   }
 
-  if (window->bounds() == bounds)
+  if (window->bounds() == bounds &&
+      GetLocalSurfaceId(window) == local_surface_id) {
     return true;
+  }
 
   ClientChange change(property_change_tracker_.get(), window,
                       ClientChangeType::kBounds);
@@ -698,18 +708,14 @@ bool WindowServiceClient::SetWindowBoundsImpl(
     return true;  // Return value doesn't matter if window destroyed.
 
   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 handles notification in this case. Note that this
+    // unconditionally returns false, because the LocalSurfaceId changes with
+    // the bounds. 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)
     return false;
 
@@ -952,8 +958,6 @@ void WindowServiceClient::SetWindowBounds(
     Id window_id,
     const gfx::Rect& bounds,
     const base::Optional<viz::LocalSurfaceId>& local_surface_id) {
-  // TODO: figure out if need to pass through |local_surface_id|. Doesn't
-  // matter in all cases.
   window_tree_client_->OnChangeCompleted(
       change_id, SetWindowBoundsImpl(MakeClientWindowId(window_id), bounds,
                                      local_surface_id));

services/ui/ws2/window_service_client.h

@@ -12,6 +12,7 @@
 #include "base/component_export.h"
 #include "base/containers/flat_map.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "services/ui/public/interfaces/window_tree.mojom.h"
 #include "services/ui/ws2/focus_handler.h"
 #include "services/ui/ws2/ids.h"
@@ -196,6 +197,10 @@ class COMPONENT_EXPORT(WINDOW_SERVICE) WindowServiceClient
   // only be the root Window of a single ClientRoot).
   void OnWillBecomeClientRootWindow(aura::Window* window);
 
+  // Returns the LocalSurfaceId for |window|. If |window| is not a ClientRoot,
+  // this returns null.
+  base::Optional<viz::LocalSurfaceId> GetLocalSurfaceId(aura::Window* window);
+
   // Methods with the name Impl() mirror those of mojom::WindowTree. The return
   // value indicates whether they succeeded or not. Generally failure means the
   // operation was not allowed.

services/ui/ws2/window_service_client_test_helper.cc

@@ -59,7 +59,15 @@ bool WindowServiceClientTestHelper::ReleaseCapture(aura::Window* window) {
       ClientWindowIdForWindow(window));
 }
 
-void WindowServiceClientTestHelper::SetWindowBounds(aura::Window* window,
+bool WindowServiceClientTestHelper::SetWindowBounds(aura::Window* window,
+                                                    const gfx::Rect& bounds) {
+  base::Optional<viz::LocalSurfaceId> local_surface_id;
+  return window_service_client_->SetWindowBoundsImpl(
+      ClientWindowIdForWindow(window), bounds, local_surface_id);
+}
+
+void WindowServiceClientTestHelper::SetWindowBoundsWithAck(
+    aura::Window* window,
     const gfx::Rect& bounds,
     uint32_t change_id) {
   base::Optional<viz::LocalSurfaceId> local_surface_id;

services/ui/ws2/window_service_client_test_helper.h

@@ -56,7 +56,10 @@ class WindowServiceClientTestHelper {
       base::flat_map<std::string, std::vector<uint8_t>> properties = {});
   bool SetCapture(aura::Window* window);
   bool ReleaseCapture(aura::Window* window);
-  void SetWindowBounds(aura::Window* window,
+  bool SetWindowBounds(aura::Window* window, const gfx::Rect& bounds);
+  // Same as SetWindowBounds(), but called in such a way that the ack
+  // (OnChangeCompleted()) is called on the client.
+  void SetWindowBoundsWithAck(aura::Window* window,
                               const gfx::Rect& bounds,
                               uint32_t change_id = 1);
   void SetClientArea(

services/ui/ws2/window_service_client_unittest.cc

@@ -112,7 +112,8 @@ TEST(WindowServiceClientTest, SetTopLevelWindowBounds) {
   setup.changes()->clear();
 
   const gfx::Rect bounds_from_client = gfx::Rect(1, 2, 300, 400);
-  setup.client_test_helper()->SetWindowBounds(top_level, bounds_from_client, 2);
+  setup.client_test_helper()->SetWindowBoundsWithAck(top_level,
+                                                     bounds_from_client, 2);
   EXPECT_EQ(bounds_from_client, top_level->bounds());
   ASSERT_EQ(2u, setup.changes()->size());
   {
@@ -142,7 +143,8 @@ TEST(WindowServiceClientTest, SetTopLevelWindowBounds) {
   const gfx::Rect restricted_bounds = gfx::Rect(401, 405, 406, 407);
   layout_manager->set_next_bounds(restricted_bounds);
   top_level->parent()->SetLayoutManager(layout_manager);
-  setup.client_test_helper()->SetWindowBounds(top_level, bounds_from_client, 3);
+  setup.client_test_helper()->SetWindowBoundsWithAck(top_level,
+                                                     bounds_from_client, 3);
   ASSERT_EQ(2u, setup.changes()->size());
   // The layout manager changes the bounds to a different value than the client
   // requested, so the client should get OnWindowBoundsChanged() with
@@ -155,6 +157,21 @@ TEST(WindowServiceClientTest, SetTopLevelWindowBounds) {
             ChangeToDescription((*setup.changes())[1]));
 }
 
+TEST(WindowServiceClientTest, SetTopLevelWindowBoundsFailsForSameSize) {
+  WindowServiceTestSetup setup;
+  aura::Window* top_level = setup.client_test_helper()->NewTopLevelWindow(1);
+  setup.changes()->clear();
+  const gfx::Rect bounds = gfx::Rect(1, 2, 300, 400);
+  top_level->SetBounds(bounds);
+  setup.changes()->clear();
+  // WindowServiceClientTestHelper::SetWindowBounds() uses a null
+  // LocalSurfaceId, which differs from the current LocalSurfaceId (assigned by
+  // ClientRoot). Because of this, the LocalSurfaceIds differ and the call
+  // returns false.
+  EXPECT_FALSE(setup.client_test_helper()->SetWindowBounds(top_level, bounds));
+  EXPECT_TRUE(setup.changes()->empty());
+}
+
 // Tests the ability of the client to change properties on the server.
 TEST(WindowServiceClientTest, SetTopLevelWindowProperty) {
   WindowServiceTestSetup setup;