Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
144713008:
[AudioPlayer] Implement shuffle mode (Closed)
|
Created:
6 years, 10 months ago by yoshiki Modified:
6 years, 10 months ago Reviewers:
hirono CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[AudioPlayer] Implement shuffle mode
BUG=340948
TEST=manually tested
R=hirono@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248897
Patch Set 1 #
Total comments: 13
Patch Set 2 : Addressed the comments #
Total comments: 6
Patch Set 3 : Addressed the comments #
Total comments: 4
Patch Set 4 : Addressed a comment #
Total comments: 1
Patch Set 5 : rebase #
Messages
Total messages: 19 (0 generated)
hirono, PTAL. Thanks,
https://codereview.chromium.org/144713008/diff/50001/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/control_panel.js (right): https://codereview.chromium.org/144713008/diff/50001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/control_panel.js:96: /* nit: /** ? Same the followings. https://codereview.chromium.org/144713008/diff/50001/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/track_list.js (right): https://codereview.chromium.org/144713008/diff/50001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:128: * @param {Event} event Click event. nit: @param {boolean} keepCurrentTrack ... ? https://codereview.chromium.org/144713008/diff/50001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:137: this.playOrder = How about: this.tracks.map(function(unused, index) { return index; }); ? https://codereview.chromium.org/144713008/diff/50001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:198: * @return {AudioPlayer.TrackInfo} TrackInfo of the next track. If there is nit: @return {number} ... ? https://codereview.chromium.org/144713008/diff/50001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:205: var defaultTrack = Please rename it with defaultTrackIndex. https://codereview.chromium.org/144713008/diff/50001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:237: if (!(0 <= newPlayOrder && newPlayOrder < this.tracks.length)) If currentPlayOrder is -1, it would return true. Is it OK?
hirono, Thanks, PTAL? https://codereview.chromium.org/144713008/diff/50001/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/track_list.js (right): https://codereview.chromium.org/144713008/diff/50001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:128: * @param {Event} event Click event. On 2014/02/03 08:09:52, hirono wrote: > nit: @param {boolean} keepCurrentTrack ... ? Yes, Thanks! https://codereview.chromium.org/144713008/diff/50001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:137: this.playOrder = On 2014/02/03 08:09:52, hirono wrote: > How about: this.tracks.map(function(unused, index) { return index; }); ? Done. https://codereview.chromium.org/144713008/diff/50001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:198: * @return {AudioPlayer.TrackInfo} TrackInfo of the next track. If there is On 2014/02/03 08:09:52, hirono wrote: > nit: @return {number} ... ? Done. https://codereview.chromium.org/144713008/diff/50001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:205: var defaultTrack = On 2014/02/03 08:09:52, hirono wrote: > Please rename it with defaultTrackIndex. Done. https://codereview.chromium.org/144713008/diff/50001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:237: if (!(0 <= newPlayOrder && newPlayOrder < this.tracks.length)) On 2014/02/03 08:09:52, hirono wrote: > If currentPlayOrder is -1, it would return true. > Is it OK? It's ok. Added the comment to explain this.
Thanks! https://codereview.chromium.org/144713008/diff/50001/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/track_list.js (right): https://codereview.chromium.org/144713008/diff/50001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:237: if (!(0 <= newPlayOrder && newPlayOrder < this.tracks.length)) On 2014/02/03 09:38:41, yoshiki wrote: > On 2014/02/03 08:09:52, hirono wrote: > > If currentPlayOrder is -1, it would return true. > > Is it OK? > > It's ok. Added the comment to explain this. Maybe I lost the point. Can we ensure currentPlayOrder is not -1? this.playOrder.indexOf() may return -1. If we cannot ensure, newPlayerOrder can become 0 as a result of currentPlayOrder + 1. It seems an unexpected behavior. https://codereview.chromium.org/144713008/diff/110001/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/track_list.js (right): https://codereview.chromium.org/144713008/diff/110001/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:242: // The below "if (!(...))" is intentional, to catch undefined and NaN. When the value can be undefined or NaN?
https://codereview.chromium.org/144713008/diff/50001/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/track_list.js (right): https://codereview.chromium.org/144713008/diff/50001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:237: if (!(0 <= newPlayOrder && newPlayOrder < this.tracks.length)) On 2014/02/03 09:57:48, hirono wrote: > On 2014/02/03 09:38:41, yoshiki wrote: > > On 2014/02/03 08:09:52, hirono wrote: > > > If currentPlayOrder is -1, it would return true. > > > Is it OK? > > > > It's ok. Added the comment to explain this. > > Maybe I lost the point. > > Can we ensure currentPlayOrder is not -1? this.playOrder.indexOf() may return > -1. > If we cannot ensure, newPlayerOrder can become 0 as a result of currentPlayOrder > + 1. It seems an unexpected behavior. That's good point. It shouldn't be occurred, but added the assertion.
https://codereview.chromium.org/144713008/diff/110001/chrome/browser/resource... > File chrome/browser/resources/file_manager/audio_player/elements/track_list.js > (right): > > https://codereview.chromium.org/144713008/diff/110001/chrome/browser/resource... > chrome/browser/resources/file_manager/audio_player/elements/track_list.js:242: > // The below "if (!(...))" is intentional, to catch undefined and NaN. > When the value can be undefined or NaN? It should be occurred when forward is undefined (when forget to specify the argument). To be clear, adds the check as the assertion instead.
https://codereview.chromium.org/144713008/diff/180001/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/track_list.js (right): https://codereview.chromium.org/144713008/diff/180001/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:207: * @return {number} The next track list. Next track index? Please add a comment for the case when the tracks is empty. https://codereview.chromium.org/144713008/diff/180001/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:209: getNextTrackIndex: function(forward) { Some lines of getNextTrackIndex and isNextTrackAvailable are duplicated. How about adding a 'circulation' flag argument to the method and returns getNextTrackIndex(forward, false) !== -1 at the isNextTrackAvailable? https://codereview.chromium.org/144713008/diff/180001/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:249: console.assert((forward !== undefined), We usually does not check if arguments are really specified or not. If the forward argument is dropped, it is just evaluated as false at #261. I think we don't have to add assertions.
hirono, PTAL. Thanks https://codereview.chromium.org/144713008/diff/180001/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/track_list.js (right): https://codereview.chromium.org/144713008/diff/180001/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:207: * @return {number} The next track list. On 2014/02/03 11:14:50, hirono wrote: > Next track index? > Please add a comment for the case when the tracks is empty. Done. https://codereview.chromium.org/144713008/diff/180001/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:209: getNextTrackIndex: function(forward) { On 2014/02/03 11:14:50, hirono wrote: > Some lines of getNextTrackIndex and isNextTrackAvailable are duplicated. How > about adding a 'circulation' flag argument to the method and returns > getNextTrackIndex(forward, false) !== -1 at the isNextTrackAvailable? Good idea! Done. https://codereview.chromium.org/144713008/diff/180001/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:249: console.assert((forward !== undefined), On 2014/02/03 11:14:50, hirono wrote: > We usually does not check if arguments are really specified or not. > If the forward argument is dropped, it is just evaluated as false at #261. > I think we don't have to add assertions. Honestly, I think we should add some check of the arguments and invariant on same ways in javascript (as we do it with DCHECK in C++). But as for now, remove it for consistency with the code around.
https://codereview.chromium.org/144713008/diff/270001/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/audio_player.js (right): https://codereview.chromium.org/144713008/diff/270001/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:199: this.trackList.getNextTrackIndex(forward, repeat); this.trackList.getNextTrackIndex(...) !== -1; https://codereview.chromium.org/144713008/diff/270001/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/control_panel.js (right): https://codereview.chromium.org/144713008/diff/270001/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/control_panel.js:96: /* nit: /** ? Same for the followings.
hirono, PTAL. https://codereview.chromium.org/144713008/diff/270001/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/audio_player.js (right): https://codereview.chromium.org/144713008/diff/270001/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:199: this.trackList.getNextTrackIndex(forward, repeat); On 2014/02/04 11:21:06, hirono wrote: > this.trackList.getNextTrackIndex(...) !== -1; Done. https://codereview.chromium.org/144713008/diff/270001/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/control_panel.js (right): https://codereview.chromium.org/144713008/diff/270001/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/control_panel.js:96: /* On 2014/02/04 11:21:06, hirono wrote: > nit: /** ? > Same for the followings. Done.
On 2014/02/04 11:39:21, yoshiki wrote: > hirono, PTAL. > > https://codereview.chromium.org/144713008/diff/270001/chrome/browser/resource... > File chrome/browser/resources/file_manager/audio_player/elements/audio_player.js > (right): > > https://codereview.chromium.org/144713008/diff/270001/chrome/browser/resource... > chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:199: > this.trackList.getNextTrackIndex(forward, repeat); > On 2014/02/04 11:21:06, hirono wrote: > > this.trackList.getNextTrackIndex(...) !== -1; > > Done. > > https://codereview.chromium.org/144713008/diff/270001/chrome/browser/resource... > File > chrome/browser/resources/file_manager/audio_player/elements/control_panel.js > (right): > > https://codereview.chromium.org/144713008/diff/270001/chrome/browser/resource... > chrome/browser/resources/file_manager/audio_player/elements/control_panel.js:96: > /* > On 2014/02/04 11:21:06, hirono wrote: > > nit: /** ? > > Same for the followings. > > Done. lgtm! Thanks!
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/144713008/280004
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
https://codereview.chromium.org/144713008/diff/280004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/track_list.js (right): https://codereview.chromium.org/144713008/diff/280004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:140: this.playOrder = JFYI: I found the idiom: Object.keys(this.tracks).map(Number); in list_selection_model.js.
On 2014/02/05 06:22:22, hirono wrote: > https://codereview.chromium.org/144713008/diff/280004/chrome/browser/resource... > File chrome/browser/resources/file_manager/audio_player/elements/track_list.js > (right): > > https://codereview.chromium.org/144713008/diff/280004/chrome/browser/resource... > chrome/browser/resources/file_manager/audio_player/elements/track_list.js:140: > this.playOrder = > JFYI: I found the idiom: > > Object.keys(this.tracks).map(Number); > > in list_selection_model.js. It may works here, but we shouldn't use it because it doesn't work if Array.prototype is extended.
Message was sent while issue was closed.
Committed patchset #5 manually as r248897 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
