|
|
Created:
4 years, 4 months ago by Qiang(Joe) Xu Modified:
4 years, 4 months ago CC:
chromium-reviews, kalyank, sadrul, sky Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix Volume slider is captured in screenshot done in touchview mode
Solution:
1. User holds volume-down key.
2. Remember the non-repeated pressed volume.
3. Immediately start reducing volume.
4. If the power key is tapped,
4a TrayAudio hides the volume slider.
4b Taking the screenshot.
4c Restore the volume (do not notify UI)
BUG=628479
TEST=tested on veyron_minnie touchview mode and it works fine. The only defect I can image is taking screenshot when the device is playing audio. If volume-down is presssed for a long time, you can feel audio volume is firstly decreased and then restored back.
Committed: https://crrev.com/fd7c37b98e7c6eca718257916ab65409ee7b2c1a
Cr-Commit-Position: refs/heads/master@{#410145}
Patch Set 1 #
Total comments: 8
Patch Set 2 : remove OnDidTakeScreenshot; clean code #
Total comments: 12
Patch Set 3 : disable hiding animation; restoring volume does not notify UI #
Total comments: 16
Patch Set 4 : based on cr #Patch Set 5 #Patch Set 6 : not using default parameter #
Total comments: 22
Patch Set 7 : based on Daniel's comments #
Total comments: 10
Patch Set 8 : based on new cr from Daniel #
Total comments: 7
Patch Set 9 : several updates included #
Total comments: 14
Patch Set 10 : OutputNodeVolumeChanged #
Total comments: 8
Patch Set 11 : audio handler side change #
Total comments: 2
Patch Set 12 : cras side unittest #
Total comments: 6
Patch Set 13 : Updated unittest which detects dbus signals #
Total comments: 6
Patch Set 14 : initialization audio state unittest #
Total comments: 2
Patch Set 15 : right version #Patch Set 16 : nit #
Total comments: 8
Patch Set 17 : nits #
Messages
Total messages: 86 (36 generated)
Description was changed from ========== Fix Volume slider is captured in screenshot done in touchview mode BUG=628479 ========== to ========== Fix Volume slider is captured in screenshot done in touchview mode Solution: 1. User holds volume-down key 2. Remember the volume when it is first pressed, which means excluding continuous pressing and it should be refreshed every time volume-down is released. 3. Immediately start reducing volume. 4. If the power key is tapped, 4a Notify to hide the volume slider. 4b Wait volume slider to finish hiding (200ms). 4c Taking the screenshot. 4d After screenshot is done, notify restoring the volume got from step 2. BUG=628479 TEST=tested on veyron_minnie touchview mode and it works fine. The only defect I can image is taking screenshot when the device is playing audio. If volume-down is presssed for a long time, you can feel audio volume is firstly decreased and then restored back. ==========
warx@chromium.org changed reviewers: + derat@chromium.org, sky@chromium.org
Hi Daniel, Scott, could you help review on this? tks!
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I'm out until Monday, so you might want to choose another reviewer.
warx@chromium.org changed reviewers: + msw@chromium.org
On 2016/07/27 23:30:05, sky wrote: > I'm out until Monday, so you might want to choose another reviewer. OK. Daniel & Mike, could you help take a look on ash/ & chrome/browser/ui? tks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
i left a high-level comment in system_tray_notifier.h https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/syst... File ash/common/system/tray/system_tray_notifier.h (right): https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/syst... ash/common/system/tray/system_tray_notifier.h:102: void AddMaxModeScreenshotObserver(MaximizeModeScreenshotObserver* observer); s/MaxMode/MaximizeMode/ here and on the next line; "MaxMode" doesn't look like it's currently used anywhere in the code https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/syst... ash/common/system/tray/system_tray_notifier.h:106: void NotifyDidTakeScreenshot(); maybe rename these to: NotifyWillTakeScreenshotInMaximizeMode NotifyDidTakeScreenshotInMaximizeMode ? the current names don't accurately reflect when they should be called. the observer would need to be updated too. actually, i'm wondering if the names should also explain that these are only called for the volume-down+power accelerator in particular. but it feels like this is so special-purpose that it's hard to justify an observer interface. can PowerButtonController call directly into TrayAudio to notify it about things, or is that a layering violation? what about if TrayAudio had a SuppressNotification() method that PBC could call, and then PBC handled saving the original volume and restoring it later all on its own? that seems like it greatly cuts down the complexity of this change. https://codereview.chromium.org/2190773002/diff/1/ash/wm/power_button_control... File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2190773002/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:177: if (!volume_down_first_) { can't you use event->is_repeat() to determine if this is a repeated event?
https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/syst... File ash/common/system/tray/system_tray_notifier.h (right): https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/syst... ash/common/system/tray/system_tray_notifier.h:106: void NotifyDidTakeScreenshot(); On 2016/07/27 23:51:31, Daniel Erat wrote: > maybe rename these to: > > NotifyWillTakeScreenshotInMaximizeMode > NotifyDidTakeScreenshotInMaximizeMode > > ? the current names don't accurately reflect when they should be called. the > observer would need to be updated too. > > actually, i'm wondering if the names should also explain that these are only > called for the volume-down+power accelerator in particular. > > but it feels like this is so special-purpose that it's hard to justify an > observer interface. can PowerButtonController call directly into TrayAudio to > notify it about things, or is that a layering violation? > > what about if TrayAudio had a SuppressNotification() method that PBC could call, > and then PBC handled saving the original volume and restoring it later all on > its own? that seems like it greatly cuts down the complexity of this change. Hi, to get a better understanding, what is SuppressNotification this paragraph meaning? I cannot see a code example in code base. Thanks! https://codereview.chromium.org/2190773002/diff/1/ash/wm/power_button_control... File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2190773002/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:177: if (!volume_down_first_) { On 2016/07/27 23:51:31, Daniel Erat wrote: > can't you use event->is_repeat() to determine if this is a repeated event? Yes. Just find it. I will update it later.
On 2016/07/27 23:51:32, Daniel Erat wrote: > i left a high-level comment in system_tray_notifier.h > > https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/syst... > File ash/common/system/tray/system_tray_notifier.h (right): > > https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/syst... > ash/common/system/tray/system_tray_notifier.h:102: void > AddMaxModeScreenshotObserver(MaximizeModeScreenshotObserver* observer); > s/MaxMode/MaximizeMode/ here and on the next line; "MaxMode" doesn't look like > it's currently used anywhere in the code > > https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/syst... > ash/common/system/tray/system_tray_notifier.h:106: void > NotifyDidTakeScreenshot(); > maybe rename these to: > > NotifyWillTakeScreenshotInMaximizeMode > NotifyDidTakeScreenshotInMaximizeMode > > ? the current names don't accurately reflect when they should be called. the > observer would need to be updated too. > > actually, i'm wondering if the names should also explain that these are only > called for the volume-down+power accelerator in particular. > > but it feels like this is so special-purpose that it's hard to justify an > observer interface. can PowerButtonController call directly into TrayAudio to > notify it about things, or is that a layering violation? > > what about if TrayAudio had a SuppressNotification() method that PBC could call, > and then PBC handled saving the original volume and restoring it later all on > its own? that seems like it greatly cuts down the complexity of this change. > > https://codereview.chromium.org/2190773002/diff/1/ash/wm/power_button_control... > File ash/wm/power_button_controller.cc (right): > > https://codereview.chromium.org/2190773002/diff/1/ash/wm/power_button_control... > ash/wm/power_button_controller.cc:177: if (!volume_down_first_) { > can't you use event->is_repeat() to determine if this is a repeated event? I just find chrome_screenshot_grabber.cc notifier is not needed. And I will also try to move the observer from system_tray_notifier to power_button_controller. Will update it when I am done. Thanks!
https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/syst... File ash/common/system/tray/system_tray_notifier.h (right): https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/syst... ash/common/system/tray/system_tray_notifier.h:106: void NotifyDidTakeScreenshot(); On 2016/07/28 00:15:39, Qiang (Joe) Xu wrote: > On 2016/07/27 23:51:31, Daniel Erat wrote: > > maybe rename these to: > > > > NotifyWillTakeScreenshotInMaximizeMode > > NotifyDidTakeScreenshotInMaximizeMode > > > > ? the current names don't accurately reflect when they should be called. the > > observer would need to be updated too. > > > > actually, i'm wondering if the names should also explain that these are only > > called for the volume-down+power accelerator in particular. > > > > but it feels like this is so special-purpose that it's hard to justify an > > observer interface. can PowerButtonController call directly into TrayAudio to > > notify it about things, or is that a layering violation? > > > > what about if TrayAudio had a SuppressNotification() method that PBC could > call, > > and then PBC handled saving the original volume and restoring it later all on > > its own? that seems like it greatly cuts down the complexity of this change. > > Hi, to get a better understanding, what is SuppressNotification this paragraph > meaning? I cannot see a code example in code base. Thanks! i meant that PBC could call a new TrayAudio method with a signature like: void SuppressNotification(bool suppress); (or maybe HideNotification is clearer) when it wants to hide or unhide notifications.
On 2016/07/28 00:25:56, Daniel Erat wrote: > https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/syst... > File ash/common/system/tray/system_tray_notifier.h (right): > > https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/syst... > ash/common/system/tray/system_tray_notifier.h:106: void > NotifyDidTakeScreenshot(); > On 2016/07/28 00:15:39, Qiang (Joe) Xu wrote: > > On 2016/07/27 23:51:31, Daniel Erat wrote: > > > maybe rename these to: > > > > > > NotifyWillTakeScreenshotInMaximizeMode > > > NotifyDidTakeScreenshotInMaximizeMode > > > > > > ? the current names don't accurately reflect when they should be called. the > > > observer would need to be updated too. > > > > > > actually, i'm wondering if the names should also explain that these are only > > > called for the volume-down+power accelerator in particular. > > > > > > but it feels like this is so special-purpose that it's hard to justify an > > > observer interface. can PowerButtonController call directly into TrayAudio > to > > > notify it about things, or is that a layering violation? > > > > > > what about if TrayAudio had a SuppressNotification() method that PBC could > > call, > > > and then PBC handled saving the original volume and restoring it later all > on > > > its own? that seems like it greatly cuts down the complexity of this change. > > > > Hi, to get a better understanding, what is SuppressNotification this paragraph > > meaning? I cannot see a code example in code base. Thanks! > > i meant that PBC could call a new TrayAudio method with a signature like: > > void SuppressNotification(bool suppress); > > (or maybe HideNotification is clearer) when it wants to hide or unhide > notifications. OK. Let me try it. If that is OK, then it will be largely simplified.
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Fix Volume slider is captured in screenshot done in touchview mode Solution: 1. User holds volume-down key 2. Remember the volume when it is first pressed, which means excluding continuous pressing and it should be refreshed every time volume-down is released. 3. Immediately start reducing volume. 4. If the power key is tapped, 4a Notify to hide the volume slider. 4b Wait volume slider to finish hiding (200ms). 4c Taking the screenshot. 4d After screenshot is done, notify restoring the volume got from step 2. BUG=628479 TEST=tested on veyron_minnie touchview mode and it works fine. The only defect I can image is taking screenshot when the device is playing audio. If volume-down is presssed for a long time, you can feel audio volume is firstly decreased and then restored back. ========== to ========== Fix Volume slider is captured in screenshot done in touchview mode Solution: 1. User holds volume-down key. 2. Remember the non-repeated pressed volume. 3. Immediately start reducing volume. 4. If the power key is tapped, 4a Notify to hide the volume slider. 4b Wait volume slider to finish hiding (200ms). 4c Taking the screenshot. 4d Restore the volume. BUG=628479 TEST=tested on veyron_minnie touchview mode and it works fine. The only defect I can image is taking screenshot when the device is playing audio. If volume-down is presssed for a long time, you can feel audio volume is firstly decreased and then restored back. ==========
Hi Daniel, here is my reply and new patch, thanks for review again. https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/syst... File ash/common/system/tray/system_tray_notifier.h (right): https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/syst... ash/common/system/tray/system_tray_notifier.h:102: void AddMaxModeScreenshotObserver(MaximizeModeScreenshotObserver* observer); On 2016/07/27 23:51:31, Daniel Erat wrote: > s/MaxMode/MaximizeMode/ here and on the next line; "MaxMode" doesn't look like > it's currently used anywhere in the code Done. https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/syst... ash/common/system/tray/system_tray_notifier.h:106: void NotifyDidTakeScreenshot(); On 2016/07/28 00:25:56, Daniel Erat wrote: > On 2016/07/28 00:15:39, Qiang (Joe) Xu wrote: > > On 2016/07/27 23:51:31, Daniel Erat wrote: > > > maybe rename these to: > > > > > > NotifyWillTakeScreenshotInMaximizeMode > > > NotifyDidTakeScreenshotInMaximizeMode > > > > > > ? the current names don't accurately reflect when they should be called. the > > > observer would need to be updated too. > > > > > > actually, i'm wondering if the names should also explain that these are only > > > called for the volume-down+power accelerator in particular. > > > > > > but it feels like this is so special-purpose that it's hard to justify an > > > observer interface. can PowerButtonController call directly into TrayAudio > to > > > notify it about things, or is that a layering violation? > > > > > > what about if TrayAudio had a SuppressNotification() method that PBC could > > call, > > > and then PBC handled saving the original volume and restoring it later all > on > > > its own? that seems like it greatly cuts down the complexity of this change. > > > > Hi, to get a better understanding, what is SuppressNotification this paragraph > > meaning? I cannot see a code example in code base. Thanks! > > i meant that PBC could call a new TrayAudio method with a signature like: > > void SuppressNotification(bool suppress); > > (or maybe HideNotification is clearer) when it wants to hide or unhide > notifications. I still keep MaximizeModeScreenshotObserver in SystemTrayNotifier for these reasons. 1. I cannot figure out a way to call methods of TrayAudio from PBC. 2. |hide_popup_| needs to be set in TrayAudio and modifies OnOutputNodeVolumeChanged method, which makes I believe it still needs the observer. 3. I tried move the observer as a nested class in PBC, however ash::Shell::GetInstance()->power_button_controller()->AddObserver in TrayAudio breaks the DEPS rule. "ash/common" cannot includes "ash/". For 1. SystemTray doesn't expose TrayAudio reference. Could I expose that? https://cs.chromium.org/chromium/src/ash/common/system/tray/system_tray.h?q=s... It only provides the accessors for testing purpose.
https://codereview.chromium.org/2190773002/diff/40001/ash/common/system/audio... File ash/common/system/audio/tray_audio.cc (right): https://codereview.chromium.org/2190773002/diff/40001/ash/common/system/audio... ash/common/system/audio/tray_audio.cc:123: hide_popup_ = false; this approach (hiding the next popup) feels a bit race-prone. it seems like it'd be cleaner if the audio handler instead notified observers about whether the change was user-initiated (in which case the popup should be shown) or automated (in which case it shouldn't). https://codereview.chromium.org/2190773002/diff/40001/ash/wm/power_button_con... File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2190773002/diff/40001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:33: const int kWaitVolumeSliderHiddenInMs = 200; this isn't good; it means there's a delay between when the user requests the screenshot and when we take it. how long does it actually take for the volume slider to be hidden? is there an animation that you're waiting for? if so, can you skip the animation? or is the problem that you're waiting for the screen to be redrawn? https://codereview.chromium.org/2190773002/diff/40001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:107: DCHECK(audio_handler->IsInitialized()); you don't need this; Get() on the previous line already has a CHECK() that it's initialized https://codereview.chromium.org/2190773002/diff/40001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:181: // Avoid saving to |saved_volume_percent_| if that is repeated pressing. nit: delete this comment; the code is self-explanatory https://codereview.chromium.org/2190773002/diff/40001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:185: DCHECK(audio_handler->IsInitialized()); delete this too https://codereview.chromium.org/2190773002/diff/40001/ash/wm/power_button_con... File ash/wm/power_button_controller.h (right): https://codereview.chromium.org/2190773002/diff/40001/ash/wm/power_button_con... ash/wm/power_button_controller.h:83: void TakingScreenshot(); rename to TakeScreenshot and add a comment describing how this is used https://codereview.chromium.org/2190773002/diff/40001/ash/wm/power_button_con... ash/wm/power_button_controller.h:93: int saved_volume_percent_; nit: rename this to something like volume_percent_before_screenshot_ so it's a bit clearer when it was saved
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix Volume slider is captured in screenshot done in touchview mode Solution: 1. User holds volume-down key. 2. Remember the non-repeated pressed volume. 3. Immediately start reducing volume. 4. If the power key is tapped, 4a Notify to hide the volume slider. 4b Wait volume slider to finish hiding (200ms). 4c Taking the screenshot. 4d Restore the volume. BUG=628479 TEST=tested on veyron_minnie touchview mode and it works fine. The only defect I can image is taking screenshot when the device is playing audio. If volume-down is presssed for a long time, you can feel audio volume is firstly decreased and then restored back. ========== to ========== Fix Volume slider is captured in screenshot done in touchview mode Solution: 1. User holds volume-down key. 2. Remember the non-repeated pressed volume. 3. Immediately start reducing volume. 4. If the power key is tapped, 4a Notify to hide the volume slider. 4b Taking the screenshot. 4d Restore the volume (do not notify UI) BUG=628479 TEST=tested on veyron_minnie touchview mode and it works fine. The only defect I can image is taking screenshot when the device is playing audio. If volume-down is presssed for a long time, you can feel audio volume is firstly decreased and then restored back. ==========
warx@chromium.org changed reviewers: + jennyz@chromium.org - msw@chromium.org, sky@chromium.org
Description was changed from ========== Fix Volume slider is captured in screenshot done in touchview mode Solution: 1. User holds volume-down key. 2. Remember the non-repeated pressed volume. 3. Immediately start reducing volume. 4. If the power key is tapped, 4a Notify to hide the volume slider. 4b Taking the screenshot. 4d Restore the volume (do not notify UI) BUG=628479 TEST=tested on veyron_minnie touchview mode and it works fine. The only defect I can image is taking screenshot when the device is playing audio. If volume-down is presssed for a long time, you can feel audio volume is firstly decreased and then restored back. ========== to ========== Fix Volume slider is captured in screenshot done in touchview mode Solution: 1. User holds volume-down key. 2. Remember the non-repeated pressed volume. 3. Immediately start reducing volume. 4. If the power key is tapped, 4a Notify to hide the volume slider. 4b Taking the screenshot. 4c Restore the volume (do not notify UI) BUG=628479 TEST=tested on veyron_minnie touchview mode and it works fine. The only defect I can image is taking screenshot when the device is playing audio. If volume-down is presssed for a long time, you can feel audio volume is firstly decreased and then restored back. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Daniel & Jenny, could you help review on the new update? Thanks! https://codereview.chromium.org/2190773002/diff/40001/ash/common/system/audio... File ash/common/system/audio/tray_audio.cc (right): https://codereview.chromium.org/2190773002/diff/40001/ash/common/system/audio... ash/common/system/audio/tray_audio.cc:123: hide_popup_ = false; On 2016/07/28 04:45:00, Daniel Erat wrote: > this approach (hiding the next popup) feels a bit race-prone. it seems like it'd > be cleaner if the audio handler instead notified observers about whether the > change was user-initiated (in which case the popup should be shown) or automated > (in which case it shouldn't). done by moving the logic to cras_audio_handler https://codereview.chromium.org/2190773002/diff/40001/ash/wm/power_button_con... File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2190773002/diff/40001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:33: const int kWaitVolumeSliderHiddenInMs = 200; On 2016/07/28 04:45:00, Daniel Erat wrote: > this isn't good; it means there's a delay between when the user requests the > screenshot and when we take it. how long does it actually take for the volume > slider to be hidden? is there an animation that you're waiting for? if so, can > you skip the animation? or is the problem that you're waiting for the screen to > be redrawn? done by disable hiding animation. https://codereview.chromium.org/2190773002/diff/40001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:107: DCHECK(audio_handler->IsInitialized()); On 2016/07/28 04:45:00, Daniel Erat wrote: > you don't need this; Get() on the previous line already has a CHECK() that it's > initialized Done. https://codereview.chromium.org/2190773002/diff/40001/ash/wm/power_button_con... File ash/wm/power_button_controller.h (right): https://codereview.chromium.org/2190773002/diff/40001/ash/wm/power_button_con... ash/wm/power_button_controller.h:83: void TakingScreenshot(); On 2016/07/28 04:45:00, Daniel Erat wrote: > rename to TakeScreenshot and add a comment describing how this is used done by deleting this method. https://codereview.chromium.org/2190773002/diff/40001/ash/wm/power_button_con... ash/wm/power_button_controller.h:93: int saved_volume_percent_; On 2016/07/28 04:45:00, Daniel Erat wrote: > nit: rename this to something like volume_percent_before_screenshot_ so it's a > bit clearer when it was saved Done.
https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:380: volume_internally_changed_ = true; Will it be possible for SetOutputVolumePercentInternal be called multiple times consecutively? If so, you may need to track its number. If not, please add some DCHECK or Error log here. https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:387: } line 381-387 is the same as SetOutputVolumePercent, why not just call SetOutputVolumePercent? https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:649: if (volume_internally_changed_) { volume_internally_changed_ is similar to initializing_audio_state_, can you merge them? https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler.h (right): https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.h:164: virtual void SetOutputVolumePercentInternally(int volume_percent); Sorry, the name is a little confusing here(my fault for suggesting it), since we use SetXXXInternal in many other CrasAudioHandler function names, but it means differently. Let's use a different name, such as "SetOutputVolumePercentInQuietMode" or, maybe just add a bool |by_user| to SetOutputVolumePercent.
https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:379: void CrasAudioHandler::SetOutputVolumePercentInternally(int volume_percent) { Please add a unit test for this new API.
i'd still prefer to not add the observer interface, but i'll let someone with more current involvement in ash comment on whether it's okay to have PowerButtonController call into TrayAudio. https://codereview.chromium.org/2190773002/diff/60001/ash/common/system/tray/... File ash/common/system/tray/system_tray.cc (right): https://codereview.chromium.org/2190773002/diff/60001/ash/common/system/tray/... ash/common/system/tray/system_tray.cc:288: ->SetVisibilityAnimationTransition( does this affect future transitions, or does a new view get created each time the bubble is shown? https://codereview.chromium.org/2190773002/diff/60001/ash/common/system/tray/... File ash/common/system/tray/system_tray.h (right): https://codereview.chromium.org/2190773002/diff/60001/ash/common/system/tray/... ash/common/system/tray/system_tray.h:79: void HideDetailedView(SystemTrayItem* item, bool hide_animation); |hide_animation| is a confusing name, since it could also be interpreted as "hide the animation", i.e. pass true to not display the animation. please rename it to something like |animate|. https://codereview.chromium.org/2190773002/diff/60001/ash/common/system/tray/... File ash/common/system/tray/system_tray_item.cc (right): https://codereview.chromium.org/2190773002/diff/60001/ash/common/system/tray/... ash/common/system/tray/system_tray_item.cc:60: void SystemTrayItem::HideDetailedView(bool hide_animation) { same comment about confusing parameter name
warx@chromium.org changed reviewers: + jamescook@chromium.org
Hi, about PowerButtonController calling trayaudio method, I don't have an idea how to attain that. Maybe we can expose the reference. I add Jamescook as reviewer to discuss later. Updates based on review are uploaded. Thanks! https://codereview.chromium.org/2190773002/diff/60001/ash/common/system/tray/... File ash/common/system/tray/system_tray.cc (right): https://codereview.chromium.org/2190773002/diff/60001/ash/common/system/tray/... ash/common/system/tray/system_tray.cc:288: ->SetVisibilityAnimationTransition( On 2016/07/28 22:20:40, Daniel Erat wrote: > does this affect future transitions, or does a new view get created each time > the bubble is shown? Yes, systembubble is autoclosed everytime. So this should be fine. https://codereview.chromium.org/2190773002/diff/60001/ash/common/system/tray/... File ash/common/system/tray/system_tray.h (right): https://codereview.chromium.org/2190773002/diff/60001/ash/common/system/tray/... ash/common/system/tray/system_tray.h:79: void HideDetailedView(SystemTrayItem* item, bool hide_animation); On 2016/07/28 22:20:40, Daniel Erat wrote: > |hide_animation| is a confusing name, since it could also be interpreted as > "hide the animation", i.e. pass true to not display the animation. please rename > it to something like |animate|. Done. https://codereview.chromium.org/2190773002/diff/60001/ash/common/system/tray/... File ash/common/system/tray/system_tray_item.cc (right): https://codereview.chromium.org/2190773002/diff/60001/ash/common/system/tray/... ash/common/system/tray/system_tray_item.cc:60: void SystemTrayItem::HideDetailedView(bool hide_animation) { On 2016/07/28 22:20:40, Daniel Erat wrote: > same comment about confusing parameter name Done. https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:379: void CrasAudioHandler::SetOutputVolumePercentInternally(int volume_percent) { On 2016/07/28 21:10:05, jennyz wrote: > Please add a unit test for this new API. Done. https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:380: volume_internally_changed_ = true; On 2016/07/28 21:02:17, jennyz wrote: > Will it be possible for SetOutputVolumePercentInternal be called multiple times > consecutively? If so, you may need to track its number. If not, please add some > DCHECK or Error log here. Currently only maximize mode screenshot calls this method. And I think everytime screenshot is taken, the flag should already be released. https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:387: } On 2016/07/28 21:02:17, jennyz wrote: > line 381-387 is the same as SetOutputVolumePercent, why not just call > SetOutputVolumePercent? Done. https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:649: if (volume_internally_changed_) { On 2016/07/28 21:02:17, jennyz wrote: > volume_internally_changed_ is similar to initializing_audio_state_, can you > merge them? I use a mask to merge them. https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler.h (right): https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.h:164: virtual void SetOutputVolumePercentInternally(int volume_percent); On 2016/07/28 21:02:17, jennyz wrote: > Sorry, the name is a little confusing here(my fault for suggesting it), since we > use SetXXXInternal in many other CrasAudioHandler function names, but it means > differently. Let's use a different name, such as > "SetOutputVolumePercentInQuietMode" or, maybe just add a bool |by_user| to > SetOutputVolumePercent. Done.
Patchset #4 (id:80001) has been deleted
i started writing comments but rietveld threw an assertion and deleted them all. >:-( i'll try again in a bit, but in the meantime: - please rename "hide_animation" to "animate" consistently. there are still a bunch of places using the old name. - i'd prefer that you avoid the default parameter value. it looks like it's no longer banned by the style guide but i haven't seen them much in chromium code. how many callers would you need to update?
On 2016/07/28 23:23:41, Daniel Erat wrote: > i started writing comments but rietveld threw an assertion and deleted them all. > >:-( > > i'll try again in a bit, but in the meantime: > > - please rename "hide_animation" to "animate" consistently. there are still a > bunch of places using the old name. > - i'd prefer that you avoid the default parameter value. it looks like it's no > longer banned by the style guide but i haven't seen them much in chromium code. > how many callers would you need to update? Sorry I think it is caused by the deletion of patch by me. Actually, there is only one more call of HideDetailedViews. I cancelled using default parameter. Thanks!
https://codereview.chromium.org/2190773002/diff/140001/ash/common/system/tray... File ash/common/system/tray/system_tray_notifier.h (right): https://codereview.chromium.org/2190773002/diff/140001/ash/common/system/tray... ash/common/system/tray/system_tray_notifier.h:106: void NotifyWillTakeScreenshotInMaximizeMode(); minor nit: maybe s/WillTake/Taking/ that seems to be the typical naming convention that's used for observer methods that get called right before something happens. https://codereview.chromium.org/2190773002/diff/140001/ash/common/wm/maximize... File ash/common/wm/maximize_mode/maximize_mode_screenshot_observer.h (right): https://codereview.chromium.org/2190773002/diff/140001/ash/common/wm/maximize... ash/common/wm/maximize_mode/maximize_mode_screenshot_observer.h:17: // power button accelerator combinations. nit: delete "combinations" https://codereview.chromium.org/2190773002/diff/140001/ash/wm/power_button_co... File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2190773002/diff/140001/ash/wm/power_button_co... ash/wm/power_button_controller.cc:34: volume_percent_before_screenshot_(0), maybe ifdef this too (also in header) since you don't use it on non-chrome-os platforms https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:28: enum NonUserVolumeChangeReason { nit: s/NonUser/Automated/ (then also update comments and variable in header) https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:29: INITIALIZING_AUDIO_STATE = 1 << 0, can you make this be an enum class instead or does it make that hard to mask it? if not, give these a more descriptive prefix, like VOLUME_CHANGE_INITIALIZING_AUDIO_STATE https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:631: if (volume_change_not_by_user_ & INITIALIZING_AUDIO_STATE) { using a mask for this seems a bit confusing. is this called asynchronously? if not, why do you need to track multiple reasons? when you track multiple reasons, it looks like you just unmask one of them with each call here. how do you know that you're handling them in the right order? https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.h (right): https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:162: // Sets all active output devices' volume level to |volume_percent|, whose nit: s/level/levels/ https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:163: // range is from 0-100%, but indicating that this will not notify observers. nit: change "but indicating that this will not notify observers." to "without notifying observers." https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:164: virtual void SetOutputVolumePercentInQuietMode(int volume_percent); "InQuietMode" seems a bit confusing, since we're already dealing with volume here. how about something like SetOutputVolumePercentWithoutNotifying() or WithoutNotifyingObservers()? or maybe even just add a "notify_observers" bool parameter to SetOutputVolumePercent(), if jenny's okay with it. https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:439: // A mask to indicate if a volume change is initiated by user. nit: document "A mask of NonUserVolumeChangeReason to indicate ..." (but see naming suggestion in .cc) https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:440: int volume_change_not_by_user_ = 0; nit: non_user_volume_change_reasons_; (but see naming suggestion in .cc)
Thanks for reviewing. Here is my reply and updated patch. https://codereview.chromium.org/2190773002/diff/140001/ash/common/system/tray... File ash/common/system/tray/system_tray_notifier.h (right): https://codereview.chromium.org/2190773002/diff/140001/ash/common/system/tray... ash/common/system/tray/system_tray_notifier.h:106: void NotifyWillTakeScreenshotInMaximizeMode(); On 2016/07/28 23:57:52, Daniel Erat wrote: > minor nit: maybe s/WillTake/Taking/ > > that seems to be the typical naming convention that's used for observer methods > that get called right before something happens. Done. https://codereview.chromium.org/2190773002/diff/140001/ash/common/wm/maximize... File ash/common/wm/maximize_mode/maximize_mode_screenshot_observer.h (right): https://codereview.chromium.org/2190773002/diff/140001/ash/common/wm/maximize... ash/common/wm/maximize_mode/maximize_mode_screenshot_observer.h:17: // power button accelerator combinations. On 2016/07/28 23:57:52, Daniel Erat wrote: > nit: delete "combinations" Done. https://codereview.chromium.org/2190773002/diff/140001/ash/wm/power_button_co... File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2190773002/diff/140001/ash/wm/power_button_co... ash/wm/power_button_controller.cc:34: volume_percent_before_screenshot_(0), On 2016/07/28 23:57:52, Daniel Erat wrote: > maybe ifdef this too (also in header) since you don't use it on non-chrome-os > platforms Done. https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:28: enum NonUserVolumeChangeReason { On 2016/07/28 23:57:52, Daniel Erat wrote: > nit: s/NonUser/Automated/ > > (then also update comments and variable in header) Done. https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:29: INITIALIZING_AUDIO_STATE = 1 << 0, On 2016/07/28 23:57:52, Daniel Erat wrote: > can you make this be an enum class instead or does it make that hard to mask it? > if not, give these a more descriptive prefix, like > VOLUME_CHANGE_INITIALIZING_AUDIO_STATE done by adding prefix https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:631: if (volume_change_not_by_user_ & INITIALIZING_AUDIO_STATE) { On 2016/07/28 23:57:52, Daniel Erat wrote: > using a mask for this seems a bit confusing. is this called asynchronously? if > not, why do you need to track multiple reasons? when you track multiple reasons, > it looks like you just unmask one of them with each call here. how do you know > that you're handling them in the right order? Yes. They are called asynchronously by observing cras_audio_client. I update the logic a little bit so that the order doesn't matter. As far as there is an AutomatedVolumeChangeReason, we should not notify observers and reset each reasons. https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.h (right): https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:162: // Sets all active output devices' volume level to |volume_percent|, whose On 2016/07/28 23:57:53, Daniel Erat wrote: > nit: s/level/levels/ Done. https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:163: // range is from 0-100%, but indicating that this will not notify observers. On 2016/07/28 23:57:52, Daniel Erat wrote: > nit: change "but indicating that this will not notify observers." to "without > notifying observers." Done. https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:164: virtual void SetOutputVolumePercentInQuietMode(int volume_percent); On 2016/07/28 23:57:52, Daniel Erat wrote: > "InQuietMode" seems a bit confusing, since we're already dealing with volume > here. how about something like SetOutputVolumePercentWithoutNotifying() or > WithoutNotifyingObservers()? > > or maybe even just add a "notify_observers" bool parameter to > SetOutputVolumePercent(), if jenny's okay with it. done by adding SetOutputVolumePercentWithoutNotifyingObservers. I will discuss with jenny offline. https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:439: // A mask to indicate if a volume change is initiated by user. On 2016/07/28 23:57:53, Daniel Erat wrote: > nit: document "A mask of NonUserVolumeChangeReason to indicate ..." (but see > naming suggestion in .cc) Done. https://codereview.chromium.org/2190773002/diff/140001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:440: int volume_change_not_by_user_ = 0; On 2016/07/28 23:57:53, Daniel Erat wrote: > nit: non_user_volume_change_reasons_; (but see naming suggestion in .cc) Done.
Patchset #7 (id:160001) has been deleted
https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:389: DCHECK(!(automated_volume_change_ & VOLUME_CHANGE_MAXIMIZE_MODE_SCREENSHOT)); this dcheck makes me nervous. if MAXIMIZE_MODE_SCREENSHOT is unset asynchronously, how can you guarantee this won't be hit if e.g. the user takes two screenshots very quickly? https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:635: // to SetOutputNodeVolume request from intializaing audio state, not from nit: s/intializaing/initializing/ https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.h (right): https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:440: // A mask with non-zero value indicates it is an automated volume changed. please document the name of the enum that is used in this bitmap https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:1999: const int kVolume = 60; move this constant up and use it in the SetOutputVolume... call https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:2005: EXPECT_EQ(kVolume, audio_pref_handler_->GetOutputVolumeValue(&device)); please also test that further updates _do_ cause notifications now
Hi new patch is uploaded based on review, thanks! https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:389: DCHECK(!(automated_volume_change_ & VOLUME_CHANGE_MAXIMIZE_MODE_SCREENSHOT)); On 2016/07/29 17:53:38, Daniel Erat wrote: > this dcheck makes me nervous. if MAXIMIZE_MODE_SCREENSHOT is unset > asynchronously, how can you guarantee this won't be hit if e.g. the user takes > two screenshots very quickly? Your concern is reasonable. I am assuming CRAS is reliable and does not have a large delay. We can use a counter so that multiple quick screenshot still works properly. https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:635: // to SetOutputNodeVolume request from intializaing audio state, not from On 2016/07/29 17:53:38, Daniel Erat wrote: > nit: s/intializaing/initializing/ Done. https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.h (right): https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:440: // A mask with non-zero value indicates it is an automated volume changed. On 2016/07/29 17:53:38, Daniel Erat wrote: > please document the name of the enum that is used in this bitmap Done. https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:1999: const int kVolume = 60; On 2016/07/29 17:53:38, Daniel Erat wrote: > move this constant up and use it in the SetOutputVolume... call Done. https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:2005: EXPECT_EQ(kVolume, audio_pref_handler_->GetOutputVolumeValue(&device)); On 2016/07/29 17:53:38, Daniel Erat wrote: > please also test that further updates _do_ cause notifications now Done.
https://codereview.chromium.org/2190773002/diff/200001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/200001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:2006: EXPECT_EQ(0, test_observer_->output_volume_changed_count()); This line can be removed. It is the same as line 2001. Nothing changed since then, so no need to verify output_volume_changed_count() again. Actually, it might be useful to test a little bit more: 1. Make another SetOutputVolumePercentWithoutNotifyingObservers call to make sure it works for multiple call case. 2. Make another SetOutputVolumePercent call to make sure the volume change will be notified.
https://codereview.chromium.org/2190773002/diff/200001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.h (right): https://codereview.chromium.org/2190773002/diff/200001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:452: int maximize_mode_screenshot_count_ = 0; keeping a bitmap and a counter in parallel seems a bit strange. i'm wondering if it'd be more appropriate to have e.g. a std::deque<AutomatedVolumeChangeReason>. assuming you get replies from cras in the same order as requests to it, could you just pop the reasons off the front of it in-order?
I'll let Dan and Jenny finish the review. Calling into SystemTrayNotifier from PowerButtonController is fine. https://codereview.chromium.org/2190773002/diff/200001/ash/common/system/tray... File ash/common/system/tray/system_tray.cc (right): https://codereview.chromium.org/2190773002/diff/200001/ash/common/system/tray... ash/common/system/tray/system_tray.cc:282: if (!animate) { nit: Could this happen after line 292 below? I'm not sure about changing the visibility transition if the bubble isn't going to be destroyed. https://codereview.chromium.org/2190773002/diff/200001/ash/wm/power_button_co... File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2190773002/diff/200001/ash/wm/power_button_co... ash/wm/power_button_controller.cc:91: wm_shell->system_tray_notifier()->NotifyTakingScreenshotInMaximizeMode(); This is fine.
On 2016/08/01 16:51:47, James Cook wrote: > I'll let Dan and Jenny finish the review. Calling into SystemTrayNotifier from > PowerButtonController is fine. what about calling into TrayAudio (currently not exposed) from PowerButtonController?
On 2016/08/01 17:01:52, Daniel Erat wrote: > On 2016/08/01 16:51:47, James Cook wrote: > > I'll let Dan and Jenny finish the review. Calling into SystemTrayNotifier from > > PowerButtonController is fine. > > what about calling into TrayAudio (currently not exposed) from > PowerButtonController? Ah, I see. Yes, it would be fine to expose a TrayAudio* from SystemTray and call a method on it, especially if it eliminates the observer.
On 2016/08/01 17:09:19, James Cook wrote: > On 2016/08/01 17:01:52, Daniel Erat wrote: > > On 2016/08/01 16:51:47, James Cook wrote: > > > I'll let Dan and Jenny finish the review. Calling into SystemTrayNotifier > from > > > PowerButtonController is fine. > > > > what about calling into TrayAudio (currently not exposed) from > > PowerButtonController? > > Ah, I see. Yes, it would be fine to expose a TrayAudio* from SystemTray and call > a method on it, especially if it eliminates the observer. OK. I will try in this way. Thanks.
Patchset #9 (id:220001) has been deleted
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #9 (id:240001) has been deleted
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #9 (id:260001) has been deleted
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Several updates: 1) exposing trayaudio to let PBC call trayaudio methods instead of using MaximizeModeScreenshotObserver 2) using std::deque instead of using integer mask 3) update cras_audio_handler_unittest https://codereview.chromium.org/2190773002/diff/200001/ash/common/system/tray... File ash/common/system/tray/system_tray.cc (right): https://codereview.chromium.org/2190773002/diff/200001/ash/common/system/tray... ash/common/system/tray/system_tray.cc:282: if (!animate) { On 2016/08/01 16:51:46, James Cook wrote: > nit: Could this happen after line 292 below? I'm not sure about changing the > visibility transition if the bubble isn't going to be destroyed. mmm... not sure, but safer to move below 293. Thanks! https://codereview.chromium.org/2190773002/diff/200001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.h (right): https://codereview.chromium.org/2190773002/diff/200001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:452: int maximize_mode_screenshot_count_ = 0; On 2016/07/29 22:27:30, Daniel Erat wrote: > keeping a bitmap and a counter in parallel seems a bit strange. i'm wondering if > it'd be more appropriate to have e.g. a std::deque<AutomatedVolumeChangeReason>. > assuming you get replies from cras in the same order as requests to it, could > you just pop the reasons off the front of it in-order? done. thanks! https://codereview.chromium.org/2190773002/diff/200001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/200001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:2006: EXPECT_EQ(0, test_observer_->output_volume_changed_count()); On 2016/07/29 22:17:14, jennyz wrote: > This line can be removed. It is the same as line 2001. Nothing changed since > then, so no need to verify output_volume_changed_count() again. > > Actually, it might be useful to test a little bit more: > 1. Make another SetOutputVolumePercentWithoutNotifyingObservers call to make > sure it works for multiple call case. > 2. Make another SetOutputVolumePercent call to make sure the volume change will > be notified. Done.
https://codereview.chromium.org/2190773002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/network/tray_sms.cc (right): https://codereview.chromium.org/2190773002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/tray_sms.cc:414: HideDetailedView(true); nit: mind documenting the argument with an inline comment, e.g. HideDetailedView(true /* animate */); https://codereview.chromium.org/2190773002/diff/280001/ash/common/system/tray... File ash/common/system/tray/system_tray.h (right): https://codereview.chromium.org/2190773002/diff/280001/ash/common/system/tray... ash/common/system/tray/system_tray.h:126: // Accessor for tray audio, can be null if !defined(OS_CHROMEOS). nit: i wouldn't mention the ifdef here, since it might become out of date. how about something like this instead? // Returns TrayAudio object if present or null otherwise. https://codereview.chromium.org/2190773002/diff/280001/ash/common/system/tray... ash/common/system/tray/system_tray.h:255: TrayAccessibility* tray_accessibility_; // not owned nit: if none of these are owned, do you mind moving the "not owned" comment to the previous line so it's at the top of the block? // These objects are not owned by this class. TrayAccessibility* ...; TrayAudio* ...; ... https://codereview.chromium.org/2190773002/diff/280001/ash/common/system/tray... ash/common/system/tray/system_tray.h:256: TrayAudio* tray_audio_; nit: document that this may be null, e.g. TrayAudio* tray_audio_; // May be null. https://codereview.chromium.org/2190773002/diff/280001/ash/wm/power_button_co... File ash/wm/power_button_controller.h (right): https://codereview.chromium.org/2190773002/diff/280001/ash/wm/power_button_co... ash/wm/power_button_controller.h:91: // Volume to be restored after a maximize mode screenshot. nit: i wouldn't mention maximize mode here (this accelerator can be used in normal mode too, right?). how about this? // Volume to be restored after a screenshot is taken by pressing // the power button while holding VKEY_VOLUME_DOWN. https://codereview.chromium.org/2190773002/diff/280001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2190773002/diff/280001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:623: bool should_not_notify_observers = false; i'd invert this to make it easier to read: bool should_notify = true; if (!automated_volume_change_reasons_.empty()) { AutomatedVolumeChangeReason reason = ... automated_volume_change_reasons_.pop_front(); // A SetOutputNodeVolume request may be dropped if cras isn't // ready during initialization. if (reason != VOLUME_CHANGE_INITIALIZING_AUDIO_STATE || (init_node_id_ == node_id && init_volume_ == volume) { should_notify = false; } } but i'm not convinced that this logic matches what was there before. to be equivalent, wouldn't the new code need to remove all the VOLUME_CHANGE_INITIALIZING_AUDIO_STATE entries from the deque? jenny, is this trying to handle the case where some of the automated requests got dropped due to cras not being ready, so now we've seen a user-initiated change and want to make sure we display a notification and clear out the old automated reasons? i'm wondering if we should instead e.g. watch for the d-bus calls to cras failing. then we would only asynchronously add the reasons to the deque when we see our d-bus method call succeed. https://codereview.chromium.org/2190773002/diff/280001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:751: // OutputNodeVolumeChanged signals. the old code was incrementing a counter here that looks like it was used to suppress multiple notifications, but i only see one reason getting added in InitializeAudioState() now. are there existing tests to make sure that this is still working correctly?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Based on comments. Thanks! https://codereview.chromium.org/2190773002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/network/tray_sms.cc (right): https://codereview.chromium.org/2190773002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/tray_sms.cc:414: HideDetailedView(true); On 2016/08/01 21:10:06, Daniel Erat wrote: > nit: mind documenting the argument with an inline comment, e.g. > > HideDetailedView(true /* animate */); Done. https://codereview.chromium.org/2190773002/diff/280001/ash/common/system/tray... File ash/common/system/tray/system_tray.h (right): https://codereview.chromium.org/2190773002/diff/280001/ash/common/system/tray... ash/common/system/tray/system_tray.h:126: // Accessor for tray audio, can be null if !defined(OS_CHROMEOS). On 2016/08/01 21:10:06, Daniel Erat wrote: > nit: i wouldn't mention the ifdef here, since it might become out of date. how > about something like this instead? > > // Returns TrayAudio object if present or null otherwise. Done. https://codereview.chromium.org/2190773002/diff/280001/ash/common/system/tray... ash/common/system/tray/system_tray.h:255: TrayAccessibility* tray_accessibility_; // not owned On 2016/08/01 21:10:06, Daniel Erat wrote: > nit: if none of these are owned, do you mind moving the "not owned" comment to > the previous line so it's at the top of the block? > > // These objects are not owned by this class. > TrayAccessibility* ...; > TrayAudio* ...; > ... Done. https://codereview.chromium.org/2190773002/diff/280001/ash/common/system/tray... ash/common/system/tray/system_tray.h:256: TrayAudio* tray_audio_; On 2016/08/01 21:10:06, Daniel Erat wrote: > nit: document that this may be null, e.g. > > TrayAudio* tray_audio_; // May be null. Done. https://codereview.chromium.org/2190773002/diff/280001/ash/wm/power_button_co... File ash/wm/power_button_controller.h (right): https://codereview.chromium.org/2190773002/diff/280001/ash/wm/power_button_co... ash/wm/power_button_controller.h:91: // Volume to be restored after a maximize mode screenshot. On 2016/08/01 21:10:06, Daniel Erat wrote: > nit: i wouldn't mention maximize mode here (this accelerator can be used in > normal mode too, right?). how about this? > > // Volume to be restored after a screenshot is taken by pressing > // the power button while holding VKEY_VOLUME_DOWN. Done. https://codereview.chromium.org/2190773002/diff/280001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2190773002/diff/280001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:623: bool should_not_notify_observers = false; On 2016/08/01 21:10:07, Daniel Erat wrote: > i'd invert this to make it easier to read: > > bool should_notify = true; > if (!automated_volume_change_reasons_.empty()) { > AutomatedVolumeChangeReason reason = ... > automated_volume_change_reasons_.pop_front(); > > // A SetOutputNodeVolume request may be dropped if cras isn't > // ready during initialization. > if (reason != VOLUME_CHANGE_INITIALIZING_AUDIO_STATE || > (init_node_id_ == node_id && init_volume_ == volume) { > should_notify = false; > } > } > > but i'm not convinced that this logic matches what was there before. to be > equivalent, wouldn't the new code need to remove all the > VOLUME_CHANGE_INITIALIZING_AUDIO_STATE entries from the deque? > > jenny, is this trying to handle the case where some of the automated requests > got dropped due to cras not being ready, so now we've seen a user-initiated > change and want to make sure we display a notification and clear out the old > automated reasons? > > i'm wondering if we should instead e.g. watch for the d-bus calls to cras > failing. then we would only asynchronously add the reasons to the deque when we > see our d-bus method call succeed. I think I do change the previous logic by mistake. I discussed with Jenny offline. Once we met VOLUME_CHANGE_INITIALIZING_AUDIO_STATE and (init_node_id_ != node_id || init_volume_ != volume), we clear the pending AutomatedVolumeChangeReasons. As currently, cras is not reliable and it may also miss other signals. https://codereview.chromium.org/2190773002/diff/280001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:751: // OutputNodeVolumeChanged signals. On 2016/08/01 21:10:07, Daniel Erat wrote: > the old code was incrementing a counter here that looks like it was used to > suppress multiple notifications, but i only see one reason getting added in > InitializeAudioState() now. are there existing tests to make sure that this is > still working correctly? I am not quite understanding the comments here. But I think the logic is still needed. And there is no tests right now covering this. Jenny, what do you think here?
jenny can comment on this, but i think it'd also be good to add an additional test for the multiple-initialization logic that passes with both the original and new code. https://codereview.chromium.org/2190773002/diff/300001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2190773002/diff/300001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:755: // OutputNodeVolumeChanged signals. i don't understand why you don't need to push another VOLUME_CHANGE_INITIALIZING_AUDIO_STATE reason here. won't you get two signals that you need to ignore? https://codereview.chromium.org/2190773002/diff/300001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.h (right): https://codereview.chromium.org/2190773002/diff/300001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:450: std::deque<AutomatedVolumeChangeReason> automated_volume_change_reasons_; i'd add a brief comment explaining how this is used. maybe something like: // FIFO list of reasons passed to // SetOutputVolumePercentWithoutNotifyingObservers() for which // we're still waiting for OutputNodeVolumeChanged() calls. // These are used to suppress notifications for those changes.
https://codereview.chromium.org/2190773002/diff/300001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2190773002/diff/300001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:632: automated_volume_change_reasons_.clear(); Can we add a warning log here for diagnostic purpose? Like LOG(WARNING) << "OutputNodeVolumeChanged signal dropped during the initialization. https://codereview.chromium.org/2190773002/diff/300001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:751: VOLUME_CHANGE_INITIALIZING_AUDIO_STATE) { Will it be more straight forward by keeping initializing_audio_state_? It looks better to keep the logic for pushing automated_volume_change_reasons_ and SetOutputNodeVolume together. if (initializing_audio_state_) SetOutputVolumePercentWithoutNotifyingObservers(volume_percent, VOLUME_CHANGE_INITIALIZING_AUDIO_STATE);
Hi Daniel & Jenny, here is my reply and new patch, thanks! https://codereview.chromium.org/2190773002/diff/300001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2190773002/diff/300001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:632: automated_volume_change_reasons_.clear(); On 2016/08/02 18:02:33, jennyz wrote: > Can we add a warning log here for diagnostic purpose? Like LOG(WARNING) << > "OutputNodeVolumeChanged signal dropped during the initialization. Done. https://codereview.chromium.org/2190773002/diff/300001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:751: VOLUME_CHANGE_INITIALIZING_AUDIO_STATE) { On 2016/08/02 18:02:33, jennyz wrote: > Will it be more straight forward by keeping initializing_audio_state_? It looks > better to keep the logic for pushing automated_volume_change_reasons_ and > SetOutputNodeVolume together. > > if (initializing_audio_state_) > SetOutputVolumePercentWithoutNotifyingObservers(volume_percent, > VOLUME_CHANGE_INITIALIZING_AUDIO_STATE); It is great, thanks. In this way, automated_volume_change_reasons_ is pushed only by SetOutputVolumePercentWithoutNotifyingObservers and poped only by OutputNodeVolumeChanged. This makes the code cleaner. https://codereview.chromium.org/2190773002/diff/300001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.cc:755: // OutputNodeVolumeChanged signals. On 2016/08/02 16:31:38, Daniel Erat wrote: > i don't understand why you don't need to push another > VOLUME_CHANGE_INITIALIZING_AUDIO_STATE reason here. won't you get two signals > that you need to ignore? Yeah, I may change the code logic from jennyz's. I adopt the suggestion from jennyz, which makes it cleaner and should have no problem then. https://codereview.chromium.org/2190773002/diff/300001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.h (right): https://codereview.chromium.org/2190773002/diff/300001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:450: std::deque<AutomatedVolumeChangeReason> automated_volume_change_reasons_; On 2016/08/02 16:31:38, Daniel Erat wrote: > i'd add a brief comment explaining how this is used. maybe something like: > > // FIFO list of reasons passed to > // SetOutputVolumePercentWithoutNotifyingObservers() for which > // we're still waiting for OutputNodeVolumeChanged() calls. > // These are used to suppress notifications for those changes. done. That looks nice.
Description was changed from ========== Fix Volume slider is captured in screenshot done in touchview mode Solution: 1. User holds volume-down key. 2. Remember the non-repeated pressed volume. 3. Immediately start reducing volume. 4. If the power key is tapped, 4a Notify to hide the volume slider. 4b Taking the screenshot. 4c Restore the volume (do not notify UI) BUG=628479 TEST=tested on veyron_minnie touchview mode and it works fine. The only defect I can image is taking screenshot when the device is playing audio. If volume-down is presssed for a long time, you can feel audio volume is firstly decreased and then restored back. ========== to ========== Fix Volume slider is captured in screenshot done in touchview mode Solution: 1. User holds volume-down key. 2. Remember the non-repeated pressed volume. 3. Immediately start reducing volume. 4. If the power key is tapped, 4a TrayAudio hides the volume slider. 4b Taking the screenshot. 4c Restore the volume (do not notify UI) BUG=628479 TEST=tested on veyron_minnie touchview mode and it works fine. The only defect I can image is taking screenshot when the device is playing audio. If volume-down is presssed for a long time, you can feel audio volume is firstly decreased and then restored back. ==========
this looks okay to me now, but please also add the unit tests for the ignoring-events-during-initialization logic, since it's tricky and this change modifies it. (sorry, i know that you're just modifying that code here and it was already missing tests.) it would be good to watch the order in which requests are made to cras and signals received on a real device, and then make the test do the same thing and verify that no notifications are sent. there appear to be edge cases in the code (e.g. cras dropping requests because it's still starting), and it would be good to make sure that there's a test mimicking that too. https://codereview.chromium.org/2190773002/diff/320001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.h (right): https://codereview.chromium.org/2190773002/diff/320001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:43: // Indicates it is from intializaing audio state. nit: initializing
I add a basic unittest when InitializingAudioState we should avoid notifying. The way I do is to signal a AudioClientRestarted to call InitializingAudioState. I didn't get a clear idea about how to simulate more involved case on code, like watching the requests, like simulating dropping requests. Upload for review but I will keep an eye thinking about it. https://codereview.chromium.org/2190773002/diff/320001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.h (right): https://codereview.chromium.org/2190773002/diff/320001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:43: // Indicates it is from intializaing audio state. On 2016/08/03 00:04:25, Daniel Erat wrote: > nit: initializing Done.
Patchset #12 (id:340001) has been deleted
https://codereview.chromium.org/2190773002/diff/360001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/360001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:424: void AudioClientRestarted(const AudioNodeList& audio_nodes) { nit: rename to RestartAudioClient()? https://codereview.chromium.org/2190773002/diff/360001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:2044: // not notify UI. i'm confused. it doesn't look like you're calling OutputNodeVolumeChanged at all via e.g. FakeCrasAudioClient::SetOutputNodeVolume here, so i'm not sure what this is actually testing. am i misunderstanding how this works? i meant that the test should be simulating what happens in production, where d-bus signals are received. https://codereview.chromium.org/2190773002/diff/360001/chromeos/dbus/fake_cra... File chromeos/dbus/fake_cras_audio_client.h (right): https://codereview.chromium.org/2190773002/diff/360001/chromeos/dbus/fake_cra... chromeos/dbus/fake_cras_audio_client.h:67: void NotifyAudioClientRestartedForTesting(const AudioNodeList& new_nodes); minor nit: since the node list isn't part of the notification, it might be better to just make this method take no params and have tests call the other method to set the nodes first.
Patchset #13 (id:380001) has been deleted
Hi Daniel, I update the unittest which is replied in comments. Let me know if I spoke your mind. Thanks! https://codereview.chromium.org/2190773002/diff/360001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/360001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:424: void AudioClientRestarted(const AudioNodeList& audio_nodes) { On 2016/08/04 00:58:19, Daniel Erat wrote: > nit: rename to RestartAudioClient()? Done. https://codereview.chromium.org/2190773002/diff/360001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:2044: // not notify UI. On 2016/08/04 00:58:19, Daniel Erat wrote: > i'm confused. it doesn't look like you're calling OutputNodeVolumeChanged at all > via e.g. FakeCrasAudioClient::SetOutputNodeVolume here, so i'm not sure what > this is actually testing. am i misunderstanding how this works? > > i meant that the test should be simulating what happens in production, where > d-bus signals are received. First, AudioClientRestarted is simulating (and will call) InitializingAudioState. This is pretty much close to the actual case. Second, I made fake_cras_audio_client_ listens to dbus signals. So we can simulate that all dbus signals can be sequentially received, but not all of them can notify AudioObserver. https://codereview.chromium.org/2190773002/diff/360001/chromeos/dbus/fake_cra... File chromeos/dbus/fake_cras_audio_client.h (right): https://codereview.chromium.org/2190773002/diff/360001/chromeos/dbus/fake_cra... chromeos/dbus/fake_cras_audio_client.h:67: void NotifyAudioClientRestartedForTesting(const AudioNodeList& new_nodes); On 2016/08/04 00:58:19, Daniel Erat wrote: > minor nit: since the node list isn't part of the notification, it might be > better to just make this method take no params and have tests call the other > method to set the nodes first. Done.
https://codereview.chromium.org/2190773002/diff/400001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/400001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:435: void RestartAudioClient(const AudioNodeList& audio_nodes) { It is better not to pass audio_nodes in RestartAudioClient, the |audio_nodes| is not associated with AudioClientRestarted event. You can separate it out. https://codereview.chromium.org/2190773002/diff/400001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:2050: // received. It is not very clear what is tested here. OutputNodeVolumeChanged is asynchronously in real cras case, but what in FakeCrasAudioClient, it is controlled by |notify_volume_change_with_delay_| which is default to false, i.e., sychronously. So you are actually not really testing asyc case here. I mean you might not want this so complicated and mixed in one test case. How many make each test case serve a single purpose. You can write one to just test AudioClientRestartCase. If you want to test async case, make another one.
https://codereview.chromium.org/2190773002/diff/400001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/400001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:2066: .Times(3); i don't understand what this MockAudioClientObserver or these expectations are for. they seem to be testing that FakeCrasAudioClient actually notifies its observers, which isn't something that this test should be doing. my suggestion for this test was meant to be: a) initialize CrasAudioHandler so it's in the state where it should avoid notifying observers b) call FakeCrasAudioClient::NotifyOutputNodeVolumeChangedForTesting (if this doesn't automatically happen in response to the handler's initialization) c) use |test_observer_| to verify that notifications weren't sent d) call SetOutputVolumePercent after initialization is done e) use |test_observer_| to verify that a notification _was_ sent now then add another test that verifies the cras-dropped-request case: a) initialize CrasAudioHandler so it's in the state where it should avoid notifying observers b) call FakeCrasAudioClient::NotifyOutputNodeVolumeChangedForTesting with a different output node e) use |test_observer_| to verify that a notification was sent the SetOutputVolumePercentWithoutNotifyingObservers test that you added looks like it adaquately covers the new SetOutputVolumePercentWithoutNotifyingObservers code that this change added; i just wanted something like the two tests that i described above to verify the initialization code (without testing *WithoutNotifyingObservers).
okay, i think that jenny and i are saying the same thing. :-)
Hi Daniel & Jenny, I updated the unittest. Thanks for reviewing! https://codereview.chromium.org/2190773002/diff/400001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/400001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:435: void RestartAudioClient(const AudioNodeList& audio_nodes) { On 2016/08/04 16:07:33, jennyz wrote: > It is better not to pass audio_nodes in RestartAudioClient, the |audio_nodes| is > not associated with AudioClientRestarted event. You can separate it out. done by deleting this method https://codereview.chromium.org/2190773002/diff/400001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:2050: // received. On 2016/08/04 16:07:33, jennyz wrote: > It is not very clear what is tested here. OutputNodeVolumeChanged is > asynchronously in real cras case, but what in FakeCrasAudioClient, it is > controlled by |notify_volume_change_with_delay_| which is default to false, > i.e., sychronously. So you are actually not really testing asyc case here. > > I mean you might not want this so complicated and mixed in one test case. How > many make each test case serve a single purpose. You can write one to just test > AudioClientRestartCase. If you want to test async case, make another one. Done. https://codereview.chromium.org/2190773002/diff/400001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:2066: .Times(3); On 2016/08/04 16:25:51, Daniel Erat wrote: > i don't understand what this MockAudioClientObserver or these expectations are > for. they seem to be testing that FakeCrasAudioClient actually notifies its > observers, which isn't something that this test should be doing. > > my suggestion for this test was meant to be: > > a) initialize CrasAudioHandler so it's in the state where it should avoid > notifying observers > b) call FakeCrasAudioClient::NotifyOutputNodeVolumeChangedForTesting (if this > doesn't automatically happen in response to the handler's initialization) > c) use |test_observer_| to verify that notifications weren't sent > d) call SetOutputVolumePercent after initialization is done > e) use |test_observer_| to verify that a notification _was_ sent now > > then add another test that verifies the cras-dropped-request case: > > a) initialize CrasAudioHandler so it's in the state where it should avoid > notifying observers > b) call FakeCrasAudioClient::NotifyOutputNodeVolumeChangedForTesting with a > different output node > e) use |test_observer_| to verify that a notification was sent > > the SetOutputVolumePercentWithoutNotifyingObservers test that you added looks > like it adaquately covers the new > SetOutputVolumePercentWithoutNotifyingObservers code that this change added; i > just wanted something like the two tests that i described above to verify the > initialization code (without testing *WithoutNotifyingObservers). The updated unittest should work in this way. Thanks!
https://codereview.chromium.org/2190773002/diff/420001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/420001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:432: cras_audio_handler_->init_node_id_ = active_output_node_id; please don't reach directly into the object being tested and set its members like this. it results in tests that need to be updated whenever the class's implementation is changed, and that may not be testing the way that things actually work in production. is there no way to get CrasAudioHandler into the correct state by calling its public methods, as would happen during initialization?
https://codereview.chromium.org/2190773002/diff/420001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/420001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:432: cras_audio_handler_->init_node_id_ = active_output_node_id; On 2016/08/04 23:08:59, Daniel Erat wrote: > please don't reach directly into the object being tested and set its members > like this. it results in tests that need to be updated whenever the class's > implementation is changed, and that may not be testing the way that things > actually work in production. > > is there no way to get CrasAudioHandler into the correct state by calling its > public methods, as would happen during initialization? Sorry wrong local branch.
thanks! the tests look good now; just a few nits. https://codereview.chromium.org/2190773002/diff/460001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/460001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:2041: const int kDefaultVolume = 75; nit: this test doesn't need to test the default volume, so i'd instead do: const int kDefaultVolume = cras_audio_handler_->GetOutputVolumePercent(); alternately, just get this from the code if it's exposed by a header. https://codereview.chromium.org/2190773002/diff/460001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:2053: // should avoid notify observers. nit: s/notify/notifying/ https://codereview.chromium.org/2190773002/diff/460001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:2075: EXPECT_EQ(kDefaultVolume, cras_audio_handler_->GetOutputVolumePercent()); nit: same conversation about removing this expectation https://codereview.chromium.org/2190773002/diff/460001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:2087: // to log warning message, clear the penging automated volume change reasons, nit: s/penging/pending/
Hi Daniel & Jenny, here is the update, thanks! https://codereview.chromium.org/2190773002/diff/460001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/460001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:2041: const int kDefaultVolume = 75; On 2016/08/05 16:00:53, Daniel Erat wrote: > nit: this test doesn't need to test the default volume, so i'd instead do: > > const int kDefaultVolume = > cras_audio_handler_->GetOutputVolumePercent(); > > alternately, just get this from the code if it's exposed by a header. Done. https://codereview.chromium.org/2190773002/diff/460001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:2053: // should avoid notify observers. On 2016/08/05 16:00:53, Daniel Erat wrote: > nit: s/notify/notifying/ Done. https://codereview.chromium.org/2190773002/diff/460001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:2075: EXPECT_EQ(kDefaultVolume, cras_audio_handler_->GetOutputVolumePercent()); On 2016/08/05 16:00:53, Daniel Erat wrote: > nit: same conversation about removing this expectation Done. https://codereview.chromium.org/2190773002/diff/460001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler_unittest.cc:2087: // to log warning message, clear the penging automated volume change reasons, On 2016/08/05 16:00:53, Daniel Erat wrote: > nit: s/penging/pending/ Done.
lgtm thanks!
lgtm
The CQ bit was checked by warx@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix Volume slider is captured in screenshot done in touchview mode Solution: 1. User holds volume-down key. 2. Remember the non-repeated pressed volume. 3. Immediately start reducing volume. 4. If the power key is tapped, 4a TrayAudio hides the volume slider. 4b Taking the screenshot. 4c Restore the volume (do not notify UI) BUG=628479 TEST=tested on veyron_minnie touchview mode and it works fine. The only defect I can image is taking screenshot when the device is playing audio. If volume-down is presssed for a long time, you can feel audio volume is firstly decreased and then restored back. ========== to ========== Fix Volume slider is captured in screenshot done in touchview mode Solution: 1. User holds volume-down key. 2. Remember the non-repeated pressed volume. 3. Immediately start reducing volume. 4. If the power key is tapped, 4a TrayAudio hides the volume slider. 4b Taking the screenshot. 4c Restore the volume (do not notify UI) BUG=628479 TEST=tested on veyron_minnie touchview mode and it works fine. The only defect I can image is taking screenshot when the device is playing audio. If volume-down is presssed for a long time, you can feel audio volume is firstly decreased and then restored back. ==========
Message was sent while issue was closed.
Committed patchset #17 (id:480001)
Message was sent while issue was closed.
Description was changed from ========== Fix Volume slider is captured in screenshot done in touchview mode Solution: 1. User holds volume-down key. 2. Remember the non-repeated pressed volume. 3. Immediately start reducing volume. 4. If the power key is tapped, 4a TrayAudio hides the volume slider. 4b Taking the screenshot. 4c Restore the volume (do not notify UI) BUG=628479 TEST=tested on veyron_minnie touchview mode and it works fine. The only defect I can image is taking screenshot when the device is playing audio. If volume-down is presssed for a long time, you can feel audio volume is firstly decreased and then restored back. ========== to ========== Fix Volume slider is captured in screenshot done in touchview mode Solution: 1. User holds volume-down key. 2. Remember the non-repeated pressed volume. 3. Immediately start reducing volume. 4. If the power key is tapped, 4a TrayAudio hides the volume slider. 4b Taking the screenshot. 4c Restore the volume (do not notify UI) BUG=628479 TEST=tested on veyron_minnie touchview mode and it works fine. The only defect I can image is taking screenshot when the device is playing audio. If volume-down is presssed for a long time, you can feel audio volume is firstly decreased and then restored back. Committed: https://crrev.com/fd7c37b98e7c6eca718257916ab65409ee7b2c1a Cr-Commit-Position: refs/heads/master@{#410145} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/fd7c37b98e7c6eca718257916ab65409ee7b2c1a Cr-Commit-Position: refs/heads/master@{#410145} |