Skip to content

GitLab

T
tangled
Commit 6698950e authored by Jiewei Qian's avatar Jiewei Qian Committed by Commit Bot
Browse files
Options

Revert "capture_mode: Events ending video recording 2" 

This reverts commit 94737673.

Reason for revert: Speculative revert for crash caused by https://chromium-review.googlesource.com/c/chromium/src/+/2536030 in Linux ChromiumOS MSan Tests.

This CL depends on the above CL, which is being reverted.

Original change's description:
> 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}

TBR=jamescook@chromium.org,afakhry@chromium.org

Change-Id: Ic3b0df16d045708402db967727c59a4bb748a3b3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1142994
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2546504Reviewed-by: default avatarJiewei Qian  <qjw@chromium.org>
Commit-Queue: Jiewei Qian  <qjw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828624}
parent 000df18f
Hide whitespace changes
Inline Side-by-side
Showing with 43 additions and 174 deletions
ash/capture_mode/capture_mode_controller.cc
View file @ 6698950e
......@@ -18,7 +18,6 @@
#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"
......@@ -275,14 +274,9 @@ 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;
}
......@@ -388,60 +382,12 @@ 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
View file @ 6698950e
......@@ -13,7 +13,6 @@
#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"
......@@ -22,7 +21,6 @@
#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"
......@@ -44,9 +42,7 @@ class VideoRecordingWatcher;
// Controls starting and ending a Capture Mode session and its behavior.
class ASH_EXPORT CaptureModeController
: public recording::mojom::RecordingServiceClient,
public SessionObserver,
public chromeos::PowerManagerClient::Observer {
: public recording::mojom::RecordingServiceClient {
public:
explicit CaptureModeController(std::unique_ptr<CaptureModeDelegate> delegate);
CaptureModeController(const CaptureModeController&) = delete;
......@@ -97,14 +93,6 @@ 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();
......@@ -112,12 +100,6 @@ 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
View file @ 6698950e
......@@ -30,9 +30,6 @@
#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"
......@@ -274,21 +271,6 @@ 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_;
};
......@@ -403,7 +385,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 animation_scale(
ui::ScopedAnimationDurationScaleMode animatin_scale(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
auto* controller = CaptureModeController::Get();
......@@ -1097,17 +1079,12 @@ TEST_F(CaptureModeTest, FullscreenCursorStates) {
EXPECT_TRUE(cursor_manager->IsCursorVisible());
// Enter tablet mode, the cursor should be hidden.
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();
TabletModeControllerTestApi().EnterTabletMode();
EXPECT_FALSE(cursor_manager->IsCursorVisible());
EXPECT_TRUE(cursor_manager->IsCursorLocked());
// Exit tablet mode, the cursor should appear again.
tablet_mode_controller_test_api.LeaveTabletMode();
TabletModeControllerTestApi().LeaveTabletMode();
EXPECT_TRUE(cursor_manager->IsCursorVisible());
EXPECT_EQ(CursorType::kCustom, cursor_manager->GetCursor().type());
EXPECT_TRUE(test_api.IsUsingCustomCursor(CaptureModeType::kVideo));
......@@ -1178,17 +1155,12 @@ TEST_F(CaptureModeTest, WindowCursorStates) {
EXPECT_TRUE(test_api.IsUsingCustomCursor(CaptureModeType::kVideo));
// Enter tablet mode, the cursor should be hidden.
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();
TabletModeControllerTestApi().EnterTabletMode();
EXPECT_FALSE(cursor_manager->IsCursorVisible());
EXPECT_TRUE(cursor_manager->IsCursorLocked());
// Exit tablet mode, the cursor should appear again.
tablet_mode_controller_test_api.LeaveTabletMode();
TabletModeControllerTestApi().LeaveTabletMode();
EXPECT_TRUE(cursor_manager->IsCursorVisible());
EXPECT_EQ(CursorType::kCustom, cursor_manager->GetCursor().type());
EXPECT_TRUE(test_api.IsUsingCustomCursor(CaptureModeType::kVideo));
......@@ -1251,7 +1223,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 animation_scale(
ui::ScopedAnimationDurationScaleMode animatin_scale(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
// Create 2 windows that overlap with each other.
......@@ -1408,67 +1380,6 @@ 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();
......@@ -1605,14 +1516,18 @@ TEST_F(CaptureModeTest, CaptureSessionSwitchedModeMetric) {
// Test that cancel recording during countdown won't cause crash.
TEST_F(CaptureModeTest, CancelCaptureDuringCountDown) {
ui::ScopedAnimationDurationScaleMode animation_scale(
ui::ScopedAnimationDurationScaleMode animatin_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);
WaitForSeconds(1);
base::RunLoop loop;
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
FROM_HERE, base::BindLambdaForTesting([&]() { loop.Quit(); }),
base::TimeDelta::FromSeconds(1));
loop.Run();
SendKey(ui::VKEY_ESCAPE, event_generator);
}
......@@ -1700,7 +1615,7 @@ TEST_F(CaptureModeTest, NumberOfCaptureRegionAdjustmentsHistogram) {
}
TEST_F(CaptureModeTest, FullscreenCapture) {
ui::ScopedAnimationDurationScaleMode animation_scale(
ui::ScopedAnimationDurationScaleMode animatin_scale(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
CaptureModeController* controller = StartCaptureSession(
CaptureModeSource::kFullscreen, CaptureModeType::kImage);
......
ash/capture_mode/video_recording_watcher.cc
View file @ 6698950e
......@@ -6,6 +6,8 @@
#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"
......@@ -22,12 +24,14 @@ 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) {
......@@ -64,4 +68,18 @@ 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
View file @ 6698950e
......@@ -5,6 +5,7 @@
#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 {
......@@ -13,11 +14,14 @@ 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 or moved between displays, or a display being fullscreen-recorded
// gets disconnected.
// 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.
// 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 {
class VideoRecordingWatcher : public aura::WindowObserver,
public SessionObserver {
public:
VideoRecordingWatcher(CaptureModeController* controller,
aura::Window* window_being_recorded);
......@@ -31,6 +35,10 @@ 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_;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment