Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2485353002:
Fix the volume strength icon to reflect the volume control clicks (Closed)
DescriptionFix the volume strength icon to reflect the volume control clicks
- When the thumb is dragged to the left, the volume strength icon still
shows that the audio is not muted. This bug is caused by the internal
audio state, which takes int only, is different from the slider volume
state, which takes floats only.
- When user clicks on volume control slider, the volume strength icon is not
updated accordingly. This is because the volume strength icon was not called
to update.
TEST=MANUAL
- Build the Chrome OS to an actual device and test if the bugs are fixed.
BUG=651930, 651916
Committed: https://crrev.com/480579a68ad8f86bd7e7b0d798d515550644fd9c
Cr-Commit-Position: refs/heads/master@{#431811}
Patch Set 1 #
Total comments: 4
Patch Set 2 : fix variable type #
Total comments: 4
Patch Set 3 : fix comments #
Total comments: 1
Patch Set 4 : address comments #
Messages
Total messages: 50 (39 generated)
Description was changed from ========== 651930 BUG=651930 ========== to ========== Fix the muted state of the volume strength icon When the thumb is dragged to the left, the volume strength icon does not set to muted state. BUG=651930 ==========
The CQ bit was checked by yiyix@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 the muted state of the volume strength icon When the thumb is dragged to the left, the volume strength icon does not set to muted state. BUG=651930 ========== to ========== Fix the muted state of the volume strength icon When the thumb is dragged to the left, the volume strength icon and the slider still shows that the audio is not muted. This is because the changed volume is less than 1% and it is not significant to show the change. BUG=651930 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yiyix@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...
The CQ bit was checked by yiyix@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...
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Fix the muted state of the volume strength icon When the thumb is dragged to the left, the volume strength icon and the slider still shows that the audio is not muted. This is because the changed volume is less than 1% and it is not significant to show the change. BUG=651930 ========== to ========== Fix the muted state of the volume strength icon - When the thumb is dragged to the left, the volume strength icon and the slider still shows that the audio is not muted. This bug is caused by the internal audio state is different from the slider volume state. - When user clicks on volume control slider, the volume strength icon is not updated accordingly. This is because the volume strength could change even when the percentage was small. TEST=MANUEL - Build the Chrome OS to an actual device and test if the bugs are fixed. BUG=651930, 651916 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix the muted state of the volume strength icon - When the thumb is dragged to the left, the volume strength icon and the slider still shows that the audio is not muted. This bug is caused by the internal audio state is different from the slider volume state. - When user clicks on volume control slider, the volume strength icon is not updated accordingly. This is because the volume strength could change even when the percentage was small. TEST=MANUEL - Build the Chrome OS to an actual device and test if the bugs are fixed. BUG=651930, 651916 ========== to ========== Fix the volume strength icon to reflect the volume control clicks - When the thumb is dragged to the left, the volume strength icon still shows that the audio is not muted. This bug is caused by the internal audio state which takes int only and is different from the slider volume state which takes floats. - When user clicks on volume control slider, the volume strength icon is not updated accordingly. This is because the volume strength icon was not called to update. TEST=MANUEL - Build the Chrome OS to an actual device and test if the bugs are fixed. BUG=651930, 651916 ==========
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
The CQ bit was checked by yiyix@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...
yiyix@chromium.org changed reviewers: + jamescook@chromium.org, tdanderson@chromium.org
@tdanderson, @jamescook, Could you please take a look at this change?
Description was changed from ========== Fix the volume strength icon to reflect the volume control clicks - When the thumb is dragged to the left, the volume strength icon still shows that the audio is not muted. This bug is caused by the internal audio state which takes int only and is different from the slider volume state which takes floats. - When user clicks on volume control slider, the volume strength icon is not updated accordingly. This is because the volume strength icon was not called to update. TEST=MANUEL - Build the Chrome OS to an actual device and test if the bugs are fixed. BUG=651930, 651916 ========== to ========== Fix the volume strength icon to reflect the volume control clicks - When the thumb is dragged to the left, the volume strength icon still shows that the audio is not muted. This bug is caused by the internal audio state, which takes int only, is different from the slider volume state, which takes floats only. - When user clicks on volume control slider, the volume strength icon is not updated accordingly. This is because the volume strength icon was not called to update. TEST=MANUEL - Build the Chrome OS to an actual device and test if the bugs are fixed. BUG=651930, 651916 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #1 (id:140001) has been deleted
The CQ bit was checked by yiyix@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2485353002/diff/160001/ash/common/system/chro... File ash/common/system/chromeos/audio/volume_view.cc (right): https://codereview.chromium.org/2485353002/diff/160001/ash/common/system/chro... ash/common/system/chromeos/audio/volume_view.cc:313: float current_volume = audio_delegate_->GetOutputVolumeLevel(); It looks like TrayAudioDelegate has an integer return type for GetOutputVolumeLevel(). Is keeping |current_volume| as a float intentional here? https://codereview.chromium.org/2485353002/diff/160001/ash/common/system/chro... File ash/common/system/chromeos/audio/volume_view.h (right): https://codereview.chromium.org/2485353002/diff/160001/ash/common/system/chro... ash/common/system/chromeos/audio/volume_view.h:6: #define ASH_COMMON_SYSTEM_CHROMEOS_AUDIO_VOLUME_VIEW_H_ spelling nit: TEST=MANUAL in CL description
Description was changed from ========== Fix the volume strength icon to reflect the volume control clicks - When the thumb is dragged to the left, the volume strength icon still shows that the audio is not muted. This bug is caused by the internal audio state, which takes int only, is different from the slider volume state, which takes floats only. - When user clicks on volume control slider, the volume strength icon is not updated accordingly. This is because the volume strength icon was not called to update. TEST=MANUEL - Build the Chrome OS to an actual device and test if the bugs are fixed. BUG=651930, 651916 ========== to ========== Fix the volume strength icon to reflect the volume control clicks - When the thumb is dragged to the left, the volume strength icon still shows that the audio is not muted. This bug is caused by the internal audio state, which takes int only, is different from the slider volume state, which takes floats only. - When user clicks on volume control slider, the volume strength icon is not updated accordingly. This is because the volume strength icon was not called to update. TEST=MANUAL - Build the Chrome OS to an actual device and test if the bugs are fixed. BUG=651930, 651916 ==========
@tdanderson, @jamescook: could you please check it again? Thank you. https://codereview.chromium.org/2485353002/diff/160001/ash/common/system/chro... File ash/common/system/chromeos/audio/volume_view.cc (right): https://codereview.chromium.org/2485353002/diff/160001/ash/common/system/chro... ash/common/system/chromeos/audio/volume_view.cc:313: float current_volume = audio_delegate_->GetOutputVolumeLevel(); On 2016/11/11 18:30:24, tdanderson wrote: > It looks like TrayAudioDelegate has an integer return type for > GetOutputVolumeLevel(). Is keeping |current_volume| as a float intentional here? Sorry, I forget to update this type. There is no reason to keep it as a float. https://codereview.chromium.org/2485353002/diff/160001/ash/common/system/chro... File ash/common/system/chromeos/audio/volume_view.h (right): https://codereview.chromium.org/2485353002/diff/160001/ash/common/system/chro... ash/common/system/chromeos/audio/volume_view.h:6: #define ASH_COMMON_SYSTEM_CHROMEOS_AUDIO_VOLUME_VIEW_H_ On 2016/11/11 18:30:25, tdanderson wrote: > spelling nit: TEST=MANUAL in CL description Done.
One more comment below. LGTM unless James has any objections (I know he's working on the audio code currently) https://codereview.chromium.org/2485353002/diff/180001/ash/common/system/chro... File ash/common/system/chromeos/audio/volume_view.cc (right): https://codereview.chromium.org/2485353002/diff/180001/ash/common/system/chro... ash/common/system/chromeos/audio/volume_view.cc:316: if (std::abs(new_volume - current_volume) < 1) if both values are now ints, the only way this can be true is if they are equal. It seems you could simplify this to if (new_volume == current_volume) return; If you do this then I don't think the comment on line 314-5 would add any value so I suggest removing it.
LGTM after fixing tdanderson's nit https://codereview.chromium.org/2485353002/diff/180001/ash/common/system/chro... File ash/common/system/chromeos/audio/volume_view.cc (right): https://codereview.chromium.org/2485353002/diff/180001/ash/common/system/chro... ash/common/system/chromeos/audio/volume_view.cc:312: int new_volume = value * 100; Aside: I'm a little surprised that this doesn't generate a compiler warning about potential integer range problems.
https://codereview.chromium.org/2485353002/diff/180001/ash/common/system/chro... File ash/common/system/chromeos/audio/volume_view.cc (right): https://codereview.chromium.org/2485353002/diff/180001/ash/common/system/chro... ash/common/system/chromeos/audio/volume_view.cc:312: int new_volume = value * 100; On 2016/11/11 22:21:15, James Cook wrote: > Aside: I'm a little surprised that this doesn't generate a compiler warning > about potential integer range problems. Do you think adding a comment here would help the reader to understand? https://codereview.chromium.org/2485353002/diff/180001/ash/common/system/chro... ash/common/system/chromeos/audio/volume_view.cc:316: if (std::abs(new_volume - current_volume) < 1) On 2016/11/11 19:48:35, tdanderson wrote: > if both values are now ints, the only way this can be true is if they are equal. > It seems you could simplify this to if (new_volume == current_volume) return; > > If you do this then I don't think the comment on line 314-5 would add any value > so I suggest removing it. You are right. Done
optional nit, still lgtm https://codereview.chromium.org/2485353002/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/audio/volume_view.cc (right): https://codereview.chromium.org/2485353002/diff/200001/ash/common/system/chro... ash/common/system/chromeos/audio/volume_view.cc:312: int new_volume = value * 100; an explicit static_cast<int>(value * 100) would inform the reader that you thought about it
On 2016/11/11 22:30:26, James Cook wrote: > optional nit, still lgtm > > https://codereview.chromium.org/2485353002/diff/200001/ash/common/system/chro... > File ash/common/system/chromeos/audio/volume_view.cc (right): > > https://codereview.chromium.org/2485353002/diff/200001/ash/common/system/chro... > ash/common/system/chromeos/audio/volume_view.cc:312: int new_volume = value * > 100; > an explicit static_cast<int>(value * 100) would inform the reader that you > thought about it I agree, this would be better. Thanks.
The CQ bit was checked by yiyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2485353002/#ps220001 (title: "address comments")
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 the volume strength icon to reflect the volume control clicks - When the thumb is dragged to the left, the volume strength icon still shows that the audio is not muted. This bug is caused by the internal audio state, which takes int only, is different from the slider volume state, which takes floats only. - When user clicks on volume control slider, the volume strength icon is not updated accordingly. This is because the volume strength icon was not called to update. TEST=MANUAL - Build the Chrome OS to an actual device and test if the bugs are fixed. BUG=651930, 651916 ========== to ========== Fix the volume strength icon to reflect the volume control clicks - When the thumb is dragged to the left, the volume strength icon still shows that the audio is not muted. This bug is caused by the internal audio state, which takes int only, is different from the slider volume state, which takes floats only. - When user clicks on volume control slider, the volume strength icon is not updated accordingly. This is because the volume strength icon was not called to update. TEST=MANUAL - Build the Chrome OS to an actual device and test if the bugs are fixed. BUG=651930, 651916 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Fix the volume strength icon to reflect the volume control clicks - When the thumb is dragged to the left, the volume strength icon still shows that the audio is not muted. This bug is caused by the internal audio state, which takes int only, is different from the slider volume state, which takes floats only. - When user clicks on volume control slider, the volume strength icon is not updated accordingly. This is because the volume strength icon was not called to update. TEST=MANUAL - Build the Chrome OS to an actual device and test if the bugs are fixed. BUG=651930, 651916 ========== to ========== Fix the volume strength icon to reflect the volume control clicks - When the thumb is dragged to the left, the volume strength icon still shows that the audio is not muted. This bug is caused by the internal audio state, which takes int only, is different from the slider volume state, which takes floats only. - When user clicks on volume control slider, the volume strength icon is not updated accordingly. This is because the volume strength icon was not called to update. TEST=MANUAL - Build the Chrome OS to an actual device and test if the bugs are fixed. BUG=651930, 651916 Committed: https://crrev.com/480579a68ad8f86bd7e7b0d798d515550644fd9c Cr-Commit-Position: refs/heads/master@{#431811} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/480579a68ad8f86bd7e7b0d798d515550644fd9c Cr-Commit-Position: refs/heads/master@{#431811} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
