Lacros: Remove deprecated ScreenManager methods.

This CL is a refactor with no intended behavior changes.

The new ScreenManager interface was landed in M88. Ash beta is now M88,
and Lacros is not available on Ash stable M87, so Lacros can always
assume that the new ScreenManager interfaces exist.

Removing this dead code is a prerequisite to refactoring the test code,
which itself is a prerequisite for other lacros_chrome_browsertests.

Change-Id: Ia5311010c8003f763409b2dc556a7dd6a99fc80d
Bug: 1155662, 1156872, 1094460
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2580107Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835254}

chrome/browser/chromeos/crosapi/screen_manager_ash.cc

@@ -206,34 +206,17 @@ void ScreenManagerAsh::BindReceiver(
 
 void ScreenManagerAsh::DeprecatedTakeScreenSnapshot(
     DeprecatedTakeScreenSnapshotCallback callback) {
-  GetScreenCapturerImpl()->TakeSnapshot(
-      GetScreenCapturerImpl()->GetPrimaryRootWindowId(),
-      base::BindOnce(
-          [](DeprecatedTakeScreenSnapshotCallback callback, bool success,
-             const SkBitmap& bitmap) {
-            // Ignore |success| param.
-            std::move(callback).Run(BitmapFromSkBitmap(bitmap));
-          },
-          std::move(callback)));
+  NOTIMPLEMENTED();
 }
-
 void ScreenManagerAsh::DeprecatedListWindows(
     DeprecatedListWindowsCallback callback) {
-  GetWindowCapturerImpl()->ListSources(std::move(callback));
+  NOTIMPLEMENTED();
 }
-
 void ScreenManagerAsh::DeprecatedTakeWindowSnapshot(
     uint64_t id,
     DeprecatedTakeWindowSnapshotCallback callback) {
-  GetWindowCapturerImpl()->TakeSnapshot(
-      id, base::BindOnce(
-              [](DeprecatedTakeWindowSnapshotCallback callback, bool success,
-                 const SkBitmap& bitmap) {
-                std::move(callback).Run(success, BitmapFromSkBitmap(bitmap));
-              },
-              std::move(callback)));
+  NOTIMPLEMENTED();
 }
-
 void ScreenManagerAsh::GetScreenCapturer(
     mojo::PendingReceiver<mojom::SnapshotCapturer> receiver) {
   GetScreenCapturerImpl()->BindReceiver(std::move(receiver));

chrome/browser/chromeos/crosapi/screen_manager_ash_browsertest.cc

@@ -12,7 +12,6 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/test/base/in_process_browser_test.h"
-#include "chromeos/crosapi/cpp/bitmap.h"
 #include "chromeos/crosapi/mojom/screen_manager.mojom.h"
 #include "content/public/test/browser_test.h"
 #include "mojo/public/cpp/bindings/remote.h"
@@ -80,65 +79,6 @@ class ScreenManagerAshBrowserTest : public InProcessBrowserTest {
   std::unique_ptr<SMRemote> screen_manager_remote_;
 };
 
-// Tests that taking a screen snapshot works.
-IN_PROC_BROWSER_TEST_F(ScreenManagerAshBrowserTest,
-                       DeprecatedTakeScreenSnapshot) {
-  base::RunLoop run_loop;
-  Bitmap snapshot;
-
-  // Take a snapshot on a background sequence. The call is blocking, so when it
-  // finishes, we can also unblock the main thread.
-  auto take_snapshot_background = base::BindOnce(
-      [](SMRemote* remote, Bitmap* snapshot) {
-        mojo::ScopedAllowSyncCallForTesting allow_sync;
-        (*remote)->DeprecatedTakeScreenSnapshot(snapshot);
-      },
-      screen_manager_remote_.get(), &snapshot);
-  background_sequence_->PostTaskAndReply(
-      FROM_HERE, std::move(take_snapshot_background), run_loop.QuitClosure());
-  run_loop.Run();
-
-  // Check that the screenshot has the right dimensions.
-  aura::Window* primary_window =
-      browser()->window()->GetNativeWindow()->GetRootWindow();
-  EXPECT_EQ(int{snapshot.width}, primary_window->bounds().width());
-  EXPECT_EQ(int{snapshot.height}, primary_window->bounds().height());
-}
-
-IN_PROC_BROWSER_TEST_F(ScreenManagerAshBrowserTest,
-                       DeprecatedTakeWindowSnapshot) {
-  base::RunLoop run_loop;
-  bool success;
-  Bitmap snapshot;
-
-  // Take a snapshot on a background sequence. The call is blocking, so when it
-  // finishes, we can also unblock the main thread.
-  auto take_snapshot_background = base::BindOnce(
-      [](SMRemote* remote, bool* success, Bitmap* snapshot) {
-        mojo::ScopedAllowSyncCallForTesting allow_sync;
-        std::vector<mojom::SnapshotSourcePtr> windows;
-        (*remote)->DeprecatedListWindows(&windows);
-
-        // There should be exactly 1 window.
-        ASSERT_EQ(1u, windows.size());
-
-        (*remote)->DeprecatedTakeWindowSnapshot(windows[0]->id, success,
-                                                snapshot);
-      },
-      screen_manager_remote_.get(), &success, &snapshot);
-  background_sequence_->PostTaskAndReply(
-      FROM_HERE, std::move(take_snapshot_background), run_loop.QuitClosure());
-  run_loop.Run();
-
-  // Check that the IPC succeeded.
-  ASSERT_TRUE(success);
-
-  // Check that the screenshot has the right dimensions.
-  aura::Window* window = browser()->window()->GetNativeWindow();
-  EXPECT_EQ(int{snapshot.width}, window->bounds().width());
-  EXPECT_EQ(int{snapshot.height}, window->bounds().height());
-}
-
 IN_PROC_BROWSER_TEST_F(ScreenManagerAshBrowserTest, ScreenCapturer) {
   base::RunLoop run_loop;
   bool success;

chrome/browser/lacros/screen_manager_lacros_browsertest.cc

@@ -108,60 +108,6 @@ class ScreenManagerLacrosBrowserTest : public InProcessBrowserTest {
   mojo::Remote<crosapi::mojom::ScreenManager> screen_manager_;
 };
 
-// Tests that taking a screen snapshot via crosapi works.
-IN_PROC_BROWSER_TEST_F(ScreenManagerLacrosBrowserTest,
-                       DeprecatedTakeScreenSnapshot) {
-  BindScreenManager();
-  crosapi::Bitmap snapshot;
-  {
-    mojo::ScopedAllowSyncCallForTesting allow_sync_call;
-    screen_manager_->DeprecatedTakeScreenSnapshot(&snapshot);
-  }
-  // Verify the snapshot is non-empty.
-  EXPECT_GT(snapshot.height, 0u);
-  EXPECT_GT(snapshot.width, 0u);
-  EXPECT_GT(snapshot.pixels.size(), 0u);
-}
-
-// Tests that taking a screen snapshot via crosapi works.
-// This test makes the browser load a page with specific title, and then scans
-// through a list of windows to look for the window with the expected title.
-// This test cannot simply assert exactly 1 window is present because currently
-// in lacros_chrome_browsertests, different browser tests share the same
-// ash-chrome, so a window could come from any one of them.
-IN_PROC_BROWSER_TEST_F(ScreenManagerLacrosBrowserTest,
-                       DeprecatedTakeWindowSnapshot) {
-  GURL url(std::string("data:text/html,") + kLacrosPageTitleHTML);
-  ui_test_utils::NavigateToURL(browser(), url);
-
-  BindScreenManager();
-
-  auto list_windows = base::BindRepeating(
-      [](mojo::Remote<crosapi::mojom::ScreenManager>* screen_manager,
-         std::vector<crosapi::mojom::SnapshotSourcePtr>* windows) {
-        mojo::ScopedAllowSyncCallForTesting allow_sync_call;
-        (*screen_manager)->DeprecatedListWindows(windows);
-      },
-      &screen_manager_);
-
-  uint64_t window_id;
-  bool found_window = FindTestWindow(std::move(list_windows), &window_id);
-  ASSERT_TRUE(found_window);
-
-  bool success = false;
-  crosapi::Bitmap snapshot;
-  {
-    mojo::ScopedAllowSyncCallForTesting allow_sync_call;
-    screen_manager_->DeprecatedTakeWindowSnapshot(window_id, &success,
-                                                  &snapshot);
-  }
-  ASSERT_TRUE(success);
-  // Verify the snapshot is non-empty.
-  EXPECT_GT(snapshot.height, 0u);
-  EXPECT_GT(snapshot.width, 0u);
-  EXPECT_GT(snapshot.pixels.size(), 0u);
-}
-
 // Tests that taking a screen snapshot via crosapi works.
 IN_PROC_BROWSER_TEST_F(ScreenManagerLacrosBrowserTest, ScreenCapturer) {
   BindScreenManager();

chromeos/crosapi/mojom/bitmap.mojom

@@ -6,7 +6,7 @@ module crosapi.mojom;
 
 // Each pixel is represented by 4 bytes [RGBA].
 // NOTE: For UI images prefer gfx.mojom.ImageSkia, which supports high-DPI.
-// TODO(https://crbug.com/1094460): Use a more performant transport mechanism.
+// TODO(https://crbug.com/1156872): This struct is unused and should be deleted.
 [Stable]
 struct Bitmap {
   uint32 width@0;

chromeos/crosapi/mojom/screen_manager.mojom

@@ -46,27 +46,12 @@ interface SnapshotCapturer {
 // interface in the future.
 [Stable, Uuid="95c3035c-5c63-45e3-8ec8-dd2a344c7dc2"]
 interface ScreenManager {
-  // DEPRECATED. Use |GetScreenCapturer| instead.
-  //
-  // This method assumes that there's exactly one screen. The implementation
-  // of this method takes a screenshot and converts it into a bitmap. Each
-  // pixel is represented by 4 bytes [RGBA].
-  //
-  // The media screen capture implementation assumes that every platform
-  // provides a synchronous method to take a snapshot of the screen.
+  // DEPRECATED. Unimplemented and unused. Delete this once deletion is
+  // supported. https://crbug.com/1156872.
   [Sync]
   DeprecatedTakeScreenSnapshot@0() => (Bitmap snapshot);
-
-  // DEPRECATED. Use |GetWindowCapturer| instead.
-  //
-  // Returns a list of visible, focusable top-level windows.
   [Sync]
   DeprecatedListWindows@1() => (array<SnapshotSource> windows);
-
-  // DEPRECATED. Use |GetWindowCapturer| instead.
-  //
-  // Take a screenshot of a window with an id from ListWindows. If |success|
-  // is false, then the window no longer exists.
   [Sync]
   DeprecatedTakeWindowSnapshot@2(uint64 id) => (bool success, Bitmap snapshot);
 

content/browser/media/capture/desktop_capturer_lacros.cc

@@ -4,13 +4,9 @@
 
 #include "content/browser/media/capture/desktop_capturer_lacros.h"
 
-#include "base/run_loop.h"
-#include "base/synchronization/waitable_event.h"
-#include "base/task/task_traits.h"
-#include "base/task/thread_pool.h"
-
-#include "chromeos/crosapi/cpp/bitmap.h"
 #include "chromeos/lacros/lacros_chrome_service_impl.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
 
 namespace content {
 namespace {
@@ -55,20 +51,9 @@ bool DesktopCapturerLacros::GetSourceList(SourceList* result) {
 
   std::vector<crosapi::mojom::SnapshotSourcePtr> sources;
 
-  if (snapshot_capturer_) {
+  {
     mojo::SyncCallRestrictions::ScopedAllowSyncCall allow_sync_call;
     snapshot_capturer_->ListSources(&sources);
-  } else {
-    if (capture_type_ == kScreen) {
-      // TODO(https://crbug.com/1094460): Implement this source list
-      // appropriately.
-      Source w;
-      w.id = 1;
-      result->push_back(w);
-      return true;
-    }
-    mojo::SyncCallRestrictions::ScopedAllowSyncCall allow_sync_call;
-    screen_manager_->DeprecatedListWindows(&sources);
   }
 
   for (auto& source : sources) {
@@ -105,25 +90,17 @@ void DesktopCapturerLacros::Start(Callback* callback) {
   lacros_chrome_service->BindScreenManagerReceiver(
       screen_manager_.BindNewPipeAndPassReceiver());
 
-  // Do a round-trip to make sure we know what version of the screen manager we
-  // are talking to. It is safe to run a nested loop here because we are on a
-  // dedicated thread (see DesktopCaptureDevice) with nothing else happening.
-  // TODO(crbug/1094460): Avoid this nested event loop.
-  base::RunLoop run_loop(base::RunLoop::Type::kNestableTasksAllowed);
-  screen_manager_.QueryVersion(
-      base::BindOnce([](base::OnceClosure quit_closure,
-                        uint32_t version) { std::move(quit_closure).Run(); },
-                     run_loop.QuitClosure()));
-  run_loop.Run();
-
-  if (screen_manager_.version() >= 1) {
-    if (capture_type_ == kScreen) {
-      screen_manager_->GetScreenCapturer(
-          snapshot_capturer_.BindNewPipeAndPassReceiver());
-    } else {
-      screen_manager_->GetWindowCapturer(
-          snapshot_capturer_.BindNewPipeAndPassReceiver());
-    }
+  // Lacros can assume that Ash is at least M88.
+  int version = lacros_chrome_service->GetInterfaceVersion(
+      crosapi::mojom::ScreenManager::Uuid_);
+  CHECK_GE(version, 1);
+
+  if (capture_type_ == kScreen) {
+    screen_manager_->GetScreenCapturer(
+        snapshot_capturer_.BindNewPipeAndPassReceiver());
+  } else {
+    screen_manager_->GetWindowCapturer(
+        snapshot_capturer_.BindNewPipeAndPassReceiver());
   }
 }
 
@@ -135,23 +112,9 @@ void DesktopCapturerLacros::CaptureFrame() {
   capturing_frame_ = true;
 #endif
 
-  if (snapshot_capturer_) {
-    snapshot_capturer_->TakeSnapshot(
-        selected_source_,
-        base::BindOnce(&DesktopCapturerLacros::DidTakeSnapshot,
-                       weak_factory_.GetWeakPtr()));
-  } else {
-    if (capture_type_ == kScreen) {
-      screen_manager_->DeprecatedTakeScreenSnapshot(
-          base::BindOnce(&DesktopCapturerLacros::DeprecatedDidTakeSnapshot,
-                         weak_factory_.GetWeakPtr(), /*success*/ true));
-    } else {
-      screen_manager_->DeprecatedTakeWindowSnapshot(
-          selected_source_,
-          base::BindOnce(&DesktopCapturerLacros::DeprecatedDidTakeSnapshot,
-                         weak_factory_.GetWeakPtr()));
-    }
-  }
+  snapshot_capturer_->TakeSnapshot(
+      selected_source_, base::BindOnce(&DesktopCapturerLacros::DidTakeSnapshot,
+                                       weak_factory_.GetWeakPtr()));
 }
 
 bool DesktopCapturerLacros::IsOccluded(const webrtc::DesktopVector& pos) {
@@ -181,32 +144,4 @@ void DesktopCapturerLacros::DidTakeSnapshot(bool success,
                              std::make_unique<DesktopFrameSkia>(snapshot));
 }
 
-void DesktopCapturerLacros::DeprecatedDidTakeSnapshot(
-    bool success,
-    crosapi::Bitmap snapshot) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-#if DCHECK_IS_ON()
-  capturing_frame_ = false;
-#endif
-
-  if (!success) {
-    callback_->OnCaptureResult(Result::ERROR_PERMANENT,
-                               std::unique_ptr<webrtc::DesktopFrame>());
-    return;
-  }
-
-  std::unique_ptr<webrtc::DesktopFrame> frame =
-      std::make_unique<webrtc::BasicDesktopFrame>(
-          webrtc::DesktopSize(snapshot.width, snapshot.height));
-
-  // This code assumes that the stride is 4 * width. This relies on the
-  // assumption that there's no padding and each pixel is 4 bytes.
-  frame->CopyPixelsFrom(
-      snapshot.pixels.data(), 4 * snapshot.width,
-      webrtc::DesktopRect::MakeWH(snapshot.width, snapshot.height));
-
-  callback_->OnCaptureResult(Result::SUCCESS, std::move(frame));
-}
-
 }  // namespace content

content/browser/media/capture/desktop_capturer_lacros.h

@@ -12,10 +12,6 @@
 #include "third_party/webrtc/modules/desktop_capture/desktop_capture_options.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_capturer.h"
 
-namespace crosapi {
-struct Bitmap;
-}  // namespace crosapi
-
 namespace content {
 
 // This class is responsible for communicating with ash-chrome to get snapshots
@@ -50,7 +46,6 @@ class DesktopCapturerLacros : public webrtc::DesktopCapturer {
   // Callback for when ash-chrome returns a snapshot of the screen or window as
   // a bitmap.
   void DidTakeSnapshot(bool success, const SkBitmap& snapshot);
-  void DeprecatedDidTakeSnapshot(bool success, crosapi::Bitmap snapshot);
 
   SEQUENCE_CHECKER(sequence_checker_);
 
@@ -76,8 +71,8 @@ class DesktopCapturerLacros : public webrtc::DesktopCapturer {
   // The remote connection to the screen manager.
   mojo::Remote<crosapi::mojom::ScreenManager> screen_manager_;
 
-  // Only set if we are using a newer screen manager. Otherwise, we fallback to
-  // the deprecated methods.
+  // A remote for an ash interface that is responsible for either capturing
+  // screen snapshots or window snapshots.
   mojo::Remote<crosapi::mojom::SnapshotCapturer> snapshot_capturer_;
 
 #if DCHECK_IS_ON()