 Chromium Code Reviews
 Chromium Code Reviews Issue 2190773002:
  Fix Volume slider is captured in screenshot done in touchview mode  (Closed) 
  Base URL: https://chromium.googlesource.com/chromium/src.git@master
    
  
    Issue 2190773002:
  Fix Volume slider is captured in screenshot done in touchview mode  (Closed) 
  Base URL: https://chromium.googlesource.com/chromium/src.git@master| Index: chromeos/audio/cras_audio_handler.cc | 
| diff --git a/chromeos/audio/cras_audio_handler.cc b/chromeos/audio/cras_audio_handler.cc | 
| index 6a12d344cf4e662c2368882f380ed6b82867c889..ce35a06902f86a83debbcb0e12afcff737dd8dd1 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,28 @@ 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; | 
| + bool should_notify = true; | 
| + if (!automated_volume_change_reasons_.empty()) { | 
| + AutomatedVolumeChangeReason reason = | 
| + automated_volume_change_reasons_.front(); | 
| + if (reason == VOLUME_CHANGE_INITIALIZING_AUDIO_STATE && | 
| + (init_node_id_ != node_id || init_volume_ != volume)) { | 
| + // A SetOutputNodeVolume request may be dropped if cras isn't ready | 
| + // during initialization. In this case, we clear the pending automated | 
| + // volume change reasons as they might also be dropped by cras. | 
| + automated_volume_change_reasons_.clear(); | 
| 
jennyz
2016/08/02 18:02:33
Can we add a warning log here for diagnostic purpo
 
Qiang(Joe) Xu
2016/08/02 22:45:58
Done.
 | 
| } 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; | 
| + // In other cases, sequential AutomatedVolumeChangeReason corresponds to | 
| + // sequential avoiding notifying observers. | 
| + should_notify = false; | 
| + automated_volume_change_reasons_.pop_front(); | 
| } | 
| } | 
| - FOR_EACH_OBSERVER(AudioObserver, observers_, | 
| - OnOutputNodeVolumeChanged(node_id, volume)); | 
| + if (should_notify) { | 
| + FOR_EACH_OBSERVER(AudioObserver, observers_, | 
| + OnOutputNodeVolumeChanged(node_id, volume)); | 
| + } | 
| } | 
| void CrasAudioHandler::ActiveOutputNodeChanged(uint64_t node_id) { | 
| @@ -738,12 +746,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) { | 
| 
jennyz
2016/08/02 18:02:33
Will it be more straight forward by keeping initia
 
Qiang(Joe) Xu
2016/08/02 22:45:58
It is great, thanks. In this way, automated_volume
 | 
| // 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/02 16:31:38
i don't understand why you don't need to push anot
 
Qiang(Joe) Xu
2016/08/02 22:45:58
Yeah, I may change the code logic from jennyz's. I
 | 
| - init_volume_count_++; | 
| init_node_id_ = active_output_node_id_; | 
| init_volume_ = output_volume_; | 
| } | 
| @@ -774,7 +783,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. |