Move code from SubmitCompositorFrame to OnFirstSurfaceActivation

Also clean up the code and re-enable
RenderWidgetHostViewGuestSurfaceTest.TestGuestSurface.

Bug: 730193, 844469
Change-Id: I20f17fda6094b1ffb6bbb1482ccd14f6c20175ea
Reviewed-on: https://chromium-review.googlesource.com/1104798
Commit-Queue: Saman Sami <samans@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568514}

content/browser/browser_plugin/browser_plugin_guest.cc

@@ -87,7 +87,6 @@ BrowserPluginGuest::BrowserPluginGuest(bool has_render_view,
     : WebContentsObserver(web_contents),
       owner_web_contents_(nullptr),
       attached_(false),
-      has_attached_since_surface_set_(false),
       browser_plugin_instance_id_(browser_plugin::kInstanceIDNone),
       focused_(false),
       mouse_locked_(false),
@@ -409,7 +408,6 @@ void BrowserPluginGuest::PointerLockPermissionResponse(bool allow) {
 
 void BrowserPluginGuest::SetChildFrameSurface(
     const viz::SurfaceInfo& surface_info) {
-  has_attached_since_surface_set_ = false;
   if (features::IsAshInBrowserProcess()) {
     SendMessageToEmbedder(
         std::make_unique<BrowserPluginMsg_SetChildFrameSurface>(
@@ -796,7 +794,6 @@ void BrowserPluginGuest::OnWillAttachComplete(
   InitInternal(params, embedder_web_contents);
 
   attached_ = true;
-  has_attached_since_surface_set_ = true;
   SendQueuedMessages();
 
   delegate_->DidAttach(GetGuestProxyRoutingID());

content/browser/browser_plugin/browser_plugin_guest.h

@@ -215,12 +215,6 @@ class CONTENT_EXPORT BrowserPluginGuest : public GuestHost,
   // Returns whether the guest is attached to an embedder.
   bool attached() const { return attached_; }
 
-  // Returns true when an attachment has taken place since the last time the
-  // compositor surface was set.
-  bool has_attached_since_surface_set() const {
-    return has_attached_since_surface_set_;
-  }
-
   // Attaches this BrowserPluginGuest to the provided |embedder_web_contents|
   // and initializes the guest with the provided |params|. Attaching a guest
   // to an embedder implies that this guest's lifetime is no longer managed
@@ -271,11 +265,6 @@ class CONTENT_EXPORT BrowserPluginGuest : public GuestHost,
                      WebContentsImpl* web_contents,
                      BrowserPluginGuestDelegate* delegate);
 
-  // Protected for testing.
-  void set_has_attached_since_surface_set_for_test(bool has_attached) {
-    has_attached_since_surface_set_ = has_attached;
-  }
-
   void set_attached_for_test(bool attached) {
     attached_ = attached;
   }
@@ -397,10 +386,6 @@ class CONTENT_EXPORT BrowserPluginGuest : public GuestHost,
   // Indicates whether this guest has been attached to a container.
   bool attached_;
 
-  // Used to signal if a browser plugin has been attached since the last time
-  // the compositing surface was set.
-  bool has_attached_since_surface_set_;
-
   // An identifier that uniquely identifies a browser plugin within an embedder.
   int browser_plugin_instance_id_;
   gfx::Rect frame_rect_;

content/browser/frame_host/render_widget_host_view_guest.cc

@@ -153,8 +153,7 @@ void RenderWidgetHostViewGuest::Show() {
     // Since we were last shown, our renderer may have had a different surface
     // set (e.g. showing an interstitial), so we resend our current surface to
     // the renderer.
-    if (last_received_local_surface_id_.is_valid())
-      SendSurfaceInfoToEmbedder();
+    SendSurfaceInfoToEmbedder();
   }
   host()->WasShown(false /* record_presentation_time */);
 }
@@ -251,7 +250,7 @@ gfx::PointF RenderWidgetHostViewGuest::TransformPointToRootCoordSpaceF(
     const gfx::PointF& point) {
   // LocalSurfaceId is not needed in Viz hit-test.
   if (!guest_ ||
-      (!use_viz_hit_test_ && !last_received_local_surface_id_.is_valid())) {
+      (!use_viz_hit_test_ && !last_activated_surface_info_.is_valid())) {
     return point;
   }
 
@@ -264,9 +263,7 @@ gfx::PointF RenderWidgetHostViewGuest::TransformPointToRootCoordSpaceF(
   // guarantee not to change transformed_point on failure, then we could skip
   // checking the function return value and directly return transformed_point.
   if (!root_rwhv->TransformPointToLocalCoordSpace(
-          point,
-          viz::SurfaceId(frame_sink_id_, last_received_local_surface_id_),
-          &transformed_point)) {
+          point, last_activated_surface_info_.id(), &transformed_point)) {
     return point;
   }
   return transformed_point;
@@ -277,19 +274,18 @@ bool RenderWidgetHostViewGuest::TransformPointToLocalCoordSpaceLegacy(
     const viz::SurfaceId& original_surface,
     gfx::PointF* transformed_point) {
   *transformed_point = point;
-  if (!guest_ || !last_received_local_surface_id_.is_valid())
+  if (!guest_ || !last_activated_surface_info_.is_valid())
     return false;
 
-  auto local_surface_id =
-      viz::SurfaceId(frame_sink_id_, last_received_local_surface_id_);
-  if (original_surface == local_surface_id)
+  if (original_surface == last_activated_surface_info_.id())
     return true;
 
   *transformed_point =
       gfx::ConvertPointToPixel(current_surface_scale_factor(), point);
   viz::SurfaceHittest hittest(nullptr,
                               GetFrameSinkManager()->surface_manager());
-  if (!hittest.TransformPointToTargetSurface(original_surface, local_surface_id,
+  if (!hittest.TransformPointToTargetSurface(original_surface,
+                                             last_activated_surface_info_.id(),
                                              transformed_point)) {
     return false;
   }
@@ -394,6 +390,7 @@ void RenderWidgetHostViewGuest::OnAttached() {
                        weak_ptr_factory_.GetWeakPtr()));
   }
 #endif
+  SendSurfaceInfoToEmbedder();
 }
 
 bool RenderWidgetHostViewGuest::OnMessageReceived(const IPC::Message& msg) {
@@ -817,10 +814,6 @@ void RenderWidgetHostViewGuest::OnHandleInputEvent(
   }
 }
 
-bool RenderWidgetHostViewGuest::HasEmbedderChanged() {
-  return guest_ && guest_->has_attached_since_surface_set();
-}
-
 #if defined(USE_AURA)
 void RenderWidgetHostViewGuest::OnGotEmbedToken(
     const base::UnguessableToken& token) {

content/browser/frame_host/render_widget_host_view_guest.h

@@ -190,8 +190,6 @@ class CONTENT_EXPORT RenderWidgetHostViewGuest
   void ProcessTouchpadPinchAckInRoot(const blink::WebGestureEvent& event,
                                      InputEventAckState ack_result);
 
-  bool HasEmbedderChanged() override;
-
 #if defined(USE_AURA)
   void OnGotEmbedToken(const base::UnguessableToken& token);
 #endif

content/browser/frame_host/render_widget_host_view_guest_unittest.cc

@@ -28,7 +28,6 @@
 #include "content/public/test/mock_render_process_host.h"
 #include "content/public/test/test_browser_context.h"
 #include "content/public/test/test_browser_thread_bundle.h"
-#include "content/test/fake_renderer_compositor_frame_sink.h"
 #include "content/test/mock_render_widget_host_delegate.h"
 #include "content/test/mock_widget_impl.h"
 #include "content/test/test_render_view_host.h"
@@ -114,11 +113,6 @@ class TestBrowserPluginGuest : public BrowserPluginGuest {
 
   void ResetTestData() { last_surface_info_ = viz::SurfaceInfo(); }
 
-  void set_has_attached_since_surface_set(bool has_attached_since_surface_set) {
-    BrowserPluginGuest::set_has_attached_since_surface_set_for_test(
-        has_attached_since_surface_set);
-  }
-
   void set_attached(bool attached) {
     BrowserPluginGuest::set_attached_for_test(attached);
   }
@@ -131,9 +125,7 @@ class TestBrowserPluginGuest : public BrowserPluginGuest {
 };
 
 // TODO(wjmaclean): we should restructure RenderWidgetHostViewChildFrameTest to
-// look more like this one, and then this one could be derived from it. Also,
-// include CreateDelegatedFrame as part of the test class so we don't have to
-// repeat it here.
+// look more like this one, and then this one could be derived from it.
 class RenderWidgetHostViewGuestSurfaceTest
     : public testing::Test {
  public:
@@ -162,16 +154,6 @@ class RenderWidgetHostViewGuestSurfaceTest
     view_ = RenderWidgetHostViewGuest::Create(
         widget_host_, browser_plugin_guest_,
         (new TestRenderWidgetHostView(widget_host_))->GetWeakPtr());
-    viz::mojom::CompositorFrameSinkPtr sink;
-    viz::mojom::CompositorFrameSinkRequest sink_request =
-        mojo::MakeRequest(&sink);
-    viz::mojom::CompositorFrameSinkClientRequest client_request =
-        mojo::MakeRequest(&renderer_compositor_frame_sink_ptr_);
-    renderer_compositor_frame_sink_ =
-        std::make_unique<FakeRendererCompositorFrameSink>(
-            std::move(sink), std::move(client_request));
-    view_->DidCreateNewRendererCompositorFrameSink(
-        renderer_compositor_frame_sink_ptr_.get());
   }
 
   void TearDown() override {
@@ -192,10 +174,7 @@ class RenderWidgetHostViewGuestSurfaceTest
     DCHECK(view_);
     RenderWidgetHostViewChildFrame* rwhvcf =
         static_cast<RenderWidgetHostViewChildFrame*>(view_);
-    if (!rwhvcf->last_received_local_surface_id_.is_valid())
-      return viz::SurfaceId();
-    return viz::SurfaceId(rwhvcf->frame_sink_id_,
-                          rwhvcf->last_received_local_surface_id_);
+    return rwhvcf->last_activated_surface_info_.id();
   }
 
  protected:
@@ -211,8 +190,6 @@ class RenderWidgetHostViewGuestSurfaceTest
   std::unique_ptr<MockWidgetImpl> widget_impl_;
   RenderWidgetHostImpl* widget_host_;
   RenderWidgetHostViewGuest* view_;
-  std::unique_ptr<FakeRendererCompositorFrameSink>
-      renderer_compositor_frame_sink_;
 
  private:
   viz::mojom::CompositorFrameSinkClientPtr renderer_compositor_frame_sink_ptr_;
@@ -220,33 +197,13 @@ class RenderWidgetHostViewGuestSurfaceTest
   DISALLOW_COPY_AND_ASSIGN(RenderWidgetHostViewGuestSurfaceTest);
 };
 
-namespace {
-viz::CompositorFrame CreateDelegatedFrame(float scale_factor,
-                                          gfx::Size size,
-                                          const gfx::Rect& damage) {
-  viz::CompositorFrame frame;
-  frame.metadata.device_scale_factor = scale_factor;
-  frame.metadata.begin_frame_ack = viz::BeginFrameAck(0, 1, true);
-
-  std::unique_ptr<viz::RenderPass> pass = viz::RenderPass::Create();
-  pass->SetNew(1, gfx::Rect(size), damage, gfx::Transform());
-  frame.render_pass_list.push_back(std::move(pass));
-  return frame;
-}
-}  // anonymous namespace
-
 TEST_F(RenderWidgetHostViewGuestSurfaceTest, TestGuestSurface) {
-  // TODO(jonross): Delete this test once Viz launches as it will be obsolete.
-  // https://crbug.com/844469
-  if (base::FeatureList::IsEnabled(features::kVizDisplayCompositor) ||
-      !features::IsAshInBrowserProcess()) {
-    return;
-  }
-
   gfx::Size view_size(100, 100);
   gfx::Rect view_rect(view_size);
   float scale_factor = 1.f;
   viz::LocalSurfaceId local_surface_id(1, base::UnguessableToken::Create());
+  viz::SurfaceId surface_id(view_->GetFrameSinkId(), local_surface_id);
+  viz::SurfaceInfo surface_info(surface_id, scale_factor, view_size);
 
   ASSERT_TRUE(browser_plugin_guest_);
 
@@ -254,46 +211,24 @@ TEST_F(RenderWidgetHostViewGuestSurfaceTest, TestGuestSurface) {
   view_->Show();
 
   browser_plugin_guest_->set_attached(true);
-  view_->SubmitCompositorFrame(
-      local_surface_id,
-      CreateDelegatedFrame(scale_factor, view_size, view_rect), base::nullopt);
 
-  viz::SurfaceId id = GetSurfaceId();
+  view_->OnFirstSurfaceActivation(surface_info);
 
-  EXPECT_TRUE(id.is_valid());
+  EXPECT_EQ(surface_id, GetSurfaceId());
 
-#if !defined(OS_ANDROID)
-  viz::SurfaceManager* manager = ImageTransportFactory::GetInstance()
-                                     ->GetContextFactoryPrivate()
-                                     ->GetFrameSinkManager()
-                                     ->surface_manager();
-  viz::Surface* surface = manager->GetSurfaceForId(id);
-  EXPECT_TRUE(surface);
-#endif
   // Surface ID should have been passed to BrowserPluginGuest to
   // be sent to the embedding renderer.
-  EXPECT_EQ(viz::SurfaceInfo(id, scale_factor, view_size),
-            browser_plugin_guest_->last_surface_info_);
+  EXPECT_EQ(surface_info, browser_plugin_guest_->last_surface_info_);
 
   browser_plugin_guest_->ResetTestData();
-  browser_plugin_guest_->set_has_attached_since_surface_set(true);
 
-  view_->SubmitCompositorFrame(
-      local_surface_id,
-      CreateDelegatedFrame(scale_factor, view_size, view_rect), base::nullopt);
+  // The last received SurfaceInfo must be sent to BrowserPluginGuest on
+  // attachment.
+  view_->OnAttached();
 
-  // Since we have not changed the frame size and scale factor, the same surface
-  // id must be used.
-  DCHECK_EQ(id, GetSurfaceId());
-
-#if !defined(OS_ANDROID)
-  surface = manager->GetSurfaceForId(id);
-  EXPECT_TRUE(surface);
-#endif
   // Surface ID should have been passed to BrowserPluginGuest to
   // be sent to the embedding renderer.
-  EXPECT_EQ(viz::SurfaceInfo(id, scale_factor, view_size),
-            browser_plugin_guest_->last_surface_info_);
+  EXPECT_EQ(surface_info, browser_plugin_guest_->last_surface_info_);
 
   browser_plugin_guest_->set_attached(false);
   browser_plugin_guest_->ResetTestData();

content/browser/renderer_host/render_widget_host_view_child_frame.cc

@@ -69,7 +69,6 @@ RenderWidgetHostViewChildFrame::RenderWidgetHostViewChildFrame(
       frame_sink_id_(
           base::checked_cast<uint32_t>(widget_host->GetProcess()->GetID()),
           base::checked_cast<uint32_t>(widget_host->GetRoutingID())),
-      current_surface_scale_factor_(1.f),
       frame_connector_(nullptr),
       enable_viz_(
           base::FeatureList::IsEnabled(features::kVizDisplayCompositor)),
@@ -134,7 +133,6 @@ void RenderWidgetHostViewChildFrame::SetFrameConnectorDelegate(
 
   if (frame_connector_) {
     SetParentFrameSinkId(viz::FrameSinkId());
-    last_received_local_surface_id_ = viz::LocalSurfaceId();
 
     // Unlocks the mouse if this RenderWidgetHostView holds the lock.
     UnlockMouse();
@@ -175,6 +173,8 @@ void RenderWidgetHostViewChildFrame::SetFrameConnectorDelegate(
         GetWindowTreeClientFromRenderer());
   }
 #endif
+
+  SendSurfaceInfoToEmbedder();
 }
 
 #if defined(USE_AURA)
@@ -636,10 +636,9 @@ void RenderWidgetHostViewChildFrame::SetParentFrameSinkId(
 void RenderWidgetHostViewChildFrame::SendSurfaceInfoToEmbedder() {
   if (!features::IsAshInBrowserProcess())
     return;
-  viz::SurfaceId surface_id(frame_sink_id_, last_received_local_surface_id_);
-  viz::SurfaceInfo surface_info(surface_id, current_surface_scale_factor_,
-                                current_surface_size_);
-  SendSurfaceInfoToEmbedderImpl(surface_info);
+  if (!last_activated_surface_info_.is_valid())
+    return;
+  SendSurfaceInfoToEmbedderImpl(last_activated_surface_info_);
 }
 
 void RenderWidgetHostViewChildFrame::SendSurfaceInfoToEmbedderImpl(
@@ -655,20 +654,8 @@ void RenderWidgetHostViewChildFrame::SubmitCompositorFrame(
   DCHECK(!enable_viz_);
   TRACE_EVENT0("content",
                "RenderWidgetHostViewChildFrame::OnSwapCompositorFrame");
-  current_surface_size_ = frame.size_in_pixels();
-  current_surface_scale_factor_ = frame.device_scale_factor();
-
   support_->SubmitCompositorFrame(local_surface_id, std::move(frame),
                                   std::move(hit_test_region_list));
-  has_frame_ = true;
-
-  if (last_received_local_surface_id_ != local_surface_id ||
-      HasEmbedderChanged()) {
-    last_received_local_surface_id_ = local_surface_id;
-    SendSurfaceInfoToEmbedder();
-  }
-
-  ProcessFrameSwappedCallbacks();
 }
 
 void RenderWidgetHostViewChildFrame::OnDidNotProduceFrame(
@@ -770,7 +757,7 @@ viz::FrameSinkId RenderWidgetHostViewChildFrame::GetRootFrameSinkId() {
 }
 
 viz::SurfaceId RenderWidgetHostViewChildFrame::GetCurrentSurfaceId() const {
-  return viz::SurfaceId(frame_sink_id_, last_received_local_surface_id_);
+  return last_activated_surface_info_.id();
 }
 
 bool RenderWidgetHostViewChildFrame::HasSize() const {
@@ -781,12 +768,12 @@ gfx::PointF RenderWidgetHostViewChildFrame::TransformPointToRootCoordSpaceF(
     const gfx::PointF& point) {
   // LocalSurfaceId is not needed in Viz hit-test.
   if (!frame_connector_ ||
-      (!use_viz_hit_test_ && !last_received_local_surface_id_.is_valid())) {
+      (!use_viz_hit_test_ && !last_activated_surface_info_.is_valid())) {
     return point;
   }
 
   return frame_connector_->TransformPointToRootCoordSpace(
-      point, viz::SurfaceId(frame_sink_id_, last_received_local_surface_id_));
+      point, last_activated_surface_info_.id());
 }
 
 bool RenderWidgetHostViewChildFrame::TransformPointToLocalCoordSpaceLegacy(
@@ -794,12 +781,11 @@ bool RenderWidgetHostViewChildFrame::TransformPointToLocalCoordSpaceLegacy(
     const viz::SurfaceId& original_surface,
     gfx::PointF* transformed_point) {
   *transformed_point = point;
-  if (!frame_connector_ || !last_received_local_surface_id_.is_valid())
+  if (!frame_connector_ || !last_activated_surface_info_.is_valid())
     return false;
 
   return frame_connector_->TransformPointToLocalCoordSpaceLegacy(
-      point, original_surface,
-      viz::SurfaceId(frame_sink_id_, last_received_local_surface_id_),
+      point, original_surface, last_activated_surface_info_.id(),
       transformed_point);
 }
 
@@ -810,7 +796,7 @@ bool RenderWidgetHostViewChildFrame::TransformPointToCoordSpaceForView(
     viz::EventSource source) {
   // LocalSurfaceId is not needed in Viz hit-test.
   if (!frame_connector_ ||
-      (!use_viz_hit_test_ && !last_received_local_surface_id_.is_valid())) {
+      (!use_viz_hit_test_ && !last_activated_surface_info_.is_valid())) {
     return false;
   }
 
@@ -820,9 +806,8 @@ bool RenderWidgetHostViewChildFrame::TransformPointToCoordSpaceForView(
   }
 
   return frame_connector_->TransformPointToCoordSpaceForView(
-      point, target_view,
-      viz::SurfaceId(frame_sink_id_, last_received_local_surface_id_),
-      transformed_point, source);
+      point, target_view, last_activated_surface_info_.id(), transformed_point,
+      source);
 }
 
 gfx::PointF RenderWidgetHostViewChildFrame::TransformRootPointToViewCoordSpace(
@@ -913,11 +898,11 @@ void RenderWidgetHostViewChildFrame::CopyFromSurface(
               std::move(callback)));
 
   if (src_subrect.IsEmpty()) {
-    request->set_area(gfx::Rect(current_surface_size_));
+    request->set_area(gfx::Rect(last_activated_surface_info_.size_in_pixels()));
   } else {
     // |src_subrect| is in DIP coordinates; convert to Surface coordinates.
-    request->set_area(
-        gfx::ScaleToRoundedRect(src_subrect, current_surface_scale_factor_));
+    request->set_area(gfx::ScaleToRoundedRect(
+        src_subrect, last_activated_surface_info_.device_scale_factor()));
   }
 
   if (!output_size.IsEmpty()) {
@@ -934,8 +919,7 @@ void RenderWidgetHostViewChildFrame::CopyFromSurface(
   }
 
   GetHostFrameSinkManager()->RequestCopyOfOutput(
-      viz::SurfaceId(frame_sink_id_, last_received_local_surface_id_),
-      std::move(request));
+      last_activated_surface_info_.id(), std::move(request));
 }
 
 void RenderWidgetHostViewChildFrame::ReclaimResources(
@@ -958,7 +942,10 @@ void RenderWidgetHostViewChildFrame::OnBeginFramePausedChanged(bool paused) {
 
 void RenderWidgetHostViewChildFrame::OnFirstSurfaceActivation(
     const viz::SurfaceInfo& surface_info) {
+  last_activated_surface_info_ = surface_info;
+  has_frame_ = true;
   SendSurfaceInfoToEmbedderImpl(surface_info);
+  ProcessFrameSwappedCallbacks();
 }
 
 void RenderWidgetHostViewChildFrame::OnFrameTokenChanged(uint32_t frame_token) {
@@ -1150,10 +1137,6 @@ void RenderWidgetHostViewChildFrame::ResetCompositorFrameSinkSupport() {
   support_.reset();
 }
 
-bool RenderWidgetHostViewChildFrame::HasEmbedderChanged() {
-  return false;
-}
-
 bool RenderWidgetHostViewChildFrame::GetSelectionRange(
     gfx::Range* range) const {
   if (!text_input_manager_ || !GetFocusedWidget())

content/browser/renderer_host/render_widget_host_view_child_frame.h

@@ -212,7 +212,9 @@ class CONTENT_EXPORT RenderWidgetHostViewChildFrame
   }
 
   // Returns the current surface scale factor.
-  float current_surface_scale_factor() { return current_surface_scale_factor_; }
+  float current_surface_scale_factor() {
+    return last_activated_surface_info_.device_scale_factor();
+  }
 
   // Returns the view into which this view is directly embedded. This can
   // return nullptr when this view's associated child frame is not connected
@@ -267,9 +269,7 @@ class CONTENT_EXPORT RenderWidgetHostViewChildFrame
 
   // Surface-related state.
   std::unique_ptr<viz::CompositorFrameSinkSupport> support_;
-  viz::LocalSurfaceId last_received_local_surface_id_;
-  gfx::Size current_surface_size_;
-  float current_surface_scale_factor_;
+  viz::SurfaceInfo last_activated_surface_info_;
   gfx::Rect last_screen_rect_;
 
   // frame_connector_ provides a platform abstraction. Messages
@@ -296,8 +296,6 @@ class CONTENT_EXPORT RenderWidgetHostViewChildFrame
   void ResetCompositorFrameSinkSupport();
   void DetachFromTouchSelectionClientManagerIfNecessary();
 
-  virtual bool HasEmbedderChanged();
-
   // Returns false if the view cannot be shown. This is the case where the frame
   // associated with this view or a cross process ancestor frame has been hidden
   // using CSS.

content/browser/renderer_host/render_widget_host_view_child_frame_unittest.cc

@@ -141,12 +141,11 @@ class RenderWidgetHostViewChildFrameTest : public testing::Test {
   }
 
   viz::SurfaceId GetSurfaceId() const {
-    return viz::SurfaceId(view_->frame_sink_id_,
-                          view_->last_received_local_surface_id_);
+    return view_->last_activated_surface_info_.id();
   }
 
   viz::LocalSurfaceId GetLocalSurfaceId() const {
-    return view_->last_received_local_surface_id_;
+    return GetSurfaceId().local_surface_id();
   }
 
  protected: