Reland "[CameraRoll]Send camera roll setting to the Android device in CrosState"

This is a reland of 3895114

Camera roll feature was not part of default feature map in MultiDeviceSetupClient,thus the newly added camera roll feature state entry in unit test was not destroyed correctly, causing memory leak.
Added new camera roll feature into the default feature map in MultiDeviceSetupClient to address memory leak.

Original change's description:
> [CameraRoll]Send camera roll setting to the Android device in CrosState
>
> Include camera roll setting state as part of CrosState,
> so connected mobile device would be able to get update
> when setting value is toggled.
>
> Change-Id: I04d0ed3872d5adeff5e8f8dc76c6eb6df3a50b9c
> Bug: https://crbug.com/1221297
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3173740
> Commit-Queue: Jianbing Wu <[email protected]>
> Auto-Submit: Jianbing Wu <[email protected]>
> Reviewed-by: Jon Mann <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#924995}

Bug: https://crbug.com/1221297
Change-Id: If1db5963f2a6689583f90c780c0a214421870180
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3187416
Commit-Queue: Jianbing Wu <[email protected]>
Auto-Submit: Jianbing Wu <[email protected]>
Reviewed-by: Jon Mann <[email protected]>
Cr-Commit-Position: refs/heads/main@{#925646}

Author: jianbingfruit

chromeos/components/phonehub/cros_state_sender.cc

@@ -89,10 +89,16 @@ void CrosStateSender::PerformUpdateCrosState() {
   bool are_notifications_enabled =
       multidevice_setup_client_->GetFeatureState(
           Feature::kPhoneHubNotifications) == FeatureState::kEnabledByUser;
+  bool is_camera_roll_enabled =
+      multidevice_setup_client_->GetFeatureState(
+          Feature::kPhoneHubCameraRoll) == FeatureState::kEnabledByUser;
 
   PA_LOG(INFO) << "Attempting to send cros state with notifications enabled "
-               << "state as: " << are_notifications_enabled;
-  message_sender_->SendCrosState(are_notifications_enabled);
+               << "state as: " << are_notifications_enabled
+               << " and camera roll enabled state as: "
+               << is_camera_roll_enabled;
+  message_sender_->SendCrosState(are_notifications_enabled,
+                                 is_camera_roll_enabled);
 
   retry_timer_->Start(FROM_HERE, retry_delay_,
                       base::BindOnce(&CrosStateSender::OnRetryTimerFired,

chromeos/components/phonehub/cros_state_sender_unittest.cc

@@ -106,6 +106,9 @@ TEST_F(CrosStateSenderTest, UpdatesOnConnected) {
   // Set notification feature to be enabled.
   fake_multidevice_setup_client_->SetFeatureState(
       Feature::kPhoneHubNotifications, FeatureState::kEnabledByUser);
+  // Set camera roll feature to be enabled.
+  fake_multidevice_setup_client_->SetFeatureState(Feature::kPhoneHubCameraRoll,
+                                                  FeatureState::kEnabledByUser);
   // Expect no new messages since connection has not been established.
   EXPECT_EQ(0u, fake_message_sender_->GetCrosStateCallCount());
   EXPECT_FALSE(mock_timer_->IsRunning());
@@ -120,7 +123,8 @@ TEST_F(CrosStateSenderTest, UpdatesOnConnected) {
   // Simulate connected state. Expect a new message to be sent.
   fake_connection_manager_->SetStatus(
       secure_channel::ConnectionManager::Status::kConnected);
-  EXPECT_TRUE(fake_message_sender_->GetRecentCrosState());
+  EXPECT_TRUE(fake_message_sender_->GetRecentCrosState().first);
+  EXPECT_TRUE(fake_message_sender_->GetRecentCrosState().second);
   EXPECT_EQ(1u, fake_message_sender_->GetCrosStateCallCount());
 
   // Phone model is populated.
@@ -131,7 +135,8 @@ TEST_F(CrosStateSenderTest, UpdatesOnConnected) {
   // Simulate disconnected state, this should not trigger a new request.
   fake_connection_manager_->SetStatus(
       secure_channel::ConnectionManager::Status::kDisconnected);
-  EXPECT_TRUE(fake_message_sender_->GetRecentCrosState());
+  EXPECT_TRUE(fake_message_sender_->GetRecentCrosState().first);
+  EXPECT_TRUE(fake_message_sender_->GetRecentCrosState().second);
   EXPECT_EQ(1u, fake_message_sender_->GetCrosStateCallCount());
   EXPECT_FALSE(mock_timer_->IsRunning());
 }
@@ -146,36 +151,44 @@ TEST_F(CrosStateSenderTest, NotificationFeatureStateChanged) {
   EXPECT_TRUE(mock_timer_->IsRunning());
 
   // Expect new messages to be sent when connection state is connected.
-  EXPECT_FALSE(fake_message_sender_->GetRecentCrosState());
+  EXPECT_FALSE(fake_message_sender_->GetRecentCrosState().first);
+  EXPECT_FALSE(fake_message_sender_->GetRecentCrosState().second);
   EXPECT_EQ(1u, fake_message_sender_->GetCrosStateCallCount());
   mock_timer_->Fire();
 
   // Simulate enabling notification feature state and expect cros state to be
   // enabled.
   fake_multidevice_setup_client_->SetFeatureState(
       Feature::kPhoneHubNotifications, FeatureState::kEnabledByUser);
-  EXPECT_TRUE(fake_message_sender_->GetRecentCrosState());
+  EXPECT_TRUE(fake_message_sender_->GetRecentCrosState().first);
   EXPECT_EQ(2u, fake_message_sender_->GetCrosStateCallCount());
   mock_timer_->Fire();
 
   // Update a different feature state and expect that it did not affect the
   // cros state.
   fake_multidevice_setup_client_->SetFeatureState(
       Feature::kSmartLock, FeatureState::kDisabledByUser);
-  EXPECT_TRUE(fake_message_sender_->GetRecentCrosState());
+  EXPECT_TRUE(fake_message_sender_->GetRecentCrosState().first);
   EXPECT_EQ(3u, fake_message_sender_->GetCrosStateCallCount());
   mock_timer_->Fire();
 
   // Simulate disabling notification feature state and expect cros state to be
   // disabled.
   fake_multidevice_setup_client_->SetFeatureState(
       Feature::kPhoneHubNotifications, FeatureState::kDisabledByUser);
-  EXPECT_FALSE(fake_message_sender_->GetRecentCrosState());
+  EXPECT_FALSE(fake_message_sender_->GetRecentCrosState().first);
   EXPECT_EQ(4u, fake_message_sender_->GetCrosStateCallCount());
 
+  // Simulate enabling camera roll feature state and expect cros state to be
+  // updated.
+  fake_multidevice_setup_client_->SetFeatureState(Feature::kPhoneHubCameraRoll,
+                                                  FeatureState::kEnabledByUser);
+  EXPECT_TRUE(fake_message_sender_->GetRecentCrosState().second);
+  EXPECT_EQ(5u, fake_message_sender_->GetCrosStateCallCount());
+
   // Firing the timer does not cause the cros state to be sent again.
   mock_timer_->Fire();
-  EXPECT_EQ(4u, fake_message_sender_->GetCrosStateCallCount());
+  EXPECT_EQ(5u, fake_message_sender_->GetCrosStateCallCount());
 }
 
 }  // namespace phonehub

chromeos/components/phonehub/fake_message_sender.cc

@@ -10,8 +10,10 @@ namespace phonehub {
 FakeMessageSender::FakeMessageSender() = default;
 FakeMessageSender::~FakeMessageSender() = default;
 
-void FakeMessageSender::SendCrosState(bool notification_enabled) {
-  cros_states_.push_back(notification_enabled);
+void FakeMessageSender::SendCrosState(bool notification_enabled,
+                                      bool camera_roll_enabled) {
+  cros_states_.push_back(
+      std::make_pair(notification_enabled, camera_roll_enabled));
 }
 
 void FakeMessageSender::SendUpdateNotificationModeRequest(
@@ -77,7 +79,7 @@ size_t FakeMessageSender::GetFetchCameraRollItemsRequestCallCount() const {
   return fetch_camera_roll_items_requests_.size();
 }
 
-bool FakeMessageSender::GetRecentCrosState() const {
+std::pair<bool, bool> FakeMessageSender::GetRecentCrosState() const {
   return cros_states_.back();
 }
 

chromeos/components/phonehub/fake_message_sender.h

@@ -22,7 +22,8 @@ class FakeMessageSender : public MessageSender {
   ~FakeMessageSender() override;
 
   // MessageSender:
-  void SendCrosState(bool notification_enabled) override;
+  void SendCrosState(bool notification_enabled,
+                     bool camera_roll_enabled) override;
   void SendUpdateNotificationModeRequest(bool do_not_disturb_enabled) override;
   void SendUpdateBatteryModeRequest(bool battery_saver_mode_enabled) override;
   void SendDismissNotificationRequest(int64_t notification_id) override;
@@ -34,7 +35,7 @@ class FakeMessageSender : public MessageSender {
   void SendFetchCameraRollItemsRequest(
       const proto::FetchCameraRollItemsRequest& request) override;
 
-  bool GetRecentCrosState() const;
+  std::pair<bool, bool> GetRecentCrosState() const;
   bool GetRecentUpdateNotificationModeRequest() const;
   bool GetRecentUpdateBatteryModeRequest() const;
   int64_t GetRecentDismissNotificationRequest() const;
@@ -63,7 +64,9 @@ class FakeMessageSender : public MessageSender {
   size_t GetFetchCameraRollItemsRequestCallCount() const;
 
  private:
-  std::vector<bool> cros_states_;
+  std::vector<std::pair</*is_notifications_setting_enabled*/ bool,
+                        /*is_camera_roll_setting_enabled*/ bool>>
+      cros_states_;
   std::vector<bool> update_notification_mode_requests_;
   std::vector<bool> update_battery_mode_requests_;
   std::vector<int64_t> dismiss_notification_requests_;

chromeos/components/phonehub/message_sender.h

@@ -22,7 +22,8 @@ class MessageSender {
   virtual ~MessageSender() = default;
 
   // Sends whether the notification setting is enabled in the Chrome OS device.
-  virtual void SendCrosState(bool notification_setting_enabled) = 0;
+  virtual void SendCrosState(bool notification_setting_enabled,
+                             bool camera_roll_setting_enabled) = 0;
 
   // Requests that the phone enables or disables Do Not Disturb mode.
   virtual void SendUpdateNotificationModeRequest(

chromeos/components/phonehub/message_sender_impl.cc

@@ -38,13 +38,18 @@ MessageSenderImpl::MessageSenderImpl(
 
 MessageSenderImpl::~MessageSenderImpl() = default;
 
-void MessageSenderImpl::SendCrosState(bool notification_setting_enabled) {
+void MessageSenderImpl::SendCrosState(bool notification_setting_enabled,
+                                      bool camera_roll_setting_enabled) {
   proto::NotificationSetting is_notification_enabled =
       notification_setting_enabled
           ? proto::NotificationSetting::NOTIFICATIONS_ON
           : proto::NotificationSetting::NOTIFICATIONS_OFF;
+  proto::CameraRollSetting is_camera_roll_enabled =
+      camera_roll_setting_enabled ? proto::CameraRollSetting::CAMERA_ROLL_ON
+                                  : proto::CameraRollSetting::CAMERA_ROLL_OFF;
   proto::CrosState request;
   request.set_notification_setting(is_notification_enabled);
+  request.set_camera_roll_setting(is_camera_roll_enabled);
 
   SendMessage(proto::MessageType::PROVIDE_CROS_STATE, &request);
 }

chromeos/components/phonehub/message_sender_impl.h

@@ -27,7 +27,8 @@ class MessageSenderImpl : public MessageSender {
   ~MessageSenderImpl() override;
 
   // MessageSender:
-  void SendCrosState(bool notification_setting_enabled) override;
+  void SendCrosState(bool notification_setting_enabled,
+                     bool camera_roll_setting_enabled) override;
   void SendUpdateNotificationModeRequest(bool do_not_disturb_enabled) override;
   void SendUpdateBatteryModeRequest(bool battery_saver_mode_enabled) override;
   void SendDismissNotificationRequest(int64_t notification_id) override;

chromeos/components/phonehub/message_sender_unittest.cc

@@ -64,8 +64,10 @@ TEST_F(MessageSenderImplTest, SendCrossState) {
   proto::CrosState request;
   request.set_notification_setting(
       proto::NotificationSetting::NOTIFICATIONS_ON);
+  request.set_camera_roll_setting(proto::CameraRollSetting::CAMERA_ROLL_OFF);
 
-  message_sender_->SendCrosState(/*notification_enabled=*/true);
+  message_sender_->SendCrosState(/*notification_enabled=*/true,
+                                 /*camera_roll_enabled=*/false);
   VerifyMessage(proto::MessageType::PROVIDE_CROS_STATE, &request,
                 fake_connection_manager_->sent_messages().back());
 }

chromeos/components/phonehub/proto/phonehub_api.proto

@@ -40,6 +40,11 @@ enum NotificationSetting {
   NOTIFICATIONS_ON = 1;
 }
 
+enum CameraRollSetting {
+  CAMERA_ROLL_OFF = 0;
+  CAMERA_ROLL_ON = 1;
+}
+
 enum ChargingState {
   NOT_CHARGING = 0;
   CHARGING_AC = 1;
@@ -162,6 +167,7 @@ message App {
 
 message CrosState {
   NotificationSetting notification_setting = 1;
+  CameraRollSetting camera_roll_setting = 2;
 }
 
 message Action {

chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.cc

@@ -28,6 +28,8 @@ MultiDeviceSetupClient::GenerateDefaultFeatureStatesMap() {
       {mojom::Feature::kPhoneHub, mojom::FeatureState::kProhibitedByPolicy},
       {mojom::Feature::kPhoneHubNotifications,
        mojom::FeatureState::kProhibitedByPolicy},
+      {mojom::Feature::kPhoneHubCameraRoll,
+       mojom::FeatureState::kProhibitedByPolicy},
       {mojom::Feature::kPhoneHubTaskContinuation,
        mojom::FeatureState::kProhibitedByPolicy},
       {mojom::Feature::kWifiSync, mojom::FeatureState::kProhibitedByPolicy},