RemoteViewHost embeds after added to a widget

The embedding aura::Window must be attached to a root window so that
window server could figure out the correct display and pass it to the
other side. Deferring the EmbedUsingToken call until RemoteViewHost
is added to a widget.

Bug: 812434
Change-Id: Ie65d12d8b883b1a74426c33c3f106bbb637c0f69
Reviewed-on: https://chromium-review.googlesource.com/998498Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548529}

ui/views/mus/remote_view/remote_view_host.cc

@@ -24,9 +24,21 @@ void RemoteViewHost::EmbedUsingToken(const base::UnguessableToken& embed_token,
   // Only works with mus.
   DCHECK_EQ(aura::Env::Mode::MUS, aura::Env::GetInstanceDontCreate()->mode());
 
+  embed_token_ = embed_token;
+  embed_flags_ = embed_flags;
+  embed_callback_ = std::move(callback);
+
+  if (GetWidget())
+    CreateEmbeddingRoot();
+}
+
+void RemoteViewHost::CreateEmbeddingRoot() {
   // Should not be attached to anything.
   DCHECK(!native_view());
 
+  // There is a pending embed request.
+  DCHECK(!embed_token_.is_empty());
+
   embedding_root_ = std::make_unique<aura::Window>(nullptr);
   embedding_root_->set_owned_by_parent(false);
 
@@ -36,33 +48,29 @@ void RemoteViewHost::EmbedUsingToken(const base::UnguessableToken& embed_token,
   embedding_root_->SetType(aura::client::WINDOW_TYPE_CONTROL);
   embedding_root_->Init(ui::LAYER_NOT_DRAWN);
 
+  // Must happen before EmbedUsingToken call for window server to figure out
+  // the relevant display.
+  Attach(embedding_root_.get());
+
   aura::WindowPortMus::Get(embedding_root_.get())
-      ->EmbedUsingToken(embed_token, embed_flags,
+      ->EmbedUsingToken(embed_token_, embed_flags_,
                         base::BindOnce(&RemoteViewHost::OnEmbedResult,
-                                       weak_ptr_factory_.GetWeakPtr(),
-                                       embed_token, std::move(callback)));
+                                       weak_ptr_factory_.GetWeakPtr()));
 }
 
-void RemoteViewHost::OnEmbedResult(const base::UnguessableToken& token,
-                                   EmbedCallback callback,
-                                   bool success) {
-  LOG_IF(ERROR, !success) << "Failed to embed, token=" << token;
-
-  // Attach to |embedding_root_| if embed succeeds and this view is added to a
-  // widget but has not yet attached to |embedding_root_|.
-  if (success && GetWidget() && !native_view())
-    Attach(embedding_root_.get());
+void RemoteViewHost::OnEmbedResult(bool success) {
+  LOG_IF(ERROR, !success) << "Failed to embed, token=" << embed_token_;
 
   if (!success && embedding_root_)
     embedding_root_.reset();
 
-  if (callback)
-    std::move(callback).Run(success);
+  if (embed_callback_)
+    std::move(embed_callback_).Run(success);
 }
 
 void RemoteViewHost::AddedToWidget() {
-  if (embedding_root_ && !native_view())
-    Attach(embedding_root_.get());
+  if (!native_view() && !embed_token_.is_empty())
+    CreateEmbeddingRoot();
 }
 
 }  // namespace views

ui/views/mus/remote_view/remote_view_host.h

@@ -23,25 +23,32 @@ class RemoteViewHost : public views::NativeViewHost {
   RemoteViewHost();
   ~RemoteViewHost() override;
 
-  // Creates an aura::window to embed the remote contents and attach to it when
-  // the embed succeeds. |embed_token| is the token obtained from the WindowTree
-  // embed API (ScheduleEmbed/ForExistingClient). |embed_flags| are the
-  // embedding flags (see window_tree_constants.mojom). |callback| is an
-  // optional callback invoked with the embed result.
+  // Embeds the remote contents after this view is added to a widget.
+  // |embed_token| is the token obtained from the WindowTree embed API
+  // (ScheduleEmbed/ForExistingClient). |embed_flags| are the embedding flags
+  // (see window_tree_constants.mojom). |callback| is an optional callback
+  // invoked with the embed result.
+  // Note that |callback| should not be used to add the view to a widget because
+  // the actual embedding only happens after the view is added.
   using EmbedCallback = base::OnceCallback<void(bool success)>;
   void EmbedUsingToken(const base::UnguessableToken& embed_token,
                        int embed_flags,
                        EmbedCallback callback);
 
  private:
+  // Creates the embedding aura::Window and attach to it.
+  void CreateEmbeddingRoot();
+
   // Invoked after the embed operation.
-  void OnEmbedResult(const base::UnguessableToken& token,
-                     EmbedCallback callback,
-                     bool success);
+  void OnEmbedResult(bool success);
 
   // views::NativeViewHost:
   void AddedToWidget() override;
 
+  base::UnguessableToken embed_token_;
+  int embed_flags_ = 0;
+  EmbedCallback embed_callback_;
+
   std::unique_ptr<aura::Window> embedding_root_;
   base::WeakPtrFactory<RemoteViewHost> weak_ptr_factory_{this};
 

ui/views/mus/remote_view/remote_view_host_unittest.cc

@@ -87,13 +87,13 @@ TEST_F(RemoteViewHostTest, BadEmbed) {
 
   // Ownership will be passed to |widget| later.
   RemoteViewHost* host = new RemoteViewHost();
+  std::unique_ptr<views::Widget> widget = CreateTestWidget(host);
+  EXPECT_FALSE(host->native_view());
 
   // Embed fails with unknown token.
   EXPECT_FALSE(Embed(host, unknown_token));
 
-  // |host| is not attached before and after adding to a widget.
-  EXPECT_FALSE(host->native_view());
-  std::unique_ptr<views::Widget> widget = CreateTestWidget(host);
+  // |host| is not attached after adding to a widget.
   EXPECT_FALSE(host->native_view());
 }
 
@@ -117,43 +117,34 @@ TEST_F(RemoteViewHostTest, AddToWidgetBeforeEmbed) {
   EXPECT_TRUE(host->native_view());
 }
 
-// Tests when RemoveViewHost is added to a widget while embedding.
-TEST_F(RemoteViewHostTest, AddToWidgetWhileEmbedding) {
+// Tests when RemoveViewHost is added to a widget after embedding.
+TEST_F(RemoteViewHostTest, AddToWidgetAfterEmbed) {
   const base::UnguessableToken token = base::UnguessableToken::Create();
   window_tree()->AddScheduledEmbedToken(token);
 
   // Ownership will be passed to |widget| later.
   RemoteViewHost* host = new RemoteViewHost();
 
-  // |host| is not attached because embed operation is not performed.
-  EXPECT_FALSE(host->native_view());
-
+  // Request embedding but it will be deferred until added to a widget.
   base::RunLoop run_loop;
-  std::unique_ptr<views::Widget> widget;
+  bool embed_result = false;
   host->EmbedUsingToken(
       token, 0u /* no flags */,
-      base::BindOnce(&RemoteViewHostTest::CreateTestWidgetWhileEmbeddingHelper,
-                     base::Unretained(this), &run_loop, host, &widget));
-  run_loop.Run();
-
-  // |host| is attached to the embedding window.
-  EXPECT_TRUE(host->native_view());
-}
-
-// Tests when RemoveViewHost is added to a widget after embedding.
-TEST_F(RemoteViewHostTest, AddToWidgetAfterEmbed) {
-  const base::UnguessableToken token = base::UnguessableToken::Create();
-  window_tree()->AddScheduledEmbedToken(token);
-
-  // Ownership will be passed to |widget| later.
-  RemoteViewHost* host = new RemoteViewHost();
-
-  // Embed succeeds.
-  EXPECT_TRUE(Embed(host, token));
+      base::BindOnce(
+          [](base::RunLoop* run_loop, bool* result, bool success) {
+            *result = success;
+            run_loop->Quit();
+          },
+          &run_loop, &embed_result));
 
-  // |host| is attached after adding to a widget.
+  // |host| is not attached before adding to a widget.
   EXPECT_FALSE(host->native_view());
+
+  // Add to a widget and wait for embed to finish.
   std::unique_ptr<views::Widget> widget = CreateTestWidget(host);
+  run_loop.Run();
+
+  // |host| is attached after added to a widget.
   EXPECT_TRUE(host->native_view());
 }