Remove uses of CompositorFrameMetadata in SurfaceSync for WebView

DevTools was using CompositorFrameMetadata for screen capture.
This change refactors things so that:
- Metadata update and actual capture are split into two steps.
- When SurfaceSync is on we use RenderFrameMetadata for update.

Bug: 955042
Change-Id: I7479305c1bcbcecf94587cb69960da7bb6ff01a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1584923
Commit-Queue: Eric Karl <ericrk@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#654651}

content/browser/BUILD.gn

@@ -656,6 +656,8 @@ jumbo_source_set("browser") {
     "devtools/devtools_agent_host_impl.h",
     "devtools/devtools_background_services_context.cc",
     "devtools/devtools_background_services_context.h",
+    "devtools/devtools_frame_metadata.cc",
+    "devtools/devtools_frame_metadata.h",
     "devtools/devtools_frame_trace_recorder.cc",
     "devtools/devtools_frame_trace_recorder.h",
     "devtools/devtools_http_handler.cc",

content/browser/android/synchronous_compositor_host.cc

@@ -425,7 +425,8 @@ void SynchronousCompositorHost::UpdatePresentedFrameToken(
   if (!viz::FrameTokenGT(frame_token, last_frame_token_))
     return;
   last_frame_token_ = frame_token;
-  rwhva_->FrameTokenChangedForSynchronousCompositor(frame_token);
+  rwhva_->FrameTokenChangedForSynchronousCompositor(frame_token,
+                                                    root_scroll_offset_);
 }
 
 void SynchronousCompositorHost::SetMemoryPolicy(size_t bytes_limit) {

content/browser/devtools/devtools_frame_metadata.cc

+// Copyright 2019 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/devtools/devtools_frame_metadata.h"
+
+#include "build/build_config.h"
+#include "cc/trees/render_frame_metadata.h"
+#include "components/viz/common/quads/compositor_frame_metadata.h"
+
+namespace content {
+
+#if defined(OS_ANDROID)
+DevToolsFrameMetadata::DevToolsFrameMetadata(
+    const cc::RenderFrameMetadata& metadata)
+    : device_scale_factor(metadata.device_scale_factor),
+      page_scale_factor(metadata.page_scale_factor),
+      root_scroll_offset(
+          metadata.root_scroll_offset.value_or(gfx::Vector2dF())),
+      top_controls_height(metadata.top_controls_height),
+      top_controls_shown_ratio(metadata.top_controls_shown_ratio),
+      scrollable_viewport_size(metadata.scrollable_viewport_size) {}
+
+DevToolsFrameMetadata::DevToolsFrameMetadata(
+    const viz::CompositorFrameMetadata& metadata)
+    : device_scale_factor(metadata.device_scale_factor),
+      page_scale_factor(metadata.page_scale_factor),
+      root_scroll_offset(metadata.root_scroll_offset),
+      top_controls_height(metadata.top_controls_height),
+      top_controls_shown_ratio(metadata.top_controls_shown_ratio),
+      scrollable_viewport_size(metadata.scrollable_viewport_size) {}
+#else
+// On non-Android, this class should never be created.
+DevToolsFrameMetadata::DevToolsFrameMetadata(
+    const cc::RenderFrameMetadata& metadata) {
+  NOTREACHED();
+}
+
+DevToolsFrameMetadata::DevToolsFrameMetadata(
+    const viz::CompositorFrameMetadata& metadata) {
+  NOTREACHED();
+}
+#endif  // defined(OS_ANDROID)
+
+}  // namespace content

content/browser/devtools/devtools_frame_metadata.h

+// Copyright 2019 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.
+
+#ifndef CONTENT_BROWSER_DEVTOOLS_DEVTOOLS_FRAME_METADATA_H_
+#define CONTENT_BROWSER_DEVTOOLS_DEVTOOLS_FRAME_METADATA_H_
+
+#include "ui/gfx/geometry/size_f.h"
+#include "ui/gfx/geometry/vector2d_f.h"
+
+namespace cc {
+class RenderFrameMetadata;
+}
+
+namespace viz {
+class CompositorFrameMetadata;
+}
+
+namespace content {
+
+// A subset of the information in Render/CompositorFrameMetadata used in
+// DevTools with Android WebView.
+struct DevToolsFrameMetadata {
+ public:
+  explicit DevToolsFrameMetadata(const cc::RenderFrameMetadata& metadata);
+  explicit DevToolsFrameMetadata(const viz::CompositorFrameMetadata& metadata);
+
+  float device_scale_factor;
+  float page_scale_factor;
+  gfx::Vector2dF root_scroll_offset;
+  float top_controls_height;
+  float top_controls_shown_ratio;
+  gfx::SizeF scrollable_viewport_size;
+};
+
+}  // namespace content
+
+#endif  //  CONTENT_BROWSER_DEVTOOLS_DEVTOOLS_FRAME_METADATA_H_

content/browser/devtools/devtools_frame_trace_recorder.cc

@@ -13,7 +13,7 @@
 #include "base/bind.h"
 #include "base/memory/ref_counted.h"
 #include "base/trace_event/trace_event_impl.h"
-#include "components/viz/common/quads/compositor_frame_metadata.h"
+#include "content/browser/devtools/devtools_frame_metadata.h"
 #include "content/browser/devtools/devtools_traceable_screenshot.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/renderer_host/render_widget_host_view_base.h"
@@ -42,7 +42,7 @@ void FrameCaptured(base::TimeTicks timestamp, const SkBitmap& bitmap) {
 }
 
 void CaptureFrame(RenderFrameHostImpl* host,
-                  const viz::CompositorFrameMetadata& metadata) {
+                  const DevToolsFrameMetadata& metadata) {
   RenderWidgetHostViewBase* view =
       static_cast<RenderWidgetHostViewBase*>(host->GetView());
   if (!view)
@@ -82,18 +82,15 @@ DevToolsFrameTraceRecorder::~DevToolsFrameTraceRecorder() { }
 
 void DevToolsFrameTraceRecorder::OnSynchronousSwapCompositorFrame(
     RenderFrameHostImpl* host,
-    const viz::CompositorFrameMetadata& frame_metadata) {
+    const DevToolsFrameMetadata& metadata) {
   if (!host || !ScreenshotCategoryEnabled()) {
-    last_metadata_.reset();
     return;
   }
 
   bool is_new_trace;
   TRACE_EVENT_IS_NEW_TRACE(&is_new_trace);
-  if (!is_new_trace && last_metadata_)
-    CaptureFrame(host, *last_metadata_);
-  last_metadata_.reset(new viz::CompositorFrameMetadata);
-  *last_metadata_ = frame_metadata.Clone();
+  if (!is_new_trace)
+    CaptureFrame(host, metadata);
 }
 
 }  // namespace content

content/browser/devtools/devtools_frame_trace_recorder.h

@@ -5,34 +5,27 @@
 #ifndef CONTENT_BROWSER_DEVTOOLS_DEVTOOLS_FRAME_TRACE_RECORDER_H_
 #define CONTENT_BROWSER_DEVTOOLS_DEVTOOLS_FRAME_TRACE_RECORDER_H_
 
-#include <memory>
-
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 
-namespace viz {
-class CompositorFrameMetadata;
-}
-
 namespace content {
 
 class RenderFrameHostImpl;
+struct DevToolsFrameMetadata;
 
 class DevToolsFrameTraceRecorder {
  public:
   DevToolsFrameTraceRecorder();
   ~DevToolsFrameTraceRecorder();
 
-  void OnSynchronousSwapCompositorFrame(
-      RenderFrameHostImpl* host,
-      const viz::CompositorFrameMetadata& frame_metadata);
+  void OnSynchronousSwapCompositorFrame(RenderFrameHostImpl* host,
+                                        const DevToolsFrameMetadata& metadata);
 
   static constexpr int kMaximumNumberOfScreenshots = 450;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(DevToolsFrameTraceRecorder);
-  std::unique_ptr<viz::CompositorFrameMetadata> last_metadata_;
 };
 
 }  // namespace content

content/browser/devtools/protocol/page_handler.cc

@@ -192,7 +192,6 @@ PageHandler::PageHandler(EmulationHandler* emulation_handler,
       screencast_max_height_(-1),
       capture_every_nth_frame_(1),
       capture_retry_count_(0),
-      has_compositor_frame_metadata_(false),
       session_id_(0),
       frame_counter_(0),
       frames_in_flight_(0),
@@ -269,17 +268,10 @@ void PageHandler::Wire(UberDispatcher* dispatcher) {
 }
 
 void PageHandler::OnSynchronousSwapCompositorFrame(
-    viz::CompositorFrameMetadata frame_metadata) {
-  if (has_compositor_frame_metadata_) {
-    last_compositor_frame_metadata_ =
-        std::move(next_compositor_frame_metadata_);
-  } else {
-    last_compositor_frame_metadata_ = frame_metadata.Clone();
-  }
-  next_compositor_frame_metadata_ = std::move(frame_metadata);
-
-  has_compositor_frame_metadata_ = true;
-
+    const DevToolsFrameMetadata& frame_metadata) {
+  // Cache |frame_metadata_| as InnerSwapCompositorFrame may also be called on
+  // screencast start.
+  frame_metadata_ = frame_metadata;
   if (screencast_enabled_)
     InnerSwapCompositorFrame();
 }
@@ -829,7 +821,7 @@ Response PageHandler::StartScreencast(Maybe<std::string> format,
   if (!visible)
     return Response::FallThrough();
 
-  if (has_compositor_frame_metadata_) {
+  if (frame_metadata_) {
     InnerSwapCompositorFrame();
   } else {
     widget_host->Send(
@@ -982,17 +974,15 @@ void PageHandler::InnerSwapCompositorFrame() {
   if (snapshot_size.IsEmpty())
     return;
 
-  double top_controls_height =
-      last_compositor_frame_metadata_.top_controls_height;
-  double top_controls_shown_ratio =
-      last_compositor_frame_metadata_.top_controls_shown_ratio;
+  double top_controls_height = frame_metadata_->top_controls_height;
+  double top_controls_shown_ratio = frame_metadata_->top_controls_shown_ratio;
 
   std::unique_ptr<Page::ScreencastFrameMetadata> page_metadata =
       BuildScreencastFrameMetadata(
-          surface_size, last_compositor_frame_metadata_.device_scale_factor,
-          last_compositor_frame_metadata_.page_scale_factor,
-          last_compositor_frame_metadata_.root_scroll_offset,
-          top_controls_height, top_controls_shown_ratio);
+          surface_size, frame_metadata_->device_scale_factor,
+          frame_metadata_->page_scale_factor,
+          frame_metadata_->root_scroll_offset, top_controls_height,
+          top_controls_shown_ratio);
   if (!page_metadata)
     return;
 

content/browser/devtools/protocol/page_handler.h

@@ -18,7 +18,7 @@
 #include "base/scoped_observer.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
-#include "components/viz/common/quads/compositor_frame_metadata.h"
+#include "content/browser/devtools/devtools_frame_metadata.h"
 #include "content/browser/devtools/devtools_video_consumer.h"
 #include "content/browser/devtools/protocol/devtools_domain_handler.h"
 #include "content/browser/devtools/protocol/devtools_download_manager_delegate.h"
@@ -71,7 +71,7 @@ class PageHandler : public DevToolsDomainHandler,
                    RenderFrameHostImpl* frame_host) override;
   // Instrumentation signals.
   void OnSynchronousSwapCompositorFrame(
-      viz::CompositorFrameMetadata frame_metadata);
+      const DevToolsFrameMetadata& frame_metadata);
   void DidAttachInterstitialPage();
   void DidDetachInterstitialPage();
   bool screencast_enabled() const { return enabled_ && screencast_enabled_; }
@@ -202,9 +202,7 @@ class PageHandler : public DevToolsDomainHandler,
   int screencast_max_height_;
   int capture_every_nth_frame_;
   int capture_retry_count_;
-  bool has_compositor_frame_metadata_;
-  viz::CompositorFrameMetadata next_compositor_frame_metadata_;
-  viz::CompositorFrameMetadata last_compositor_frame_metadata_;
+  base::Optional<DevToolsFrameMetadata> frame_metadata_;
   int session_id_;
   int frame_counter_;
   int frames_in_flight_;

content/browser/devtools/render_frame_devtools_agent_host.cc

@@ -717,7 +717,7 @@ base::TimeTicks RenderFrameDevToolsAgentHost::GetLastActivityTime() {
 
 void RenderFrameDevToolsAgentHost::SignalSynchronousSwapCompositorFrame(
     RenderFrameHost* frame_host,
-    viz::CompositorFrameMetadata frame_metadata) {
+    const DevToolsFrameMetadata& frame_metadata) {
   scoped_refptr<RenderFrameDevToolsAgentHost> dtah(FindAgentHost(
       static_cast<RenderFrameHostImpl*>(frame_host)->frame_tree_node()));
   if (dtah) {
@@ -726,14 +726,14 @@ void RenderFrameDevToolsAgentHost::SignalSynchronousSwapCompositorFrame(
         FROM_HERE, {BrowserThread::UI},
         base::BindOnce(
             &RenderFrameDevToolsAgentHost::SynchronousSwapCompositorFrame,
-            dtah.get(), std::move(frame_metadata)));
+            dtah.get(), frame_metadata));
   }
 }
 
 void RenderFrameDevToolsAgentHost::SynchronousSwapCompositorFrame(
-    viz::CompositorFrameMetadata frame_metadata) {
+    const DevToolsFrameMetadata& frame_metadata) {
   for (auto* page : protocol::PageHandler::ForAgentHost(this))
-    page->OnSynchronousSwapCompositorFrame(frame_metadata.Clone());
+    page->OnSynchronousSwapCompositorFrame(frame_metadata);
 
   if (!frame_trace_recorder_)
     return;

content/browser/devtools/render_frame_devtools_agent_host.h

@@ -27,10 +27,6 @@
 #include "ui/android/view_android.h"
 #endif  // OS_ANDROID
 
-namespace viz {
-class CompositorFrameMetadata;
-}
-
 namespace content {
 
 class BrowserContext;
@@ -38,6 +34,7 @@ class DevToolsFrameTraceRecorder;
 class FrameTreeNode;
 class NavigationHandleImpl;
 class RenderFrameHostImpl;
+struct DevToolsFrameMetadata;
 
 class CONTENT_EXPORT RenderFrameDevToolsAgentHost
     : public DevToolsAgentHostImpl,
@@ -64,7 +61,7 @@ class CONTENT_EXPORT RenderFrameDevToolsAgentHost
 
   static void SignalSynchronousSwapCompositorFrame(
       RenderFrameHost* frame_host,
-      viz::CompositorFrameMetadata frame_metadata);
+      const DevToolsFrameMetadata& frame_metadata);
 
   FrameTreeNode* frame_tree_node() { return frame_tree_node_; }
 
@@ -119,7 +116,6 @@ class CONTENT_EXPORT RenderFrameDevToolsAgentHost
 
   bool IsChildFrame();
 
-  void OnSwapCompositorFrame(const IPC::Message& message);
   void DestroyOnRenderFrameGone();
   void UpdateFrameHost(RenderFrameHostImpl* frame_host);
   void SetFrameTreeNode(FrameTreeNode* frame_tree_node);
@@ -131,7 +127,7 @@ class CONTENT_EXPORT RenderFrameDevToolsAgentHost
 #endif
 
   void SynchronousSwapCompositorFrame(
-      viz::CompositorFrameMetadata frame_metadata);
+      const DevToolsFrameMetadata& frame_metadata);
   void UpdateResourceLoaderFactories();
 
   std::unique_ptr<DevToolsFrameTraceRecorder> frame_trace_recorder_;

content/browser/renderer_host/render_widget_host_view_android.cc

@@ -9,6 +9,7 @@
 #include <utility>
 
 #include "base/android/build_info.h"
+#include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
@@ -354,6 +355,11 @@ void RenderWidgetHostViewAndroid::OnRenderFrameMetadataChangedBeforeActivation(
   if (!features::IsSurfaceSynchronizationEnabled())
     return;
 
+  if (!using_browser_compositor_) {
+    // DevTools ScreenCast support for Android WebView.
+    last_devtools_frame_metadata_.emplace(metadata);
+  }
+
   bool is_mobile_optimized = IsMobileOptimizedFrame(
       metadata.page_scale_factor, metadata.min_page_scale_factor,
       metadata.max_page_scale_factor, metadata.scrollable_viewport_size,
@@ -1040,14 +1046,14 @@ bool RenderWidgetHostViewAndroid::RequestRepaintForTesting() {
 
 void RenderWidgetHostViewAndroid::SynchronousFrameMetadata(
     viz::CompositorFrameMetadata metadata) {
+  // TODO(ericrk): Remove this function once surface synchronization feature is
+  // no longer optional.
+  if (features::IsSurfaceSynchronizationEnabled())
+    return;
+
   if (!view_.parent())
     return;
 
-  // With SurfaceSynchronization, this logic happens during RenderFrameMetadata
-  // processing.
-  // TODO(ericrk): Remove this once surface synchronization feature is no
-  // longer optional.
-  if (!features::IsSurfaceSynchronizationEnabled()) {
     bool is_mobile_optimized = IsMobileOptimizedFrame(
         metadata.page_scale_factor, metadata.min_page_scale_factor,
         metadata.max_page_scale_factor, metadata.scrollable_viewport_size,
@@ -1059,20 +1065,35 @@ void RenderWidgetHostViewAndroid::SynchronousFrameMetadata(
     // This is a subset of OnSwapCompositorFrame() used in the synchronous
     // compositor flow.
     OnFrameMetadataUpdated(metadata.Clone(), false);
-  }
 
-  // DevTools ScreenCast support for Android WebView.
-  RenderFrameHost* frame_host = RenderViewHost::From(host())->GetMainFrame();
-  if (frame_host) {
-    RenderFrameDevToolsAgentHost::SignalSynchronousSwapCompositorFrame(
-        frame_host, std::move(metadata));
-  }
+    // DevTools ScreenCast support for Android WebView.
+    last_devtools_frame_metadata_.emplace(metadata);
 }
 
 void RenderWidgetHostViewAndroid::FrameTokenChangedForSynchronousCompositor(
-    uint32_t frame_token) {
-  if (host() && frame_token)
+    uint32_t frame_token,
+    const gfx::ScrollOffset& root_scroll_offset) {
+  if (host() && frame_token) {
     host()->DidProcessFrame(frame_token);
+
+    // DevTools ScreenCast support for Android WebView. Don't call this if
+    // we're currently in SynchronousCopyContents, as this can lead to
+    // redundant copies.
+    if (!in_sync_copy_contents_) {
+      RenderFrameHost* frame_host =
+          RenderViewHost::From(host())->GetMainFrame();
+      if (frame_host && last_devtools_frame_metadata_) {
+        // Update our |root_scroll_offset|, as changes to this value do not
+        // trigger a new RenderFrameMetadata, and it may be out of date. This
+        // is needed for devtools DOM node selection.
+        DevToolsFrameMetadata updated_metadata = *last_devtools_frame_metadata_;
+        updated_metadata.root_scroll_offset =
+            gfx::ScrollOffsetToVector2dF(root_scroll_offset);
+        RenderFrameDevToolsAgentHost::SignalSynchronousSwapCompositorFrame(
+            frame_host, updated_metadata);
+      }
+    }
+  }
 }
 
 void RenderWidgetHostViewAndroid::SetSynchronousCompositorClient(
@@ -1174,6 +1195,10 @@ void RenderWidgetHostViewAndroid::SynchronousCopyContents(
     const gfx::Rect& src_subrect_dip,
     const gfx::Size& dst_size_in_pixel,
     base::OnceCallback<void(const SkBitmap&)> callback) {
+  // Track that we're in this function to avoid repeatedly calling DevTools
+  // capture logic.
+  base::AutoReset<bool> in_sync_copy_contents(&in_sync_copy_contents_, true);
+
   // Note: When |src_subrect| is empty, a conversion from the view size must
   // be made instead of using |current_frame_size_|. The latter sometimes also
   // includes extra height for the toolbar UI, which is not intended for

content/browser/renderer_host/render_widget_host_view_android.h

@@ -24,6 +24,7 @@
 #include "components/viz/common/presentation_feedback_map.h"
 #include "components/viz/common/quads/selection.h"
 #include "components/viz/common/surfaces/parent_local_surface_id_allocator.h"
+#include "content/browser/devtools/devtools_frame_metadata.h"
 #include "content/browser/renderer_host/input/mouse_wheel_phase_handler.h"
 #include "content/browser/renderer_host/input/stylus_text_selector.h"
 #include "content/browser/renderer_host/render_widget_host_view_base.h"
@@ -303,7 +304,11 @@ class CONTENT_EXPORT RenderWidgetHostViewAndroid
                                 int end_adjust);
 
   void SynchronousFrameMetadata(viz::CompositorFrameMetadata frame_metadata);
-  void FrameTokenChangedForSynchronousCompositor(uint32_t frame_token);
+  // TODO(ericrk): Ideally we'd reemove |root_scroll_offset| from this function
+  // once we have a reliable way to get it through RenderFrameMetadata.
+  void FrameTokenChangedForSynchronousCompositor(
+      uint32_t frame_token,
+      const gfx::ScrollOffset& root_scroll_offset);
 
   void SetSynchronousCompositorClient(SynchronousCompositorClient* client);
 
@@ -528,6 +533,15 @@ class CONTENT_EXPORT RenderWidgetHostViewAndroid
 
   viz::PresentationFeedbackMap presentation_feedbacks_;
 
+  // Tracks whether we are in SynchronousCopyContents to avoid repeated calls
+  // into DevTools capture logic.
+  // TODO(ericrk): Make this more robust.
+  bool in_sync_copy_contents_ = false;
+
+  // A cached copy of the most up to date DevToolsFrameMetadata, computed from
+  // either RenderFrameMetadata or CompositorFrameMetadata.
+  base::Optional<DevToolsFrameMetadata> last_devtools_frame_metadata_;
+
   base::WeakPtrFactory<RenderWidgetHostViewAndroid> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(RenderWidgetHostViewAndroid);