Skip to content

Commit

Permalink
[Privacy Hub] Clear camera HW switch notifications properly
Browse files Browse the repository at this point in the history 
This CL makes sure camera hardware switch notification for a camera is
cleared when no application is accessing that camera or when that camera
is detached from the ChromeOS device.

Bug: b:258666404, b:258666148
Change-Id: Ica5614c54a5ab05ba20932740e045d6dd3e77bcc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4188533
Reviewed-by: Christoph Schlosser <cschlosser@chromium.org>
Commit-Queue: Md Shahadat Hossain Shahin <shahinmd@google.com>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1098502}
  • Loading branch information
Md Shahadat Hossain Shahin authored and Chromium LUCI CQ committed Jan 30, 2023
1 parent 1c01291 commit ffd86a6
Show file tree
Hide file tree
Showing 3 changed files with 131 additions and 36 deletions.
61 changes: 35 additions & 26 deletions chrome/browser/ui/ash/media_client_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,11 +263,10 @@ std::string GetDeviceName(
}

// Small helper to make sure that `kCameraPrivacySwitchOnNotificationIdPrefix`
// combined with `device_name` always produce the same identifier.
// combined with `device_id` always produce the same identifier.
std::string PrivacySwitchOnNotificationIdForDevice(
const std::string& device_name) {
return base::StrCat(
{kCameraPrivacySwitchOnNotificationIdPrefix, device_name});
const std::string& device_id) {
return base::StrCat({kCameraPrivacySwitchOnNotificationIdPrefix, device_id});
}

} // namespace
Expand Down Expand Up @@ -477,10 +476,10 @@ void MediaClientImpl::OnCameraSWPrivacySwitchStateChanged(
weak_ptr_factory_.GetWeakPtr()));
} else if (state == cros::mojom::CameraPrivacySwitchState::ON) {
// The software switch is ON. Clear all hardware switch notifications.
for (const auto& notification_id : existing_notifications_) {
SystemNotificationHelper::GetInstance()->Close(notification_id);
for (auto it = devices_having_visible_notification_.begin();
it != devices_having_visible_notification_.end();) {
it = RemoveCameraOffNotificationForDevice(*it);
}
existing_notifications_.clear();
}
}

Expand Down Expand Up @@ -541,8 +540,7 @@ void MediaClientImpl::OnCapabilityAccessUpdate(
if (active_camera_client_count_ > 0) {
ShowCameraOffNotification(device_id, device_name, /*resurface=*/false);
} else {
RemoveCameraOffNotificationByID(
PrivacySwitchOnNotificationIdForDevice(device_name));
RemoveCameraOffNotificationForDevice(device_id);
}
}

Expand Down Expand Up @@ -699,28 +697,31 @@ void MediaClientImpl::ShowCameraOffNotification(const std::string& device_id,
app_name, std::make_pair(device_id, device_name));
}

const std::string notification_id =
PrivacySwitchOnNotificationIdForDevice(device_name);

if (resurface) {
RemoveCameraOffNotificationByID(notification_id);
RemoveCameraOffNotificationForDevice(device_id);
}

SystemNotificationHelper::GetInstance()->Display(
notification_.builder()
.SetId(notification_id)
.SetId(PrivacySwitchOnNotificationIdForDevice(device_id))
.SetTitleWithArgs(IDS_CAMERA_PRIVACY_SWITCH_ON_NOTIFICATION_TITLE,
{device_name_u16})
.SetMessageWithArgs(IDS_CAMERA_PRIVACY_SWITCH_ON_NOTIFICATION_MESSAGE,
{device_name_u16})
.Build());
existing_notifications_.insert(notification_id);
devices_having_visible_notification_.insert(device_id);
}

void MediaClientImpl::RemoveCameraOffNotificationByID(
const std::string& notification_id) {
SystemNotificationHelper::GetInstance()->Close(notification_id);
existing_notifications_.erase(notification_id);
base::flat_set<std::string>::iterator
MediaClientImpl::RemoveCameraOffNotificationForDevice(
const std::string& device_id) {
auto it = devices_having_visible_notification_.find(device_id);
if (it != devices_having_visible_notification_.end()) {
SystemNotificationHelper::GetInstance()->Close(
PrivacySwitchOnNotificationIdForDevice(device_id));
return devices_having_visible_notification_.erase(it);
}
return it;
}

void MediaClientImpl::OnGetSourceInfosByCameraHWPrivacySwitchStateChanged(
Expand Down Expand Up @@ -810,8 +811,7 @@ void MediaClientImpl::OnGetSourceInfosByCameraHWPrivacySwitchStateChanged(
}

if (state == cros::mojom::CameraPrivacySwitchState::OFF) {
RemoveCameraOffNotificationByID(
PrivacySwitchOnNotificationIdForDevice(device_name));
RemoveCameraOffNotificationForDevice(device_id);
}
}

Expand All @@ -827,11 +827,20 @@ void MediaClientImpl::OnGetSourceInfosByActiveClientChanged(
// As the device is being actively used by the client, display a
// notification.
ShowCameraOffNotification(device_id, device_name);
} else if (active_camera_client_count_ == 0) {
// Clear the notification for this device as no client is trying to use
// this camera anymore.
RemoveCameraOffNotificationByID(
PrivacySwitchOnNotificationIdForDevice(device_name));
} else if (!IsDeviceActive(device_id)) {
// No application is actively using this camera. Remove the notification
// for the device if exists.
RemoveCameraOffNotificationForDevice(device_id);
}
}

// Remove notifications for detached devices if any.
for (auto it = devices_having_visible_notification_.begin();
it != devices_having_visible_notification_.end();) {
if (IsDeviceActive(*it)) {
++it;
} else {
it = RemoveCameraOffNotificationForDevice(*it);
}
}
}
Expand Down
13 changes: 7 additions & 6 deletions chrome/browser/ui/ash/media_client_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,10 @@ class MediaClientImpl : public ash::MediaClient,
const std::string& device_name,
bool resurface = true);

// Removes the camera notification with the give id `notification_id` and
// updates the internal data structures of the class accordingly.
void RemoveCameraOffNotificationByID(const std::string& notification_id);
// Removes the camera notification for device with id `device_id` and returns
// iterator to the next device id in `devices_having_visible_notification_`.
base::flat_set<std::string>::iterator RemoveCameraOffNotificationForDevice(
const std::string& device_id);

void OnGetSourceInfosByCameraHWPrivacySwitchStateChanged(
const std::string& device_id,
Expand Down Expand Up @@ -204,9 +205,9 @@ class MediaClientImpl : public ash::MediaClient,
base::flat_map<cros::mojom::CameraClientType, base::flat_set<std::string>>
devices_used_by_client_;

// IDs of the existing camera hardware switch notifications in the message
// center.
base::flat_set<std::string> existing_notifications_;
// Set of IDs of the camera devices having a visible notification in the
// message center.
base::flat_set<std::string> devices_having_visible_notification_;

// Stores the state of the camera software privacy switch state locally.
cros::mojom::CameraPrivacySwitchState camera_sw_privacy_switch_state_ =
Expand Down
93 changes: 89 additions & 4 deletions chrome/browser/ui/ash/media_client_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
#include "chrome/browser/ui/ash/media_client_impl.h"

#include <memory>
#include <string>
#include <vector>

#include "ash/public/cpp/media_controller.h"
#include "ash/public/cpp/test/test_new_window_delegate.h"
Expand All @@ -24,6 +26,7 @@
#include "components/services/app_service/public/cpp/capability_access.h"
#include "components/services/app_service/public/cpp/capability_access_update.h"
#include "components/user_manager/fake_user_manager.h"
#include "media/capture/video/video_capture_device_info.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "ui/base/accelerators/media_keys_listener.h"
Expand Down Expand Up @@ -107,11 +110,14 @@ class FakeNotificationDisplayService : public NotificationDisplayService {
void RemoveObserver(NotificationDisplayService::Observer* observer) override {
}

bool HasNotificationMessageContaining(const std::string& app_name) const {
const std::u16string app_name_u16 = base::UTF8ToUTF16(app_name);
// Returns true if any existing notification contains `keywords` as a
// substring.
bool HasNotificationMessageContaining(const std::string& keywords) const {
const std::u16string keywords_u16 = base::UTF8ToUTF16(keywords);
for (const auto& [notification_id, notification] : active_notifications_) {
if (notification.message().find(app_name_u16) != std::u16string::npos)
if (notification.message().find(keywords_u16) != std::u16string::npos) {
return true;
}
}
return false;
}
Expand Down Expand Up @@ -318,6 +324,29 @@ class MediaClientAppUsingCameraInBrowserEnvironmentTest
device_id};
}

void OnActiveClientChange(
cros::mojom::CameraClientType type,
const base::flat_set<std::string>& active_device_ids,
int active_client_count) {
media_client_.devices_used_by_client_.insert_or_assign(type,
active_device_ids);
media_client_.active_camera_client_count_ = active_client_count;

media_client_.OnGetSourceInfosByActiveClientChanged(active_device_ids,
video_capture_devices_);
}

void AttachCamera(const std::string& device_id,
const std::string& device_name) {
media::VideoCaptureDeviceInfo device_info;
device_info.descriptor.device_id = device_id;
device_info.descriptor.set_display_name(device_name);
video_capture_devices_.push_back(device_info);
}

// Detaches the most recently attached camera.
void DetachCamera() { video_capture_devices_.pop_back(); }

void ShowCameraOffNotification(const std::string& device_id,
const std::string& device_name) {
media_client_.ShowCameraOffNotification(device_id, device_name);
Expand Down Expand Up @@ -355,6 +384,7 @@ class MediaClientAppUsingCameraInBrowserEnvironmentTest
user_manager::FakeUserManager user_manager_;
base::raw_ptr<MockNewWindowDelegate> new_window_delegate_ = nullptr;
std::unique_ptr<ash::TestNewWindowDelegateProvider> window_delegate_provider_;
std::vector<media::VideoCaptureDeviceInfo> video_capture_devices_;
};

TEST_F(MediaClientTest, HandleMediaAccelerators) {
Expand Down Expand Up @@ -595,7 +625,62 @@ TEST_F(MediaClientAppUsingCameraInBrowserEnvironmentTest,
EXPECT_CALL(*new_window_delegate_, OpenUrl).Times(1);

notification_display_service->SimulateClick(
"ash.media.camera.activity_with_privacy_switch_on.device_name", 0);
"ash.media.camera.activity_with_privacy_switch_on.device_id", 0);

EXPECT_EQ(notification_display_service->NumberOfActiveNotifications(), 0u);
}

TEST_F(MediaClientAppUsingCameraInBrowserEnvironmentTest,
NotificationRemovedWhenCameraDetachedOrInactive) {
FakeNotificationDisplayService* notification_display_service =
SetSystemNotificationService();

// No notification initially.
EXPECT_EQ(0u, notification_display_service->NumberOfActiveNotifications());

const std::string camera1 = "camera1";
const std::string camera1_name = "Fake camera 1";
const std::string camera2 = "camera2";
const std::string camera2_name = "Fake camera 2";

// Attach two cameras to the device. Both of the cameras have HW switch. Turn
// the HW switch ON for both of the devices.
AttachCamera(camera1, camera1_name);
SetCameraHWPrivacySwitchState(camera1,
cros::mojom::CameraPrivacySwitchState::ON);
AttachCamera(camera2, camera2_name);
SetCameraHWPrivacySwitchState(camera2,
cros::mojom::CameraPrivacySwitchState::ON);

// Still no notification.
EXPECT_EQ(notification_display_service->NumberOfActiveNotifications(), 0u);

// `CHROME` client starts accessing camera1. A hardware switch notification
// for camera1 should be displayed.
OnActiveClientChange(cros::mojom::CameraClientType::CHROME, {camera1}, 1);
EXPECT_EQ(1u, notification_display_service->NumberOfActiveNotifications());
EXPECT_TRUE(notification_display_service->HasNotificationMessageContaining(
camera1_name));

// `CHROME` client starts accessing camera2 as well. A hardware switch
// notification for camera2 should be displayed.
OnActiveClientChange(cros::mojom::CameraClientType::CHROME,
{camera1, camera2}, 1);
EXPECT_EQ(2u, notification_display_service->NumberOfActiveNotifications());
EXPECT_TRUE(notification_display_service->HasNotificationMessageContaining(
camera2_name));

// `CHROME` client stops accessing camera1. The respective notification should
// be removed.
OnActiveClientChange(cros::mojom::CameraClientType::CHROME, {camera2}, 1);
EXPECT_EQ(1u, notification_display_service->NumberOfActiveNotifications());
EXPECT_FALSE(notification_display_service->HasNotificationMessageContaining(
camera1_name));

// Detach camera2.
DetachCamera();
// `CHROME` client stops accessing camera2 as the camera is detached. The
// respective notification should be removed.
OnActiveClientChange(cros::mojom::CameraClientType::CHROME, {}, 0);
EXPECT_EQ(0u, notification_display_service->NumberOfActiveNotifications());
}

0 comments on commit ffd86a6

Please sign in to comment.