capture_mode: Events ending video recording 2

In this CL, we handle:
- Imminent suspends.
- User switches in multi-profile sessions.

BUG=1142994
TEST=Added new tests

Change-Id: I31da5c8cfb4f2648b2a34b206673eb59cce69f29
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2538590
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828602}

ash/capture_mode/capture_mode_controller.cc

@@ -18,6 +18,7 @@
 #include "ash/public/cpp/holding_space/holding_space_controller.h"
 #include "ash/public/cpp/notification_utils.h"
 #include "ash/resources/vector_icons/vector_icons.h"
+#include "ash/session/session_controller_impl.h"
 #include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "ash/system/status_area_widget.h"
@@ -274,9 +275,14 @@ CaptureModeController::CaptureModeController(
       base::BindRepeating(
           &CaptureModeController::RecordAndResetScreenshotsTakenInLastWeek,
           weak_ptr_factory_.GetWeakPtr()));
+
+  Shell::Get()->session_controller()->AddObserver(this);
+  chromeos::PowerManagerClient::Get()->AddObserver(this);
 }
 
 CaptureModeController::~CaptureModeController() {
+  chromeos::PowerManagerClient::Get()->RemoveObserver(this);
+  Shell::Get()->session_controller()->RemoveObserver(this);
   DCHECK_EQ(g_instance, this);
   g_instance = nullptr;
 }
@@ -382,12 +388,60 @@ void CaptureModeController::OnRecordingEnded(bool success) {
                            weak_ptr_factory_.GetWeakPtr()));
 }
 
+void CaptureModeController::OnActiveUserSessionChanged(
+    const AccountId& account_id) {
+  EndSessionOrRecording(/*for_suspend=*/false);
+}
+
+void CaptureModeController::OnSessionStateChanged(
+    session_manager::SessionState state) {
+  if (Shell::Get()->session_controller()->IsUserSessionBlocked())
+    EndSessionOrRecording(/*for_suspend=*/false);
+}
+
+void CaptureModeController::OnChromeTerminating() {
+  EndSessionOrRecording(/*for_suspend=*/false);
+}
+
+void CaptureModeController::SuspendImminent(
+    power_manager::SuspendImminent::Reason reason) {
+  EndSessionOrRecording(/*for_suspend=*/true);
+}
+
 void CaptureModeController::StartVideoRecordingImmediatelyForTesting() {
   DCHECK(IsActive());
   DCHECK_EQ(type_, CaptureModeType::kVideo);
   OnVideoRecordCountDownFinished();
 }
 
+void CaptureModeController::EndSessionOrRecording(bool for_suspend) {
+  if (IsActive()) {
+    // Suspend or user session changes can happen while the capture mode session
+    // is active or after the three-second countdown had started but not
+    // finished yet.
+    Stop();
+    return;
+  }
+
+  if (!is_recording_in_progress_)
+    return;
+
+  if (for_suspend) {
+    // If suspend happens while recording is in progress, we consider this a
+    // failure, and cut the recording immediately. The recording service may
+    // have some buffered chunks that will never be received, and as a result,
+    // the a few seconds at the end of the recording may get lost.
+    // TODO(afakhry): Think whether this is what we want. We might be able to
+    // end the recording normally by asking the service to StopRecording(), and
+    // block the suspend until all chunks have been received, and then we can
+    // resume it.
+    OnRecordingEnded(/*success=*/false);
+    return;
+  }
+
+  EndVideoRecording();
+}
+
 base::Optional<CaptureModeController::CaptureParams>
 CaptureModeController::GetCaptureParams() const {
   DCHECK(IsActive());

ash/capture_mode/capture_mode_controller.h

@@ -13,6 +13,7 @@
 #include "ash/capture_mode/capture_mode_types.h"
 #include "ash/capture_mode/video_file_handler.h"
 #include "ash/public/cpp/capture_mode_delegate.h"
+#include "ash/public/cpp/session/session_observer.h"
 #include "ash/services/recording/public/mojom/recording_service.mojom.h"
 #include "base/callback_forward.h"
 #include "base/memory/ref_counted_memory.h"
@@ -21,6 +22,7 @@
 #include "base/optional.h"
 #include "base/threading/sequence_bound.h"
 #include "base/timer/timer.h"
+#include "chromeos/dbus/power/power_manager_client.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/image/image.h"
@@ -42,7 +44,9 @@ class VideoRecordingWatcher;
 
 // Controls starting and ending a Capture Mode session and its behavior.
 class ASH_EXPORT CaptureModeController
-    : public recording::mojom::RecordingServiceClient {
+    : public recording::mojom::RecordingServiceClient,
+      public SessionObserver,
+      public chromeos::PowerManagerClient::Observer {
  public:
   explicit CaptureModeController(std::unique_ptr<CaptureModeDelegate> delegate);
   CaptureModeController(const CaptureModeController&) = delete;
@@ -93,6 +97,14 @@ class ASH_EXPORT CaptureModeController
   void OnMuxerOutput(const std::string& chunk) override;
   void OnRecordingEnded(bool success) override;
 
+  // SessionObserver:
+  void OnActiveUserSessionChanged(const AccountId& account_id) override;
+  void OnSessionStateChanged(session_manager::SessionState state) override;
+  void OnChromeTerminating() override;
+
+  // chromeos::PowerManagerClient::Observer:
+  void SuspendImminent(power_manager::SuspendImminent::Reason reason) override;
+
   // Skips the 3-second count down, and IsCaptureAllowed() checks, and starts
   // video recording right away for testing purposes.
   void StartVideoRecordingImmediatelyForTesting();
@@ -100,6 +112,12 @@ class ASH_EXPORT CaptureModeController
  private:
   friend class CaptureModeTestApi;
 
+  // Used by user session change, and suspend events to end the capture mode
+  // session if it's active, or stop the video recording if one is in progress.
+  // |for_suspend| is true when this is called from |SuspendImminent()|, which
+  // leads to ending the video recording immediately as if it's a failure.
+  void EndSessionOrRecording(bool for_suspend);
+
   // Returns the capture parameters for the capture operation that is about to
   // be performed (i.e. the window to be captured, and the capture bounds). If
   // nothing is to be captured (e.g. when there's no window selected in a

ash/capture_mode/capture_mode_unittests.cc

@@ -30,6 +30,9 @@
 #include "base/test/bind.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
+#include "chromeos/dbus/power/fake_power_manager_client.h"
+#include "chromeos/dbus/power_manager/suspend.pb.h"
+#include "components/account_id/account_id.h"
 #include "ui/aura/window_tracker.h"
 #include "ui/compositor/scoped_animation_duration_scale_mode.h"
 #include "ui/events/keycodes/keyboard_codes_posix.h"
@@ -271,6 +274,21 @@ class CaptureModeTest : public AshTestBase {
     base::RunLoop().RunUntilIdle();
   }
 
+  void SwitchToUser2() {
+    auto* session_controller = GetSessionControllerClient();
+    constexpr char kUserEmail[] = "user2@capture_mode";
+    session_controller->AddUserSession(kUserEmail);
+    session_controller->SwitchActiveUser(AccountId::FromUserEmail(kUserEmail));
+  }
+
+  void WaitForSeconds(int seconds) {
+    base::RunLoop loop;
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE, base::BindLambdaForTesting([&]() { loop.Quit(); }),
+        base::TimeDelta::FromSeconds(seconds));
+    loop.Run();
+  }
+
  private:
   base::test::ScopedFeatureList scoped_feature_list_;
 };
@@ -385,7 +403,7 @@ TEST_F(CaptureModeTest, ChangeTypeAndSourceFromUI) {
 // TODO(https://crbug.com/1141927): test is flakey.
 TEST_F(CaptureModeTest, DISABLED_VideoRecordingUiBehavior) {
   // We need a non-zero duration to avoid infinite loop on countdown.
-  ui::ScopedAnimationDurationScaleMode animatin_scale(
+  ui::ScopedAnimationDurationScaleMode animation_scale(
       ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
 
   auto* controller = CaptureModeController::Get();
@@ -1079,12 +1097,17 @@ TEST_F(CaptureModeTest, FullscreenCursorStates) {
   EXPECT_TRUE(cursor_manager->IsCursorVisible());
 
   // Enter tablet mode, the cursor should be hidden.
-  TabletModeControllerTestApi().EnterTabletMode();
+  TabletModeControllerTestApi tablet_mode_controller_test_api;
+  // To avoid flaky failures due to mouse devices blocking entering tablet mode,
+  // we detach all mouse devices. This shouldn't affect testing the cursor
+  // status.
+  tablet_mode_controller_test_api.DetachAllMice();
+  tablet_mode_controller_test_api.EnterTabletMode();
   EXPECT_FALSE(cursor_manager->IsCursorVisible());
   EXPECT_TRUE(cursor_manager->IsCursorLocked());
 
   // Exit tablet mode, the cursor should appear again.
-  TabletModeControllerTestApi().LeaveTabletMode();
+  tablet_mode_controller_test_api.LeaveTabletMode();
   EXPECT_TRUE(cursor_manager->IsCursorVisible());
   EXPECT_EQ(CursorType::kCustom, cursor_manager->GetCursor().type());
   EXPECT_TRUE(test_api.IsUsingCustomCursor(CaptureModeType::kVideo));
@@ -1155,12 +1178,17 @@ TEST_F(CaptureModeTest, WindowCursorStates) {
   EXPECT_TRUE(test_api.IsUsingCustomCursor(CaptureModeType::kVideo));
 
   // Enter tablet mode, the cursor should be hidden.
-  TabletModeControllerTestApi().EnterTabletMode();
+  TabletModeControllerTestApi tablet_mode_controller_test_api;
+  // To avoid flaky failures due to mouse devices blocking entering tablet mode,
+  // we detach all mouse devices. This shouldn't affect testing the cursor
+  // status.
+  tablet_mode_controller_test_api.DetachAllMice();
+  tablet_mode_controller_test_api.EnterTabletMode();
   EXPECT_FALSE(cursor_manager->IsCursorVisible());
   EXPECT_TRUE(cursor_manager->IsCursorLocked());
 
   // Exit tablet mode, the cursor should appear again.
-  TabletModeControllerTestApi().LeaveTabletMode();
+  tablet_mode_controller_test_api.LeaveTabletMode();
   EXPECT_TRUE(cursor_manager->IsCursorVisible());
   EXPECT_EQ(CursorType::kCustom, cursor_manager->GetCursor().type());
   EXPECT_TRUE(test_api.IsUsingCustomCursor(CaptureModeType::kVideo));
@@ -1223,7 +1251,7 @@ TEST_F(CaptureModeTest, RegionDragCursorCompositing) {
 // incoming input events.
 TEST_F(CaptureModeTest, DoNotHandleEventDuringCountDown) {
   // We need a non-zero duration to avoid infinite loop on countdown.
-  ui::ScopedAnimationDurationScaleMode animatin_scale(
+  ui::ScopedAnimationDurationScaleMode animation_scale(
       ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
 
   // Create 2 windows that overlap with each other.
@@ -1380,6 +1408,67 @@ TEST_F(CaptureModeTest, DetachDisplayWhileWindowRecording) {
   EXPECT_TRUE(stop_recording_button->visible_preferred());
 }
 
+TEST_F(CaptureModeTest, SuspendWhileSessionIsActive) {
+  auto* controller = StartCaptureSession(CaptureModeSource::kFullscreen,
+                                         CaptureModeType::kVideo);
+  EXPECT_TRUE(controller->IsActive());
+  power_manager_client()->SendSuspendImminent(
+      power_manager::SuspendImminent::IDLE);
+  EXPECT_FALSE(controller->IsActive());
+}
+
+TEST_F(CaptureModeTest, SuspendAfterCountdownStarts) {
+  // User NORMAL_DURATION for the countdown animation so we can have predictable
+  // timings.
+  ui::ScopedAnimationDurationScaleMode animation_scale(
+      ui::ScopedAnimationDurationScaleMode::NORMAL_DURATION);
+  auto* controller = StartCaptureSession(CaptureModeSource::kFullscreen,
+                                         CaptureModeType::kVideo);
+  // Hit Enter to begin recording, wait for 1 second, then suspend the device.
+  auto* event_generator = GetEventGenerator();
+  SendKey(ui::VKEY_RETURN, event_generator);
+  WaitForSeconds(1);
+  power_manager_client()->SendSuspendImminent(
+      power_manager::SuspendImminent::IDLE);
+  EXPECT_FALSE(controller->IsActive());
+  EXPECT_FALSE(controller->is_recording_in_progress());
+}
+
+TEST_F(CaptureModeTest, SuspendAfterRecordingStarts) {
+  auto* controller = StartCaptureSession(CaptureModeSource::kFullscreen,
+                                         CaptureModeType::kVideo);
+  controller->StartVideoRecordingImmediatelyForTesting();
+  EXPECT_TRUE(controller->is_recording_in_progress());
+  power_manager_client()->SendSuspendImminent(
+      power_manager::SuspendImminent::IDLE);
+  EXPECT_FALSE(controller->is_recording_in_progress());
+}
+
+TEST_F(CaptureModeTest, SwitchUsersWhileRecording) {
+  auto* controller = StartCaptureSession(CaptureModeSource::kFullscreen,
+                                         CaptureModeType::kVideo);
+  controller->StartVideoRecordingImmediatelyForTesting();
+  EXPECT_TRUE(controller->is_recording_in_progress());
+  SwitchToUser2();
+  EXPECT_FALSE(controller->is_recording_in_progress());
+}
+
+TEST_F(CaptureModeTest, SwitchUsersAfterCountdownStarts) {
+  // User NORMAL_DURATION for the countdown animation so we can have predictable
+  // timings.
+  ui::ScopedAnimationDurationScaleMode animation_scale(
+      ui::ScopedAnimationDurationScaleMode::NORMAL_DURATION);
+  auto* controller = StartCaptureSession(CaptureModeSource::kFullscreen,
+                                         CaptureModeType::kVideo);
+  // Hit Enter to begin recording, wait for 1 second, then switch users.
+  auto* event_generator = GetEventGenerator();
+  SendKey(ui::VKEY_RETURN, event_generator);
+  WaitForSeconds(1);
+  SwitchToUser2();
+  EXPECT_FALSE(controller->IsActive());
+  EXPECT_FALSE(controller->is_recording_in_progress());
+}
+
 TEST_F(CaptureModeTest, ClosingDisplayBeingFullscreenRecorded) {
   UpdateDisplay("400x400,401+0-400x400");
   auto roots = Shell::GetAllRootWindows();
@@ -1516,18 +1605,14 @@ TEST_F(CaptureModeTest, CaptureSessionSwitchedModeMetric) {
 
 // Test that cancel recording during countdown won't cause crash.
 TEST_F(CaptureModeTest, CancelCaptureDuringCountDown) {
-  ui::ScopedAnimationDurationScaleMode animatin_scale(
+  ui::ScopedAnimationDurationScaleMode animation_scale(
       ui::ScopedAnimationDurationScaleMode::NORMAL_DURATION);
   StartCaptureSession(CaptureModeSource::kFullscreen, CaptureModeType::kVideo);
   // Hit Enter to begin recording, Wait for 1 second, then press ESC while count
   // down is in progress.
   auto* event_generator = GetEventGenerator();
   SendKey(ui::VKEY_RETURN, event_generator);
-  base::RunLoop loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE, base::BindLambdaForTesting([&]() { loop.Quit(); }),
-      base::TimeDelta::FromSeconds(1));
-  loop.Run();
+  WaitForSeconds(1);
   SendKey(ui::VKEY_ESCAPE, event_generator);
 }
 
@@ -1615,7 +1700,7 @@ TEST_F(CaptureModeTest, NumberOfCaptureRegionAdjustmentsHistogram) {
 }
 
 TEST_F(CaptureModeTest, FullscreenCapture) {
-  ui::ScopedAnimationDurationScaleMode animatin_scale(
+  ui::ScopedAnimationDurationScaleMode animation_scale(
       ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
   CaptureModeController* controller = StartCaptureSession(
       CaptureModeSource::kFullscreen, CaptureModeType::kImage);

ash/capture_mode/video_recording_watcher.cc

@@ -6,8 +6,6 @@
 
 #include "ash/capture_mode/capture_mode_controller.h"
 #include "ash/capture_mode/capture_mode_util.h"
-#include "ash/session/session_controller_impl.h"
-#include "ash/shell.h"
 #include "base/check.h"
 #include "base/check_op.h"
 #include "base/notreached.h"
@@ -24,14 +22,12 @@ VideoRecordingWatcher::VideoRecordingWatcher(
   DCHECK(controller_->is_recording_in_progress());
 
   window_being_recorded_->AddObserver(this);
-  Shell::Get()->session_controller()->AddObserver(this);
 }
 
 VideoRecordingWatcher::~VideoRecordingWatcher() {
   DCHECK(window_being_recorded_);
 
   window_being_recorded_->RemoveObserver(this);
-  Shell::Get()->session_controller()->RemoveObserver(this);
 }
 
 void VideoRecordingWatcher::OnWindowDestroying(aura::Window* window) {
@@ -68,18 +64,4 @@ void VideoRecordingWatcher::OnWindowRemovingFromRootWindow(
   capture_mode_util::SetStopRecordingButtonVisibility(new_root, true);
 }
 
-void VideoRecordingWatcher::OnSessionStateChanged(
-    session_manager::SessionState state) {
-  DCHECK(controller_->is_recording_in_progress());
-
-  if (Shell::Get()->session_controller()->IsUserSessionBlocked())
-    controller_->EndVideoRecording();
-}
-
-void VideoRecordingWatcher::OnChromeTerminating() {
-  DCHECK(controller_->is_recording_in_progress());
-
-  controller_->EndVideoRecording();
-}
-
 }  // namespace ash

ash/capture_mode/video_recording_watcher.h

@@ -5,7 +5,6 @@
 #ifndef ASH_CAPTURE_MODE_VIDEO_RECORDING_WATCHER_H_
 #define ASH_CAPTURE_MODE_VIDEO_RECORDING_WATCHER_H_
 
-#include "ash/public/cpp/session/session_observer.h"
 #include "ui/aura/window_observer.h"
 
 namespace ash {
@@ -14,14 +13,11 @@ class CaptureModeController;
 
 // An instance of this class is created while video recording is in progress to
 // watch for events that end video recording, such as a window being recorded
-// gets closed, a display being fullscreen-recorded gets disconnected, the user
-// session gets blocked, or while shutting down.
-// TODO(https://crbug.com/1142994): Decide what to do when switching users in
-// multi-profile.
+// gets closed or moved between displays, or a display being fullscreen-recorded
+// gets disconnected.
 // TODO(https://crbug.com/1145003): Use this to paint a border around the area
 // being recorded while recording is in progress.
-class VideoRecordingWatcher : public aura::WindowObserver,
-                              public SessionObserver {
+class VideoRecordingWatcher : public aura::WindowObserver {
  public:
   VideoRecordingWatcher(CaptureModeController* controller,
                         aura::Window* window_being_recorded);
@@ -35,10 +31,6 @@ class VideoRecordingWatcher : public aura::WindowObserver,
   void OnWindowRemovingFromRootWindow(aura::Window* window,
                                       aura::Window* new_root) override;
 
-  // SessionObserver:
-  void OnSessionStateChanged(session_manager::SessionState state) override;
-  void OnChromeTerminating() override;
-
  private:
   CaptureModeController* const controller_;
   aura::Window* const window_being_recorded_;