Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9169033:
Support for showing/hiding status area volume controls in desktop devices (Closed)
|
Created:
8 years, 11 months ago by achuithb Modified:
8 years, 10 months ago CC:
chromium-reviews, sadrul, nkostylev+watch_chromium.org, derat+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, yusukes+watch_chromium.org Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionSupport for showing/hiding status area volume controls in desktop devices.
* Show status area volume control when the command line has --show-volume-status
* Create AudioHandler::VolumeObserver class and helpers so VolumeMenuButton and VolumeControlView can observe volume changes from key presses.
* Move GetAudioHandler() common code in VolumeMenuButton and SystemKeyEventListener that returns the AudioHandler singleton only if the mixer has finished initializing to AudioHandler::GetInitialized().
* Handle mute in VolumeMenuButton correctly.
BUG=chromium-os:22080
TEST=Should see the volume control in the status area on builds with USE flag is_desktop set. The volume control icon and menu should be in sync with volume key button controls.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=119948
Patch Set 1 #Patch Set 2 : lint errors #
Total comments: 9
Patch Set 3 : Xiyuan review feedback #Patch Set 4 : rebase #Patch Set 5 : Add AudioHandler::VolumeObserver #Patch Set 6 : compile errors #Patch Set 7 : move AddVolumeObserver/RemoveVolumeObserver to VolumeObserver #Patch Set 8 : Get rid of SystemKeyEventListener::StatusAreaVolumeObserver #Patch Set 9 : minor fixes #
Total comments: 24
Patch Set 10 : Dan and Xiyuan review feedback #Patch Set 11 : minor #
Total comments: 2
Patch Set 12 : move IsMixerInitialized #
Total comments: 13
Patch Set 13 : stevenjb feedback #Patch Set 14 : minor #
Total comments: 4
Patch Set 15 : derat nits #Patch Set 16 : rebase #Messages
Total messages: 29 (0 generated)
This CL is dependent on https://gerrit.chromium.org/gerrit/#change,14745 which sets the --show-volume-status command line on desktop devices. This CL enables the status area volume control on all desktop devices (basically whenever is_desktop USE flag is set). There's an additional change which is required - to hide the status area volume control when we detect an official keyboard with volume controls, but I'm going to tackle that in a separate CL. Thanks!
http://codereview.chromium.org/9169033/diff/4002/chrome/browser/chromeos/syst... File chrome/browser/chromeos/system_key_event_listener.cc (right): http://codereview.chromium.org/9169033/diff/4002/chrome/browser/chromeos/syst... chrome/browser/chromeos/system_key_event_listener.cc:179: ShowStatusAreaVolume(show_status_area_volume_); I don't think this should be here. Could we do something like this instead: - Expose an accessor show_status_area_volume() in SystemKeyEventListener's interface - VolumeMenuButton's ctor based on the value of show_status_area_volume to decide initial visibility state of itself and then AddStatusAreaVolumeObserver http://codereview.chromium.org/9169033/diff/4002/chrome/browser/chromeos/syst... chrome/browser/chromeos/system_key_event_listener.cc:233: return; Would those system volume keys still work after this change? Or there would be no such keys for on a desktop system? http://codereview.chromium.org/9169033/diff/4002/chrome/browser/chromeos/syst... File chrome/browser/chromeos/system_key_event_listener.h (right): http://codereview.chromium.org/9169033/diff/4002/chrome/browser/chromeos/syst... chrome/browser/chromeos/system_key_event_listener.h:116: bool show_status_area_volume_; Add a comment that this controls whether or not to show volume button in status area and is mutually exclusive with VolumeBubble. http://codereview.chromium.org/9169033/diff/4002/chrome/common/chrome_switches.h File chrome/common/chrome_switches.h (right): http://codereview.chromium.org/9169033/diff/4002/chrome/common/chrome_switche... chrome/common/chrome_switches.h:292: extern const char kShowVolumeStatus[]; Should this go with other chromeos only switches?
Thanks for the feedback. PTAL. http://codereview.chromium.org/9169033/diff/4002/chrome/browser/chromeos/syst... File chrome/browser/chromeos/system_key_event_listener.cc (right): http://codereview.chromium.org/9169033/diff/4002/chrome/browser/chromeos/syst... chrome/browser/chromeos/system_key_event_listener.cc:179: ShowStatusAreaVolume(show_status_area_volume_); On 2012/01/25 04:43:12, xiyuan wrote: > I don't think this should be here. > > Could we do something like this instead: > - Expose an accessor show_status_area_volume() in SystemKeyEventListener's > interface > - VolumeMenuButton's ctor based on the value of show_status_area_volume to > decide initial visibility state of itself and then AddStatusAreaVolumeObserver Done. http://codereview.chromium.org/9169033/diff/4002/chrome/browser/chromeos/syst... chrome/browser/chromeos/system_key_event_listener.cc:233: return; On 2012/01/25 04:43:12, xiyuan wrote: > Would those system volume keys still work after this change? Or there would be > no such keys for on a desktop system? The keys would no longer work. The idea is that there is only one way to control the volume - either the F8-F10 keys, or the VolumeMenuButton. If the VolumeMenuButton is visible, we don't want to use the keyboard volume keys. I'll add a comment to this effect. http://codereview.chromium.org/9169033/diff/4002/chrome/browser/chromeos/syst... File chrome/browser/chromeos/system_key_event_listener.h (right): http://codereview.chromium.org/9169033/diff/4002/chrome/browser/chromeos/syst... chrome/browser/chromeos/system_key_event_listener.h:116: bool show_status_area_volume_; On 2012/01/25 04:43:12, xiyuan wrote: > Add a comment that this controls whether or not to show volume button in status > area and is mutually exclusive with VolumeBubble. Done. http://codereview.chromium.org/9169033/diff/4002/chrome/common/chrome_switches.h File chrome/common/chrome_switches.h (right): http://codereview.chromium.org/9169033/diff/4002/chrome/common/chrome_switche... chrome/common/chrome_switches.h:292: extern const char kShowVolumeStatus[]; On 2012/01/25 04:43:12, xiyuan wrote: > Should this go with other chromeos only switches? Done.
Looks like the rebase messed the diffs up, sorry :(
http://codereview.chromium.org/9169033/diff/4002/chrome/browser/chromeos/syst... File chrome/browser/chromeos/system_key_event_listener.cc (right): http://codereview.chromium.org/9169033/diff/4002/chrome/browser/chromeos/syst... chrome/browser/chromeos/system_key_event_listener.cc:233: return; On 2012/01/25 10:46:06, achuith.bhandarkar wrote: > On 2012/01/25 04:43:12, xiyuan wrote: > > Would those system volume keys still work after this change? Or there would be > > no such keys for on a desktop system? > > The keys would no longer work. The idea is that there is only one way to control > the volume - either the F8-F10 keys, or the VolumeMenuButton. If the > VolumeMenuButton is visible, we don't want to use the keyboard volume keys. I'll > add a comment to this effect. What's the motivation for making the keys not control the volume? Do you plan to disable Chrome OS functionality for all of the function keys when using non-standard keyboards and send F1-12 to webpages instead? This may be a difficult change to do.
On 2012/01/25 15:58:50, Daniel Erat wrote: > http://codereview.chromium.org/9169033/diff/4002/chrome/browser/chromeos/syst... > File chrome/browser/chromeos/system_key_event_listener.cc (right): > > http://codereview.chromium.org/9169033/diff/4002/chrome/browser/chromeos/syst... > chrome/browser/chromeos/system_key_event_listener.cc:233: return; > On 2012/01/25 10:46:06, achuith.bhandarkar wrote: > > On 2012/01/25 04:43:12, xiyuan wrote: > > > Would those system volume keys still work after this change? Or there would > be > > > no such keys for on a desktop system? > > > > The keys would no longer work. The idea is that there is only one way to > control > > the volume - either the F8-F10 keys, or the VolumeMenuButton. If the > > VolumeMenuButton is visible, we don't want to use the keyboard volume keys. > I'll > > add a comment to this effect. > > What's the motivation for making the keys not control the volume? Do you plan > to disable Chrome OS functionality for all of the function keys when using > non-standard keyboards and send F1-12 to webpages instead? This may be a > difficult change to do. I have concern on this too. It would be strange that the keys are not working. Ideally, we should make the keys work with the status area volume button too. I would suggest we only disable the VolumeBubble but still keep the keys usable. For example, we can add more functions to StatusAreaVolumeObserver to wire the mute, volume up/down system key event to volume button.
On 2012/01/25 17:54:40, xiyuan wrote: > On 2012/01/25 15:58:50, Daniel Erat wrote: > > > http://codereview.chromium.org/9169033/diff/4002/chrome/browser/chromeos/syst... > > File chrome/browser/chromeos/system_key_event_listener.cc (right): > > > > > http://codereview.chromium.org/9169033/diff/4002/chrome/browser/chromeos/syst... > > chrome/browser/chromeos/system_key_event_listener.cc:233: return; > > On 2012/01/25 10:46:06, achuith.bhandarkar wrote: > > > On 2012/01/25 04:43:12, xiyuan wrote: > > > > Would those system volume keys still work after this change? Or there > would > > be > > > > no such keys for on a desktop system? > > > > > > The keys would no longer work. The idea is that there is only one way to > > control > > > the volume - either the F8-F10 keys, or the VolumeMenuButton. If the > > > VolumeMenuButton is visible, we don't want to use the keyboard volume keys. > > I'll > > > add a comment to this effect. > > > > What's the motivation for making the keys not control the volume? Do you plan > > to disable Chrome OS functionality for all of the function keys when using > > non-standard keyboards and send F1-12 to webpages instead? This may be a > > difficult change to do. > > I have concern on this too. It would be strange that the keys are not working. > Ideally, we should make the keys work with the status area volume button too. > > I would suggest we only disable the VolumeBubble but still keep the keys usable. > For example, we can add more functions to StatusAreaVolumeObserver to wire the > mute, volume up/down system key event to volume button. Yup, I agree with you. The idea from the PMs and UX folks was that they wanted one way of controlling the volume, so they wanted to disable the volume keys. But you're right - it is inconsistent since the rest of the F 1-7 keys continue to function. Also, figuring out that an official chromeos keyboard is connected in a future-proof manner is quite difficult. I talked to Kuscher, and he agreed that it would be ok to keep the F8-F10 keys alive. I'll rework this CL. Thanks for the feedback!
I've reworked the patch. Daniel and Xiyuan, PTAL. Thanks!
http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.cc:69: VolumeChanged(); nit: unless you anticipate adding more code to VolumeChanged() in the near future, please just remove it and call FOR_EACH_OBSERVER() directly here http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.cc:124: void AudioHandler::AddVolumeObserver(VolumeObserver* observer) { please match the order from the header file http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.cc:137: if (AudioHandler::GetInstance()) // Mixer may be uninitialized here. so after instantiating an object derived from VolumeObserver, it may or may not have automatically registered itself in the AudioHandler singleton depending on when it was created, and if it didn't, i need to call AddVolumeObserver() myself at some point in the future? please remove this and make the implementation responsible for registering and unregistering itself. http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... File chrome/browser/chromeos/audio/audio_handler.h (right): http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.h:10: #include "base/memory/scoped_ptr.h" add base/observer_list.h http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.h:40: static AudioHandler* GetInitialized(); chrome os code includes too many static methods. remove this, please. http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.h:57: // Adds/Removes a VolumeObserver. nit: remove unnecessary comment http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/stat... File chrome/browser/chromeos/status/volume_menu_button.cc (right): http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/stat... chrome/browser/chromeos/status/volume_menu_button.cc:35: bool ShowStatusAreaVolume() { rename to "ShouldShowStatusAreaVolume"; otherwise the name makes it sound like calling this will show the volume http://codereview.chromium.org/9169033/diff/5020/chrome/common/chrome_switche... File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/9169033/diff/5020/chrome/common/chrome_switche... chrome/common/chrome_switches.cc:1253: const char kShowStatusAreaVolume[] = "show-volume-status"; any reason to not make the variable name match the string, i.e. kShowVolumeStatus?
http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.cc:69: VolumeChanged(); nit: If you want to keep this function, please rename it to NotifyVolumeChanged to be consistent with other code. http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... File chrome/browser/chromeos/audio/audio_handler.h (right): http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.h:23: VolumeObserver(); nit: move this into protected as we don't expect people to use this class directly. http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.h:40: static AudioHandler* GetInitialized(); On 2012/01/26 17:55:21, Daniel Erat wrote: > chrome os code includes too many static methods. remove this, please. Yeah, let's reuse the GetInstance() above.
I'll fix the other issues you brought up, but please let me know your thoughts on the below. http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.cc:137: if (AudioHandler::GetInstance()) // Mixer may be uninitialized here. On 2012/01/26 17:55:21, Daniel Erat wrote: > so after instantiating an object derived from VolumeObserver, it may or may not > have automatically registered itself in the AudioHandler singleton depending on > when it was created, and if it didn't, i need to call AddVolumeObserver() myself > at some point in the future? please remove this and make the implementation > responsible for registering and unregistering itself. The comment refers to the fact that we are using GetInstance and not GetInitialized. Even if the audio mixer is not initialized (which it is not when VolumeMenuButton is constructed), we still want to add ourselves as an observer. In all other uses of AudioHandler in this file, we only want AudioHandler* that has a properly initialized mixer (ie, result of GetAudioHandler or AudioHandler::GetInitialized()). The check for GetInstance() is a guard for chromeos-chrome on linux, where AudioHandler is not initialized. If GetInstance() is false here, it's not going to be true in the future. I had it the way you want it (register/deregister done in the ctors/dtors of VolumeMenuButton and VolumeControView), and I think this way is better. It makes more sense to register ourselves in the ctor of the Observer, than in the ctor of the class that derives from it. This reduces boiler-plate cut n paste in all the derived classes as well as unnecessary clutter in the ctor/dtor of those classes. VolumeMenuButton/VolumeControView just care about doing something useful in the OnVolumeChanged callback. http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... File chrome/browser/chromeos/audio/audio_handler.h (right): http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.h:40: static AudioHandler* GetInitialized(); On 2012/01/26 18:07:10, xiyuan wrote: > On 2012/01/26 17:55:21, Daniel Erat wrote: > > chrome os code includes too many static methods. remove this, please. > > Yeah, let's reuse the GetInstance() above. Both SystemKeyEventListener and VolumeMenuBotton have need for a static/anonymous function of the nature: AudioHandler* GetAudioHandler() { return AudioHandler::GetInstance() && AudioHandler::GetInstance()->IsMixerInitialized() ? AudioHandler::GetInstance() : NULL; } GetInstance() does not suffice if you want to read/write volume, since we have to wait for the mixer to be initialized as well. I was trying to avoid code duplication by putting this here. So your preference would be to have the two copies of the anonymous namespace functions in the cc files instead?
http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... File chrome/browser/chromeos/audio/audio_handler.h (right): http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.h:40: static AudioHandler* GetInitialized(); On 2012/01/26 23:19:34, achuith.bhandarkar wrote: > On 2012/01/26 18:07:10, xiyuan wrote: > > On 2012/01/26 17:55:21, Daniel Erat wrote: > > > chrome os code includes too many static methods. remove this, please. > > > > Yeah, let's reuse the GetInstance() above. > > Both SystemKeyEventListener and VolumeMenuBotton have need for a > static/anonymous function of the nature: > AudioHandler* GetAudioHandler() { > return AudioHandler::GetInstance() && > AudioHandler::GetInstance()->IsMixerInitialized() ? AudioHandler::GetInstance() > : NULL; > } > > GetInstance() does not suffice if you want to read/write volume, since we have > to wait for the mixer to be initialized as well. > > I was trying to avoid code duplication by putting this here. So your preference > would be to have the two copies of the anonymous namespace functions in the cc > files instead? Okay. In this case, I am okay to have it here so that the logic could be shared.
http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.cc:137: if (AudioHandler::GetInstance()) // Mixer may be uninitialized here. On 2012/01/26 23:19:34, achuith.bhandarkar wrote: > On 2012/01/26 17:55:21, Daniel Erat wrote: > > so after instantiating an object derived from VolumeObserver, it may or may > not > > have automatically registered itself in the AudioHandler singleton depending > on > > when it was created, and if it didn't, i need to call AddVolumeObserver() > myself > > at some point in the future? please remove this and make the implementation > > responsible for registering and unregistering itself. > > The comment refers to the fact that we are using GetInstance and not > GetInitialized. Even if the audio mixer is not initialized (which it is not when > VolumeMenuButton is constructed), we still want to add ourselves as an observer. > In all other uses of AudioHandler in this file, we only want AudioHandler* that > has a properly initialized mixer (ie, result of GetAudioHandler or > AudioHandler::GetInitialized()). My comment wasn't related to the comment in the code; it was related to the fact that after instantiating a VolumeObserver, it may or may not be registered depending on whether the AudioHandler singleton has been created or not. > The check for GetInstance() is a guard for chromeos-chrome on linux, where > AudioHandler is not initialized. If GetInstance() is false here, it's not going > to be true in the future. VolumeObservers get instantiated on platforms where we don't instantiate AudioHandler? That seems wrong. > I had it the way you want it (register/deregister done in the ctors/dtors of > VolumeMenuButton and VolumeControView), and I think this way is better. > > It makes more sense to register ourselves in the ctor of the Observer, than in > the ctor of the class that derives from it. This reduces boiler-plate cut n > paste in all the derived classes as well as unnecessary clutter in the ctor/dtor > of those classes. VolumeMenuButton/VolumeControView just care about doing > something useful in the OnVolumeChanged callback. If you want to do the registration automatically, you should: - add a DCHECK(AudioHandler::GetInstance()) - make AddVolumeObserver() and RemoveVolumeObserver() be private methods so users can't double-register - document in the header that registration and unregistration are automatic http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... File chrome/browser/chromeos/audio/audio_handler.h (right): http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.h:40: static AudioHandler* GetInitialized(); On 2012/01/26 23:19:34, achuith.bhandarkar wrote: > On 2012/01/26 18:07:10, xiyuan wrote: > > On 2012/01/26 17:55:21, Daniel Erat wrote: > > > chrome os code includes too many static methods. remove this, please. > > > > Yeah, let's reuse the GetInstance() above. > > Both SystemKeyEventListener and VolumeMenuBotton have need for a > static/anonymous function of the nature: > AudioHandler* GetAudioHandler() { > return AudioHandler::GetInstance() && > AudioHandler::GetInstance()->IsMixerInitialized() ? AudioHandler::GetInstance() > : NULL; > } > > GetInstance() does not suffice if you want to read/write volume, since we have > to wait for the mixer to be initialized as well. > > I was trying to avoid code duplication by putting this here. So your preference > would be to have the two copies of the anonymous namespace functions in the cc > files instead? Well, my preference would be for the above TODO to be fixed so that AudioHandler is usable immediately. :-/ In the meantime, I'm okay with this if you rename it to GetInstanceIfInitialized() or GetInstanceIfReady() or something along those lines.
http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.cc:137: if (AudioHandler::GetInstance()) // Mixer may be uninitialized here. On 2012/01/26 23:53:28, Daniel Erat wrote: > On 2012/01/26 23:19:34, achuith.bhandarkar wrote: > > On 2012/01/26 17:55:21, Daniel Erat wrote: > > > so after instantiating an object derived from VolumeObserver, it may or may > > not > > > have automatically registered itself in the AudioHandler singleton depending > > on > > > when it was created, and if it didn't, i need to call AddVolumeObserver() > > myself > > > at some point in the future? please remove this and make the implementation > > > responsible for registering and unregistering itself. > > > > The comment refers to the fact that we are using GetInstance and not > > GetInitialized. Even if the audio mixer is not initialized (which it is not > when > > VolumeMenuButton is constructed), we still want to add ourselves as an > observer. > > In all other uses of AudioHandler in this file, we only want AudioHandler* > that > > has a properly initialized mixer (ie, result of GetAudioHandler or > > AudioHandler::GetInitialized()). > > My comment wasn't related to the comment in the code; it was related to the fact > that after instantiating a VolumeObserver, it may or may not be registered > depending on whether the AudioHandler singleton has been created or not. We create the AudioHandler singleton before the status area, and the code does not try to handle a re-ordering of the initialization sequence of these two components. I'll add a DCHECK like: DCHECK(AudioHandler::GetInstance() || !system::runtime_environment::IsRunningOnChromeOS()); FWIW, caps_lock_menu_button.cc has a similar situation - SystemKeyEventListener is not initialized on chromeos-chrome for linux and the initialization dependency is guarded by a similar DCHECK. > > The check for GetInstance() is a guard for chromeos-chrome on linux, where > > AudioHandler is not initialized. If GetInstance() is false here, it's not > going > > to be true in the future. > > VolumeObservers get instantiated on platforms where we don't instantiate > AudioHandler? That seems wrong. The status area volume icon VolumeMenuButton derives from VolumeObserver, and gets created on chromeos-chrome on linux. It's been useful for testing to have it available on linux... > > I had it the way you want it (register/deregister done in the ctors/dtors of > > VolumeMenuButton and VolumeControView), and I think this way is better. > > > > It makes more sense to register ourselves in the ctor of the Observer, than in > > the ctor of the class that derives from it. This reduces boiler-plate cut n > > paste in all the derived classes as well as unnecessary clutter in the > ctor/dtor > > of those classes. VolumeMenuButton/VolumeControView just care about doing > > something useful in the OnVolumeChanged callback. > > If you want to do the registration automatically, you should: > - add a DCHECK(AudioHandler::GetInstance()) Will do. The DCHECK will assert that we're either not on the device, or the audiohandler instance exists, as noted earlier. > - make AddVolumeObserver() and RemoveVolumeObserver() be private methods so > users can't double-register I thought of doing this - it would require me to make VolumeObserver a friend of AudioHandler so it could access those private methods. I can do this, but I was under the impression that friend classes were generally frowned on. > - document in the header that registration and unregistration are automatic will do.
Daniel, Xiyuan: Thanks for the feedback, PTAL! http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.cc:69: VolumeChanged(); On 2012/01/26 17:55:21, Daniel Erat wrote: > nit: unless you anticipate adding more code to VolumeChanged() in the near > future, please just remove it and call FOR_EACH_OBSERVER() directly here Done. http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.cc:124: void AudioHandler::AddVolumeObserver(VolumeObserver* observer) { On 2012/01/26 17:55:21, Daniel Erat wrote: > please match the order from the header file Done. These files have been moved to the bottom in the header file. http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... File chrome/browser/chromeos/audio/audio_handler.h (right): http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.h:10: #include "base/memory/scoped_ptr.h" On 2012/01/26 17:55:21, Daniel Erat wrote: > add base/observer_list.h Done. http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.h:23: VolumeObserver(); On 2012/01/26 18:07:10, xiyuan wrote: > nit: move this into protected as we don't expect people to use this class > directly. Done. http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/audi... chrome/browser/chromeos/audio/audio_handler.h:40: static AudioHandler* GetInitialized(); On 2012/01/26 23:53:28, Daniel Erat wrote: > On 2012/01/26 23:19:34, achuith.bhandarkar wrote: > > On 2012/01/26 18:07:10, xiyuan wrote: > > > On 2012/01/26 17:55:21, Daniel Erat wrote: > > > > chrome os code includes too many static methods. remove this, please. > > > > > > Yeah, let's reuse the GetInstance() above. > > > > Both SystemKeyEventListener and VolumeMenuBotton have need for a > > static/anonymous function of the nature: > > AudioHandler* GetAudioHandler() { > > return AudioHandler::GetInstance() && > > AudioHandler::GetInstance()->IsMixerInitialized() ? > AudioHandler::GetInstance() > > : NULL; > > } > > > > GetInstance() does not suffice if you want to read/write volume, since we have > > to wait for the mixer to be initialized as well. > > > > I was trying to avoid code duplication by putting this here. So your > preference > > would be to have the two copies of the anonymous namespace functions in the cc > > files instead? > > Well, my preference would be for the above TODO to be fixed so that AudioHandler > is usable immediately. :-/ > > In the meantime, I'm okay with this if you rename it to > GetInstanceIfInitialized() or GetInstanceIfReady() or something along those > lines. Done. http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/stat... File chrome/browser/chromeos/status/volume_menu_button.cc (right): http://codereview.chromium.org/9169033/diff/5020/chrome/browser/chromeos/stat... chrome/browser/chromeos/status/volume_menu_button.cc:35: bool ShowStatusAreaVolume() { On 2012/01/26 17:55:21, Daniel Erat wrote: > rename to "ShouldShowStatusAreaVolume"; otherwise the name makes it sound like > calling this will show the volume Done. http://codereview.chromium.org/9169033/diff/5020/chrome/common/chrome_switche... File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/9169033/diff/5020/chrome/common/chrome_switche... chrome/common/chrome_switches.cc:1253: const char kShowStatusAreaVolume[] = "show-volume-status"; On 2012/01/26 17:55:21, Daniel Erat wrote: > any reason to not make the variable name match the string, i.e. > kShowVolumeStatus? Done.
Adding Steven for owners once-over for chromeos/status/* Dan: You should probably be owner for chromeos/audio/*, no?
On 2012/01/27 01:51:05, achuith.bhandarkar wrote: > Adding Steven for owners once-over for chromeos/status/* > > Dan: You should probably be owner for chromeos/audio/*, no? Apologies for the rebase...
LGTM with one nit http://codereview.chromium.org/9169033/diff/10004/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/9169033/diff/10004/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_handler.cc:53: bool AudioHandler::IsMixerInitialized() { nit: move this to after AudioHandler::~AudioHandler since we have moved it in header.
http://codereview.chromium.org/9169033/diff/10004/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/9169033/diff/10004/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_handler.cc:53: bool AudioHandler::IsMixerInitialized() { On 2012/01/27 03:30:29, xiyuan wrote: > nit: move this to after AudioHandler::~AudioHandler since we have moved it in > header. Done.
When the VolumeMenuButton is initialized before the mixer is initialized, how do you get the correct volume later? It seems to me like you might need to send an OnVolumeChanged() notification when the mixer is first initialized; otherwise (unless I'm missing something) I don't see how the button gets the correct volume until the first time that the user changes it. (At this point, it might be easier to just try to resolve the TODO to make AudioMixer's public interface deal with percentages instead of decibels, so that AudioHandler can be used to set and get the volume even before ALSA is ready. I can try to work on this today, although I don't know that it'll get in before R18, if that's an issue.) http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_handler.cc:138: if (AudioHandler::GetInstance()) remove the if statement; you already have a DCHECK() http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_handler.h (right): http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_handler.h:34: // GetInstance returns NULL if not initialized or if already shutdown. not your fault, but mind updating this comment to clarify that it returns NULL if the AudioHandler hasn't been initialized, and that the mixer may still not be initialized at this point? http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/sta... File chrome/browser/chromeos/status/volume_menu_button.cc (right): http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/sta... chrome/browser/chromeos/status/volume_menu_button.cc:29: static const int kMenuItemId = 100; // arbitrary menu id. nit: don't need static on globals that are in an anonymous namespace
On 2012/01/27 18:03:57, Daniel Erat wrote: > When the VolumeMenuButton is initialized before the mixer is initialized, how do > you get the correct volume later? It seems to me like you might need to send an > OnVolumeChanged() notification when the mixer is first initialized; otherwise > (unless I'm missing something) I don't see how the button gets the correct > volume until the first time that the user changes it. > > (At this point, it might be easier to just try to resolve the TODO to make > AudioMixer's public interface deal with percentages instead of decibels, so that > AudioHandler can be used to set and get the volume even before ALSA is ready. I > can try to work on this today, although I don't know that it'll get in before > R18, if that's an issue.) Yup, you are right - there is an issue at startup where the volume button displays the wrong status (it defaults to mute). We need to fix this in one of the ways you suggest - by either having the mixer call an AudioHandler callback like AudioHandler::MixerInitialized (posting a task to the UI thread after the mixer connection succeeds), or by resolving the TODO. I'd prefer to not gate this CL on getting this startup bug resolved. I feel like we could patch the fix into R18 early next week? Wdyt? This feature is needed for stumpy (which will auto-update to R18). Without this CL there's no discoverable way to control the volume on a stumpy connected to a generic keyboard.
http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_handler.cc:138: if (AudioHandler::GetInstance()) On 2012/01/27 18:03:57, Daniel Erat wrote: > remove the if statement; you already have a DCHECK() AudioHandler::GetInstance() is NULL on chrome-chromeos for linux. I need the NULL check (or I could alternately use IsRunningOnChromeOS, but this is clearer).
http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_handler.cc:138: if (AudioHandler::GetInstance()) On 2012/01/27 18:53:44, achuith.bhandarkar wrote: > On 2012/01/27 18:03:57, Daniel Erat wrote: > > remove the if statement; you already have a DCHECK() > > AudioHandler::GetInstance() is NULL on chrome-chromeos for linux. I need the > NULL check (or I could alternately use IsRunningOnChromeOS, but this is > clearer). I'm not sure that this is clearer; how about at the top of the file: if (!system::runtime_environment::IsRunningOnChromeOS()) return; Or, if it's possible that AudioHandler::GetInstance() might be true on non chromeos: if (!system::runtime_environment::IsRunningOnChromeOS() && !AudioHandler::GetInstance()) return; This way it's clear that when not running on chromeos, we expect that the audio handler may not be initialized and just want to exit. Then we do the DCHECK and call AddVolimeObserver unconditionally. http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/sta... File chrome/browser/chromeos/status/volume_menu_button.cc (right): http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/sta... chrome/browser/chromeos/status/volume_menu_button.cc:85: public AudioHandler::VolumeObserver { Technically this is dual inheritance; in particular this will have two constructors. It doesn't look like this needs to be a VolumeObserver; all that OnVolumeChanged() does is call SchedulePaint; since VolumeControlView is a non-widget child of VolumeMenuButton, it seems it would just end up calling SchedulePaintInRect() on its parent anyway, so can't we just have VolumeMenuButton call ScedulePaint() in its observer? http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_key_event_listener.h (right): http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_key_event_listener.h:32: virtual ~CapsLockObserver() {} DISALLOW_COPY_AND_ASSIGN
http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_handler.cc:138: if (AudioHandler::GetInstance()) On 2012/01/27 19:45:02, stevenjb (chromium) wrote: > On 2012/01/27 18:53:44, achuith.bhandarkar wrote: > > On 2012/01/27 18:03:57, Daniel Erat wrote: > > > remove the if statement; you already have a DCHECK() > > > > AudioHandler::GetInstance() is NULL on chrome-chromeos for linux. I need the > > NULL check (or I could alternately use IsRunningOnChromeOS, but this is > > clearer). > I'm not sure that this is clearer; how about at the top of the file: > if (!system::runtime_environment::IsRunningOnChromeOS()) > return; > Or, if it's possible that AudioHandler::GetInstance() might be true on non > chromeos: > if (!system::runtime_environment::IsRunningOnChromeOS() && > !AudioHandler::GetInstance()) > return; > This way it's clear that when not running on chromeos, we expect that the audio > handler may not be initialized and just want to exit. Then we do the DCHECK and > call AddVolimeObserver unconditionally. yeah, i'd also prefer: if (system::runtime_environment::IsRunningOnChromeOS() { DCHECK(AudioHandler::GetInstance()); AudioHandler::GetInstance()->AddVolumeObserver(this); } DCHECK() is more-or-less optional at this point, since we're going to crash on a null deref otherwise. the code in the d'tor should be similar.
Addressed review feedback. PTAL - Steven and Daniel. Sorry about the rebase. You can compare with patchset 12 for changes since your last review. Thanks! http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_handler.cc (right): http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_handler.cc:138: if (AudioHandler::GetInstance()) On 2012/01/27 19:45:02, stevenjb (chromium) wrote: > On 2012/01/27 18:53:44, achuith.bhandarkar wrote: > > On 2012/01/27 18:03:57, Daniel Erat wrote: > > > remove the if statement; you already have a DCHECK() > > > > AudioHandler::GetInstance() is NULL on chrome-chromeos for linux. I need the > > NULL check (or I could alternately use IsRunningOnChromeOS, but this is > > clearer). > I'm not sure that this is clearer; how about at the top of the file: > if (!system::runtime_environment::IsRunningOnChromeOS()) > return; > Or, if it's possible that AudioHandler::GetInstance() might be true on non > chromeos: > if (!system::runtime_environment::IsRunningOnChromeOS() && > !AudioHandler::GetInstance()) > return; > This way it's clear that when not running on chromeos, we expect that the audio > handler may not be initialized and just want to exit. Then we do the DCHECK and > call AddVolimeObserver unconditionally. Well, I was following the pattern in: http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/sta... Steven - since you wrote that code, I'd like to understand why your opinion has changed? I like the way it's written and I copied it exactly here. If we add a mock audio handler for chromeos-chrome for linux (something that we should do), testing for runtime_environment will no longer be correct. Null-checking a pointer that could be NULL will always be correct. I'll switch this to using the runtime_environment check for now, using Dan's arrangement (leaving out the DCHECK since we're about to crash anyway). http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio/audio_handler.h (right): http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio/audio_handler.h:34: // GetInstance returns NULL if not initialized or if already shutdown. On 2012/01/27 18:03:57, Daniel Erat wrote: > not your fault, but mind updating this comment to clarify that it returns NULL > if the AudioHandler hasn't been initialized, and that the mixer may still not be > initialized at this point? Done. http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/sta... File chrome/browser/chromeos/status/volume_menu_button.cc (right): http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/sta... chrome/browser/chromeos/status/volume_menu_button.cc:29: static const int kMenuItemId = 100; // arbitrary menu id. On 2012/01/27 18:03:57, Daniel Erat wrote: > nit: don't need static on globals that are in an anonymous namespace Done. http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/sta... chrome/browser/chromeos/status/volume_menu_button.cc:85: public AudioHandler::VolumeObserver { On 2012/01/27 19:45:02, stevenjb (chromium) wrote: > Technically this is dual inheritance; in particular this will have two > constructors. > > It doesn't look like this needs to be a VolumeObserver; all that > OnVolumeChanged() does is call SchedulePaint; since VolumeControlView is a > non-widget child of VolumeMenuButton, it seems it would just end up calling > SchedulePaintInRect() on its parent anyway, so can't we just have > VolumeMenuButton call ScedulePaint() in its observer? I'll reverse my decision to put code in the ctor of VolumeObserver. SchedulePaint in VolumeMenuButton::OnVolumeChanged() doesn't repaint VolumeControlView, and I don't know enough about our Views subsystem to know why. http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system_key_event_listener.h (right): http://codereview.chromium.org/9169033/diff/12004/chrome/browser/chromeos/sys... chrome/browser/chromeos/system_key_event_listener.h:32: virtual ~CapsLockObserver() {} On 2012/01/27 19:45:02, stevenjb (chromium) wrote: > DISALLOW_COPY_AND_ASSIGN Done.
lgtm http://codereview.chromium.org/9169033/diff/21001/chrome/browser/chromeos/sta... File chrome/browser/chromeos/status/volume_menu_button.cc (right): http://codereview.chromium.org/9169033/diff/21001/chrome/browser/chromeos/sta... chrome/browser/chromeos/status/volume_menu_button.cc:99: nit: delete blank line http://codereview.chromium.org/9169033/diff/21001/chrome/browser/chromeos/sta... chrome/browser/chromeos/status/volume_menu_button.cc:112: VolumeMenuButton* volume_menu_button_; nit: add "// not owned" to the end of the line if this is owned by someone else
Thanks for the review, Dan! Waiting for owner lgtm from Steven... http://codereview.chromium.org/9169033/diff/21001/chrome/browser/chromeos/sta... File chrome/browser/chromeos/status/volume_menu_button.cc (right): http://codereview.chromium.org/9169033/diff/21001/chrome/browser/chromeos/sta... chrome/browser/chromeos/status/volume_menu_button.cc:99: On 2012/01/31 01:06:02, Daniel Erat wrote: > nit: delete blank line Done. http://codereview.chromium.org/9169033/diff/21001/chrome/browser/chromeos/sta... chrome/browser/chromeos/status/volume_menu_button.cc:112: VolumeMenuButton* volume_menu_button_; On 2012/01/31 01:06:02, Daniel Erat wrote: > nit: add "// not owned" to the end of the line if this is owned by someone else Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/achuith@chromium.org/9169033/25001 |
