Fix Volume slider is captured in screenshot done in touchview mode

chromeos/audio/cras_audio_handler_unittest.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/audio/cras_audio_handler.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <memory>
 
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "chromeos/audio/audio_devices_pref_handler.h"
 #include "chromeos/audio/audio_devices_pref_handler_stub.h"
 #include "chromeos/dbus/audio_node.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/fake_cras_audio_client.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace chromeos {
 namespace {
 
 const uint64_t kInternalSpeakerId = 10001;
 const uint64_t kHeadphoneId = 10002;
 const uint64_t kInternalMicId = 10003;
 const uint64_t kUSBMicId = 10004;
 const uint64_t kBluetoothHeadsetId = 10005;
@@ skipping 265 matching lines @@
   int output_mute_changed_count_;
   int input_mute_changed_count_;
   int output_volume_changed_count_;
   int input_gain_changed_count_;
   bool output_mute_by_system_;  // output mute state adjusted by system.
   int output_channel_remixing_changed_count_;
 
   DISALLOW_COPY_AND_ASSIGN(TestObserver);
 };
 
+// A Mock CrasAudioClientObserver to detect if Cras side dbus signals are
+// correctly received.
+class MockAudioClientObserver : public CrasAudioClient::Observer {
+ public:
+  MockAudioClientObserver() {}
+  ~MockAudioClientObserver() override {}
+
+  MOCK_METHOD2(OutputNodeVolumeChanged, void(uint64_t node_id, int volume));
+};
+
 }  // namespace
 
 class CrasAudioHandlerTest : public testing::Test {
  public:
   CrasAudioHandlerTest() : cras_audio_handler_(NULL),
                            fake_cras_audio_client_(NULL) {
   }
   ~CrasAudioHandlerTest() override {}
 
   void SetUp() override {}
@@ skipping 95 matching lines @@
   }
 
   void SetHDMIRediscoverGracePeriodDuration(int duration_in_ms) {
     cras_audio_handler_->SetHDMIRediscoverGracePeriodForTesting(duration_in_ms);
   }
 
   bool IsDuringHDMIRediscoverGracePeriod() {
     return cras_audio_handler_->hdmi_rediscovering();
   }
 
+  void RestartAudioClient(const AudioNodeList& audio_nodes) {

[jennyz, 2016/08/04 16:07:33]

It is better not to pass audio_nodes in RestartAudioClient, the |audio_nodes| is
not associated with AudioClientRestarted event. You can separate it out.

[Qiang(Joe) Xu, 2016/08/04 22:51:58]

done by deleting this method

+    fake_cras_audio_client_->SetAudioNodesForTesting(audio_nodes);
+    fake_cras_audio_client_->NotifyAudioClientRestartedForTesting();
+    message_loop_.RunUntilIdle();
+  }
+
  protected:
   base::MessageLoopForUI message_loop_;
   CrasAudioHandler* cras_audio_handler_;  // Not owned.
   FakeCrasAudioClient* fake_cras_audio_client_;  // Not owned.
   std::unique_ptr<TestObserver> test_observer_;
   scoped_refptr<AudioDevicesPrefHandlerStub> audio_pref_handler_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(CrasAudioHandlerTest);
 };
@@ skipping 1531 matching lines @@
   EXPECT_FALSE(cras_audio_handler_->IsInputMuted());
   EXPECT_EQ(2, test_observer_->input_mute_changed_count());
 }
 
 TEST_F(CrasAudioHandlerTest, SetOutputVolumePercent) {
   AudioNodeList audio_nodes;
   audio_nodes.push_back(kInternalSpeaker);
   SetUpCrasAudioHandler(audio_nodes);
   EXPECT_EQ(0, test_observer_->output_volume_changed_count());
 
-  cras_audio_handler_->SetOutputVolumePercent(60);
+  const int kVolume = 60;
+  cras_audio_handler_->SetOutputVolumePercent(kVolume);
 
   // Verify the output volume is changed to the designated value,
   // OnOutputNodeVolumeChanged event is fired, and the device volume value
-  // is saved the preferences.
-  const int kVolume = 60;
+  // is saved in the preferences.
   EXPECT_EQ(kVolume, cras_audio_handler_->GetOutputVolumePercent());
   EXPECT_EQ(1, test_observer_->output_volume_changed_count());
   AudioDevice device;
   EXPECT_TRUE(cras_audio_handler_->GetPrimaryActiveOutputDevice(&device));
   EXPECT_EQ(device.id, kInternalSpeaker.id);
   EXPECT_EQ(kVolume, audio_pref_handler_->GetOutputVolumeValue(&device));
 }
 
+TEST_F(CrasAudioHandlerTest, SetOutputVolumePercentWithoutNotifyingObservers) {
+  AudioNodeList audio_nodes;
+  audio_nodes.push_back(kInternalSpeaker);
+  SetUpCrasAudioHandler(audio_nodes);
+  EXPECT_EQ(0, test_observer_->output_volume_changed_count());
+
+  const int kVolume1 = 60;
+  const int kVolume2 = 80;
+  cras_audio_handler_->SetOutputVolumePercentWithoutNotifyingObservers(
+      kVolume1, CrasAudioHandler::VOLUME_CHANGE_MAXIMIZE_MODE_SCREENSHOT);
+  // Verify the output volume is changed to the designated value,
+  // OnOutputNodeVolumeChanged event is not fired, and the device volume value
+  // is saved in the preferences.
+  EXPECT_EQ(kVolume1, cras_audio_handler_->GetOutputVolumePercent());
+  EXPECT_EQ(0, test_observer_->output_volume_changed_count());
+  AudioDevice device;
+  EXPECT_TRUE(cras_audio_handler_->GetPrimaryActiveOutputDevice(&device));
+  EXPECT_EQ(device.id, kInternalSpeaker.id);
+  EXPECT_EQ(kVolume1, audio_pref_handler_->GetOutputVolumeValue(&device));
+
+  // Make another SetOutputVolumePercentWithoutNotifyingObservers call to make
+  // sure everything is right.
+  cras_audio_handler_->SetOutputVolumePercentWithoutNotifyingObservers(
+      kVolume2, CrasAudioHandler::VOLUME_CHANGE_MAXIMIZE_MODE_SCREENSHOT);
+  EXPECT_EQ(kVolume2, cras_audio_handler_->GetOutputVolumePercent());
+  EXPECT_EQ(0, test_observer_->output_volume_changed_count());
+  EXPECT_TRUE(cras_audio_handler_->GetPrimaryActiveOutputDevice(&device));
+  EXPECT_EQ(device.id, kInternalSpeaker.id);
+  EXPECT_EQ(kVolume2, audio_pref_handler_->GetOutputVolumeValue(&device));
+
+  // Make a final SetOutputVolumePercent call to check if
+  // SetOutputVolumePercentWithoutNotifyingObservers may block subsequent
+  // notifying observers.
+  cras_audio_handler_->SetOutputVolumePercent(kVolume1);
+  EXPECT_EQ(kVolume1, cras_audio_handler_->GetOutputVolumePercent());
+  EXPECT_EQ(1, test_observer_->output_volume_changed_count());
+  EXPECT_TRUE(cras_audio_handler_->GetPrimaryActiveOutputDevice(&device));
+  EXPECT_EQ(device.id, kInternalSpeaker.id);
+  EXPECT_EQ(kVolume1, audio_pref_handler_->GetOutputVolumeValue(&device));
+}
+
+// OutputNodeVolumeChanged can be asynchronously fired by several reaons,
+// such as restarting audio client, SetOutputVolumePercent and
+// SetOutputVolumePercentWithoutNotifyingObservers. They should be correctly
+// received.

[jennyz, 2016/08/04 16:07:33]

It is not very clear what is tested here. OutputNodeVolumeChanged is
asynchronously in real cras case, but what in FakeCrasAudioClient, it is
controlled by |notify_volume_change_with_delay_| which is default to false,
i.e., sychronously. So you are actually not really testing asyc case here.

I mean you might not want this so complicated and mixed in one test case. How
many make each test case serve a single purpose. You can write one to just test
AudioClientRestartCase. If you want to test async case, make another one.

[Qiang(Joe) Xu, 2016/08/04 22:51:58]

Done.

+TEST_F(CrasAudioHandlerTest, OutputNodeVolumeChangedTest) {
+  AudioNodeList audio_nodes;
+  audio_nodes.push_back(kInternalSpeaker);
+  SetUpCrasAudioHandler(audio_nodes);
+  MockAudioClientObserver audio_client_observer;
+  fake_cras_audio_client_->AddObserver(&audio_client_observer);
+
+  // Set Cras OutputNodeVolumeChanged signal expectations. They are sequentially
+  // received according to the CrasAudioHandler's calls.
+  const int kVolume1 = 30;
+  const int kVolume2 = 40;
+  const int kVolume3 = 50;
+  const int kVolume4 = 60;
+  EXPECT_CALL(audio_client_observer,
+              OutputNodeVolumeChanged(kInternalSpeaker.id, kVolume1))
+      .Times(3);

[Daniel Erat, 2016/08/04 16:25:51]

i don't understand what this MockAudioClientObserver or these expectations are
for. they seem to be testing that FakeCrasAudioClient actually notifies its
observers, which isn't something that this test should be doing.

my suggestion for this test was meant to be:

a) initialize CrasAudioHandler so it's in the state where it should avoid
notifying observers
b) call FakeCrasAudioClient::NotifyOutputNodeVolumeChangedForTesting (if this
doesn't automatically happen in response to the handler's initialization)
c) use |test_observer_| to verify that notifications weren't sent
d) call SetOutputVolumePercent after initialization is done
e) use |test_observer_| to verify that a notification _was_ sent now

then add another test that verifies the cras-dropped-request case:

a) initialize CrasAudioHandler so it's in the state where it should avoid
notifying observers
b) call FakeCrasAudioClient::NotifyOutputNodeVolumeChangedForTesting with a
different output node
e) use |test_observer_| to verify that a notification was sent

the SetOutputVolumePercentWithoutNotifyingObservers test that you added looks
like it adaquately covers the new
SetOutputVolumePercentWithoutNotifyingObservers code that this change added; i
just wanted something like the two tests that i described above to verify the
initialization code (without testing *WithoutNotifyingObservers).

[Qiang(Joe) Xu, 2016/08/04 22:51:58]

The updated unittest should work in this way. Thanks!

+  EXPECT_CALL(audio_client_observer,
+              OutputNodeVolumeChanged(kInternalSpeaker.id, kVolume2))
+      .Times(1);
+  EXPECT_CALL(audio_client_observer,
+              OutputNodeVolumeChanged(kInternalSpeaker.id, kVolume3))
+      .Times(1);
+  EXPECT_CALL(audio_client_observer,
+              OutputNodeVolumeChanged(kInternalSpeaker.id, kVolume4))
+      .Times(1);
+
+  // Sequenced calls which will raise Cras side OutputNodeVolumeChanged signals.
+  cras_audio_handler_->SetOutputVolumePercent(kVolume1);
+  // RestartAudioClient causes InitializingAudioState, which should not notify
+  // AudioObserver::OnOutputNodeVolumeChanged.
+  RestartAudioClient(audio_nodes);
+  RestartAudioClient(audio_nodes);
+  cras_audio_handler_->SetOutputVolumePercentWithoutNotifyingObservers(
+      kVolume2, CrasAudioHandler::VOLUME_CHANGE_MAXIMIZE_MODE_SCREENSHOT);
+  cras_audio_handler_->SetOutputVolumePercentWithoutNotifyingObservers(
+      kVolume3, CrasAudioHandler::VOLUME_CHANGE_MAXIMIZE_MODE_SCREENSHOT);
+  cras_audio_handler_->SetOutputVolumePercent(kVolume4);
+
+  // Only the two SetOutputVolumePercent calls notifying observers.
+  EXPECT_EQ(2, test_observer_->output_volume_changed_count());
+
+  fake_cras_audio_client_->RemoveObserver(&audio_client_observer);
+  message_loop_.RunUntilIdle();
+}
+
 TEST_F(CrasAudioHandlerTest, SetOutputVolumeWithDelayedSignal) {
   AudioNodeList audio_nodes;
   audio_nodes.push_back(kInternalSpeaker);
   SetUpCrasAudioHandler(audio_nodes);
   EXPECT_EQ(0, test_observer_->output_volume_changed_count());
 
   const int kDefaultVolume = 75;
   EXPECT_EQ(kDefaultVolume, cras_audio_handler_->GetOutputVolumePercent());
 
   // Disable the auto OutputNodeVolumeChanged signal.
@@ skipping 1294 matching lines @@
       cras_audio_handler_->GetPrimaryActiveOutputDevice(&active_output));
   EXPECT_EQ(kInternalSpeaker.id, active_output.id);
   EXPECT_EQ(kInternalSpeaker.id,
             cras_audio_handler_->GetPrimaryActiveOutputNode());
   EXPECT_FALSE(cras_audio_handler_->IsOutputMuted());
   EXPECT_EQ(1, test_observer_->output_mute_changed_count());
   EXPECT_TRUE(test_observer_->output_mute_by_system());
 }
 
 }  // namespace chromeos