Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(50)

Issue 2190773002: Fix Volume slider is captured in screenshot done in touchview mode (Closed)

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.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+243 lines, -34 lines) Patch
M ash/common/system/chromeos/network/tray_sms.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M ash/common/system/tray/system_tray.h View 1 2 3 4 5 6 7 8 9 4 chunks +10 lines, -3 lines 0 comments Download
M ash/common/system/tray/system_tray.cc View 1 2 3 4 5 6 7 8 5 chunks +21 lines, -2 lines 0 comments Download
M ash/common/system/tray/system_tray_item.h View 1 2 3 4 5 1 chunk +3 lines, -2 lines 0 comments Download
M ash/common/system/tray/system_tray_item.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M ash/wm/power_button_controller.h View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -0 lines 0 comments Download
M ash/wm/power_button_controller.cc View 1 2 3 4 5 6 7 8 5 chunks +29 lines, -1 line 0 comments Download
M chromeos/audio/cras_audio_handler.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +22 lines, -2 lines 0 comments Download
M chromeos/audio/cras_audio_handler.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +36 lines, -18 lines 0 comments Download
M chromeos/audio/cras_audio_handler_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +113 lines, -3 lines 0 comments Download

Messages

Total messages: 86 (36 generated)
Qiang(Joe) Xu
Hi Daniel, Scott, could you help review on this? tks!
4 years, 4 months ago (2016-07-27 23:03:59 UTC) #3
sky
I'm out until Monday, so you might want to choose another reviewer.
4 years, 4 months ago (2016-07-27 23:30:05 UTC) #6
Qiang(Joe) Xu
On 2016/07/27 23:30:05, sky wrote: > I'm out until Monday, so you might want to ...
4 years, 4 months ago (2016-07-27 23:34:27 UTC) #8
Daniel Erat
i left a high-level comment in system_tray_notifier.h https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/system_tray_notifier.h File ash/common/system/tray/system_tray_notifier.h (right): https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/system_tray_notifier.h#newcode102 ash/common/system/tray/system_tray_notifier.h:102: void AddMaxModeScreenshotObserver(MaximizeModeScreenshotObserver* ...
4 years, 4 months ago (2016-07-27 23:51:32 UTC) #11
Qiang(Joe) Xu
https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/system_tray_notifier.h File ash/common/system/tray/system_tray_notifier.h (right): https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/system_tray_notifier.h#newcode106 ash/common/system/tray/system_tray_notifier.h:106: void NotifyDidTakeScreenshot(); On 2016/07/27 23:51:31, Daniel Erat wrote: > ...
4 years, 4 months ago (2016-07-28 00:15:39 UTC) #12
Qiang(Joe) Xu
On 2016/07/27 23:51:32, Daniel Erat wrote: > i left a high-level comment in system_tray_notifier.h > ...
4 years, 4 months ago (2016-07-28 00:22:38 UTC) #13
Daniel Erat
https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/system_tray_notifier.h File ash/common/system/tray/system_tray_notifier.h (right): https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/system_tray_notifier.h#newcode106 ash/common/system/tray/system_tray_notifier.h:106: void NotifyDidTakeScreenshot(); On 2016/07/28 00:15:39, Qiang (Joe) Xu wrote: ...
4 years, 4 months ago (2016-07-28 00:25:56 UTC) #14
Qiang(Joe) Xu
On 2016/07/28 00:25:56, Daniel Erat wrote: > https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/system_tray_notifier.h > File ash/common/system/tray/system_tray_notifier.h (right): > > https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/system_tray_notifier.h#newcode106 ...
4 years, 4 months ago (2016-07-28 00:28:45 UTC) #15
Qiang(Joe) Xu
Hi Daniel, here is my reply and new patch, thanks for review again. https://codereview.chromium.org/2190773002/diff/1/ash/common/system/tray/system_tray_notifier.h File ...
4 years, 4 months ago (2016-07-28 03:27:18 UTC) #18
Daniel Erat
https://codereview.chromium.org/2190773002/diff/40001/ash/common/system/audio/tray_audio.cc File ash/common/system/audio/tray_audio.cc (right): https://codereview.chromium.org/2190773002/diff/40001/ash/common/system/audio/tray_audio.cc#newcode123 ash/common/system/audio/tray_audio.cc:123: hide_popup_ = false; this approach (hiding the next popup) ...
4 years, 4 months ago (2016-07-28 04:45:00 UTC) #19
Qiang(Joe) Xu
Hi Daniel & Jenny, could you help review on the new update? Thanks! https://codereview.chromium.org/2190773002/diff/40001/ash/common/system/audio/tray_audio.cc File ...
4 years, 4 months ago (2016-07-28 20:25:03 UTC) #27
jennyz
https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_audio_handler.cc File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_audio_handler.cc#newcode380 chromeos/audio/cras_audio_handler.cc:380: volume_internally_changed_ = true; Will it be possible for SetOutputVolumePercentInternal ...
4 years, 4 months ago (2016-07-28 21:02:17 UTC) #28
jennyz
https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_audio_handler.cc File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2190773002/diff/60001/chromeos/audio/cras_audio_handler.cc#newcode379 chromeos/audio/cras_audio_handler.cc:379: void CrasAudioHandler::SetOutputVolumePercentInternally(int volume_percent) { Please add a unit test ...
4 years, 4 months ago (2016-07-28 21:10:05 UTC) #29
Daniel Erat
i'd still prefer to not add the observer interface, but i'll let someone with more ...
4 years, 4 months ago (2016-07-28 22:20:40 UTC) #30
Qiang(Joe) Xu
Hi, about PowerButtonController calling trayaudio method, I don't have an idea how to attain that. ...
4 years, 4 months ago (2016-07-28 23:15:57 UTC) #32
Daniel Erat
i started writing comments but rietveld threw an assertion and deleted them all. >:-( i'll ...
4 years, 4 months ago (2016-07-28 23:23:41 UTC) #34
Qiang(Joe) Xu
On 2016/07/28 23:23:41, Daniel Erat wrote: > i started writing comments but rietveld threw an ...
4 years, 4 months ago (2016-07-28 23:41:24 UTC) #35
Daniel Erat
https://codereview.chromium.org/2190773002/diff/140001/ash/common/system/tray/system_tray_notifier.h File ash/common/system/tray/system_tray_notifier.h (right): https://codereview.chromium.org/2190773002/diff/140001/ash/common/system/tray/system_tray_notifier.h#newcode106 ash/common/system/tray/system_tray_notifier.h:106: void NotifyWillTakeScreenshotInMaximizeMode(); minor nit: maybe s/WillTake/Taking/ that seems to ...
4 years, 4 months ago (2016-07-28 23:57:53 UTC) #36
Qiang(Joe) Xu
Thanks for reviewing. Here is my reply and updated patch. https://codereview.chromium.org/2190773002/diff/140001/ash/common/system/tray/system_tray_notifier.h File ash/common/system/tray/system_tray_notifier.h (right): https://codereview.chromium.org/2190773002/diff/140001/ash/common/system/tray/system_tray_notifier.h#newcode106 ...
4 years, 4 months ago (2016-07-29 04:30:32 UTC) #37
Daniel Erat
https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_audio_handler.cc File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_audio_handler.cc#newcode389 chromeos/audio/cras_audio_handler.cc:389: DCHECK(!(automated_volume_change_ & VOLUME_CHANGE_MAXIMIZE_MODE_SCREENSHOT)); this dcheck makes me nervous. if ...
4 years, 4 months ago (2016-07-29 17:53:38 UTC) #39
Qiang(Joe) Xu
Hi new patch is uploaded based on review, thanks! https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_audio_handler.cc File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2190773002/diff/180001/chromeos/audio/cras_audio_handler.cc#newcode389 chromeos/audio/cras_audio_handler.cc:389: ...
4 years, 4 months ago (2016-07-29 18:58:23 UTC) #40
jennyz
https://codereview.chromium.org/2190773002/diff/200001/chromeos/audio/cras_audio_handler_unittest.cc File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/200001/chromeos/audio/cras_audio_handler_unittest.cc#newcode2006 chromeos/audio/cras_audio_handler_unittest.cc:2006: EXPECT_EQ(0, test_observer_->output_volume_changed_count()); This line can be removed. It is ...
4 years, 4 months ago (2016-07-29 22:17:14 UTC) #41
Daniel Erat
https://codereview.chromium.org/2190773002/diff/200001/chromeos/audio/cras_audio_handler.h File chromeos/audio/cras_audio_handler.h (right): https://codereview.chromium.org/2190773002/diff/200001/chromeos/audio/cras_audio_handler.h#newcode452 chromeos/audio/cras_audio_handler.h:452: int maximize_mode_screenshot_count_ = 0; keeping a bitmap and a ...
4 years, 4 months ago (2016-07-29 22:27:31 UTC) #42
James Cook
I'll let Dan and Jenny finish the review. Calling into SystemTrayNotifier from PowerButtonController is fine. ...
4 years, 4 months ago (2016-08-01 16:51:47 UTC) #43
Daniel Erat
On 2016/08/01 16:51:47, James Cook wrote: > I'll let Dan and Jenny finish the review. ...
4 years, 4 months ago (2016-08-01 17:01:52 UTC) #44
James Cook
On 2016/08/01 17:01:52, Daniel Erat wrote: > On 2016/08/01 16:51:47, James Cook wrote: > > ...
4 years, 4 months ago (2016-08-01 17:09:19 UTC) #45
Qiang(Joe) Xu
On 2016/08/01 17:09:19, James Cook wrote: > On 2016/08/01 17:01:52, Daniel Erat wrote: > > ...
4 years, 4 months ago (2016-08-01 17:20:55 UTC) #46
Qiang(Joe) Xu
Several updates: 1) exposing trayaudio to let PBC call trayaudio methods instead of using MaximizeModeScreenshotObserver ...
4 years, 4 months ago (2016-08-01 20:28:09 UTC) #56
Daniel Erat
https://codereview.chromium.org/2190773002/diff/280001/ash/common/system/chromeos/network/tray_sms.cc File ash/common/system/chromeos/network/tray_sms.cc (right): https://codereview.chromium.org/2190773002/diff/280001/ash/common/system/chromeos/network/tray_sms.cc#newcode414 ash/common/system/chromeos/network/tray_sms.cc:414: HideDetailedView(true); nit: mind documenting the argument with an inline ...
4 years, 4 months ago (2016-08-01 21:10:07 UTC) #57
Qiang(Joe) Xu
Based on comments. Thanks! https://codereview.chromium.org/2190773002/diff/280001/ash/common/system/chromeos/network/tray_sms.cc File ash/common/system/chromeos/network/tray_sms.cc (right): https://codereview.chromium.org/2190773002/diff/280001/ash/common/system/chromeos/network/tray_sms.cc#newcode414 ash/common/system/chromeos/network/tray_sms.cc:414: HideDetailedView(true); On 2016/08/01 21:10:06, Daniel ...
4 years, 4 months ago (2016-08-01 22:56:48 UTC) #60
Daniel Erat
jenny can comment on this, but i think it'd also be good to add an ...
4 years, 4 months ago (2016-08-02 16:31:38 UTC) #61
jennyz
https://codereview.chromium.org/2190773002/diff/300001/chromeos/audio/cras_audio_handler.cc File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2190773002/diff/300001/chromeos/audio/cras_audio_handler.cc#newcode632 chromeos/audio/cras_audio_handler.cc:632: automated_volume_change_reasons_.clear(); Can we add a warning log here for ...
4 years, 4 months ago (2016-08-02 18:02:33 UTC) #62
Qiang(Joe) Xu
Hi Daniel & Jenny, here is my reply and new patch, thanks! https://codereview.chromium.org/2190773002/diff/300001/chromeos/audio/cras_audio_handler.cc File chromeos/audio/cras_audio_handler.cc ...
4 years, 4 months ago (2016-08-02 22:45:58 UTC) #63
Daniel Erat
this looks okay to me now, but please also add the unit tests for the ...
4 years, 4 months ago (2016-08-03 00:04:25 UTC) #65
Qiang(Joe) Xu
I add a basic unittest when InitializingAudioState we should avoid notifying. The way I do ...
4 years, 4 months ago (2016-08-04 00:35:42 UTC) #66
Daniel Erat
https://codereview.chromium.org/2190773002/diff/360001/chromeos/audio/cras_audio_handler_unittest.cc File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/360001/chromeos/audio/cras_audio_handler_unittest.cc#newcode424 chromeos/audio/cras_audio_handler_unittest.cc:424: void AudioClientRestarted(const AudioNodeList& audio_nodes) { nit: rename to RestartAudioClient()? ...
4 years, 4 months ago (2016-08-04 00:58:19 UTC) #68
Qiang(Joe) Xu
Hi Daniel, I update the unittest which is replied in comments. Let me know if ...
4 years, 4 months ago (2016-08-04 04:02:16 UTC) #70
jennyz
https://codereview.chromium.org/2190773002/diff/400001/chromeos/audio/cras_audio_handler_unittest.cc File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/400001/chromeos/audio/cras_audio_handler_unittest.cc#newcode435 chromeos/audio/cras_audio_handler_unittest.cc:435: void RestartAudioClient(const AudioNodeList& audio_nodes) { It is better not ...
4 years, 4 months ago (2016-08-04 16:07:33 UTC) #71
Daniel Erat
https://codereview.chromium.org/2190773002/diff/400001/chromeos/audio/cras_audio_handler_unittest.cc File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/400001/chromeos/audio/cras_audio_handler_unittest.cc#newcode2066 chromeos/audio/cras_audio_handler_unittest.cc:2066: .Times(3); i don't understand what this MockAudioClientObserver or these ...
4 years, 4 months ago (2016-08-04 16:25:51 UTC) #72
Daniel Erat
okay, i think that jenny and i are saying the same thing. :-)
4 years, 4 months ago (2016-08-04 16:26:28 UTC) #73
Qiang(Joe) Xu
Hi Daniel & Jenny, I updated the unittest. Thanks for reviewing! https://codereview.chromium.org/2190773002/diff/400001/chromeos/audio/cras_audio_handler_unittest.cc File chromeos/audio/cras_audio_handler_unittest.cc (right): ...
4 years, 4 months ago (2016-08-04 22:51:58 UTC) #74
Daniel Erat
https://codereview.chromium.org/2190773002/diff/420001/chromeos/audio/cras_audio_handler_unittest.cc File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/420001/chromeos/audio/cras_audio_handler_unittest.cc#newcode432 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 ...
4 years, 4 months ago (2016-08-04 23:08:59 UTC) #75
Qiang(Joe) Xu
https://codereview.chromium.org/2190773002/diff/420001/chromeos/audio/cras_audio_handler_unittest.cc File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/420001/chromeos/audio/cras_audio_handler_unittest.cc#newcode432 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: ...
4 years, 4 months ago (2016-08-04 23:41:03 UTC) #76
Daniel Erat
thanks! the tests look good now; just a few nits. https://codereview.chromium.org/2190773002/diff/460001/chromeos/audio/cras_audio_handler_unittest.cc File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/460001/chromeos/audio/cras_audio_handler_unittest.cc#newcode2041 ...
4 years, 4 months ago (2016-08-05 16:00:53 UTC) #77
Qiang(Joe) Xu
Hi Daniel & Jenny, here is the update, thanks! https://codereview.chromium.org/2190773002/diff/460001/chromeos/audio/cras_audio_handler_unittest.cc File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2190773002/diff/460001/chromeos/audio/cras_audio_handler_unittest.cc#newcode2041 chromeos/audio/cras_audio_handler_unittest.cc:2041: ...
4 years, 4 months ago (2016-08-05 17:03:30 UTC) #78
Daniel Erat
lgtm thanks!
4 years, 4 months ago (2016-08-05 17:06:54 UTC) #79
jennyz
lgtm
4 years, 4 months ago (2016-08-05 18:24:46 UTC) #80
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2190773002/480001
4 years, 4 months ago (2016-08-05 18:32:41 UTC) #82
commit-bot: I haz the power
Committed patchset #17 (id:480001)
4 years, 4 months ago (2016-08-05 19:38:38 UTC) #84
commit-bot: I haz the power
4 years, 4 months ago (2016-08-05 19:40:47 UTC) #86
Message was sent while issue was closed.
Patchset 17 (id:??) landed as
https://crrev.com/fd7c37b98e7c6eca718257916ab65409ee7b2c1a
Cr-Commit-Position: refs/heads/master@{#410145}

Powered by Google App Engine
This is Rietveld 408576698