Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
144883002:
[Files.app] Initial implementation of new audio player (Closed)
|
Created:
6 years, 11 months ago by yoshiki Modified:
6 years, 10 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, arv+watch_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Files.app] Initial implementation of new audio player
Implements the new audio player of Files.app. See the issue for detail. Although it has still room to implement and improve, but basically it works.
BUG=273308
TEST=manually tested
R=dgozman@chromium.org, mtomasz@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247833
Patch Set 1 #Patch Set 2 : Separete the image files from this patch #
Total comments: 122
Patch Set 3 : Addressed comments #
Total comments: 46
Patch Set 4 : Addressed the comments #
Total comments: 4
Patch Set 5 : Addressed the comments #
Total comments: 2
Patch Set 6 : Fix the crash #Patch Set 7 : Update the comments #Patch Set 8 : rebase #
Total comments: 2
Patch Set 9 : Make the script/css files flattened. #Messages
Total messages: 27 (0 generated)
@mtomasz: Sorry for the very big patch, but could you take a first look? Please feel free to ask me if you have a question. Thanks.
On 2014/01/23 01:52:59, yoshiki wrote: > @mtomasz: Sorry for the very big patch, but could you take a first look? Please > feel free to ask me if you have a question. Thanks. Hm, I can't patch it locally. Do you know the way I could patch it to test locally? I think the problem may be the binary files.
On 2014/01/23 04:11:25, mtomasz wrote: > On 2014/01/23 01:52:59, yoshiki wrote: > > @mtomasz: Sorry for the very big patch, but could you take a first look? > Please > > feel free to ask me if you have a question. Thanks. > > Hm, I can't patch it locally. Do you know the way I could patch it to test > locally? I think the problem may be the binary files. It should be due to binary files. Current rietveld doesn't support applying a patch which contains binary file. I removed the image files and zip'd them. Could you extract it and copy it to 'assets' directory manually? https://drive.google.com/a/google.com/file/d/0B2rbPi65SRFwcGo3MmZQQ2FQZ0k/edi... And I'll create a separated patch which contains the image files removed from this patch soon.
Looks awesome! https://codereview.chromium.org/144883002/diff/60016/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/144883002/diff/60016/chrome/app/generated_res... chrome/app/generated_resources.grd:12099: + Use experimental new audio player instead of stable one. nit: new -> the new nit: stable one -> the stable one https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player.html (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player.html:2: -- Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: Without (c). https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player.html:15: href="foreground/images/media/audio_player.png"> Setting an icon this way doesn't work for packaged apps' panel windows. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player.html:146: <!-- Volume botton in the bottom line --> nit: period (.) at the end of sentences in comments. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player.html:158: <!-- Playlist botton in the bottom line --> typo: buttons https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player.html:162: on-click="{{laylistButtonClick}}"> nit: laylist -> playlist. However, somehow it works. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/css/audio_player.css (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/css/audio_player.css:1: /* Copyright (c) 2013 The Chromium Authors. All rights reserved. ditto (c) https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/css/audio_player.css:10: -webkit-box-orient: vertical; AFAIK -webkit-box is deprecated in favour of flex. I think we shouldn't use it in completely new code. WDYT? https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/css/audio_player.css:41: height: 16px; Can you please copy paste the scrollbar from Files app so it is consistent? On the mocks it looks the same. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/css/audio_player.css:56: padding-top: 100px; Is this needed? https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/css/audio_player.css:60: background-color: rgba(255,255,255,0.20); nit: (,,,) -> (, , ,) https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/audio_player.js (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:115: * @param {number} oldValue old value. nit: Please add a unit (ms?) https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:150: this.advance_(true, true); Can we use an enum instead? It is hard to understand, and remember the order. Or please add comments for the arguments. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:179: * This handler is registered in this.ready(). @private missing https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:189: * @param {boolean} forward True if next, false if previous. One @param missing. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:211: * cancelAutoAdvance_(). No indent for jsdoc descriptions. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:218: * Schedule automatic advance to the next track after a timeout. ditto: jsdoc missing https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:231: 3000); Why do we need a timeout? I think users don't expect 3 seconds of a gap between tracks. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:255: * This ends current operation including playback, and restarts playback if jsdoc missing. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:273: * Getter of 'tracks' property. ditto: jsdoc missing https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/control_panel.css (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:10: background-color: #FFF; nit: #FFF -> #fff https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:26: .audio-controls { The left margin is larger than the right in the control panel (buttons line). https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:36: .media-button { The hover state is missing comparing to mocks. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:106: } nit: \n missing https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/control_panel.js (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/control_panel.js:34: return ~~(time / 60000) + ':' + ('0' + ~~(time / 1000 % 60)).slice(-2); How about using toLocaleTimeString() on Date? It should simplify this crazy expression, and would be also locale friendly. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/control_panel.js:49: * Current elapsed time in the current music in second. Seconds or milliseconds? Please confirm. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/track_list.css (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.css:1: /* Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: 2014 https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.css:4: The last track is not fully visible, if there is a scrollbar. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.css:25: #track-list, Since empty can we remove? https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.css:55: padding-left: 20px; nit: The padding is too large comparing to specs. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.css:76: z-index: 1; Why z-index is needed? https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/track_list.js (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:28: * track number. AFAIR we don't put indentations for the jsdoc description. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:35: * Invoked the current track index is changed. nit: the -> when the https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:51: return; nit: return not needed https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:62: if (oldValue != newValue) { nit: != -> !== https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:83: this.tracks[this.currentTrackIndex].active = true; Do we need to set the previous track as inactive? https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:97: * Please be consistent with new lines after jsdoc description. Here it is, but in #35 there is none (both are 3-lines length). I put \n when jsdoc.lines.length > 3. Any way is fine, but I think we should keep consistency. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:102: var index = -1; Can we use Array.indexOf? https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:115: * nit: ditto https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:122: if (this.currentTrackIndex < 0 || How about moving this if from the getter to changing handler in #79? https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:131: * Returns the next ir prev track in the track list. typo: ir https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:133: * @param {boolean} forward Specify direction: forward or prev mode. Can we use an enum for the direction? https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:185: })(); // Anonymosus closure typo: Anonymous https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/volume_controller.css (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/volume_controller.css:4: nit: Shadow for the volume controller is too strong comparing to the mocks. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/volume_controller.css:35: url(../assets/100/player_timeline_handler.png) 2x); 100 -> 200, please double check other places too. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/volume_controller.css:49: z-index: 1; Is the z-index needed? https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/volume_controller.js (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/volume_controller.js:49: * called. Dynamic change is not supprted. typo: supported https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/js/audio_player.js (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:8: * TODO(mtomasz): Rewrite the entire audio player. Please remove. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:108: this.trackListItems_.splice(0); indentation is off https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:277: /** Are all of these constants still necessary? https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:405: this.artist = PathUtil.basename(entry.fullPath); PathUtil.basename(entry.fullPath); -> entry.name https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:406: this.title = this.getDefaultArtist(); Is it correct? Isn't title with artist swapped? https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/js/audio_player_scripts.js (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/js/audio_player_scripts.js:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. ditto 2014 https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/background/js/background.js (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/background.js:670: var audioPlayerLaunchHandler = new AsyncUtil.Group(); Please add comments, what's the purpose of it. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/background.js:672: chrome.commandLinePrivate.hasSwitch( I think this is racy. launchAudioPlayer() may be called, before hasSwitch finishes. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/background.js:692: audioPlayerLaunchHandler.run(); Please add comments, this is not easy to understand. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/background.js:704: audioPlayerLaunchHandler.add(launchAudioPlayer.bind(null, playlist)); What's the purpose of this group? Is it needed? https://codereview.chromium.org/144883002/diff/60016/chromeos/chromeos_switch... File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/144883002/diff/60016/chromeos/chromeos_switch... chromeos/chromeos_switches.cc:130: // Shows the selecting checkboxes in the Files.app. The comment is wrong. https://codereview.chromium.org/144883002/diff/60016/chromeos/chromeos_switch... chromeos/chromeos_switches.cc:132: "file-manager-enable-new-audio-player"; In JS it is just --new-audio-player. Please choose one.
@mtomasz: Thanks for reviewing huge code. PTAL? https://codereview.chromium.org/144883002/diff/60016/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/144883002/diff/60016/chrome/app/generated_res... chrome/app/generated_resources.grd:12099: + Use experimental new audio player instead of stable one. On 2014/01/23 08:05:09, mtomasz wrote: > nit: new -> the new > nit: stable one -> the stable one Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player.html (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player.html:2: -- Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2014/01/23 08:05:09, mtomasz wrote: > nit: Without (c). Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player.html:15: href="foreground/images/media/audio_player.png"> On 2014/01/23 08:05:09, mtomasz wrote: > Setting an icon this way doesn't work for packaged apps' panel windows. Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player.html:146: <!-- Volume botton in the bottom line --> On 2014/01/23 08:05:09, mtomasz wrote: > nit: period (.) at the end of sentences in comments. Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player.html:158: <!-- Playlist botton in the bottom line --> On 2014/01/23 08:05:09, mtomasz wrote: > typo: buttons Done. And here 'button' should be singular. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player.html:162: on-click="{{laylistButtonClick}}"> On 2014/01/23 08:05:09, mtomasz wrote: > nit: laylist -> playlist. However, somehow it works. This handler is not necessary. Removed. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/css/audio_player.css (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/css/audio_player.css:1: /* Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2014/01/23 08:05:09, mtomasz wrote: > ditto (c) Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/css/audio_player.css:10: -webkit-box-orient: vertical; On 2014/01/23 08:05:09, mtomasz wrote: > AFAIK -webkit-box is deprecated in favour of flex. I think we shouldn't use it > in completely new code. WDYT? Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/css/audio_player.css:41: height: 16px; On 2014/01/23 08:05:09, mtomasz wrote: > Can you please copy paste the scrollbar from Files app so it is consistent? On > the mocks it looks the same. Scrollbar in Files.app doesn't work here for some reason (maybe due to shadowdom?). I'll investigate farther, but let me keep it as is in this patch. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/css/audio_player.css:56: padding-top: 100px; On 2014/01/23 08:05:09, mtomasz wrote: > Is this needed? Removed. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/css/audio_player.css:60: background-color: rgba(255,255,255,0.20); On 2014/01/23 08:05:09, mtomasz wrote: > nit: (,,,) -> (, , ,) Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/audio_player.js (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:115: * @param {number} oldValue old value. On 2014/01/23 08:05:09, mtomasz wrote: > nit: Please add a unit (ms?) Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:150: this.advance_(true, true); On 2014/01/23 08:05:09, mtomasz wrote: > Can we use an enum instead? It is hard to understand, and remember the order. Or > please add comments for the arguments. Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:179: * This handler is registered in this.ready(). On 2014/01/23 08:05:09, mtomasz wrote: > @private missing Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:189: * @param {boolean} forward True if next, false if previous. On 2014/01/23 08:05:09, mtomasz wrote: > One @param missing. Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:211: * cancelAutoAdvance_(). On 2014/01/23 08:05:09, mtomasz wrote: > No indent for jsdoc descriptions. Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:218: * Schedule automatic advance to the next track after a timeout. On 2014/01/23 08:05:09, mtomasz wrote: > ditto: jsdoc missing Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:231: 3000); On 2014/01/23 08:05:09, mtomasz wrote: > Why do we need a timeout? I think users don't expect 3 seconds of a gap between > tracks. This gap happens when error is happened and the player about to go to next track. This is same behavior as the previous music player. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:255: * This ends current operation including playback, and restarts playback if On 2014/01/23 08:05:09, mtomasz wrote: > jsdoc missing. Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:273: * Getter of 'tracks' property. On 2014/01/23 08:05:09, mtomasz wrote: > ditto: jsdoc missing Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/control_panel.css (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:10: background-color: #FFF; On 2014/01/23 08:05:09, mtomasz wrote: > nit: #FFF -> #fff Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:26: .audio-controls { On 2014/01/23 08:05:09, mtomasz wrote: > The left margin is larger than the right in the control panel (buttons line). Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:36: .media-button { On 2014/01/23 08:05:09, mtomasz wrote: > The hover state is missing comparing to mocks. Mocks don't have the hover state. (but I've just noticed I've missed to implement pressed state. I'll do it in another patch.) https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:106: } On 2014/01/23 08:05:09, mtomasz wrote: > nit: \n missing Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/control_panel.js (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/control_panel.js:34: return ~~(time / 60000) + ':' + ('0' + ~~(time / 1000 % 60)).slice(-2); On 2014/01/23 08:05:09, mtomasz wrote: > How about using toLocaleTimeString() on Date? It should simplify this crazy > expression, and would be also locale friendly. This "time" here is not a time of date but time duration (length of time). I think toLocaleTimeString() is not good to use here. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/control_panel.js:49: * Current elapsed time in the current music in second. On 2014/01/23 08:05:09, mtomasz wrote: > Seconds or milliseconds? Please confirm. Milliseconds. Fixed. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/track_list.css (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.css:1: /* Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/01/23 08:05:09, mtomasz wrote: > nit: 2014 Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.css:4: On 2014/01/23 08:05:09, mtomasz wrote: > The last track is not fully visible, if there is a scrollbar. Fixed the height of bottom margin. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.css:25: #track-list, On 2014/01/23 08:05:09, mtomasz wrote: > Since empty can we remove? Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.css:55: padding-left: 20px; On 2014/01/23 08:05:09, mtomasz wrote: > nit: The padding is too large comparing to specs. Removed the padding in .track.data. Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.css:76: z-index: 1; On 2014/01/23 08:05:09, mtomasz wrote: > Why z-index is needed? Sorry, removed unnecessary styles. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/track_list.js (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:28: * track number. On 2014/01/23 08:05:09, mtomasz wrote: > AFAIR we don't put indentations for the jsdoc description. Yes, you're correct. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:35: * Invoked the current track index is changed. On 2014/01/23 08:05:09, mtomasz wrote: > nit: the -> when the Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:51: return; On 2014/01/23 08:05:09, mtomasz wrote: > nit: return not needed Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:62: if (oldValue != newValue) { On 2014/01/23 08:05:09, mtomasz wrote: > nit: != -> !== Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:83: this.tracks[this.currentTrackIndex].active = true; On 2014/01/23 08:05:09, mtomasz wrote: > Do we need to set the previous track as inactive? In this case, the index of current track is not changed. What is changed is a value in the this.tracks array. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:97: * On 2014/01/23 08:05:09, mtomasz wrote: > Please be consistent with new lines after jsdoc description. Here it is, but in > #35 there is none (both are 3-lines length). I put \n when jsdoc.lines.length > > 3. Any way is fine, but I think we should keep consistency. Removed the blank line. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:102: var index = -1; On 2014/01/23 08:05:09, mtomasz wrote: > Can we use Array.indexOf? I've just realized comparing URLs is better here. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:115: * On 2014/01/23 08:05:09, mtomasz wrote: > nit: ditto Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:122: if (this.currentTrackIndex < 0 || On 2014/01/23 08:05:09, mtomasz wrote: > How about moving this if from the getter to changing handler in #79? This check is already done in l.46. Removed it here. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:131: * Returns the next ir prev track in the track list. On 2014/01/23 08:05:09, mtomasz wrote: > typo: ir Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:133: * @param {boolean} forward Specify direction: forward or prev mode. On 2014/01/23 08:05:09, mtomasz wrote: > Can we use an enum for the direction? I think boolean is enough, since this takes only two values. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:185: })(); // Anonymosus closure On 2014/01/23 08:05:09, mtomasz wrote: > typo: Anonymous Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/volume_controller.css (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/volume_controller.css:4: On 2014/01/23 08:05:09, mtomasz wrote: > nit: Shadow for the volume controller is too strong comparing to the mocks. Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/volume_controller.css:35: url(../assets/100/player_timeline_handler.png) 2x); On 2014/01/23 08:05:09, mtomasz wrote: > 100 -> 200, please double check other places too. Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/volume_controller.css:49: z-index: 1; On 2014/01/23 08:05:09, mtomasz wrote: > Is the z-index needed? Removed. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/volume_controller.js (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/volume_controller.js:49: * called. Dynamic change is not supprted. On 2014/01/23 08:05:09, mtomasz wrote: > typo: supported Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/js/audio_player.js (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:8: * TODO(mtomasz): Rewrite the entire audio player. On 2014/01/23 08:05:09, mtomasz wrote: > Please remove. Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:108: this.trackListItems_.splice(0); On 2014/01/23 08:05:09, mtomasz wrote: > indentation is off Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:277: /** On 2014/01/23 08:05:09, mtomasz wrote: > Are all of these constants still necessary? Yes, they are necessary to calculate default window size and expanded window size. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:405: this.artist = PathUtil.basename(entry.fullPath); On 2014/01/23 08:05:09, mtomasz wrote: > PathUtil.basename(entry.fullPath); -> entry.name Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:406: this.title = this.getDefaultArtist(); On 2014/01/23 08:05:09, mtomasz wrote: > Is it correct? Isn't title with artist swapped? Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/js/audio_player_scripts.js (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/js/audio_player_scripts.js:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2014/01/23 08:05:09, mtomasz wrote: > ditto 2014 Done. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/background/js/background.js (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/background.js:670: var audioPlayerLaunchHandler = new AsyncUtil.Group(); On 2014/01/23 08:05:09, mtomasz wrote: > Please add comments, what's the purpose of it. Added a comment and changed the name of the variable. It prevents the race. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/background.js:672: chrome.commandLinePrivate.hasSwitch( On 2014/01/23 08:05:09, mtomasz wrote: > I think this is racy. launchAudioPlayer() may be called, before hasSwitch > finishes. It won't be happened. In the case that the 'audioPlayer' object is not initializad, the code is stored into 'audioPlayerLaunchHandler' as a closure instead. https://codereview.chromium.org/144883002/diff/60016/chromeos/chromeos_switch... File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/144883002/diff/60016/chromeos/chromeos_switch... chromeos/chromeos_switches.cc:130: // Shows the selecting checkboxes in the Files.app. On 2014/01/23 08:05:09, mtomasz wrote: > The comment is wrong. Done. https://codereview.chromium.org/144883002/diff/60016/chromeos/chromeos_switch... chromeos/chromeos_switches.cc:132: "file-manager-enable-new-audio-player"; On 2014/01/23 08:05:09, mtomasz wrote: > In JS it is just --new-audio-player. Please choose one. Done.
https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player.html (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player.html:15: href="foreground/images/media/audio_player.png"> On 2014/01/24 15:34:19, yoshiki wrote: > On 2014/01/23 08:05:09, mtomasz wrote: > > Setting an icon this way doesn't work for packaged apps' panel windows. > > Done. typo: plauer -> player. I think we can remove it. Does this icon display anywhere? https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/css/audio_player.css (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/css/audio_player.css:41: height: 16px; On 2014/01/24 15:34:19, yoshiki wrote: > On 2014/01/23 08:05:09, mtomasz wrote: > > Can you please copy paste the scrollbar from Files app so it is consistent? On > > the mocks it looks the same. > > Scrollbar in Files.app doesn't work here for some reason (maybe due to > shadowdom?). I'll investigate farther, but let me keep it as is in this patch. SGTM. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/audio_player.js (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:211: * cancelAutoAdvance_(). On 2014/01/24 15:34:19, yoshiki wrote: > On 2014/01/23 08:05:09, mtomasz wrote: > > No indent for jsdoc descriptions. > > Done. The indent still looks off. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/control_panel.css (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:284: margin-left: 10px; Does it work in RTL? Please use -webkit-margin-start/end https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:319: margin-left: 10px; We can put -webkit-margin-start on .media-button, and -webkit-margin-end on .media-button:last-child to avoid duplication. https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/control_panel.js (right): https://codereview.chromium.org/144883002/diff/60016/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/control_panel.js:34: return ~~(time / 60000) + ':' + ('0' + ~~(time / 1000 % 60)).slice(-2); On 2014/01/24 15:34:19, yoshiki wrote: > On 2014/01/23 08:05:09, mtomasz wrote: > > How about using toLocaleTimeString() on Date? It should simplify this crazy > > expression, and would be also locale friendly. > > This "time" here is not a time of date but time duration (length of time). I > think toLocaleTimeString() is not good to use here. toLocaleTimeString() whould work very well with durations. The current code is cool, but very complicated and not readable. It is like reinventing the wheel IMHO. Also, I'm not sure if all locales use : as a time separator. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player.html (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player.html:158: <!-- Playlist button in the bottom line --> nit: period missing at the end of some comments, which are sentences. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/audio_player.js (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:220: * Schedule automatic advance to the next track after a timeout. Schedule -> Schedules https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:239: * Cancel the scheduled auto advance. Cancel -> Cancels and so on for method names. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:249: set currentTrackIndex(value) { nit: Please add a simple jsdoc. Without description is fine, but type is helpful. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:289: * Returns whether the track list is expanded or not. jsdoc missing https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:296: * Expands or collapse the track list. ditto, please check jsdocs. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/control_panel.css (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:7: align-items: center; Thx for the flex! https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:98: url('../assets/100/player_timeline_handler.png') 2x); 100 -> 200. Please check other places. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:104: url('../assets/100/player_timeline_handler_pressed.png') 2x); ditto https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:397: left: 0; Will it work for RTL? https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/control_panel.js (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/control_panel.js:9: * Move |target| element above |anchor| element, in order to match the nit: Move -> Moves https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/track_list.js (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:1: // Cpyright 2014 The Chromium Authors. All rights reserved. typo: Copyright https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:123: * Returns the next (or prev) track in the track list. nit: prev -> previous https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/volume_controller.css (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/volume_controller.css:1: /* Cpyright 2014 The Chromium Authors. All rights reserved. typo https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/volume_controller.css:27: z-index: 2; Is this z-index necessary? https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/volume_controller.js (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/volume_controller.js:1: // Cpyright 2014 The Chromium Authors. All rights reserved. typo https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/js/audio_player.js (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:359: this.title = PathUtil.basename(entry.name); PathUtil.basename is a no-op here. Please use entry.name directly. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:403: this.title = PathUtil.basename(entry.fullPath); entry.name can be used instead https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/background/js/background.js (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/background/js/background.js:671: // is finished, not to use an audioPlayer object before initializiation. I think it would simpler to execute hasSwitch and launchAudioPlayer in the same queue. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/background/js/background.js:707: closuresDeferredUntilAudioPlayerInitialization.add( AsyncGroup.Queue doesn't have add() method. https://codereview.chromium.org/144883002/diff/140004/chromeos/chromeos_switc... File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/144883002/diff/140004/chromeos/chromeos_switc... chromeos/chromeos_switches.cc:132: "enable-new-audio-player"; All of the flags follow the pattern, that the constant name is same as its value, but not this one flag. I think we should be consistent. Also, since all of the file manager flags have the file manager, prefix, I think we should keep it. Sorry for not being clear earlier.
@mtomasz: Thanks. PTAL https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player.html (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player.html:158: <!-- Playlist button in the bottom line --> On 2014/01/27 01:09:24, mtomasz wrote: > nit: period missing at the end of some comments, which are sentences. Done. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/audio_player.js (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:220: * Schedule automatic advance to the next track after a timeout. On 2014/01/27 01:09:24, mtomasz wrote: > Schedule -> Schedules Done. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:239: * Cancel the scheduled auto advance. On 2014/01/27 01:09:24, mtomasz wrote: > Cancel -> Cancels and so on for method names. Done. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:249: set currentTrackIndex(value) { On 2014/01/27 01:09:24, mtomasz wrote: > nit: Please add a simple jsdoc. Without description is fine, but type is > helpful. Done. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:289: * Returns whether the track list is expanded or not. On 2014/01/27 01:09:24, mtomasz wrote: > jsdoc missing Done. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/audio_player.js:296: * Expands or collapse the track list. On 2014/01/27 01:09:24, mtomasz wrote: > ditto, please check jsdocs. Done. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/control_panel.css (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:98: url('../assets/100/player_timeline_handler.png') 2x); On 2014/01/27 01:09:24, mtomasz wrote: > 100 -> 200. Please check other places. Thanks, Done. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:104: url('../assets/100/player_timeline_handler_pressed.png') 2x); On 2014/01/27 01:09:24, mtomasz wrote: > ditto Done. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:397: left: 0; On 2014/01/27 01:09:24, mtomasz wrote: > Will it work for RTL? The audio player must be displayed as LTR even when the display language is RTL. We don't need to care about RTL language. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/control_panel.js (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/control_panel.js:9: * Move |target| element above |anchor| element, in order to match the On 2014/01/27 01:09:24, mtomasz wrote: > nit: Move -> Moves Done. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/track_list.js (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:1: // Cpyright 2014 The Chromium Authors. All rights reserved. On 2014/01/27 01:09:24, mtomasz wrote: > typo: Copyright Done. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:123: * Returns the next (or prev) track in the track list. On 2014/01/27 01:09:24, mtomasz wrote: > nit: prev -> previous Done. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/volume_controller.css (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/volume_controller.css:1: /* Cpyright 2014 The Chromium Authors. All rights reserved. On 2014/01/27 01:09:24, mtomasz wrote: > typo Done. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/volume_controller.css:27: z-index: 2; On 2014/01/27 01:09:24, mtomasz wrote: > Is this z-index necessary? We need to put the <input> element over the bar elements in the volume slider. It is necessary to do it. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/volume_controller.js (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/volume_controller.js:1: // Cpyright 2014 The Chromium Authors. All rights reserved. On 2014/01/27 01:09:24, mtomasz wrote: > typo Done. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/js/audio_player.js (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:359: this.title = PathUtil.basename(entry.name); On 2014/01/27 01:09:24, mtomasz wrote: > PathUtil.basename is a no-op here. Please use entry.name directly. Done. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:403: this.title = PathUtil.basename(entry.fullPath); On 2014/01/27 01:09:24, mtomasz wrote: > entry.name can be used instead Done. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/background/js/background.js (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/background/js/background.js:671: // is finished, not to use an audioPlayer object before initializiation. On 2014/01/27 01:09:24, mtomasz wrote: > I think it would simpler to execute hasSwitch and launchAudioPlayer in the same > queue. The queue is never necessary after the initialization is complete. I know the code become a bit complex, but I think it's better not to use the queue when unnecessary for memory and overheads. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/background/js/background.js:707: closuresDeferredUntilAudioPlayerInitialization.add( On 2014/01/27 01:09:24, mtomasz wrote: > AsyncGroup.Queue doesn't have add() method. Thanks. Done. https://codereview.chromium.org/144883002/diff/140004/chromeos/chromeos_switc... File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/144883002/diff/140004/chromeos/chromeos_switc... chromeos/chromeos_switches.cc:132: "enable-new-audio-player"; On 2014/01/27 01:09:24, mtomasz wrote: > All of the flags follow the pattern, that the constant name is same as its > value, but not this one flag. I think we should be consistent. > > Also, since all of the file manager flags have the file manager, prefix, I think > we should keep it. Sorry for not being clear earlier. Done.
https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/control_panel.css (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:397: left: 0; On 2014/01/27 07:12:08, yoshiki wrote: > On 2014/01/27 01:09:24, mtomasz wrote: > > Will it work for RTL? > > The audio player must be displayed as LTR even when the display language is RTL. > We don't need to care about RTL language. Any background for this decision? I thought we need to support RTL everywhere. https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/background/js/background.js (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/background/js/background.js:4: Running the Audio player more than once causes JS errors. Please check. https://codereview.chromium.org/144883002/diff/2000001/chrome/browser/resourc... File chrome/browser/resources/file_manager/background/js/background.js (right): https://codereview.chromium.org/144883002/diff/2000001/chrome/browser/resourc... chrome/browser/resources/file_manager/background/js/background.js:695: closuresDeferredUntilAudioPlayerInitialization.run(); I think this should be removed. Queue.run() without a callback is not allowed. https://codereview.chromium.org/144883002/diff/2000001/chrome/browser/resourc... chrome/browser/resources/file_manager/background/js/background.js:707: closuresDeferredUntilAudioPlayerInitialization.run( I think this doesn't work as intended. Since the queue is empty, the closure will be executed immediately, causing an infinite recursion.
https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/control_panel.css (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:397: left: 0; On 2014/01/27 09:43:39, mtomasz wrote: > On 2014/01/27 07:12:08, yoshiki wrote: > > On 2014/01/27 01:09:24, mtomasz wrote: > > > Will it work for RTL? > > > > The audio player must be displayed as LTR even when the display language is > RTL. > > We don't need to care about RTL language. > > Any background for this decision? I thought we need to support RTL everywhere. Let me rephrase it correctly. Some components (eg. inspector, old audio player and image viewer in Files.app) support RTL language but their UI direction are LTR. Since the old audio players is so, I think it is enough for new audio player as for now. I'll work it as an another work and file an issue. (And I don't have enough knowledge about RTL and I have a question to ask it to someone, eg, which direction does the progress bar in the music player go?) https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/background/js/background.js (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/background/js/background.js:4: On 2014/01/27 09:43:39, mtomasz wrote: > Running the Audio player more than once causes JS errors. Please check. Fixed. I didn't notice errors in background. Thanks! https://codereview.chromium.org/144883002/diff/2000001/chrome/browser/resourc... File chrome/browser/resources/file_manager/background/js/background.js (right): https://codereview.chromium.org/144883002/diff/2000001/chrome/browser/resourc... chrome/browser/resources/file_manager/background/js/background.js:695: closuresDeferredUntilAudioPlayerInitialization.run(); On 2014/01/27 09:43:39, mtomasz wrote: > I think this should be removed. Queue.run() without a callback is not allowed. Thanks. I was confusing about Queue and Group. I covered the entire code with a queued closure as you said previous comment. https://codereview.chromium.org/144883002/diff/2000001/chrome/browser/resourc... chrome/browser/resources/file_manager/background/js/background.js:707: closuresDeferredUntilAudioPlayerInitialization.run( On 2014/01/27 09:43:39, mtomasz wrote: > I think this doesn't work as intended. Since the queue is empty, the closure > will be executed immediately, causing an infinite recursion. You're correct. I was confusing.
@mtomasz: PTAL
On 2014/01/28 05:58:01, yoshiki wrote: > @mtomasz: PTAL Please try this: 1. open files app 2. launch audio player 3. close audio player 4. launch audio player again If it works, then LGTM with a nit!
https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... File chrome/browser/resources/file_manager/audio_player/elements/control_panel.css (right): https://codereview.chromium.org/144883002/diff/140004/chrome/browser/resource... chrome/browser/resources/file_manager/audio_player/elements/control_panel.css:397: left: 0; On 2014/01/28 05:57:56, yoshiki wrote: > On 2014/01/27 09:43:39, mtomasz wrote: > > On 2014/01/27 07:12:08, yoshiki wrote: > > > On 2014/01/27 01:09:24, mtomasz wrote: > > > > Will it work for RTL? > > > > > > The audio player must be displayed as LTR even when the display language is > > RTL. > > > We don't need to care about RTL language. > > > > Any background for this decision? I thought we need to support RTL everywhere. > > Let me rephrase it correctly. Some components (eg. inspector, old audio player > and image viewer in Files.app) support RTL language but their UI direction are > LTR. > > Since the old audio players is so, I think it is enough for new audio player as > for now. I'll work it as an another work and file an issue. (And I don't have > enough knowledge about RTL and I have a question to ask it to someone, eg, which > direction does the progress bar in the music player go?) I received this guidance about RTL from UX guys: Flip everything. Basic RTL is easy. Details may be tricky, but we shouldn't worry about it in early stage I think. So, basically we should use -webkit-margin-start instead of margin-left, same for paddings. Instead of left: 0 absolute values, we should use flex. AFAIR this is enough. I'm fine to work on this separately, but please file a bug. https://codereview.chromium.org/144883002/diff/450002/chrome/browser/resource... File chrome/browser/resources/file_manager/background/js/background.js (right): https://codereview.chromium.org/144883002/diff/450002/chrome/browser/resource... chrome/browser/resources/file_manager/background/js/background.js:671: // Stores closures to be executed after the initialization of the audio player nit: Please update the comment.
On 2014/01/28 06:09:30, mtomasz wrote: > On 2014/01/28 05:58:01, yoshiki wrote: > > @mtomasz: PTAL > > Please try this: > 1. open files app > 2. launch audio player > 3. close audio player > 4. launch audio player again > > If it works, then LGTM with a nit! I found the crash in the C++ world (it seems to be in blink)? As for now, I added a hack preventing the crash in JavaScript world, and it works fine now.
https://codereview.chromium.org/144883002/diff/450002/chrome/browser/resource... File chrome/browser/resources/file_manager/background/js/background.js (right): https://codereview.chromium.org/144883002/diff/450002/chrome/browser/resource... chrome/browser/resources/file_manager/background/js/background.js:671: // Stores closures to be executed after the initialization of the audio player On 2014/01/28 06:09:45, mtomasz wrote: > nit: Please update the comment. Done.
On 2014/01/28 08:31:29, yoshiki wrote: > On 2014/01/28 06:09:30, mtomasz wrote: > > On 2014/01/28 05:58:01, yoshiki wrote: > > > @mtomasz: PTAL > > > > Please try this: > > 1. open files app > > 2. launch audio player > > 3. close audio player > > 4. launch audio player again > > > > If it works, then LGTM with a nit! > > I found the crash in the C++ world (it seems to be in blink)? As for now, I > added a hack preventing the crash in JavaScript world, and it works fine now. If it is a hack, then please add a comment mentioning that. Otherwise, we will never remove it. Besides, lgtm.
On 2014/01/28 08:35:32, mtomasz wrote: > On 2014/01/28 08:31:29, yoshiki wrote: > > On 2014/01/28 06:09:30, mtomasz wrote: > > > On 2014/01/28 05:58:01, yoshiki wrote: > > > > @mtomasz: PTAL > > > > > > Please try this: > > > 1. open files app > > > 2. launch audio player > > > 3. close audio player > > > 4. launch audio player again > > > > > > If it works, then LGTM with a nit! > > > > I found the crash in the C++ world (it seems to be in blink)? As for now, I > > added a hack preventing the crash in JavaScript world, and it works fine now. > > If it is a hack, then please add a comment mentioning that. Otherwise, we will > never remove it. > Besides, lgtm. I have added the comment, Thanks!
dgozman@chromium.org: Could you review component_extension_resources.grd? Thanks.
https://codereview.chromium.org/144883002/diff/2260001/chrome/browser/resourc... File chrome/browser/resources/component_extension_resources.grd (right): https://codereview.chromium.org/144883002/diff/2260001/chrome/browser/resourc... chrome/browser/resources/component_extension_resources.grd:96: <include name="IDR_FILE_MANAGER_AUDIO_PLAYER_ELEMENTS_TRACK_LIST_JS" file="file_manager/audio_player/elements/track_list.js" flattenhtml="false" type="BINDATA" /> You only need resources for files mentioned in manifest: html file, scripts used in background page, some images and audio_player_scrips.js for easy development. Everything else will be inlined by html flattener, including css.
dgozman@, PTAL. Thanks https://codereview.chromium.org/144883002/diff/2260001/chrome/browser/resourc... File chrome/browser/resources/component_extension_resources.grd (right): https://codereview.chromium.org/144883002/diff/2260001/chrome/browser/resourc... chrome/browser/resources/component_extension_resources.grd:96: <include name="IDR_FILE_MANAGER_AUDIO_PLAYER_ELEMENTS_TRACK_LIST_JS" file="file_manager/audio_player/elements/track_list.js" flattenhtml="false" type="BINDATA" /> On 2014/01/29 05:51:17, dgozman wrote: > You only need resources for files mentioned in manifest: html file, scripts used > in background page, some images and audio_player_scrips.js for easy development. > Everything else will be inlined by html flattener, including css. Thanks for guidance, Done.
component_extension_resources.grd lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/144883002/2300001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/144883002/2360001
Failed to get patchset properties (patchset not found?)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/144883002/2300001
Message was sent while issue was closed.
Committed patchset #9 manually as r247833 (presubmit successful).
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring. |
