From b15bd9f7a4ff962d21e2263bbe7f6f1c59037c57 Mon Sep 17 00:00:00 2001 From: Mark Pearson Date: Mon, 27 Sep 2021 01:49:45 +0000 Subject: [PATCH] Revert "[CameraRoll]Send camera roll setting to the Android device in CrosState" This reverts commit 3895114c9dd91cc02e5d289bad5ef3cb6316a1c9. Reason for revert: likely cause of failures Step "chromeos_components_unittests on Ubuntu-18.04" failing on builder "Linux Chromium OS ASan LSan Tests (1)" The first run with the relevant failures: https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20(1)/40730/overview The following tests consistently fail starting after this first run: CrosStateSenderTest.NotificationFeatureStateChanged CrosStateSenderTest.PerformUpdateCrosStateRetrySequence The first fails with this stack trace: --- [ RUN ] CrosStateSenderTest.NotificationFeatureStateChanged ================================================================= ==4136==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60700004b31c at pc 0x5559c16fed11 bp 0x7fffb87b9250 sp 0x7fffb87b9248 READ of size 4 at 0x60700004b31c thread T0 #0 0x5559c16fed10 in chromeos::multidevice_setup::MultiDeviceSetupClient::GetFeatureState(chromeos::multidevice_setup::mojom::Feature) const ./../../chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.cc:51:44 #1 0x5559ce6da0ed in chromeos::phonehub::CrosStateSender::PerformUpdateCrosState() ./../../chromeos/components/phonehub/cros_state_sender.cc:93:34 #2 0x5559ce6d9e1d in chromeos::phonehub::CrosStateSender::AttemptUpdateCrosState() ./../../chromeos/components/phonehub/cros_state_sender.cc:85:3 #3 0x5559c17102b7 in chromeos::secure_channel::ConnectionManager::NotifyStatusChanged() ./../../chromeos/services/secure_channel/public/cpp/client/connection_manager.cc:23:14 #4 0x5559ba7314dc in chromeos::phonehub::CrosStateSenderTest_NotificationFeatureStateChanged_Test::TestBody() ./../../chromeos/components/phonehub/cros_state_sender_unittest.cc:146:29 #5 0x5559baf612a1 in HandleExceptionsInMethodIfSupported ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0 #6 0x5559baf612a1 in testing::Test::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2706:5 #7 0x5559baf62ca4 in testing::TestInfo::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2885:11 #8 0x5559baf647b3 in testing::TestSuite::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:3044:30 #9 0x5559baf87628 in testing::internal::UnitTestImpl::RunAllTests() ./../../third_party/googletest/src/googletest/src/gtest.cc:5903:44 #10 0x5559baf86d49 in HandleExceptionsInMethodIfSupported ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0 #11 0x5559baf86d49 in testing::UnitTest::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:5470:10 #12 0x5559c6e8422f in RUN_ALL_TESTS ./../../third_party/googletest/src/googletest/include/gtest/gtest.h:2492:46 #13 0x5559c6e8422f in base::TestSuite::Run() ./../../base/test/test_suite.cc:465:16 #14 0x5559bd07db0e in base::OnceCallback::Run() && ./../../base/callback.h:99:12 #15 0x5559c6e8a853 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback, unsigned long, int, unsigned long, bool, base::OnceCallback) ./../../base/test/launcher/unit_test_launcher.cc:177:38 #16 0x5559c6e8a4d5 in base::LaunchUnitTests(int, char**, base::OnceCallback, unsigned long) ./../../base/test/launcher/unit_test_launcher.cc:247:10 #17 0x5559ba482d45 in main ./../../chromeos/components/run_all_unittests.cc:22:10 #18 0x7f002b751bf6 in __libc_start_main ??:0:0 0x60700004b31c is located 4 bytes to the right of 72-byte region [0x60700004b2d0,0x60700004b318) allocated by thread T0 here: #0 0x5559ba47faed in operator new(unsigned long) /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cpp:95:3 #1 0x5559c14ad498 in __libcpp_operator_new ./../../buildtools/third_party/libc++/trunk/include/new:235:10 #2 0x5559c14ad498 in __libcpp_allocate ./../../buildtools/third_party/libc++/trunk/include/new:261:10 #3 0x5559c14ad498 in allocate ./../../buildtools/third_party/libc++/trunk/include/__memory/allocator.h:82:38 #4 0x5559c14ad498 in allocate ./../../buildtools/third_party/libc++/trunk/include/__memory/allocator_traits.h:261:20 #5 0x5559c14ad498 in std::__1::vector, std::__1::allocator > >::__vallocate(unsigned long) ./../../buildtools/third_party/libc++/trunk/include/vector:994:37 #6 0x5559c16fdfe3 in vector *> ./../../buildtools/third_party/libc++/trunk/include/vector:1224:9 #7 0x5559c16fdfe3 in flat_tree *> ./../../base/containers/flat_tree.h:571:20 #8 0x5559c16fdfe3 in flat_tree ./../../base/containers/flat_tree.h:595:7 #9 0x5559c16fdfe3 in flat_tree ./../../base/containers/flat_map.h:211:15 #10 0x5559c16fdfe3 in chromeos::multidevice_setup::MultiDeviceSetupClient::GenerateDefaultFeatureStatesMap() ./../../chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.cc:21:10 #11 0x5559d736b57e in chromeos::multidevice_setup::FakeMultiDeviceSetupClient::FakeMultiDeviceSetupClient() ./../../chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.cc:13:27 #12 0x5559ba733ad7 in make_unique ./../../buildtools/third_party/libc++/trunk/include/__memory/unique_ptr.h:725:32 #13 0x5559ba733ad7 in chromeos::phonehub::CrosStateSenderTest::SetUp() ./../../chromeos/components/phonehub/cros_state_sender_unittest.cc:40:9 #14 0x5559baf6118e in HandleExceptionsInMethodIfSupported ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0 #15 0x5559baf6118e in testing::Test::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2701:3 #16 0x5559baf62ca4 in testing::TestInfo::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2885:11 #17 0x5559baf647b3 in testing::TestSuite::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:3044:30 #18 0x5559baf87628 in testing::internal::UnitTestImpl::RunAllTests() ./../../third_party/googletest/src/googletest/src/gtest.cc:5903:44 #19 0x5559baf86d49 in HandleExceptionsInMethodIfSupported ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0 #20 0x5559baf86d49 in testing::UnitTest::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:5470:10 #21 0x5559c6e8422f in RUN_ALL_TESTS ./../../third_party/googletest/src/googletest/include/gtest/gtest.h:2492:46 #22 0x5559c6e8422f in base::TestSuite::Run() ./../../base/test/test_suite.cc:465:16 #23 0x5559bd07db0e in base::OnceCallback::Run() && ./../../base/callback.h:99:12 #24 0x5559c6e8a853 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback, unsigned long, int, unsigned long, bool, base::OnceCallback) ./../../base/test/launcher/unit_test_launcher.cc:177:38 #25 0x5559c6e8a4d5 in base::LaunchUnitTests(int, char**, base::OnceCallback, unsigned long) ./../../base/test/launcher/unit_test_launcher.cc:247:10 #26 0x5559ba482d45 in main ./../../chromeos/components/run_all_unittests.cc:22:10 #27 0x7f002b751bf6 in __libc_start_main ??:0:0 SUMMARY: AddressSanitizer: heap-buffer-overflow (/b/s/w/ir/out/Release/chromeos_components_unittests+0x15f40d10) Shadow bytes around the buggy address: 0x0c0e80001610: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fa fa 0x0c0e80001620: fa fa fd fd fd fd fd fd fd fd fd fd fa fa fa fa 0x0c0e80001630: fd fd fd fd fd fd fd fd fd fd fa fa fa fa fd fd 0x0c0e80001640: fd fd fd fd fd fd fd fd fa fa fa fa fd fd fd fd 0x0c0e80001650: fd fd fd fd fd fd fa fa fa fa 00 00 00 00 00 00 =>0x0c0e80001660: 00 00 00[fa]fa fa fa fa fd fd fd fd fd fd fd fd 0x0c0e80001670: fd fa fa fa fa fa fd fd fd fd fd fd fd fd fd fd 0x0c0e80001680: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 fa fa 0x0c0e80001690: fa fa fd fd fd fd fd fd fd fd fd fd fa fa fa fa 0x0c0e800016a0: fd fd fd fd fd fd fd fd fd fd fa fa fa fa 00 00 0x0c0e800016b0: 00 00 00 00 00 00 00 00 fa fa fa fa 00 00 00 00 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==4136==ABORTING --- The second fails with this one: --- [ RUN ] CrosStateSenderTest.PerformUpdateCrosStateRetrySequence ================================================================= ==3811==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60700005f9ec at pc 0x5579c6175d11 bp 0x7ffd5e6a0e10 sp 0x7ffd5e6a0e08 READ of size 4 at 0x60700005f9ec thread T0 #0 0x5579c6175d10 in chromeos::multidevice_setup::MultiDeviceSetupClient::GetFeatureState(chromeos::multidevice_setup::mojom::Feature) const ./../../chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.cc:51:44 #1 0x5579d31510ed in chromeos::phonehub::CrosStateSender::PerformUpdateCrosState() ./../../chromeos/components/phonehub/cros_state_sender.cc:93:34 #2 0x5579d3150e1d in chromeos::phonehub::CrosStateSender::AttemptUpdateCrosState() ./../../chromeos/components/phonehub/cros_state_sender.cc:85:3 #3 0x5579c61872b7 in chromeos::secure_channel::ConnectionManager::NotifyStatusChanged() ./../../chromeos/services/secure_channel/public/cpp/client/connection_manager.cc:23:14 #4 0x5579bf1a3085 in chromeos::phonehub::CrosStateSenderTest_PerformUpdateCrosStateRetrySequence_Test::TestBody() ./../../chromeos/components/phonehub/cros_state_sender_unittest.cc:63:29 #5 0x5579bf9d82a1 in HandleExceptionsInMethodIfSupported ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0 #6 0x5579bf9d82a1 in testing::Test::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2706:5 #7 0x5579bf9d9ca4 in testing::TestInfo::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2885:11 #8 0x5579bf9db7b3 in testing::TestSuite::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:3044:30 #9 0x5579bf9fe628 in testing::internal::UnitTestImpl::RunAllTests() ./../../third_party/googletest/src/googletest/src/gtest.cc:5903:44 #10 0x5579bf9fdd49 in HandleExceptionsInMethodIfSupported ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0 #11 0x5579bf9fdd49 in testing::UnitTest::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:5470:10 #12 0x5579cb8fb22f in RUN_ALL_TESTS ./../../third_party/googletest/src/googletest/include/gtest/gtest.h:2492:46 #13 0x5579cb8fb22f in base::TestSuite::Run() ./../../base/test/test_suite.cc:465:16 #14 0x5579c1af4b0e in base::OnceCallback::Run() && ./../../base/callback.h:99:12 #15 0x5579cb901853 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback, unsigned long, int, unsigned long, bool, base::OnceCallback) ./../../base/test/launcher/unit_test_launcher.cc:177:38 #16 0x5579cb9014d5 in base::LaunchUnitTests(int, char**, base::OnceCallback, unsigned long) ./../../base/test/launcher/unit_test_launcher.cc:247:10 #17 0x5579beef9d45 in main ./../../chromeos/components/run_all_unittests.cc:22:10 #18 0x7fc58b24bbf6 in __libc_start_main ??:0:0 0x60700005f9ec is located 4 bytes to the right of 72-byte region [0x60700005f9a0,0x60700005f9e8) allocated by thread T0 here: #0 0x5579beef6aed in operator new(unsigned long) /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cpp:95:3 #1 0x5579c5f24498 in __libcpp_operator_new ./../../buildtools/third_party/libc++/trunk/include/new:235:10 #2 0x5579c5f24498 in __libcpp_allocate ./../../buildtools/third_party/libc++/trunk/include/new:261:10 #3 0x5579c5f24498 in allocate ./../../buildtools/third_party/libc++/trunk/include/__memory/allocator.h:82:38 #4 0x5579c5f24498 in allocate ./../../buildtools/third_party/libc++/trunk/include/__memory/allocator_traits.h:261:20 #5 0x5579c5f24498 in std::__1::vector, std::__1::allocator > >::__vallocate(unsigned long) ./../../buildtools/third_party/libc++/trunk/include/vector:994:37 #6 0x5579c6174fe3 in vector *> ./../../buildtools/third_party/libc++/trunk/include/vector:1224:9 #7 0x5579c6174fe3 in flat_tree *> ./../../base/containers/flat_tree.h:571:20 #8 0x5579c6174fe3 in flat_tree ./../../base/containers/flat_tree.h:595:7 #9 0x5579c6174fe3 in flat_tree ./../../base/containers/flat_map.h:211:15 #10 0x5579c6174fe3 in chromeos::multidevice_setup::MultiDeviceSetupClient::GenerateDefaultFeatureStatesMap() ./../../chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.cc:21:10 #11 0x5579dbde257e in chromeos::multidevice_setup::FakeMultiDeviceSetupClient::FakeMultiDeviceSetupClient() ./../../chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.cc:13:27 #12 0x5579bf1aaad7 in make_unique ./../../buildtools/third_party/libc++/trunk/include/__memory/unique_ptr.h:725:32 #13 0x5579bf1aaad7 in chromeos::phonehub::CrosStateSenderTest::SetUp() ./../../chromeos/components/phonehub/cros_state_sender_unittest.cc:40:9 #14 0x5579bf9d818e in HandleExceptionsInMethodIfSupported ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0 #15 0x5579bf9d818e in testing::Test::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2701:3 #16 0x5579bf9d9ca4 in testing::TestInfo::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2885:11 #17 0x5579bf9db7b3 in testing::TestSuite::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:3044:30 #18 0x5579bf9fe628 in testing::internal::UnitTestImpl::RunAllTests() ./../../third_party/googletest/src/googletest/src/gtest.cc:5903:44 #19 0x5579bf9fdd49 in HandleExceptionsInMethodIfSupported ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0 #20 0x5579bf9fdd49 in testing::UnitTest::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:5470:10 #21 0x5579cb8fb22f in RUN_ALL_TESTS ./../../third_party/googletest/src/googletest/include/gtest/gtest.h:2492:46 #22 0x5579cb8fb22f in base::TestSuite::Run() ./../../base/test/test_suite.cc:465:16 #23 0x5579c1af4b0e in base::OnceCallback::Run() && ./../../base/callback.h:99:12 #24 0x5579cb901853 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback, unsigned long, int, unsigned long, bool, base::OnceCallback) ./../../base/test/launcher/unit_test_launcher.cc:177:38 #25 0x5579cb9014d5 in base::LaunchUnitTests(int, char**, base::OnceCallback, unsigned long) ./../../base/test/launcher/unit_test_launcher.cc:247:10 #26 0x5579beef9d45 in main ./../../chromeos/components/run_all_unittests.cc:22:10 #27 0x7fc58b24bbf6 in __libc_start_main ??:0:0 SUMMARY: AddressSanitizer: heap-buffer-overflow (/b/s/w/ir/out/Release/chromeos_components_unittests+0x15f40d10) Shadow bytes around the buggy address: 0x0c0e80003ee0: fd fd fd fd fd fd fd fd fd fd fa fa fa fa fd fd 0x0c0e80003ef0: fd fd fd fd fd fd fd fd fa fa fa fa fd fd fd fd 0x0c0e80003f00: fd fd fd fd fd fd fa fa fa fa fd fd fd fd fd fd 0x0c0e80003f10: fd fd fd fd fa fa fa fa fd fd fd fd fd fd fd fd 0x0c0e80003f20: fd fd fa fa fa fa fd fd fd fd fd fd fd fd fd fd =>0x0c0e80003f30: fa fa fa fa 00 00 00 00 00 00 00 00 00[fa]fa fa 0x0c0e80003f40: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c0e80003f50: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c0e80003f60: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c0e80003f70: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c0e80003f80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==3811==ABORTING --- 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 > Auto-Submit: Jianbing Wu > Reviewed-by: Jon Mann > Cr-Commit-Position: refs/heads/main@{#924995} Bug: https://crbug.com/1221297 Change-Id: Ic87d96786b4244b27b1e284f801df8799911b1fa Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3184482 Bot-Commit: Rubber Stamper Auto-Submit: Mark Pearson Commit-Queue: Jon Mann Reviewed-by: Jon Mann Cr-Commit-Position: refs/heads/main@{#925118} --- .../components/phonehub/cros_state_sender.cc | 10 ++----- .../phonehub/cros_state_sender_unittest.cc | 27 +++++-------------- .../phonehub/fake_message_sender.cc | 8 +++--- .../components/phonehub/fake_message_sender.h | 9 +++---- chromeos/components/phonehub/message_sender.h | 3 +-- .../phonehub/message_sender_impl.cc | 7 +---- .../components/phonehub/message_sender_impl.h | 3 +-- .../phonehub/message_sender_unittest.cc | 4 +-- .../phonehub/proto/phonehub_api.proto | 6 ----- 9 files changed, 19 insertions(+), 58 deletions(-) diff --git a/chromeos/components/phonehub/cros_state_sender.cc b/chromeos/components/phonehub/cros_state_sender.cc index 01786746f36d80..361a07045d965a 100644 --- a/chromeos/components/phonehub/cros_state_sender.cc +++ b/chromeos/components/phonehub/cros_state_sender.cc @@ -89,16 +89,10 @@ 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 - << " and camera roll enabled state as: " - << is_camera_roll_enabled; - message_sender_->SendCrosState(are_notifications_enabled, - is_camera_roll_enabled); + << "state as: " << are_notifications_enabled; + message_sender_->SendCrosState(are_notifications_enabled); retry_timer_->Start(FROM_HERE, retry_delay_, base::BindOnce(&CrosStateSender::OnRetryTimerFired, diff --git a/chromeos/components/phonehub/cros_state_sender_unittest.cc b/chromeos/components/phonehub/cros_state_sender_unittest.cc index 60dbdd771d799e..58a32f587544dc 100644 --- a/chromeos/components/phonehub/cros_state_sender_unittest.cc +++ b/chromeos/components/phonehub/cros_state_sender_unittest.cc @@ -106,9 +106,6 @@ 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()); @@ -123,8 +120,7 @@ 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().first); - EXPECT_TRUE(fake_message_sender_->GetRecentCrosState().second); + EXPECT_TRUE(fake_message_sender_->GetRecentCrosState()); EXPECT_EQ(1u, fake_message_sender_->GetCrosStateCallCount()); // Phone model is populated. @@ -135,8 +131,7 @@ 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().first); - EXPECT_TRUE(fake_message_sender_->GetRecentCrosState().second); + EXPECT_TRUE(fake_message_sender_->GetRecentCrosState()); EXPECT_EQ(1u, fake_message_sender_->GetCrosStateCallCount()); EXPECT_FALSE(mock_timer_->IsRunning()); } @@ -151,8 +146,7 @@ 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().first); - EXPECT_FALSE(fake_message_sender_->GetRecentCrosState().second); + EXPECT_FALSE(fake_message_sender_->GetRecentCrosState()); EXPECT_EQ(1u, fake_message_sender_->GetCrosStateCallCount()); mock_timer_->Fire(); @@ -160,7 +154,7 @@ TEST_F(CrosStateSenderTest, NotificationFeatureStateChanged) { // enabled. fake_multidevice_setup_client_->SetFeatureState( Feature::kPhoneHubNotifications, FeatureState::kEnabledByUser); - EXPECT_TRUE(fake_message_sender_->GetRecentCrosState().first); + EXPECT_TRUE(fake_message_sender_->GetRecentCrosState()); EXPECT_EQ(2u, fake_message_sender_->GetCrosStateCallCount()); mock_timer_->Fire(); @@ -168,7 +162,7 @@ TEST_F(CrosStateSenderTest, NotificationFeatureStateChanged) { // cros state. fake_multidevice_setup_client_->SetFeatureState( Feature::kSmartLock, FeatureState::kDisabledByUser); - EXPECT_TRUE(fake_message_sender_->GetRecentCrosState().first); + EXPECT_TRUE(fake_message_sender_->GetRecentCrosState()); EXPECT_EQ(3u, fake_message_sender_->GetCrosStateCallCount()); mock_timer_->Fire(); @@ -176,19 +170,12 @@ TEST_F(CrosStateSenderTest, NotificationFeatureStateChanged) { // disabled. fake_multidevice_setup_client_->SetFeatureState( Feature::kPhoneHubNotifications, FeatureState::kDisabledByUser); - EXPECT_FALSE(fake_message_sender_->GetRecentCrosState().first); + EXPECT_FALSE(fake_message_sender_->GetRecentCrosState()); 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(5u, fake_message_sender_->GetCrosStateCallCount()); + EXPECT_EQ(4u, fake_message_sender_->GetCrosStateCallCount()); } } // namespace phonehub diff --git a/chromeos/components/phonehub/fake_message_sender.cc b/chromeos/components/phonehub/fake_message_sender.cc index 506fcdd047d916..180d57a87986c3 100644 --- a/chromeos/components/phonehub/fake_message_sender.cc +++ b/chromeos/components/phonehub/fake_message_sender.cc @@ -10,10 +10,8 @@ namespace phonehub { FakeMessageSender::FakeMessageSender() = default; FakeMessageSender::~FakeMessageSender() = default; -void FakeMessageSender::SendCrosState(bool notification_enabled, - bool camera_roll_enabled) { - cros_states_.push_back( - std::make_pair(notification_enabled, camera_roll_enabled)); +void FakeMessageSender::SendCrosState(bool notification_enabled) { + cros_states_.push_back(notification_enabled); } void FakeMessageSender::SendUpdateNotificationModeRequest( @@ -79,7 +77,7 @@ size_t FakeMessageSender::GetFetchCameraRollItemsRequestCallCount() const { return fetch_camera_roll_items_requests_.size(); } -std::pair FakeMessageSender::GetRecentCrosState() const { +bool FakeMessageSender::GetRecentCrosState() const { return cros_states_.back(); } diff --git a/chromeos/components/phonehub/fake_message_sender.h b/chromeos/components/phonehub/fake_message_sender.h index aeb2b82de8ac7a..8ac1cfaf718080 100644 --- a/chromeos/components/phonehub/fake_message_sender.h +++ b/chromeos/components/phonehub/fake_message_sender.h @@ -22,8 +22,7 @@ class FakeMessageSender : public MessageSender { ~FakeMessageSender() override; // MessageSender: - void SendCrosState(bool notification_enabled, - bool camera_roll_enabled) override; + void SendCrosState(bool notification_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; @@ -35,7 +34,7 @@ class FakeMessageSender : public MessageSender { void SendFetchCameraRollItemsRequest( const proto::FetchCameraRollItemsRequest& request) override; - std::pair GetRecentCrosState() const; + bool GetRecentCrosState() const; bool GetRecentUpdateNotificationModeRequest() const; bool GetRecentUpdateBatteryModeRequest() const; int64_t GetRecentDismissNotificationRequest() const; @@ -64,9 +63,7 @@ class FakeMessageSender : public MessageSender { size_t GetFetchCameraRollItemsRequestCallCount() const; private: - std::vector> - cros_states_; + std::vector cros_states_; std::vector update_notification_mode_requests_; std::vector update_battery_mode_requests_; std::vector dismiss_notification_requests_; diff --git a/chromeos/components/phonehub/message_sender.h b/chromeos/components/phonehub/message_sender.h index a9c529992fe925..4cd09b95294488 100644 --- a/chromeos/components/phonehub/message_sender.h +++ b/chromeos/components/phonehub/message_sender.h @@ -22,8 +22,7 @@ class MessageSender { virtual ~MessageSender() = default; // Sends whether the notification setting is enabled in the Chrome OS device. - virtual void SendCrosState(bool notification_setting_enabled, - bool camera_roll_setting_enabled) = 0; + virtual void SendCrosState(bool notification_setting_enabled) = 0; // Requests that the phone enables or disables Do Not Disturb mode. virtual void SendUpdateNotificationModeRequest( diff --git a/chromeos/components/phonehub/message_sender_impl.cc b/chromeos/components/phonehub/message_sender_impl.cc index d16e36d4430425..d3d2cd312cd5f2 100644 --- a/chromeos/components/phonehub/message_sender_impl.cc +++ b/chromeos/components/phonehub/message_sender_impl.cc @@ -38,18 +38,13 @@ MessageSenderImpl::MessageSenderImpl( MessageSenderImpl::~MessageSenderImpl() = default; -void MessageSenderImpl::SendCrosState(bool notification_setting_enabled, - bool camera_roll_setting_enabled) { +void MessageSenderImpl::SendCrosState(bool notification_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); } diff --git a/chromeos/components/phonehub/message_sender_impl.h b/chromeos/components/phonehub/message_sender_impl.h index cd689b7061f0e2..0f19ec24c89d4d 100644 --- a/chromeos/components/phonehub/message_sender_impl.h +++ b/chromeos/components/phonehub/message_sender_impl.h @@ -27,8 +27,7 @@ class MessageSenderImpl : public MessageSender { ~MessageSenderImpl() override; // MessageSender: - void SendCrosState(bool notification_setting_enabled, - bool camera_roll_setting_enabled) override; + void SendCrosState(bool notification_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; diff --git a/chromeos/components/phonehub/message_sender_unittest.cc b/chromeos/components/phonehub/message_sender_unittest.cc index 9d1d053155ccdd..5df0ddd7c98589 100644 --- a/chromeos/components/phonehub/message_sender_unittest.cc +++ b/chromeos/components/phonehub/message_sender_unittest.cc @@ -64,10 +64,8 @@ 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, - /*camera_roll_enabled=*/false); + message_sender_->SendCrosState(/*notification_enabled=*/true); VerifyMessage(proto::MessageType::PROVIDE_CROS_STATE, &request, fake_connection_manager_->sent_messages().back()); } diff --git a/chromeos/components/phonehub/proto/phonehub_api.proto b/chromeos/components/phonehub/proto/phonehub_api.proto index 6ff53376be0246..527dee8b764587 100644 --- a/chromeos/components/phonehub/proto/phonehub_api.proto +++ b/chromeos/components/phonehub/proto/phonehub_api.proto @@ -40,11 +40,6 @@ enum NotificationSetting { NOTIFICATIONS_ON = 1; } -enum CameraRollSetting { - CAMERA_ROLL_OFF = 0; - CAMERA_ROLL_ON = 1; -} - enum ChargingState { NOT_CHARGING = 0; CHARGING_AC = 1; @@ -167,7 +162,6 @@ message App { message CrosState { NotificationSetting notification_setting = 1; - CameraRollSetting camera_roll_setting = 2; } message Action {