Fix Volume slider is captured in screenshot done in touchview mode

chromeos/audio/cras_audio_handler.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 <algorithm>
@@ skipping 358 matching lines @@
   // Set all active devices to the same volume.
   for (AudioDeviceMap::const_iterator it = audio_devices_.begin();
        it != audio_devices_.end();
        it++) {
     const AudioDevice& device = it->second;
     if (!device.is_input && device.active)
       SetOutputNodeVolumePercent(device.id, 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?
   for (AudioDeviceMap::const_iterator it = audio_devices_.begin();
        it != audio_devices_.end();
        it++) {
     const AudioDevice& device = it->second;
     if (device.is_input && device.active)
       SetInputNodeGainPercent(active_input_node_id_, gain_percent);
   }
@@ skipping 217 matching lines @@
   }
 
   // Sync internal volume state and notify UI for the change. We trust cras
   // signal to report the volume state of the device, no matter which source
   // set the volume, i.e., volume could be set from non-chrome source, like
   // Bluetooth headset, etc. Assume all active output devices share a single
   // 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) {
   if (active_output_node_id_ == node_id)
     return;
 
   // Active audio output device should always be changed by chrome.
   // During system boot, cras may change active input to unknown device 0x1,
   // we don't need to log it, since it is not an valid device.
   if (GetDeviceFromId(node_id)) {
@@ skipping 84 matching lines @@
   if (hdmi_rediscovering_ && !IsHDMIPrimaryOutputDevice()) {
     VLOG(1) << "Mute the output during HDMI re-discovering grace period";
     output_mute_on_ = true;
   } else {
     output_mute_on_ = audio_pref_handler_->GetMuteValue(*device);
   }
   output_volume_ = audio_pref_handler_->GetOutputVolumeValue(device);
 
   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_;
   }
   SetOutputNodeVolume(active_output_node_id_, output_volume_);
 }
 
 // This sets up the state of an additional active node.
 void CrasAudioHandler::SetupAdditionalActiveAudioNodeState(uint64_t node_id) {
   const AudioDevice* device = GetDeviceFromId(node_id);
   if (!device) {
@@ skipping 10 matching lines @@
   // devices. For input devices, we don't restore their gain value so far.
   // TODO(jennyz): crbug.com/417418, track the status for the decison if
   // we should persist input gain value in prefs.
   if (!device->is_input) {
     audio_pref_handler_->SetMuteValue(*device, IsOutputMuted());
     SetOutputNodeVolumePercent(node_id, GetOutputVolumePercent());
   }
 }
 
 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.
   cras_service_available_ = false;
   chromeos::DBusThreadManager::Get()
       ->GetCrasAudioClient()
       ->WaitForServiceToBeAvailable(base::Bind(
           &CrasAudioHandler::InitializeAudioAfterCrasServiceAvailable,
           weak_ptr_factory_.GetWeakPtr()));
 }
@@ skipping 584 matching lines @@
                      hdmi_rediscover_grace_period_duration_in_ms_),
       this, &CrasAudioHandler::UpdateAudioAfterHDMIRediscoverGracePeriod);
 }
 
 void CrasAudioHandler::SetHDMIRediscoverGracePeriodForTesting(
     int duration_in_ms) {
   hdmi_rediscover_grace_period_duration_in_ms_ = duration_in_ms;
 }
 
 }  // namespace chromeos