Adds a CloneAndAnimate function to WindowManagerInternalClient

mojo/services/view_manager/view_manager_service_unittest.cc

Index: mojo/services/view_manager/view_manager_service_unittest.cc
diff --git a/mojo/services/view_manager/view_manager_service_unittest.cc b/mojo/services/view_manager/view_manager_service_unittest.cc
index 1079c933283020dc58a990dab04c3db91731a698..2e160b5a4ee576fe766e1ad68e5eb97b22762724 100644
--- a/mojo/services/view_manager/view_manager_service_unittest.cc
+++ b/mojo/services/view_manager/view_manager_service_unittest.cc
@@ -5,7 +5,7 @@
 #include <string>
 #include <vector>
 
-#include "mojo/common/common_type_converters.h"
+#include "base/message_loop/message_loop.h"
 #include "mojo/converters/geometry/geometry_type_converters.h"
 #include "mojo/public/interfaces/application/service_provider.mojom.h"
 #include "mojo/services/public/cpp/view_manager/types.h"
@@ -18,6 +18,7 @@
 #include "mojo/services/view_manager/connection_manager_delegate.h"
 #include "mojo/services/view_manager/display_manager.h"
 #include "mojo/services/view_manager/ids.h"
+#include "mojo/services/view_manager/server_view.h"
 #include "mojo/services/view_manager/test_change_tracker.h"
 #include "mojo/services/view_manager/view_manager_service_impl.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -113,12 +114,14 @@ class TestClientConnection : public ClientConnection {
 // Empty implementation of ConnectionManagerDelegate.
 class TestConnectionManagerDelegate : public ConnectionManagerDelegate {
  public:
-  TestConnectionManagerDelegate() : last_client_(nullptr) {}
+  TestConnectionManagerDelegate() : last_connection_(nullptr) {}
   ~TestConnectionManagerDelegate() override {}
 
-  // TestViewManagerClient that was created by the last
-  // CreateClientConnectionForEmbedAtView() call.
-  TestViewManagerClient* last_client() { return last_client_; }
+  TestViewManagerClient* last_client() {
+    return last_connection_ ? last_connection_->client() : nullptr;
+  }
+
+  TestClientConnection* last_connection() { return last_connection_; }
 
  private:
   // ConnectionManagerDelegate:
@@ -132,12 +135,11 @@ class TestConnectionManagerDelegate : public ConnectionManagerDelegate {
       const ViewId& root_id) override {
     scoped_ptr<ViewManagerServiceImpl> service(new ViewManagerServiceImpl(
         connection_manager, creator_id, creator_url, url, root_id));
-    TestClientConnection* connection = new TestClientConnection(service.Pass());
-    last_client_ = connection->client();
-    return connection;
+    last_connection_ = new TestClientConnection(service.Pass());
+    return last_connection_;
   }
 
-  TestViewManagerClient* last_client_;
+  TestClientConnection* last_connection_;
 
   DISALLOW_COPY_AND_ASSIGN(TestConnectionManagerDelegate);
 };
@@ -186,42 +188,233 @@ class ViewManagerServiceTest : public testing::Test {
   ViewManagerServiceTest() : wm_client_(nullptr) {}
   ~ViewManagerServiceTest() override {}
 
+  // ViewManagerServiceImpl for the window manager.
+  ViewManagerServiceImpl* wm_connection() {
+    return connection_manager_->GetConnection(1);
+  }
+
+  TestViewManagerClient* last_view_manager_client() {
+    return delegate_.last_client();
+  }
+
+  TestClientConnection* last_client_connection() {
+    return delegate_.last_connection();
+  }
+
+  ConnectionManager* connection_manager() { return connection_manager_.get(); }
+
+  TestViewManagerClient* wm_client() { return wm_client_; }
+
  protected:
   // testing::Test:
   void SetUp() override {
     connection_manager_.reset(new ConnectionManager(
         &delegate_, scoped_ptr<DisplayManager>(new TestDisplayManager),
         &wm_internal_));
-
     scoped_ptr<ViewManagerServiceImpl> service(new ViewManagerServiceImpl(
         connection_manager_.get(), kInvalidConnectionId, std::string(),
         std::string("mojo:window_manager"), RootViewId()));
     scoped_ptr<TestClientConnection> client_connection(
         new TestClientConnection(service.Pass()));
     wm_client_ = client_connection->client();
+    ASSERT_TRUE(wm_client_ != nullptr);
     connection_manager_->SetWindowManagerClientConnection(
         client_connection.Pass());
+    ASSERT_TRUE(wm_connection() != nullptr);
+    ASSERT_TRUE(wm_connection()->root() != nullptr);
   }
 
-  // ViewManagerServiceImpl for the window manager.
-  ViewManagerServiceImpl* wm_connection() {
-    return connection_manager_->GetConnection(1);
-  }
-
+ private:
   // TestViewManagerClient that is used for the WM connection.
   TestViewManagerClient* wm_client_;
 
- private:
   TestWindowManagerInternal wm_internal_;
   TestConnectionManagerDelegate delegate_;
   scoped_ptr<ConnectionManager> connection_manager_;
+  base::MessageLoop message_loop_;
 
   DISALLOW_COPY_AND_ASSIGN(ViewManagerServiceTest);
 };
 
-TEST_F(ViewManagerServiceTest, Basic) {
-  wm_connection()->CreateView(ViewId(wm_connection()->id(), 1));
-  ASSERT_EQ(1u, wm_client_->tracker()->changes()->size());
+const ServerView* GetFirstCloned(const ServerView* view) {

[msw, 2014/11/18 23:37:43]

nit: move to anon namespace above?

[sky, 2014/11/19 00:49:40]

I placed it here to reinforce it's for the clone tests.

[msw, 2014/11/19 01:27:51]

Acknowledged.

+  for (const ServerView* child : view->GetChildren()) {
+    if (child->id() == ClonedViewId())
+      return child;
+  }
+  return nullptr;
+}
+
+// Provides common setup for animation tests. Creates the following views:
+// 0,1 (the root, provided by view manager)
+//   1,1

[msw, 2014/11/18 23:37:43]

nit: annotate this view (this comment block is great!)

[sky, 2014/11/19 00:49:40]

Done.

+//     2,1 bounds=1,2 11x22
+//       2,2 bounds=2,3 6x7
+//         2,3 bounds=3,4 6x7
+// CloneAndAnimate() is invoked for 2,2.
+void SetUpAnimate1(ViewManagerServiceTest* test, ViewId* embed_view_id) {

[msw, 2014/11/18 23:37:43]

ditto nit: move to anon namespace above? (nix '1' from function name?)

[sky, 2014/11/19 00:49:40]

Same comment about wanting to keep it close to animation related tests. I see
this file as getting other tests, not just animation related. I did stick this
(and GetFirstCloned) into an anonymous namespace though.

+  *embed_view_id = ViewId(test->wm_connection()->id(), 1);
+  EXPECT_EQ(ERROR_CODE_NONE, test->wm_connection()->CreateView(*embed_view_id));
+  EXPECT_TRUE(test->wm_connection()->AddView(*(test->wm_connection()->root()),
+                                             *embed_view_id));
+  EXPECT_TRUE(test->wm_connection()->Embed(
+      std::string(), *embed_view_id, InterfaceRequest<ServiceProvider>()));

[msw, 2014/11/18 23:37:43]

For my clarity, is this embedding a connection with nothing on the other end?

[sky, 2014/11/19 00:49:40]

Depends upon what you mean by nothing on the other end. Embed is routed through
COnnectionManagerDelegate::CreateClientConnectionForEmbedAtView, which for these
tests is TestConnectionManagerDelegate::CreateClientConnectionForEmbedAtView,
which doesn't create a real connection.

[msw, 2014/11/19 01:27:51]

Acknowledged.

+  ViewManagerServiceImpl* connection1 =
+      test->connection_manager()->GetConnectionWithRoot(*embed_view_id);

[msw, 2014/11/18 23:37:43]

Harking back to my confusion about view manager connections. Does
"GetConnectionWithRoot" return the connection embeded within the argument view?
We created embed_view_id wm_connection's id (1, right?), but this returns
connection1 (with id 2, right?), which is not wm_connection.

[sky, 2014/11/19 00:49:40]

And my overloading of 'connection'. I made it worse with ClientConnection.
Sorry.
That's right. This returns the ViewManagerServiceImpl that was created by the
Embed call on 259. This ViewManagerServiceImpl has *embed_view_id as it's root.
That's right. Every call to Embed creates a new ViewManagerServiceImpl whose id
is one greater than the last. I suspect we'll want some randomness here, but
that's how it is now.

+  ASSERT_TRUE(connection1 != nullptr);
+  ASSERT_NE(connection1, test->wm_connection());
+
+  const ViewId child1(connection1->id(), 1);
+  EXPECT_EQ(ERROR_CODE_NONE, connection1->CreateView(child1));
+  const ViewId child2(connection1->id(), 2);
+  EXPECT_EQ(ERROR_CODE_NONE, connection1->CreateView(child2));
+  const ViewId child3(connection1->id(), 3);
+  EXPECT_EQ(ERROR_CODE_NONE, connection1->CreateView(child3));
+
+  ServerView* v1 = connection1->GetView(child1);
+  v1->SetBounds(gfx::Rect(1, 2, 11, 22));
+  ServerView* v2 = connection1->GetView(child2);
+  v2->SetBounds(gfx::Rect(2, 3, 6, 7));
+  ServerView* v3 = connection1->GetView(child3);
+  v3->SetBounds(gfx::Rect(3, 4, 6, 7));
+
+  EXPECT_TRUE(connection1->AddView(*embed_view_id, child1));
+  EXPECT_TRUE(connection1->AddView(child1, child2));
+  EXPECT_TRUE(connection1->AddView(child2, child3));
+
+  TestViewManagerClient* connection1_client = test->last_view_manager_client();
+  connection1_client->tracker()->changes()->clear();
+  test->wm_client()->tracker()->changes()->clear();
+  EXPECT_TRUE(test->connection_manager()->CloneAndAnimate(child2));
+  EXPECT_TRUE(connection1_client->tracker()->changes()->empty());
+  EXPECT_TRUE(test->wm_client()->tracker()->changes()->empty());
+
+  // We cloned v2. The cloned view ends up as a sibling of it.
+  const ServerView* cloned_view = GetFirstCloned(connection1->GetView(child1));
+  ASSERT_TRUE(cloned_view);
+  // |cloned_view| should have one and only one cloned child (corresponds to
+  // |child3|).
+  ASSERT_EQ(1u, cloned_view->GetChildren().size());
+  EXPECT_TRUE(cloned_view->GetChildren()[0]->id() == ClonedViewId());
+
+  // Cloned views should match the bounds of the view they were cloned from.
+  EXPECT_EQ(gfx::Rect(2, 3, 6, 7), cloned_view->bounds());

[msw, 2014/11/18 23:37:43]

nit: compare to v2 and v3 bounds instead of copied literals?

[sky, 2014/11/19 00:49:40]

Done.

+  EXPECT_EQ(gfx::Rect(3, 4, 6, 7), cloned_view->GetChildren()[0]->bounds());
+}
+
+TEST_F(ViewManagerServiceTest, ConnectionsCantSeeClonedViews) {
+  ViewId embed_view_id;
+  EXPECT_NO_FATAL_FAILURE(SetUpAnimate1(this, &embed_view_id));
+
+  ViewManagerServiceImpl* connection1 =
+      connection_manager()->GetConnectionWithRoot(embed_view_id);
+
+  const ViewId child1(connection1->id(), 1);
+  const ViewId child2(connection1->id(), 2);
+  const ViewId child3(connection1->id(), 3);
+
+  // Verify the root doesn't see any cloned views.

[msw, 2014/11/18 23:37:43]

Should this check GetFirstCloned() on the results of GetView() or GetViewTree(),
instead of solely relying on the direct contents of GetViewTree()? (ie. ensure
those ServerViews don't return cloned views via GetChildren()?)

[sky, 2014/11/19 00:49:40]

I'm not sure I get what you're asking. SetUpAnimate1 does test views trees. Is
that what you mean?

[msw, 2014/11/19 01:27:51]

Ah, so really this is a test that *just the vector returned* by GetViewTree
doesn't return cloned views, right? Explicitly using GetView or checking the
members of that vector for their children can expose cloned views? Maybe I'm
still misunderstanding...

[sky, 2014/11/19 03:28:17]

You got it. GetViewTree is what is passed to clients in some circumstances,
which is why it's important it doesn't contain the cloned views.  Actually, if a
client ends up seeing the cloned view it isn't the end of the world, perhaps
just a bit unexpected.

[msw, 2014/11/19 18:18:29]

I feel like the name of the test and comments here are somewhat misleading in
that regard. Can you try to clarify those? Also, perhaps we should have explicit
tests (beyond SetUpAnimate1) that:
(1) Use ViewManagerServiceImpl::GetView() to directly access cloned views
(perhaps that's not possible if the clone IDs are all the same?)
(2) Use ServerView::GetChildren() on views returned by
ViewManagerServiceImpl::GetView() to access cloned views.
(3) Use ServerView::GetChildren() on views returned by
ViewManagerServiceImpl::GetViewTree() to access cloned views.
(4) Access cloned views from a connection in other ways?

[sky, 2014/11/19 18:34:53]

This isn't really interesting as cloned views aren't owned by a connection.
GetFirstCloned() effectively tests these. Assertions for it are covered in
SetupAndAnimate1.
These are really the only ways to iterate the tree.

The reason GetViewTree is important to test is that it directly corresponds to
GetViewTree as defined in the mojom. It's the one case where we want to hide
cloned view. ServerView and accessing it is really an implementation of the
server.

I added the following comment for this test:

// Verifies ViewManagerService::GetViewTree() doesn't return cloned views.

Any better?

[msw, 2014/11/19 18:47:24]

That, combined with my new understanding (clients can't use GetView and only get
view IDs from GetViewTree) is all I needed; thanks!

+  std::vector<const ServerView*> views;
+  wm_connection()->GetViewTree(*wm_connection()->root(), &views);
+  ASSERT_EQ(5u, views.size());
+  ASSERT_TRUE(views[0]->id() == *wm_connection()->root());
+  ASSERT_TRUE(views[1]->id() == embed_view_id);
+  ASSERT_TRUE(views[2]->id() == child1);
+  ASSERT_TRUE(views[3]->id() == child2);
+  ASSERT_TRUE(views[4]->id() == child3);
+
+  // Verify connection1 doesn't see any cloned views.
+  std::vector<const ServerView*> v1_views;
+  connection1->GetViewTree(embed_view_id, &v1_views);
+  ASSERT_EQ(4u, v1_views.size());
+  ASSERT_TRUE(v1_views[0]->id() == embed_view_id);
+  ASSERT_TRUE(v1_views[1]->id() == child1);
+  ASSERT_TRUE(v1_views[2]->id() == child2);
+  ASSERT_TRUE(v1_views[3]->id() == child3);
+}
+
+TEST_F(ViewManagerServiceTest, ClonedViewsPromotedOnConnectionClose) {
+  ViewId embed_view_id;
+  EXPECT_NO_FATAL_FAILURE(SetUpAnimate1(this, &embed_view_id));
+
+  // Destroy connection1, which should force the cloned view to become a child
+  // of where it was embedded (the embedded view still exists).
+  connection_manager()->OnConnectionError(last_client_connection());
+
+  ServerView* embed_view = wm_connection()->GetView(embed_view_id);
+  ASSERT_TRUE(embed_view != nullptr);
+  const ServerView* cloned_view = GetFirstCloned(embed_view);
+  ASSERT_TRUE(cloned_view);
+  ASSERT_EQ(1u, cloned_view->GetChildren().size());
+  EXPECT_TRUE(cloned_view->GetChildren()[0]->id() == ClonedViewId());
+
+  // Because the cloned view changed parents it's bounds should have changed.

[msw, 2014/11/18 23:37:43]

nit: "its" without apostrophe.

[sky, 2014/11/19 00:49:40]

Done.

+  EXPECT_EQ(gfx::Rect(3, 5, 6, 7), cloned_view->bounds());
+  // The bounds of the cloned child should not have changed though.
+  EXPECT_EQ(gfx::Rect(3, 4, 6, 7), cloned_view->GetChildren()[0]->bounds());
+}
+
+TEST_F(ViewManagerServiceTest, ClonedViewsPromotedOnHide) {
+  ViewId embed_view_id;
+  EXPECT_NO_FATAL_FAILURE(SetUpAnimate1(this, &embed_view_id));
+
+  ViewManagerServiceImpl* connection1 =
+      connection_manager()->GetConnectionWithRoot(embed_view_id);
+
+  // Hide the parent of the cloned view, should force the cloned view to become

[msw, 2014/11/18 23:37:43]

nit: view, which should

[sky, 2014/11/19 00:49:40]

Done.

+  // a sibling of the parent.
+  const ServerView* view_to_hide =
+      connection1->GetView(ViewId(connection1->id(), 1));
+  ASSERT_TRUE(connection1->SetViewVisibility(view_to_hide->id(), false));
+
+  const ServerView* cloned_view = GetFirstCloned(view_to_hide->parent());
+  ASSERT_TRUE(cloned_view);
+  ASSERT_EQ(1u, cloned_view->GetChildren().size());
+  EXPECT_TRUE(cloned_view->GetChildren()[0]->id() == ClonedViewId());
+  EXPECT_EQ(2u, cloned_view->parent()->GetChildren().size());
+  EXPECT_TRUE(cloned_view->parent()->GetChildren()[1] == cloned_view);
+}
+
+// Clone and animate on tree with more depth. Basicly that of SetUpAnimate1()

[msw, 2014/11/18 23:37:43]

nit: "a tree", also "Basically"

[sky, 2014/11/19 00:49:40]

Done.

+// but cloning 2,1.
+TEST_F(ViewManagerServiceTest, CloneAndAnimateLargerDepth) {
+  const ViewId embed_view_id(wm_connection()->id(), 1);

[msw, 2014/11/18 23:37:43]

nit: Should SetUpAnimate1 be split into two steps? One would setup the tree,
(which can be re-used here), and another step to CloneAndAnimate (2,2) and
verify the expectations.

[sky, 2014/11/19 00:49:40]

I kind of like it this way, but I of course may change that as I need more
tests.

[msw, 2014/11/19 01:27:51]

Acknowledged.

+  EXPECT_EQ(ERROR_CODE_NONE, wm_connection()->CreateView(embed_view_id));
+  EXPECT_TRUE(
+      wm_connection()->AddView(*(wm_connection()->root()), embed_view_id));
+  EXPECT_TRUE(wm_connection()->Embed(std::string(), embed_view_id,
+                                     InterfaceRequest<ServiceProvider>()));
+  ViewManagerServiceImpl* connection1 =
+      connection_manager()->GetConnectionWithRoot(embed_view_id);
+  ASSERT_TRUE(connection1 != nullptr);
+  ASSERT_NE(connection1, wm_connection());
+
+  const ViewId child1(connection1->id(), 1);
+  EXPECT_EQ(ERROR_CODE_NONE, connection1->CreateView(child1));
+  const ViewId child2(connection1->id(), 2);
+  EXPECT_EQ(ERROR_CODE_NONE, connection1->CreateView(child2));
+  const ViewId child3(connection1->id(), 3);
+  EXPECT_EQ(ERROR_CODE_NONE, connection1->CreateView(child3));
+
+  ServerView* v1 = connection1->GetView(child1);
+
+  EXPECT_TRUE(connection1->AddView(embed_view_id, child1));
+  EXPECT_TRUE(connection1->AddView(child1, child2));
+  EXPECT_TRUE(connection1->AddView(child2, child3));
+
+  TestViewManagerClient* connection1_client = last_view_manager_client();
+  connection1_client->tracker()->changes()->clear();
+  wm_client()->tracker()->changes()->clear();
+  EXPECT_TRUE(connection_manager()->CloneAndAnimate(child1));
+  EXPECT_TRUE(connection1_client->tracker()->changes()->empty());
+  EXPECT_TRUE(wm_client()->tracker()->changes()->empty());
+
+  // We cloned v1. The cloned view ends up as a sibling of it.
+  const ServerView* cloned_view = GetFirstCloned(v1->parent());
+  ASSERT_TRUE(cloned_view);
+  // |cloned_view| should have a child and it's child should have a child.

[msw, 2014/11/18 23:37:43]

nit: "its" without apostrophe.

[sky, 2014/11/19 00:49:40]

Done.

+  ASSERT_EQ(1u, cloned_view->GetChildren().size());
+  const ServerView* cloned_view_child = cloned_view->GetChildren()[0];
+  EXPECT_EQ(1u, cloned_view_child->GetChildren().size());

[msw, 2014/11/18 23:37:43]

nit: ensure the cloned_view_child has ClonedViewId()

[sky, 2014/11/19 00:49:40]

Done.

 }
 
 }  // namespace service