Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10873085:
Implement two new policies to control muting the audio I/O. (Closed)
|
Created:
8 years, 3 months ago by pastarmovj Modified:
8 years, 3 months ago CC:
chromium-reviews, sadrul, nkostylev+watch_chromium.org, ben+watch_chromium.org, Nirnimesh, dyu1, anantha, oshima+watch_chromium.org, dennis_jeffrey, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement two new policies to control muting the audio I/O.
BUG=chromium-os:11263
TEST=Manually set the policies in CPanel and observe that no sound is produced/recored on the device.
TBR=nirnimesh@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=154692
Patch Set 1 #
Total comments: 50
Patch Set 2 : Addressed comments. #
Total comments: 14
Patch Set 3 : Addressed comments. #
Total comments: 38
Patch Set 4 : Addressed even more comments. #
Total comments: 17
Patch Set 5 : Renamed both policies s/Enabled/Allowed/. #
Total comments: 16
Patch Set 6 : Final cleanup. #Patch Set 7 : Rebased to ToT. #Messages
Total messages: 18 (0 generated)
Hi Daniel&Mattias, Please review this CL which adds two new user policies to control whether aidio I/O should be enabled on chrome. This code relies on the respective Changes on the side of CRAS which are currently under review in this CL: https://gerrit.chromium.org/gerrit/#/c/29169/ . UI for those policies will come in subsequent CLs. This first one is only concerned with introducing and enforcing those policies on the device. @Daniel: Please review the audio/ash related code. @Mattias: Please review the policy related code. Thanks, Julian
http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... File chrome/browser/chromeos/audio/audio_mixer.h (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer.h:33: virtual void SetMuted(bool mute) = 0; nit: document that callers must call IsMuteLocked() first and cannot call this method if it returns true; ditto for SetCaptureMuted() http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer.h:39: virtual void SetMuteLocked(bool locked) = 0; Would we ever want to lock the device in an unmuted or capture-unmuted state? If not, it'd probably be a good idea to add a DCHECK() in the implementations of this and SetCaptureMuteLocked() to ensure that the "muted" variables are true (and to document that here). Actually, thinking about this more: Does all access to the AudioMixer classes go through AudioHandler? I believe so. If so, why not just enforce the policy in AudioHandler instead of needing to enforce it in all of the AudioMixer implementations? http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... File chrome/browser/chromeos/audio/audio_mixer_alsa.cc (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:152: if (!apply_is_pending_) nit: add curly braces since the statement doesn't fit on a single line http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:166: if (!apply_is_pending_) ditto http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:182: if (!apply_is_pending_) ditto http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:196: if (!apply_is_pending_) mind fixing this one too? http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... File chrome/browser/chromeos/audio/audio_mixer_alsa.h (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_alsa.h:99: // mimic CRASes locking functionality. nit: s/CRASes/CRAS's/ http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_alsa.h:101: bool is_mute_locked_; nit: move is_mute_locked_ and is_capture_mute_locked_ together and add a comment describing that when they're true, the state of is_muted_ and is_capture_muted_ can't be changed
http://codereview.chromium.org/10873085/diff/1/chrome/app/policy/policy_templ... File chrome/app/policy/policy_templates.json (right): http://codereview.chromium.org/10873085/diff/1/chrome/app/policy/policy_templ... chrome/app/policy/policy_templates.json:3162: When this policy is set to true, audio output will not be available on the device. I thought this is a user policy now? http://codereview.chromium.org/10873085/diff/1/chrome/app/policy/policy_templ... chrome/app/policy/policy_templates.json:3179: When this policy is set to true, audio capturing will not be available on the device. ditto http://codereview.chromium.org/10873085/diff/1/chrome/app/policy/policy_templ... chrome/app/policy/policy_templates.json:3183: If this setting is disabled or not configured then users can use all supported microphones on their device.''', s/microphones/audio inputs/ http://codereview.chromium.org/10873085/diff/1/chrome/app/policy/syntax_check... File chrome/app/policy/syntax_check_policy_template_json.py (right): http://codereview.chromium.org/10873085/diff/1/chrome/app/policy/syntax_check... chrome/app/policy/syntax_check_policy_template_json.py:52: 'AudioCaptureDisabled', Don't! Make it AudioOutputEnabled and AudioCaptureEnabled please. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_handler.cc:109: if (!chromeos::UserManager::Get()->IsUserLoggedIn()) Isn't AudioHandler a singleton? Do you call InitializePrefObservers multiple times on the same AudioHandler? http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_handler.cc:172: InitializePrefObservers(profile->GetPrefs()); So when a second profile gets created, we override the existing settings? I think a better way to do this would be to place these prefs in local_state after all, which will see policy associated with the login user. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_handler.cc:174: if (type == chrome::NOTIFICATION_PREF_CHANGED) { else if http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_handler.cc:180: } else { NOTREACHED() << type; } http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... File chrome/browser/chromeos/audio/audio_handler.h (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_handler.h:50: static void RegisterPrefs(PrefService* local_state); Rename to disambiguate? http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... File chrome/browser/chromeos/audio/audio_mixer.h (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer.h:35: // Is the device's mute state currently locked? What does it mean to mute-luck the device? User cannot change the volume, or the volume is forced to zero? http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... File chrome/browser/chromeos/audio/audio_mixer_alsa.cc (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:198: base::Bind(&AudioMixerAlsa::ApplyState, base::Unretained(this))); Lots of code duplication here. Any chance to fix? Maybe at least wrap the apply-if-not-pending logic in a function? http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... File chrome/browser/chromeos/audio/audio_mixer_alsa.h (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_alsa.h:84: // Guards |min_volume_db_|, |max_volume_db_|, |volume_db_|, the mute state, I'd just list the additional members for clarity. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_alsa.h:123: // Mic mixers. Used to mute capture. Both might be NULL on some drivers. nit: Two spaces after period seems to be the rule int this file. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... File chrome/browser/chromeos/audio/audio_mixer_cras.cc (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_cras.cc:146: base::Bind(&AudioMixerCras::ApplyState, base::Unretained(this))); Curlies missing here and above, same comment applies about duplication. One thing that's not your business but I should ask when I see it: Are there good reasons to use locks here? Chromium style generally discourages them, since they may block the UI thread after all. In this case, is it guaranteed that |thread_| never blocks on I/O? If not, it might be better to just post the settings to apply as a parameter to the task posted. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... File chrome/browser/chromeos/audio/audio_mixer_cras.h (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_cras.h:72: // Guards |volume_percent_|, the mute state, and |apply_is_pending_|. spell out the members? http://codereview.chromium.org/10873085/diff/1/chrome/browser/policy/configur... File chrome/browser/policy/configuration_policy_handler_list.cc (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/policy/configur... chrome/browser/policy/configuration_policy_handler_list.cc:324: prefs::kAudioOutputDisabled, If you change the policy to positive polarity, you'll likely want to change the prefs as well. http://codereview.chromium.org/10873085/diff/1/chrome/common/pref_names.h File chrome/common/pref_names.h (right): http://codereview.chromium.org/10873085/diff/1/chrome/common/pref_names.h#new... chrome/common/pref_names.h:660: extern const char kAudioCaptureDisabled[]; Everything here is local_state stuff. Fix the place where you put this according to the local_state decision.
Thanks for the fast review to both of you. I hope I have answered all your requests and questions. PTAL. http://codereview.chromium.org/10873085/diff/1/chrome/app/policy/policy_templ... File chrome/app/policy/policy_templates.json (right): http://codereview.chromium.org/10873085/diff/1/chrome/app/policy/policy_templ... chrome/app/policy/policy_templates.json:3162: When this policy is set to true, audio output will not be available on the device. On 2012/08/28 12:24:24, Mattias Nissler wrote: > I thought this is a user policy now? Yes it is just the wording is not very good. It still is the device that won't play sounds but only while the user session is active. I have tuned the wording accordingly. http://codereview.chromium.org/10873085/diff/1/chrome/app/policy/policy_templ... chrome/app/policy/policy_templates.json:3179: When this policy is set to true, audio capturing will not be available on the device. On 2012/08/28 12:24:24, Mattias Nissler wrote: > ditto Done. http://codereview.chromium.org/10873085/diff/1/chrome/app/policy/policy_templ... chrome/app/policy/policy_templates.json:3183: If this setting is disabled or not configured then users can use all supported microphones on their device.''', On 2012/08/28 12:24:24, Mattias Nissler wrote: > s/microphones/audio inputs/ Done. http://codereview.chromium.org/10873085/diff/1/chrome/app/policy/syntax_check... File chrome/app/policy/syntax_check_policy_template_json.py (right): http://codereview.chromium.org/10873085/diff/1/chrome/app/policy/syntax_check... chrome/app/policy/syntax_check_policy_template_json.py:52: 'AudioCaptureDisabled', On 2012/08/28 12:24:24, Mattias Nissler wrote: > Don't! Make it AudioOutputEnabled and AudioCaptureEnabled please. Fine. I wanted to keep all those policies in the same style but I have no strong feelings about that. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_handler.cc:109: if (!chromeos::UserManager::Get()->IsUserLoggedIn()) On 2012/08/28 12:24:24, Mattias Nissler wrote: > Isn't AudioHandler a singleton? Do you call InitializePrefObservers multiple > times on the same AudioHandler? Moved to local_state as suggested. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_handler.cc:172: InitializePrefObservers(profile->GetPrefs()); On 2012/08/28 12:24:24, Mattias Nissler wrote: > So when a second profile gets created, we override the existing settings? I > think a better way to do this would be to place these prefs in local_state after > all, which will see policy associated with the login user. Done. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_handler.cc:174: if (type == chrome::NOTIFICATION_PREF_CHANGED) { On 2012/08/28 12:24:24, Mattias Nissler wrote: > else if Done. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_handler.cc:180: } On 2012/08/28 12:24:24, Mattias Nissler wrote: > else { > NOTREACHED() << type; > } Done. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... File chrome/browser/chromeos/audio/audio_handler.h (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_handler.h:50: static void RegisterPrefs(PrefService* local_state); On 2012/08/28 12:24:24, Mattias Nissler wrote: > Rename to disambiguate? Done. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... File chrome/browser/chromeos/audio/audio_mixer.h (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer.h:33: virtual void SetMuted(bool mute) = 0; On 2012/08/27 17:38:35, Daniel Erat wrote: > nit: document that callers must call IsMuteLocked() first and cannot call this > method if it returns true; ditto for SetCaptureMuted() Technically nothing prevents the caller to call SetMuted regardless of the Locked state. However the mute state will not change but the observers will be called to inform the interested parties that what the state after the operation is. Added some more information about that to the comment though. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer.h:35: // Is the device's mute state currently locked? On 2012/08/28 12:24:24, Mattias Nissler wrote: > What does it mean to mute-luck the device? User cannot change the volume, or the > volume is forced to zero? To mute lock means that the audio inputs or outputs are locked for changing the mute state and not the volume. A muted output will be muted regardless of the volume level and unmuted devices will allow volume control but will not be mutable (except of course through setting the volume to 0). Right now there is no real use case for locking the mute state in unmuted so I might as well limit the API as suggested here to only allow locking in the muted state. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer.h:39: virtual void SetMuteLocked(bool locked) = 0; On 2012/08/27 17:38:35, Daniel Erat wrote: > Would we ever want to lock the device in an unmuted or capture-unmuted state? > If not, it'd probably be a good idea to add a DCHECK() in the implementations of > this and SetCaptureMuteLocked() to ensure that the "muted" variables are true > (and to document that here). > > Actually, thinking about this more: Does all access to the AudioMixer classes go > through AudioHandler? I believe so. If so, why not just enforce the policy in > AudioHandler instead of needing to enforce it in all of the AudioMixer > implementations? Actually it is needed especially in the AudioMixerCras implementation as we have support to lock the server's mute state which will guarantee that no other process will be able to unmute the device. The fact that I have implemented the locking on the client side as well is more to provide consistency with the soft locking in Alsa. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... File chrome/browser/chromeos/audio/audio_mixer_alsa.cc (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:152: if (!apply_is_pending_) On 2012/08/27 17:38:35, Daniel Erat wrote: > nit: add curly braces since the statement doesn't fit on a single line Done. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:166: if (!apply_is_pending_) On 2012/08/27 17:38:35, Daniel Erat wrote: > ditto Done. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:182: if (!apply_is_pending_) On 2012/08/27 17:38:35, Daniel Erat wrote: > ditto Done. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:196: if (!apply_is_pending_) On 2012/08/27 17:38:35, Daniel Erat wrote: > mind fixing this one too? Done. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:198: base::Bind(&AudioMixerAlsa::ApplyState, base::Unretained(this))); On 2012/08/28 12:24:24, Mattias Nissler wrote: > Lots of code duplication here. Any chance to fix? Maybe at least wrap the > apply-if-not-pending logic in a function? Done. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... File chrome/browser/chromeos/audio/audio_mixer_alsa.h (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_alsa.h:84: // Guards |min_volume_db_|, |max_volume_db_|, |volume_db_|, the mute state, On 2012/08/28 12:24:24, Mattias Nissler wrote: > I'd just list the additional members for clarity. Done. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_alsa.h:99: // mimic CRASes locking functionality. On 2012/08/27 17:38:35, Daniel Erat wrote: > nit: s/CRASes/CRAS's/ Done. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_alsa.h:101: bool is_mute_locked_; On 2012/08/27 17:38:35, Daniel Erat wrote: > nit: move is_mute_locked_ and is_capture_mute_locked_ together and add a comment > describing that when they're true, the state of is_muted_ and is_capture_muted_ > can't be changed Done. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_alsa.h:123: // Mic mixers. Used to mute capture. Both might be NULL on some drivers. On 2012/08/28 12:24:24, Mattias Nissler wrote: > nit: Two spaces after period seems to be the rule int this file. Done. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... File chrome/browser/chromeos/audio/audio_mixer_cras.cc (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_cras.cc:146: base::Bind(&AudioMixerCras::ApplyState, base::Unretained(this))); On 2012/08/28 12:24:24, Mattias Nissler wrote: > Curlies missing here and above, same comment applies about duplication. > > One thing that's not your business but I should ask when I see it: Are there > good reasons to use locks here? Chromium style generally discourages them, since > they may block the UI thread after all. In this case, is it guaranteed that > |thread_| never blocks on I/O? If not, it might be better to just post the > settings to apply as a parameter to the task posted. It is guaranteed that the lock is only held in the other thread to copy the state to local variables so no IO happens while thread_ is holding the lock. I think in this case it is relatively safe to use locks. I could make the ApplyState function accept the state as arguments but I think this is not going to contribute much to either the responsiveness or readability of this code. http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... File chrome/browser/chromeos/audio/audio_mixer_cras.h (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/chromeos/audio/... chrome/browser/chromeos/audio/audio_mixer_cras.h:72: // Guards |volume_percent_|, the mute state, and |apply_is_pending_|. On 2012/08/28 12:24:24, Mattias Nissler wrote: > spell out the members? Done. http://codereview.chromium.org/10873085/diff/1/chrome/browser/policy/configur... File chrome/browser/policy/configuration_policy_handler_list.cc (right): http://codereview.chromium.org/10873085/diff/1/chrome/browser/policy/configur... chrome/browser/policy/configuration_policy_handler_list.cc:324: prefs::kAudioOutputDisabled, On 2012/08/28 12:24:24, Mattias Nissler wrote: > If you change the policy to positive polarity, you'll likely want to change the > prefs as well. Done. http://codereview.chromium.org/10873085/diff/1/chrome/common/pref_names.h File chrome/common/pref_names.h (right): http://codereview.chromium.org/10873085/diff/1/chrome/common/pref_names.h#new... chrome/common/pref_names.h:660: extern const char kAudioCaptureDisabled[]; On 2012/08/28 12:24:24, Mattias Nissler wrote: > Everything here is local_state stuff. Fix the place where you put this according > to the local_state decision. Well turns out I have misplaced it properly :)
http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_mixer.h (right): http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer.h:45: virtual bool IsCaptureMuted() = 0; It's "mic" in AudioHandler but "capture" here. Are there any reasons they shouldn't be consistent? http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_mixer_alsa.cc (right): http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:135: if (!apply_is_pending_) nit: Use ApplyStateIfNeeded() here too. http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:498: void AudioMixerAlsa::ApplyStateIfNeeded() { nit: Add lock_.AssertAcquired(). http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_mixer_cras.cc (right): http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer_cras.cc:95: NOTREACHED() << "Mute has been locked!"; This is why I suggested documenting that callers must check Is*Locked() before trying to change the mute state: the AudioMixerCras implementation appears to require this. http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer_cras.cc:123: NOTREACHED() << "Capture mute has been locked!"; nit: indent two spaces, not three http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer_cras.cc:125: } is_capture_muted_ = locked; nit: fix formatting http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer_cras.cc:200: void AudioMixerCras::ApplyStateIfNeeded() { nit: check that the lock is held here
Addressed second round of comments. http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_mixer.h (right): http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer.h:45: virtual bool IsCaptureMuted() = 0; On 2012/08/28 20:40:51, Daniel Erat wrote: > It's "mic" in AudioHandler but "capture" here. Are there any reasons they > shouldn't be consistent? Not really I will change it to capture everywhere to be consistent with Cras. http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_mixer_alsa.cc (right): http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:135: if (!apply_is_pending_) On 2012/08/28 20:40:51, Daniel Erat wrote: > nit: Use ApplyStateIfNeeded() here too. Done. http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:498: void AudioMixerAlsa::ApplyStateIfNeeded() { On 2012/08/28 20:40:51, Daniel Erat wrote: > nit: Add lock_.AssertAcquired(). Done. http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_mixer_cras.cc (right): http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer_cras.cc:95: NOTREACHED() << "Mute has been locked!"; On 2012/08/28 20:40:51, Daniel Erat wrote: > This is why I suggested documenting that callers must check Is*Locked() before > trying to change the mute state: the AudioMixerCras implementation appears to > require this. Done. http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer_cras.cc:123: NOTREACHED() << "Capture mute has been locked!"; On 2012/08/28 20:40:51, Daniel Erat wrote: > nit: indent two spaces, not three Grrr don't know why this screwed up here so badly. :( Fixed. http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer_cras.cc:125: } is_capture_muted_ = locked; On 2012/08/28 20:40:51, Daniel Erat wrote: > nit: fix formatting Ditto. http://codereview.chromium.org/10873085/diff/15001/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer_cras.cc:200: void AudioMixerCras::ApplyStateIfNeeded() { On 2012/08/28 20:40:51, Daniel Erat wrote: > nit: check that the lock is held here Done.
This looks pretty good to me -- sorry for all the nits. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_handler.cc:88: local_state->RegisterBooleanPref(prefs::kAudioOutputEnabled, i'm a bit concerned that having both kAudioOutputEnabled and kAudioMute prefs may be confusing. renaming kAudioOutputEnabled to something like kAudioOutputAllowed or kAudioOutputPermitted would make the distinction between the two clearer. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_handler.cc:162: NOTREACHED(); nit: NOTREACHED() << "Unexpected notification " << type; http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_mixer.h (right): http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer.h:30: // this function will not change the mute state until it is unlocked with is this comment meant to be above SetMuted() instead? i wouldn't expect IsMuted() to change the mute state. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer.h:34: // Mutes or unmutes the device, possibly asynchronously. The called must nit: s/called/caller/ http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer.h:41: // Locks the mute state of the device to muted. After this is done SetMute the "to muted" bit seems inaccurate. this method doesn't appear to lock the state to muted in either implementation; rather, it just locks it in its current (either muted or unmuted) state. i'm not sure whether it'd be better to make the comment match the code or to make the code match the comment. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer.h:48: // Mutes or unmutes the capture device, possible asynchronously. The called nit: s/called/caller/ http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_mixer_alsa.cc (right): http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:144: void AudioMixerAlsa::SetMuted(bool muted) { nit: s/muted/mute/ http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:147: if (is_mute_locked_) add a NOTREACHED() here to match AudioMixerCras and the comment in AudioMixer? http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:162: ApplyStateIfNeeded(); this call doesn't appear to be necessary since the alsa implementation doesn't do anything when the lock state is changed. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:170: void AudioMixerAlsa::SetCaptureMuted(bool muted) { nit: s/muted/mute/ here too? http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:173: if (is_capture_mute_locked_) NOTREACHED()? http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:188: ApplyStateIfNeeded(); this call doesn't appear to be necessary since the alsa implementation doesn't do anything when the lock state is changed. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_mixer_alsa.h (right): http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_alsa.h:35: virtual void SetMuted(bool muted) OVERRIDE; nit: not your fault, but mind changing |muted| to |mute| here to match the parameter name in AudioMixer? http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_mixer_cras.cc (right): http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_cras.cc:91: void AudioMixerCras::SetMuted(bool muted) { s/muted/mute/ http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_cras.cc:119: void AudioMixerCras::SetCaptureMuted(bool locked) { s/locked/mute/ http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_mixer_cras.h (right): http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_cras.h:32: virtual void SetMuted(bool muted) OVERRIDE; s/muted/mute/ http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_cras.h:49: // Call ApplyState on the proper thread only if no other call is currently nit: s/Call/Calls/ http://codereview.chromium.org/10873085/diff/7016/chrome/browser/ui/ash/volum... File chrome/browser/ui/ash/volume_controller_browsertest_chromeos.cc (right): http://codereview.chromium.org/10873085/diff/7016/chrome/browser/ui/ash/volum... chrome/browser/ui/ash/volume_controller_browsertest_chromeos.cc:37: virtual void SetMuted(bool do_mute) OVERRIDE { nit: s/do_mute/mute/ http://codereview.chromium.org/10873085/diff/7016/chrome/browser/ui/ash/volum... chrome/browser/ui/ash/volume_controller_browsertest_chromeos.cc:53: void SetCaptureMuted(bool locked) OVERRIDE { s/locked/mute/
Thanks a lot for the early morning reviews, Daniel! Highly appreciate it! http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_handler.cc:88: local_state->RegisterBooleanPref(prefs::kAudioOutputEnabled, On 2012/08/29 15:37:00, Daniel Erat wrote: > i'm a bit concerned that having both kAudioOutputEnabled and kAudioMute prefs > may be confusing. renaming kAudioOutputEnabled to something like > kAudioOutputAllowed or kAudioOutputPermitted would make the distinction between > the two clearer. How strong do you feel about changing the name here. I have picked the name to be consistent with the name of the policy and the general naming scheme we use for such policies. I can of course change the name of the pref independent of the policy name and will do this if you insist. However I propose instead to extend the description of the kAudioOutputEnabled and kAudioMute policies with the following : "This pref supersedes |kAudioMute| but does not overwrite it, therefore when the policy is lifted the original mute state is restored." and the reciprocal text on the other one : "This pref is ignored if |kAudioOutputEnabled| is set to false, but its value is preserved, therefore when the policy is lifted the original mute state is restored." http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_handler.cc:162: NOTREACHED(); On 2012/08/29 15:37:00, Daniel Erat wrote: > nit: NOTREACHED() << "Unexpected notification " << type; Done. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_mixer.h (right): http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer.h:30: // this function will not change the mute state until it is unlocked with On 2012/08/29 15:37:00, Daniel Erat wrote: > is this comment meant to be above SetMuted() instead? i wouldn't expect > IsMuted() to change the mute state. Shoot! Sorry, it is of course. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer.h:34: // Mutes or unmutes the device, possibly asynchronously. The called must On 2012/08/29 15:37:00, Daniel Erat wrote: > nit: s/called/caller/ Done. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer.h:41: // Locks the mute state of the device to muted. After this is done SetMute On 2012/08/29 15:37:00, Daniel Erat wrote: > the "to muted" bit seems inaccurate. this method doesn't appear to lock the > state to muted in either implementation; rather, it just locks it in its current > (either muted or unmuted) state. i'm not sure whether it'd be better to make > the comment match the code or to make the code match the comment. I would prefer to fix the comment to match the code as this also matches the CRAS server behavior and this seems to be more future-proof without loosing the clearness of the API. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer.h:48: // Mutes or unmutes the capture device, possible asynchronously. The called On 2012/08/29 15:37:00, Daniel Erat wrote: > nit: s/called/caller/ Done. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_mixer_alsa.cc (right): http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:144: void AudioMixerAlsa::SetMuted(bool muted) { On 2012/08/29 15:37:00, Daniel Erat wrote: > nit: s/muted/mute/ Done. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:147: if (is_mute_locked_) On 2012/08/29 15:37:00, Daniel Erat wrote: > add a NOTREACHED() here to match AudioMixerCras and the comment in AudioMixer? Done. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:162: ApplyStateIfNeeded(); On 2012/08/29 15:37:00, Daniel Erat wrote: > this call doesn't appear to be necessary since the alsa implementation doesn't > do anything when the lock state is changed. True. Cleaned. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:170: void AudioMixerAlsa::SetCaptureMuted(bool muted) { On 2012/08/29 15:37:00, Daniel Erat wrote: > nit: s/muted/mute/ here too? Done. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:173: if (is_capture_mute_locked_) On 2012/08/29 15:37:00, Daniel Erat wrote: > NOTREACHED()? Done. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:188: ApplyStateIfNeeded(); On 2012/08/29 15:37:00, Daniel Erat wrote: > this call doesn't appear to be necessary since the alsa implementation doesn't > do anything when the lock state is changed. Done. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_mixer_alsa.h (right): http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_alsa.h:35: virtual void SetMuted(bool muted) OVERRIDE; On 2012/08/29 15:37:00, Daniel Erat wrote: > nit: not your fault, but mind changing |muted| to |mute| here to match the > parameter name in AudioMixer? Done. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_mixer_cras.cc (right): http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_cras.cc:91: void AudioMixerCras::SetMuted(bool muted) { On 2012/08/29 15:37:00, Daniel Erat wrote: > s/muted/mute/ Done. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_cras.cc:119: void AudioMixerCras::SetCaptureMuted(bool locked) { On 2012/08/29 15:37:00, Daniel Erat wrote: > s/locked/mute/ Done. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_mixer_cras.h (right): http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_cras.h:32: virtual void SetMuted(bool muted) OVERRIDE; On 2012/08/29 15:37:00, Daniel Erat wrote: > s/muted/mute/ Done. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_mixer_cras.h:49: // Call ApplyState on the proper thread only if no other call is currently On 2012/08/29 15:37:00, Daniel Erat wrote: > nit: s/Call/Calls/ Done. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/ui/ash/volum... File chrome/browser/ui/ash/volume_controller_browsertest_chromeos.cc (right): http://codereview.chromium.org/10873085/diff/7016/chrome/browser/ui/ash/volum... chrome/browser/ui/ash/volume_controller_browsertest_chromeos.cc:37: virtual void SetMuted(bool do_mute) OVERRIDE { On 2012/08/29 15:37:00, Daniel Erat wrote: > nit: s/do_mute/mute/ Done. http://codereview.chromium.org/10873085/diff/7016/chrome/browser/ui/ash/volum... chrome/browser/ui/ash/volume_controller_browsertest_chromeos.cc:53: void SetCaptureMuted(bool locked) OVERRIDE { On 2012/08/29 15:37:00, Daniel Erat wrote: > s/locked/mute/ Done.
(Sorry for harping on the pref names; it's just very difficult to change them later.) http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:88: local_state->RegisterBooleanPref(prefs::kAudioOutputEnabled, I feel pretty strongly about renaming the "enabled" prefs to make their meanings distinct from the mute pref's meaning. I think that seeing "audio output enabled" and "audio muted" prefs that are both set to true in a prefs file will be confusing. I also agree that it'd be good if the prefs matched the policy names, though. Are the policy names difficult to change? :-) "Enabled" makes me think that I can play/capture audio if it's set to true and I can't if it's set to false. Something like "allowed" makes it clearer that audio may or may not be audible when it's true, but can never be when it's false. http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_mixer.h (right): http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer.h:34: // If SetMuteLocked has been called with true this function will not change nit: remove the "If SetMuteLocked has been called..." sentence, since it implies that calling this function after SetMuteLocked(true) is valid (when it'll actually trigger a NOTREACHED(), which i believe crashes in debug builds)
Mostly good, just some nits (and the pref name discussion to be resolved). http://codereview.chromium.org/10873085/diff/14002/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): http://codereview.chromium.org/10873085/diff/14002/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3160: 'desc': '''Disable playing audio. Stills says Disable here and in caption, please fix. http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:88: local_state->RegisterBooleanPref(prefs::kAudioOutputEnabled, On 2012/08/29 17:35:10, Daniel Erat wrote: > I feel pretty strongly about renaming the "enabled" prefs to make their meanings > distinct from the mute pref's meaning. I think that seeing "audio output > enabled" and "audio muted" prefs that are both set to true in a prefs file will > be confusing. > > I also agree that it'd be good if the prefs matched the policy names, though. > Are the policy names difficult to change? :-) "Enabled" makes me think that I > can play/capture audio if it's set to true and I can't if it's set to false. > Something like "allowed" makes it clearer that audio may or may not be audible > when it's true, but can never be when it's false. Ha, it was me to request the policy names to not be XYZDisabled. We have lots of boolean flags, and we're trying to consolidate them to positive polarity and be consistent on that. XYZAllowed is a good idea, I support that proposal. http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:194: FOR_EACH_OBSERVER(VolumeObserver, volume_observers_, OnMuteToggled()); Just to verify: There is no capture observer, and VolumeObserver is not supposed to fire on capture events? http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:203: remove extra blank line http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_mixer_alsa.cc (right): http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:502: base::Bind(&AudioMixerAlsa::ApplyState, base::Unretained(this))); indentation: PostTask arguments should be aligned. http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_mixer_cras.cc (right): http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer_cras.cc:205: base::Bind(&AudioMixerCras::ApplyState, base::Unretained(this))); indentation http://codereview.chromium.org/10873085/diff/14002/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): http://codereview.chromium.org/10873085/diff/14002/chrome/common/pref_names.c... chrome/common/pref_names.cc:1774: // ChromeOS devices. This pref supersedes |kAudioMute| but does not overwrite nit: It doesn't supersede but rather override the other pref.
http://codereview.chromium.org/10873085/diff/14002/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): http://codereview.chromium.org/10873085/diff/14002/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3160: 'desc': '''Disable playing audio. On 2012/08/30 11:51:26, Mattias Nissler wrote: > Stills says Disable here and in caption, please fix. Done. http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:88: local_state->RegisterBooleanPref(prefs::kAudioOutputEnabled, On 2012/08/30 11:51:26, Mattias Nissler wrote: > On 2012/08/29 17:35:10, Daniel Erat wrote: > > I feel pretty strongly about renaming the "enabled" prefs to make their > meanings > > distinct from the mute pref's meaning. I think that seeing "audio output > > enabled" and "audio muted" prefs that are both set to true in a prefs file > will > > be confusing. > > > > I also agree that it'd be good if the prefs matched the policy names, though. > > Are the policy names difficult to change? :-) "Enabled" makes me think that I > > can play/capture audio if it's set to true and I can't if it's set to false. > > Something like "allowed" makes it clearer that audio may or may not be audible > > when it's true, but can never be when it's false. > > Ha, it was me to request the policy names to not be XYZDisabled. We have lots of > boolean flags, and we're trying to consolidate them to positive polarity and be > consistent on that. XYZAllowed is a good idea, I support that proposal. Both have been renamed accordingly. http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:194: FOR_EACH_OBSERVER(VolumeObserver, volume_observers_, OnMuteToggled()); On 2012/08/30 11:51:26, Mattias Nissler wrote: > Just to verify: There is no capture observer, and VolumeObserver is not supposed > to fire on capture events? No, there isn't one yet. I have it added in a secnod CL which is on standby though until we finalize how the UI for those policies will look like. http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:203: On 2012/08/30 11:51:26, Mattias Nissler wrote: > remove extra blank line Done. http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_mixer.h (right): http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer.h:34: // If SetMuteLocked has been called with true this function will not change On 2012/08/29 17:35:10, Daniel Erat wrote: > nit: remove the "If SetMuteLocked has been called..." sentence, since it implies > that calling this function after SetMuteLocked(true) is valid (when it'll > actually trigger a NOTREACHED(), which i believe crashes in debug builds) Done. http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_mixer_alsa.cc (right): http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer_alsa.cc:502: base::Bind(&AudioMixerAlsa::ApplyState, base::Unretained(this))); On 2012/08/30 11:51:26, Mattias Nissler wrote: > indentation: PostTask arguments should be aligned. Done. http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_mixer_cras.cc (right): http://codereview.chromium.org/10873085/diff/14002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_mixer_cras.cc:205: base::Bind(&AudioMixerCras::ApplyState, base::Unretained(this))); On 2012/08/30 11:51:26, Mattias Nissler wrote: > indentation Done. http://codereview.chromium.org/10873085/diff/14002/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): http://codereview.chromium.org/10873085/diff/14002/chrome/common/pref_names.c... chrome/common/pref_names.cc:1774: // ChromeOS devices. This pref supersedes |kAudioMute| but does not overwrite On 2012/08/30 11:51:26, Mattias Nissler wrote: > nit: It doesn't supersede but rather override the other pref. Done.
LGTM with nits http://codereview.chromium.org/10873085/diff/23002/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): http://codereview.chromium.org/10873085/diff/23002/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3164: This policy affects all types of audio output and not only the built-in speakers. Audio accessability features are also inhibited by this policy. Do not enable this policy if screen reader is required for this user. s/if/if a/ s/this/the/ http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:18: #include "chrome/browser/chromeos/login/user_manager.h" not longer needed http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:20: #include "chrome/browser/profiles/profile.h" not needed? http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_handler.h (right): http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.h:27: class AudioHandler : content::NotificationObserver { public
LGTM with a few comments. Thanks! http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:89: true, nit: fix indenting http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:92: true, nit: fix indenting http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:132: FOR_EACH_OBSERVER(VolumeObserver, volume_observers_, OnMuteToggled()); should this be inside the 'if' statement? otherwise, you're notifying the observers even when nothing changed, no? http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:192: mixer_->SetMuteLocked(true); might be a bit safer to do something like this: mixer_->SetMuteLocked(false); if (local_state_->GetBoolean(prefs::kAudioOutputAllowed) { mixer_->SetMuted( local_state_->GetInteger(prefs::kAudioMute) == kPrefMuteOn); } else { mixer_->SetMuted(true); mixer_->SetMuteLocked(true); } that way, ApplyAudioPolicy() can be called multiple times. otherwise, i think you'll hit the NOTREACHED() on the second call if we're locked.
http://codereview.chromium.org/10873085/diff/23002/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): http://codereview.chromium.org/10873085/diff/23002/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3164: This policy affects all types of audio output and not only the built-in speakers. Audio accessability features are also inhibited by this policy. Do not enable this policy if screen reader is required for this user. On 2012/08/31 08:06:51, Mattias Nissler wrote: > s/if/if a/ s/this/the/ Done. http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:18: #include "chrome/browser/chromeos/login/user_manager.h" On 2012/08/31 08:06:51, Mattias Nissler wrote: > not longer needed Done. http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:20: #include "chrome/browser/profiles/profile.h" On 2012/08/31 08:06:51, Mattias Nissler wrote: > not needed? Done. http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:89: true, On 2012/08/31 14:57:46, Daniel Erat wrote: > nit: fix indenting Done. http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:92: true, On 2012/08/31 14:57:46, Daniel Erat wrote: > nit: fix indenting Done. http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:132: FOR_EACH_OBSERVER(VolumeObserver, volume_observers_, OnMuteToggled()); On 2012/08/31 14:57:46, Daniel Erat wrote: > should this be inside the 'if' statement? otherwise, you're notifying the > observers even when nothing changed, no? Done. http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.cc:192: mixer_->SetMuteLocked(true); On 2012/08/31 14:57:46, Daniel Erat wrote: > might be a bit safer to do something like this: > > mixer_->SetMuteLocked(false); > if (local_state_->GetBoolean(prefs::kAudioOutputAllowed) { > mixer_->SetMuted( > local_state_->GetInteger(prefs::kAudioMute) == kPrefMuteOn); > } else { > mixer_->SetMuted(true); > mixer_->SetMuteLocked(true); > } > > that way, ApplyAudioPolicy() can be called multiple times. otherwise, i think > you'll hit the NOTREACHED() on the second call if we're locked. Done. http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... File chrome/browser/chromeos/audio/audio_handler.h (right): http://codereview.chromium.org/10873085/diff/23002/chrome/browser/chromeos/au... chrome/browser/chromeos/audio/audio_handler.h:27: class AudioHandler : content::NotificationObserver { On 2012/08/31 08:06:51, Mattias Nissler wrote: > public Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/10873085/32001
Presubmit check for 10873085-32001 failed and returned exit status 1.
Running presubmit commit checks ...
Finished checking
/b/commit-queue/workdir/chromium/chrome/app/policy/policy_templates.json. 0
errors, 0 warnings.
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
chrome/test/functional
Presubmit checks took 1.6s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/10873085/32001
Try job failure for 10873085-32001 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... |
