Skip to content

Commit

Permalink
[Merge to M-103] capture_selfie_cam: Address UXImpl feedback
Browse files Browse the repository at this point in the history
- When the folder selection dialog is shown, we decided
  to hide the camera preview along with all other capture
  UIs. Once the dialog is dismissed, the camera preview
  should show again.
  -->> A bug was found while working on this, a dismissed
       toast widget can be made visible again by mistake.
       Fixed in this CL.
- Clicking outside the bounds of the settings menu should
  dismiss it directly, rather than performing a capture, or
  selecting a region.

Fixed: 1325121
Test: Manual, added unit tests.

(cherry picked from commit c285c10)

Change-Id: I09f7d74d3290b273776e0af45be9e4bdc2793188
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3646826
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1004305}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3653442
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Ahmed Fakhry <afakhry@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5060@{#82}
Cr-Branched-From: b83393d-refs/heads/main@{#1002911}
  • Loading branch information
Ahmed Fakhry authored and Chromium LUCI CQ committed May 18, 2022
1 parent bf36747 commit 3798066
Show file tree
Hide file tree
Showing 7 changed files with 226 additions and 120 deletions.
4 changes: 2 additions & 2 deletions ash/capture_mode/capture_mode_camera_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -677,8 +677,8 @@ void CaptureModeCameraController::OnFrameHandlerFatalError() {
}

void CaptureModeCameraController::OnShuttingDown() {
// At this point `CaptureModeController` should have already ended any ongoing
// recording, and there should be no camera previews available.
// This should destroy any camera preview if present.
SetShouldShowPreview(false);
DCHECK(!should_show_preview_);
DCHECK(!camera_preview_widget_);

Expand Down
42 changes: 42 additions & 0 deletions ash/capture_mode/capture_mode_camera_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "ash/capture_mode/capture_mode_types.h"
#include "ash/capture_mode/capture_mode_util.h"
#include "ash/capture_mode/fake_camera_device.h"
#include "ash/capture_mode/fake_folder_selection_dialog_factory.h"
#include "ash/capture_mode/fake_video_source_provider.h"
#include "ash/capture_mode/test_capture_mode_delegate.h"
#include "ash/constants/ash_features.h"
Expand Down Expand Up @@ -184,11 +185,13 @@ class CaptureModeCameraTest : public AshTestBase {
// AshTestBase:
void SetUp() override {
AshTestBase::SetUp();
FakeFolderSelectionDialogFactory::Start();
window_ = CreateTestWindow(gfx::Rect(30, 40, 600, 500));
}

void TearDown() override {
window_.reset();
FakeFolderSelectionDialogFactory::Stop();
AshTestBase::TearDown();
}

Expand Down Expand Up @@ -2990,6 +2993,45 @@ class CaptureModeCameraPreviewTest
}
};

TEST_P(CaptureModeCameraPreviewTest, PreviewVisibilityWhileFolderSelection) {
AddDefaultCamera();
StartCaptureSessionWithParam();
CaptureModeTestApi().SelectCameraAtIndex(0);

// The camera preview should be initially visible.
auto* controller = CaptureModeController::Get();
ASSERT_TRUE(controller->IsActive());
auto* preview_widget = GetCameraController()->camera_preview_widget();
ASSERT_TRUE(preview_widget);
EXPECT_TRUE(preview_widget->IsVisible());

// Click on the settings button, the settings menu should open, and the camera
// preview should remain visible.
CaptureModeSessionTestApi session_test_api(
controller->capture_mode_session());
auto* settings_button =
session_test_api.GetCaptureModeBarView()->settings_button();
auto* event_generator = GetEventGenerator();
ClickOnView(settings_button, event_generator);
ASSERT_TRUE(session_test_api.GetCaptureModeSettingsWidget());
EXPECT_TRUE(preview_widget->IsVisible());

// Click on the "Select folder ..." option, the folder selection dialog should
// open, all capture UIs should hide, including the camera preview.
CaptureModeSettingsTestApi settings_test_api;
ClickOnView(settings_test_api.GetSelectFolderMenuItem(), event_generator);
EXPECT_TRUE(session_test_api.IsFolderSelectionDialogShown());
EXPECT_FALSE(session_test_api.IsAllUisVisible());
EXPECT_FALSE(preview_widget->IsVisible());

// Dismiss the folder selection dialog, all capture UIs should show again,
// including the camera preview.
FakeFolderSelectionDialogFactory::Get()->CancelDialog();
EXPECT_FALSE(session_test_api.IsFolderSelectionDialogShown());
EXPECT_TRUE(session_test_api.IsAllUisVisible());
EXPECT_TRUE(preview_widget->IsVisible());
}

// Tests that camera preview's bounds is updated after display rotations with
// two use cases, when capture session is active and when there's a video
// recording in progress.
Expand Down
8 changes: 4 additions & 4 deletions ash/capture_mode/capture_mode_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -800,12 +800,12 @@ void CaptureModeController::OnSessionStateChanged(
}

void CaptureModeController::OnChromeTerminating() {
// Order here matters. We end the recording first before we inform the camera
// controller, so that ending the recording will destroy any camera previews
// first.
EndSessionOrRecording(EndRecordingReason::kShuttingDown);
// Order here matters. We may shutdown while a session with a camera is active
// before recording starts, we need to inform the camera controller first to
// destroy the camera preview first.
if (camera_controller_)
camera_controller_->OnShuttingDown();
EndSessionOrRecording(EndRecordingReason::kShuttingDown);
}

void CaptureModeController::SuspendImminent(
Expand Down

0 comments on commit 3798066

Please sign in to comment.