Mus Demo: Refactor code to prepare multiple windows in external mode

services/ui/demo/mus_demo_external.cc

Index: services/ui/demo/mus_demo_external.cc
diff --git a/services/ui/demo/mus_demo_external.cc b/services/ui/demo/mus_demo_external.cc
index d4026285c0d9c7c271c81728abd16e17f0c9d270..38fb19d6517fce923c0dd6b415f53e3f2af8808c 100644
--- a/services/ui/demo/mus_demo_external.cc
+++ b/services/ui/demo/mus_demo_external.cc
@@ -24,49 +24,78 @@ class WindowTreeDataExternal : public WindowTreeData {
                          mojom::WindowTreeClientPtr tree_client,
                          int square_size)
       : WindowTreeData(square_size) {
+    // TODO(tonikitoo,fwang): Create the host via the window tree client.
     factory->CreateWindowTreeHost(MakeRequest(&host_), std::move(tree_client));
   }
 
  private:
   // Holds the Mojo pointer to the window tree host.
   mojom::WindowTreeHostPtr host_;
+
+  DISALLOW_COPY_AND_ASSIGN(WindowTreeDataExternal);
+};
+
+// Size of square in pixels to draw
+const int kSquareSizeFunctionIntercept = 400;
+const int kSquareSizeFunctionSlope = 50;
+
+// Number of windows to open.
+// TODO(tonikitoo,fwang): Open multiple windows.
+const unsigned kNumberOfWindows = 1;

[kylechar, 2017/02/23 22:45:30]

Same comment about type.

[fwang, 2017/02/24 10:08:51]

Acknowledged.

[fwang, 2017/02/24 10:40:11]

Done.

+
+static int GetSquareSizeForWindow(int window_index) {

[kylechar, 2017/02/23 22:45:30]

I'm not sure I actually understand what this does? Is it just to have different
size squares on each window? You probably don't need the name constants then.

[kylechar, 2017/02/23 22:45:30]

The static qualifier here isn't necessary due to the anonymous namespace.

[fwang, 2017/02/24 10:08:51]

Yes, it's just to have different size squares on each window.

[fwang, 2017/02/24 10:08:51]

Acknowledged.

[fwang, 2017/02/24 10:40:11]

Done.

+  return kSquareSizeFunctionSlope * window_index + kSquareSizeFunctionIntercept;
 };
 
-// Size of square in pixels to draw.
-const int kSquareSize = 500;
 }  // namespace
 
 MusDemoExternal::MusDemoExternal() {}
 
 MusDemoExternal::~MusDemoExternal() {}
 
-void MusDemoExternal::OnStartImpl(
-    std::unique_ptr<aura::WindowTreeClient>* window_tree_client,
-    std::unique_ptr<WindowTreeData>* window_tree_data) {
+std::unique_ptr<aura::WindowTreeClient>
+MusDemoExternal::CreateWindowTreeClient() {
+  return base::MakeUnique<aura::WindowTreeClient>(
+      context()->connector(), this, nullptr, MakeRequest(&tree_client_));
+}
+
+void MusDemoExternal::OnStartImpl() {
+  // TODO(tonikitoo,fwang): Do the connection to window tree host factory:
+  // window_tree_client()->ConnectViaWindowTreeHostFactory();

[kylechar, 2017/02/23 22:45:30]

Delete commented out code.

[fwang, 2017/02/24 10:08:51]

OK, will do that. As I said, the goal is to replace the BindInterface with
ConnectViaWindowTreeHostFactory when Antonio's API is ready.

[fwang, 2017/02/24 10:40:11]

Done.

   context()->connector()->BindInterface(ui::mojom::kServiceName,
                                         &window_tree_host_factory_);
-  mojom::WindowTreeClientPtr tree_client;
-  *window_tree_client = base::MakeUnique<aura::WindowTreeClient>(
-      context()->connector(), this, nullptr, MakeRequest(&tree_client));
-  // TODO(tonikitoo,fwang): Open two external windows with different square
-  // sizes.
-  *window_tree_data = base::MakeUnique<WindowTreeDataExternal>(
-      window_tree_host_factory_.get(), std::move(tree_client), kSquareSize);
 
   // TODO(tonikitoo,fwang): Implement management of displays in external mode.
   // For now, a fake display is created in order to work around an assertion in
   // aura::GetDeviceScaleFactorFromDisplay().
   AddPrimaryDisplay(display::Display(0));
+
+  // The number of windows to open is specified by kNumberOfWindows. The windows
+  // are opened sequentially (the first one here and the others after each call
+  // to OnEmbed) to ensure that the WindowTreeHostMus passed to OnEmbed
+  // corresponds to the WindowTreeDataExternal::host_ created in OpenNewWindow.
+  OpenNewWindow();
+}
+
+void MusDemoExternal::OpenNewWindow() {
+  AppendWindowTreeData(base::MakeUnique<WindowTreeDataExternal>(
+      window_tree_host_factory_.get(), std::move(tree_client_),

[kylechar, 2017/02/23 22:45:30]

For two windows, you move this value then reuse it. I don't see it getting
reinitialized? That might be a problem, unless I missed something about how it's
used.

[fwang, 2017/02/24 10:08:51]

You are right. As sky said, this won't work with multiple windows, we really
need to change the API to have a single WindowTreeClient for all the windows.
Antonio is working on that. Then the WindowTreeDataExternal constructor would
take an aura::WindowTreeClient* (== window_tree_client()) instead of
mojom::WindowTreeHostFactory* + mojom::WindowTreeClientPtr. The host will then
just be created with WindowTreeClient::CreateHost(MakeRequest(&host_));

[fwang, 2017/02/24 10:40:11]

Done (added a comment to clarify).

+      GetSquareSizeForWindow(initialized_windows_count_)));
 }
 
 void MusDemoExternal::OnEmbed(
     std::unique_ptr<aura::WindowTreeHostMus> window_tree_host) {
   InitWindowTreeData(std::move(window_tree_host));
+  initialized_windows_count_++;
+
+  // Open the next window until the requested number of windows is reached.
+  if (initialized_windows_count_ < kNumberOfWindows)
+    OpenNewWindow();
 }
 
 void MusDemoExternal::OnEmbedRootDestroyed(
     aura::WindowTreeHostMus* window_tree_host) {
-  CleanupWindowTreeData();
+  RemoveWindowTreeDataFor(window_tree_host);
 }
 
 }  // namespace demo