Fix Volume slider is captured in screenshot done in touchview mode

chromeos/audio/cras_audio_handler_unittest.cc

Index: chromeos/audio/cras_audio_handler_unittest.cc
diff --git a/chromeos/audio/cras_audio_handler_unittest.cc b/chromeos/audio/cras_audio_handler_unittest.cc
index 7ec72df18fa4354b38413884f88c771a07f803f1..aa6a9b64ce8e0c587e329335fabd12b6bd1ff975 100644
--- a/chromeos/audio/cras_audio_handler_unittest.cc
+++ b/chromeos/audio/cras_audio_handler_unittest.cc
@@ -21,6 +21,7 @@
 #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 {
@@ -306,6 +307,16 @@ class TestObserver : public chromeos::CrasAudioHandler::AudioObserver {
   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 {
@@ -421,6 +432,12 @@ class CrasAudioHandlerTest : public testing::Test {
     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.
@@ -1972,12 +1989,12 @@ TEST_F(CrasAudioHandlerTest, SetOutputVolumePercent) {
   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;
@@ -1986,6 +2003,96 @@ TEST_F(CrasAudioHandlerTest, SetOutputVolumePercent) {
   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);