RenderWidgetHostViewChildFrame should use CompositorFrameSinkSupport

content/browser/frame_host/render_widget_host_view_child_frame.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/render_widget_host_view_child_frame.h"
 
 #include <algorithm>
 #include <utility>
 #include <vector>
 
@@ skipping 38 matching lines @@
 
 RenderWidgetHostViewChildFrame::RenderWidgetHostViewChildFrame(
     RenderWidgetHost* widget_host)
     : host_(RenderWidgetHostImpl::From(widget_host)),
       frame_sink_id_(
           base::checked_cast<uint32_t>(widget_host->GetProcess()->GetID()),
           base::checked_cast<uint32_t>(widget_host->GetRoutingID())),
       next_surface_sequence_(1u),
       last_compositor_frame_sink_id_(0),
       current_surface_scale_factor_(1.f),
-      ack_pending_count_(0),
       frame_connector_(nullptr),
-      begin_frame_source_(nullptr),
       weak_factory_(this) {
   id_allocator_.reset(new cc::LocalSurfaceIdAllocator());
-  auto* manager = GetSurfaceManager();
-  manager->RegisterFrameSinkId(frame_sink_id_);
-  surface_factory_ =
-      base::MakeUnique<cc::SurfaceFactory>(frame_sink_id_, manager, this);
+  GetSurfaceManager()->RegisterFrameSinkId(frame_sink_id_);
+  CreateCompositorFrameSinkSupport();
 }
 
 RenderWidgetHostViewChildFrame::~RenderWidgetHostViewChildFrame() {
-  surface_factory_->EvictSurface();
+  ResetCompositorFrameSinkSupport();
   if (GetSurfaceManager())
     GetSurfaceManager()->InvalidateFrameSinkId(frame_sink_id_);
 }
 
 void RenderWidgetHostViewChildFrame::Init() {
   RegisterFrameSinkId();
   host_->SetView(this);
   GetTextInputManager();
 }
 
 void RenderWidgetHostViewChildFrame::SetCrossProcessFrameConnector(
     CrossProcessFrameConnector* frame_connector) {
   if (frame_connector_ == frame_connector)
     return;
 
   if (frame_connector_) {
-    if (parent_frame_sink_id_.is_valid()) {
-      GetSurfaceManager()->UnregisterFrameSinkHierarchy(parent_frame_sink_id_,
-                                                        frame_sink_id_);
-    }
-    // Unregister the client here, as it is not guaranteed in tests that the
-    // destructor will be called.
-    GetSurfaceManager()->UnregisterSurfaceFactoryClient(frame_sink_id_);
-
+    ResetCompositorFrameSinkSupport();
     parent_frame_sink_id_ = cc::FrameSinkId();
 
-    // After the RenderWidgetHostViewChildFrame loses the frame_connector, it
-    // won't be able to walk up the frame tree anymore. Clean up anything that
-    // needs to be done through the CrossProcessFrameConnector before it's gone.
-
     // Unlocks the mouse if this RenderWidgetHostView holds the lock.
     UnlockMouse();
   }
   frame_connector_ = frame_connector;
   if (frame_connector_) {
-    GetSurfaceManager()->RegisterSurfaceFactoryClient(frame_sink_id_, this);
+    ResetCompositorFrameSinkSupport();
     RenderWidgetHostViewBase* parent_view =
         frame_connector_->GetParentRenderWidgetHostView();
     if (parent_view) {
       parent_frame_sink_id_ = parent_view->GetFrameSinkId();
       DCHECK(parent_frame_sink_id_.is_valid());
-      GetSurfaceManager()->RegisterFrameSinkHierarchy(parent_frame_sink_id_,
-                                                      frame_sink_id_);
     }
+    CreateCompositorFrameSinkSupport();
   }
 }
 
 void RenderWidgetHostViewChildFrame::InitAsChild(
     gfx::NativeView parent_view) {
   NOTREACHED();
 }
 
 RenderWidgetHost* RenderWidgetHostViewChildFrame::GetRenderWidgetHost() const {
   return host_;
@@ skipping 221 matching lines @@
   // always forwarded and handled according to current scroll state in the
   // RenderWidgetHostInputEventRouter.
   if (!frame_connector_)
     return;
   if ((event.type() == blink::WebInputEvent::GestureScrollUpdate &&
        not_consumed) ||
       event.type() == blink::WebInputEvent::GestureScrollEnd)
     frame_connector_->BubbleScrollEvent(event);
 }
 
-void RenderWidgetHostViewChildFrame::SurfaceDrawn(
-    uint32_t compositor_frame_sink_id) {
-  DCHECK_GT(ack_pending_count_, 0U);
-  if (host_) {
-    host_->Send(new ViewMsg_ReclaimCompositorResources(
-        host_->GetRoutingID(), compositor_frame_sink_id, true /* is_swap_ack */,
-        surface_returned_resources_));
-    surface_returned_resources_.clear();
-  }
-  ack_pending_count_--;
+void RenderWidgetHostViewChildFrame::DidReceiveCompositorFrameAck() {
+  if (!host_)
+    return;
+  host_->Send(new ViewMsg_ReclaimCompositorResources(
+      host_->GetRoutingID(), last_compositor_frame_sink_id_,
+      true /* is_swap_ack */, cc::ReturnedResourceArray()));
 }
 
 bool RenderWidgetHostViewChildFrame::ShouldCreateNewSurfaceId(
     uint32_t compositor_frame_sink_id,
     const cc::CompositorFrame& frame) {
   gfx::Size new_frame_size = frame.render_pass_list.back()->output_rect.size();
   float new_scale_factor = frame.metadata.device_scale_factor;
 
   return compositor_frame_sink_id != last_compositor_frame_sink_id_ ||
          new_frame_size != current_surface_size_ ||
          new_scale_factor != current_surface_scale_factor_;
 }
 
 void RenderWidgetHostViewChildFrame::ProcessCompositorFrame(
     uint32_t compositor_frame_sink_id,
     cc::CompositorFrame frame) {
   if (ShouldCreateNewSurfaceId(compositor_frame_sink_id, frame)) {
     ClearCompositorSurfaceIfNecessary();
-    // If the renderer changed its frame sink, reset the surface factory to
-    // avoid returning stale resources.
-    if (compositor_frame_sink_id != last_compositor_frame_sink_id_)
-      surface_factory_->Reset();
+    // If the renderer changed its frame sink, reset the
+    // CompositorFrameSinkSupport to avoid returning stale resources.
+    if (compositor_frame_sink_id != last_compositor_frame_sink_id_) {
+      ResetCompositorFrameSinkSupport();
+      CreateCompositorFrameSinkSupport();
+    }
     last_compositor_frame_sink_id_ = compositor_frame_sink_id;
     current_surface_size_ = frame.render_pass_list.back()->output_rect.size();
     current_surface_scale_factor_ = frame.metadata.device_scale_factor;
   }
 
   bool allocated_new_local_surface_id = false;
   if (!local_surface_id_.is_valid()) {
     local_surface_id_ = id_allocator_->GenerateId();
     allocated_new_local_surface_id = true;
   }
 
-  cc::SurfaceFactory::DrawCallback ack_callback =
-      base::Bind(&RenderWidgetHostViewChildFrame::SurfaceDrawn, AsWeakPtr(),
-                 compositor_frame_sink_id);
-  ack_pending_count_++;
-  // If this value grows very large, something is going wrong.
-  DCHECK_LT(ack_pending_count_, 1000U);
-  surface_factory_->SubmitCompositorFrame(local_surface_id_, std::move(frame),
-                                          ack_callback);
+  support_->SubmitCompositorFrame(local_surface_id_, std::move(frame));
   if (allocated_new_local_surface_id)
     SendSurfaceInfoToEmbedder();
   ProcessFrameSwappedCallbacks();
 }
 
 void RenderWidgetHostViewChildFrame::SendSurfaceInfoToEmbedder() {
   cc::SurfaceSequence sequence =
       cc::SurfaceSequence(frame_sink_id_, next_surface_sequence_++);
   cc::SurfaceManager* manager = GetSurfaceManager();
   cc::SurfaceId surface_id(frame_sink_id_, local_surface_id_);
@@ skipping 202 matching lines @@
   SubmitSurfaceCopyRequest(src_subrect, output_size, callback,
                            preferred_color_type);
 }
 
 void RenderWidgetHostViewChildFrame::SubmitSurfaceCopyRequest(
     const gfx::Rect& src_subrect,
     const gfx::Size& output_size,
     const ReadbackRequestCallback& callback,
     const SkColorType preferred_color_type) {
   DCHECK(IsSurfaceAvailableForCopy());
+  DCHECK(support_);
 
   std::unique_ptr<cc::CopyOutputRequest> request =
       cc::CopyOutputRequest::CreateRequest(
           base::Bind(&CopyFromCompositingSurfaceHasResult, output_size,
                      preferred_color_type, callback));
   if (!src_subrect.IsEmpty())
     request->set_area(src_subrect);
 
-  surface_factory_->RequestCopyOfSurface(std::move(request));
+  support_->RequestCopyOfSurface(std::move(request));
 }
 
 void RenderWidgetHostViewChildFrame::CopyFromCompositingSurfaceToVideoFrame(
     const gfx::Rect& src_subrect,
     const scoped_refptr<media::VideoFrame>& target,
     const base::Callback<void(const gfx::Rect&, bool)>& callback) {
   NOTIMPLEMENTED();
   callback.Run(gfx::Rect(), false);
 }
 
 bool RenderWidgetHostViewChildFrame::CanCopyToVideoFrame() const {
   return false;
 }
 
 bool RenderWidgetHostViewChildFrame::HasAcceleratedSurface(
       const gfx::Size& desired_size) {
   return false;
 }
 
-// cc::SurfaceFactoryClient implementation.
-void RenderWidgetHostViewChildFrame::ReturnResources(
+void RenderWidgetHostViewChildFrame::ReclaimResources(
     const cc::ReturnedResourceArray& resources) {
-  if (resources.empty())
+  if (!host_)
     return;
-
-  if (!ack_pending_count_ && host_) {
-    host_->Send(new ViewMsg_ReclaimCompositorResources(
-        host_->GetRoutingID(), last_compositor_frame_sink_id_,
-        false /* is_swap_ack */, resources));
-    return;
-  }
-
-  std::copy(resources.begin(), resources.end(),
-            std::back_inserter(surface_returned_resources_));
-}
-
-void RenderWidgetHostViewChildFrame::SetBeginFrameSource(
-    cc::BeginFrameSource* source) {
-  bool needs_begin_frames = host_->needs_begin_frames();
-  if (begin_frame_source_ && needs_begin_frames)
-    begin_frame_source_->RemoveObserver(this);
-  begin_frame_source_ = source;
-  if (begin_frame_source_ && needs_begin_frames)
-    begin_frame_source_->AddObserver(this);
+  host_->Send(new ViewMsg_ReclaimCompositorResources(
+      host_->GetRoutingID(), last_compositor_frame_sink_id_,
+      false /* is_swap_ack */, resources));
 }
 
 void RenderWidgetHostViewChildFrame::OnBeginFrame(
     const cc::BeginFrameArgs& args) {
   host_->Send(new ViewMsg_BeginFrame(host_->GetRoutingID(), args));
-  last_begin_frame_args_ = args;
-}
-
-const cc::BeginFrameArgs&
-RenderWidgetHostViewChildFrame::LastUsedBeginFrameArgs() const {
-  return last_begin_frame_args_;
-}
-
-void RenderWidgetHostViewChildFrame::OnBeginFrameSourcePausedChanged(
-    bool paused) {
-  // Only used on Android WebView.
 }
 
 void RenderWidgetHostViewChildFrame::SetNeedsBeginFrames(
     bool needs_begin_frames) {
-  if (!begin_frame_source_)
-    return;
-
-  if (needs_begin_frames)
-    begin_frame_source_->AddObserver(this);
-  else
-    begin_frame_source_->RemoveObserver(this);
+  needs_begin_frame_ = needs_begin_frames;
+  if (support_)

[lfg, 2017/02/27 22:58:25]

Why would support_ be null?

[Saman Sami, 2017/02/28 19:17:10]

I don't fully understand how RenderWidget stuff work, but I tried removing it
and now I'm getting seg faults in this method. I imagine maybe
SetCrossProcessFrameConnector(nullptr) can leave us with a null support? Here is
one of the tests that failed:
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux...

+    support_->SetNeedsBeginFrame(needs_begin_frames);
 }
 
 InputEventAckState RenderWidgetHostViewChildFrame::FilterInputEvent(
     const blink::WebInputEvent& input_event) {
   if (input_event.type() == blink::WebInputEvent::GestureFlingStart) {
     const blink::WebGestureEvent& gesture_event =
         static_cast<const blink::WebGestureEvent&>(input_event);
     // Zero-velocity touchpad flings are an Aura-specific signal that the
     // touchpad scroll has ended, and should not be forwarded to the renderer.
     if (gesture_event.sourceDevice == blink::WebGestureDeviceTouchpad &&
@@ skipping 15 matching lines @@
 }
 
 BrowserAccessibilityManager*
 RenderWidgetHostViewChildFrame::CreateBrowserAccessibilityManager(
     BrowserAccessibilityDelegate* delegate, bool for_root_frame) {
   return BrowserAccessibilityManager::Create(
       BrowserAccessibilityManager::GetEmptyDocument(), delegate);
 }
 
 void RenderWidgetHostViewChildFrame::ClearCompositorSurfaceIfNecessary() {
-  surface_factory_->EvictSurface();
+  if (support_)

[lfg, 2017/02/27 22:58:25]

Same as above.

[Saman Sami, 2017/02/28 19:17:10]

The one above turned out to be necessary. I feel safer if I have this check here
too.

+    support_->EvictFrame();
   local_surface_id_ = cc::LocalSurfaceId();
 }
 
 bool RenderWidgetHostViewChildFrame::IsChildFrameForTesting() const {
   return true;
 }
 
 cc::SurfaceId RenderWidgetHostViewChildFrame::SurfaceIdForTesting() const {
   return cc::SurfaceId(frame_sink_id_, local_surface_id_);
 };
 
+void RenderWidgetHostViewChildFrame::CreateCompositorFrameSinkSupport() {
+  DCHECK(!support_);
+  support_ = base::MakeUnique<cc::CompositorFrameSinkSupport>(
+      this, GetSurfaceManager(), frame_sink_id_, false /* is_root */,
+      false /* handles_frame_sink_id_invalidation */,
+      true /* needs_sync_points */);
+  if (parent_frame_sink_id_.is_valid()) {
+    GetSurfaceManager()->RegisterFrameSinkHierarchy(parent_frame_sink_id_,
+                                                    frame_sink_id_);
+  }
+  if (needs_begin_frame_)
+    support_->SetNeedsBeginFrame(needs_begin_frame_);
+}
+
+void RenderWidgetHostViewChildFrame::ResetCompositorFrameSinkSupport() {
+  if (!support_)
+    return;
+  if (parent_frame_sink_id_.is_valid()) {
+    GetSurfaceManager()->UnregisterFrameSinkHierarchy(parent_frame_sink_id_,
+                                                      frame_sink_id_);
+  }
+  support_.reset();
+}
+
 }  // namespace content