Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2538403003:
Save and restore mute flag of volume level in Video Player (Closed)
|
Created:
4 years ago by yamaguchi Modified:
4 years ago Reviewers:
fukino CC:
chromium-reviews, posciak+watch_chromium.org, yamaguchi+watch_chromium.org, oka+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, fukino+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSave and restore mute flag of volume level in Video Player
BUG=669055
TEST=manual test as noted in the bug
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/d314956b2d1be54e6bdac64919d6a00a83e604a1
Cr-Commit-Position: refs/heads/master@{#435594}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Bugfix. Avoid having errornous value on drag. #Messages
Total messages: 23 (17 generated)
Description was changed from ========== Save and restore mute flag of volume level. BUG=669055 ========== to ========== Save and restore mute flag of volume level. BUG=669055 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by yamaguchi@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.
Description was changed from ========== Save and restore mute flag of volume level. BUG=669055 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Save and restore mute flag of volume level in Video Player BUG=669055 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Save and restore mute flag of volume level in Video Player BUG=669055 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Save and restore mute flag of volume level in Video Player BUG=669055 TEST=manual test as noted in the bug CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
yamaguchi@chromium.org changed reviewers: + fukino@chromium.org
lgtm with nits. https://codereview.chromium.org/2538403003/diff/1/ui/file_manager/video_playe... File ui/file_manager/video_player/js/media_controls.js (right): https://codereview.chromium.org/2538403003/diff/1/ui/file_manager/video_playe... ui/file_manager/video_player/js/media_controls.js:45: VolumeModel.prototype.moveSlider = function(value) { nit: moveSlider sounds like a function to move a slider. how about onVolumeChanged or something like that? https://codereview.chromium.org/2538403003/diff/1/ui/file_manager/video_playe... ui/file_manager/video_player/js/media_controls.js:64: VolumeModel.prototype.set = function(volume, mute) { Add annotation.
The CQ bit was checked by yamaguchi@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...
https://codereview.chromium.org/2538403003/diff/1/ui/file_manager/video_playe... File ui/file_manager/video_player/js/media_controls.js (right): https://codereview.chromium.org/2538403003/diff/1/ui/file_manager/video_playe... ui/file_manager/video_player/js/media_controls.js:45: VolumeModel.prototype.moveSlider = function(value) { On 2016/12/01 09:28:21, fukino wrote: > nit: moveSlider sounds like a function to move a slider. > how about onVolumeChanged or something like that? Done. https://codereview.chromium.org/2538403003/diff/1/ui/file_manager/video_playe... ui/file_manager/video_player/js/media_controls.js:64: VolumeModel.prototype.set = function(volume, mute) { On 2016/12/01 09:28:21, fukino wrote: > Add annotation. Done. https://codereview.chromium.org/2538403003/diff/1/ui/file_manager/video_playe... ui/file_manager/video_player/js/media_controls.js:667: this.volumeModel_.moveSlider(this.volume_.ratio); I don't think this handler should attempt to update the slider in the UI, because this event comes from the slider UI. This change causes that volume_.ratio can sometimes have a value out of the range. (e.g. max=100, value=100, but ratio=100.) Additionally, the both values (media_.volume and this.volume_.*) are not updated in every event, but the same value is read repeatedly. ( crbug.com/670192 ). So I think we shouldn't change the behavior of this handler now.
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 yamaguchi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fukino@chromium.org Link to the patchset: https://codereview.chromium.org/2538403003/#ps20001 (title: "Bugfix. Avoid having errornous value on drag.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480592443526010,
"parent_rev": "3dd416af29c2d8f7e5a01aea9d7a436405d94533", "commit_rev":
"7d1640da7ef4975b2ec29150ef68cd1db9c190c3"}
Message was sent while issue was closed.
Description was changed from ========== Save and restore mute flag of volume level in Video Player BUG=669055 TEST=manual test as noted in the bug CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Save and restore mute flag of volume level in Video Player BUG=669055 TEST=manual test as noted in the bug CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Save and restore mute flag of volume level in Video Player BUG=669055 TEST=manual test as noted in the bug CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Save and restore mute flag of volume level in Video Player BUG=669055 TEST=manual test as noted in the bug CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/d314956b2d1be54e6bdac64919d6a00a83e604a1 Cr-Commit-Position: refs/heads/master@{#435594} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d314956b2d1be54e6bdac64919d6a00a83e604a1 Cr-Commit-Position: refs/heads/master@{#435594} |
