Fix Volume slider is captured in screenshot done in touchview mode

chromeos/audio/cras_audio_handler.cc

Index: chromeos/audio/cras_audio_handler.cc
diff --git a/chromeos/audio/cras_audio_handler.cc b/chromeos/audio/cras_audio_handler.cc
index a8f8036134ead71310d5a6748f4230045971f1f3..f5b26cfd2bbc6be18a76d1907934c96efd71d4ec 100644
--- a/chromeos/audio/cras_audio_handler.cc
+++ b/chromeos/audio/cras_audio_handler.cc
@@ -376,6 +376,13 @@ void CrasAudioHandler::SetOutputVolumePercent(int volume_percent) {
   }
 }
 
+void CrasAudioHandler::SetOutputVolumePercentWithoutNotifyingObservers(
+    int volume_percent,
+    AutomatedVolumeChangeReason reason) {
+  automated_volume_change_reasons_.push_back(reason);
+  SetOutputVolumePercent(volume_percent);
+}
+
 // TODO: Rename the 'Percent' to something more meaningful.
 void CrasAudioHandler::SetInputGainPercent(int gain_percent) {
   // TODO(jennyz): Should we set all input devices' gain to the same level?
@@ -613,27 +620,24 @@ void CrasAudioHandler::OutputNodeVolumeChanged(uint64_t node_id, int volume) {
   output_volume_ = volume;
   audio_pref_handler_->SetVolumeGainValue(*device, volume);
 
-  if (initializing_audio_state_) {
-    // Do not notify the observers for volume changed event if CrasAudioHandler
-    // is initializing its state, i.e., the volume change event is in responding
-    // to SetOutputNodeVolume request from intializaing audio state, not
-    // from user action, no need to notify UI to pop uo the volume slider bar.
-    if (init_node_id_ == node_id && init_volume_ == volume) {
-      init_volume_count_--;
-      if (!init_volume_count_)
-        initializing_audio_state_ = false;
-      return;
-    } else {
-      // Reset the initializing_audio_state_ in case SetOutputNodeVolume request
-      // is lost by cras due to cras is not ready when CrasAudioHandler is being
-      // initialized.
-      initializing_audio_state_ = false;
-      init_volume_count_ = 0;
+  bool should_not_notify_observers = false;

[Daniel Erat, 2016/08/01 21:10:07]

i'd invert this to make it easier to read:

  bool should_notify = true;
  if (!automated_volume_change_reasons_.empty()) {
    AutomatedVolumeChangeReason reason = ...
    automated_volume_change_reasons_.pop_front();

    // A SetOutputNodeVolume request may be dropped if cras isn't
    // ready during initialization.
    if (reason != VOLUME_CHANGE_INITIALIZING_AUDIO_STATE ||
        (init_node_id_ == node_id && init_volume_ == volume) {
      should_notify = false;
    }
  }

but i'm not convinced that this logic matches what was there before. to be
equivalent, wouldn't the new code need to remove all the
VOLUME_CHANGE_INITIALIZING_AUDIO_STATE entries from the deque?

jenny, is this trying to handle the case where some of the automated requests
got dropped due to cras not being ready, so now we've seen a user-initiated
change and want to make sure we display a notification and clear out the old
automated reasons?

i'm wondering if we should instead e.g. watch for the d-bus calls to cras
failing. then we would only asynchronously add the reasons to the deque when we
see our d-bus method call succeed.

[Qiang(Joe) Xu, 2016/08/01 22:56:48]

I think I do change the previous logic by mistake. I discussed with Jenny
offline. Once we met VOLUME_CHANGE_INITIALIZING_AUDIO_STATE and (init_node_id_
!= node_id || init_volume_ != volume), we clear the pending
AutomatedVolumeChangeReasons. As currently, cras is not reliable and it may also
miss other signals.

+  if (!automated_volume_change_reasons_.empty()) {
+    should_not_notify_observers = true;
+    AutomatedVolumeChangeReason reason =
+        automated_volume_change_reasons_.front();
+    if (reason == VOLUME_CHANGE_INITIALIZING_AUDIO_STATE &&
+        (init_node_id_ != node_id || init_volume_ != volume)) {
+      // SetOutputNodeVolume request may lose by cras due to cras is not ready
+      // when CrasAudioHandler is being initialized.
+      should_not_notify_observers = false;
     }
+    automated_volume_change_reasons_.pop_front();
   }
 
-  FOR_EACH_OBSERVER(AudioObserver, observers_,
-                    OnOutputNodeVolumeChanged(node_id, volume));
+  if (!should_not_notify_observers) {
+    FOR_EACH_OBSERVER(AudioObserver, observers_,
+                      OnOutputNodeVolumeChanged(node_id, volume));
+  }
 }
 
 void CrasAudioHandler::ActiveOutputNodeChanged(uint64_t node_id) {
@@ -738,12 +742,13 @@ void CrasAudioHandler::SetupAudioOutputState() {
 
   SetOutputMuteInternal(output_mute_on_);
 
-  if (initializing_audio_state_) {
+  if (!automated_volume_change_reasons_.empty() &&
+      automated_volume_change_reasons_.front() ==
+          VOLUME_CHANGE_INITIALIZING_AUDIO_STATE) {
     // During power up, InitializeAudioState() could be called twice, first
     // by CrasAudioHandler constructor, then by cras server restarting signal,
     // both sending SetOutputNodeVolume requests, and could lead to two
     // OutputNodeVolumeChanged signals.

[Daniel Erat, 2016/08/01 21:10:07]

the old code was incrementing a counter here that looks like it was used to
suppress multiple notifications, but i only see one reason getting added in
InitializeAudioState() now. are there existing tests to make sure that this is
still working correctly?

[Qiang(Joe) Xu, 2016/08/01 22:56:48]

I am not quite understanding the comments here. But I think the logic is still
needed. And there is no tests right now covering this. Jenny, what do you think
here?

-    init_volume_count_++;
     init_node_id_ = active_output_node_id_;
     init_volume_ = output_volume_;
   }
@@ -774,7 +779,8 @@ void CrasAudioHandler::SetupAdditionalActiveAudioNodeState(uint64_t node_id) {
 }
 
 void CrasAudioHandler::InitializeAudioState() {
-  initializing_audio_state_ = true;
+  automated_volume_change_reasons_.push_back(
+      VOLUME_CHANGE_INITIALIZING_AUDIO_STATE);
   ApplyAudioPolicy();
 
   // Defer querying cras for GetNodes until cras service becomes available.