Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
185653014:
[AudioPlayer] Fix a bug on changing 'expanded' status (Closed)
|
Created:
6 years, 9 months ago by yoshiki Modified:
6 years, 9 months ago Reviewers:
hirono CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org, yoshiki+watch_chromium.org, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[AudioPlayer] Fix a bug on changing 'expanded' status
The playlist goes wrong when the 'expanded' status is changed in some condition.
BUG=342610, chrome-os-partner:26202
TEST=manually tested
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255394
Patch Set 1 : . #
Total comments: 11
Patch Set 2 : Adressed the comment #
Total comments: 4
Patch Set 3 : Adressed the comment #
Messages
Total messages: 15 (0 generated)
hirono, PTAL. Thanks.
https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/track_list.js (right): https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:95: * Invoked when 'tracks' property is clicked. Please fix the comment. https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:165: } else if (viewTop <= elementTop && nit: The first condition check can be removed. https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:166: elementTop + elementHeight <= viewTop + viewHeight) { nit: How about negating the condition and put the fix of bottom coordinates in the second block? https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/js/audio_player.js (right): https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:123: Platform.performMicrotaskCheckpoint(); Just curious, what goes wrong without the line? I'm guessing it is needed to ensure to assign -1 to currentTrackIndex, but not sure. https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... File chrome/browser/resources/file_manager/background/js/background.js (right): https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/background.js:767: minHeight: newAudioPlayerEnabled ? (44 + 73) : (35 + 58), nit: Maybe inline style comments about what the number equals to help to sync the values.
https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/track_list.js (right): https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:166: elementTop + elementHeight <= viewTop + viewHeight) { On 2014/03/05 17:24:01, hirono wrote: > nit: How about negating the condition and put the fix of bottom coordinates in > the second block? I think it's better for readability, specifying we have 3 conditions of the track position: (1) the target track is above the viewport, (2) it is in the viewport, and (3) it is below the viewport. https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/js/audio_player.js (right): https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:123: Platform.performMicrotaskCheckpoint(); On 2014/03/05 17:24:01, hirono wrote: > Just curious, what goes wrong without the line? > I'm guessing it is needed to ensure to assign -1 to currentTrackIndex, but not > sure. Yes, that's correct.
https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/track_list.js (right): https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:166: elementTop + elementHeight <= viewTop + viewHeight) { On 2014/03/05 18:30:41, yoshiki wrote: > On 2014/03/05 17:24:01, hirono wrote: > > nit: How about negating the condition and put the fix of bottom coordinates in > > the second block? > > I think it's better for readability, specifying we have 3 conditions of the > track position: (1) the target track is above the viewport, (2) it is in the > viewport, and (3) it is below the viewport. One point I cared a bit is the condition check is repeated at #165 to show the condition explicitly but is not repeated at #168. Another is (1) and (3) are not exclusive as long as we don't know the viewport height is never less than the element height. But I don't have a strong opinion here. The current code is still l g t m. https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/js/audio_player.js (right): https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:123: Platform.performMicrotaskCheckpoint(); On 2014/03/05 18:30:41, yoshiki wrote: > On 2014/03/05 17:24:01, hirono wrote: > > Just curious, what goes wrong without the line? > > I'm guessing it is needed to ensure to assign -1 to currentTrackIndex, but not > > sure. > > Yes, that's correct. So how about removing this.trackListItems_ member and reassigning the new track items to this.player_.tracks here? It shows the idea of replacing whole the track list more explicitly. We can remove the hack. And it saves the number of update from ArrayObserver.
hirono, PTAL https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/track_list.js (right): https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:165: } else if (viewTop <= elementTop && On 2014/03/05 17:24:01, hirono wrote: > nit: The first condition check can be removed. Done. https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/js/audio_player.js (right): https://codereview.chromium.org/185653014/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:123: Platform.performMicrotaskCheckpoint(); On 2014/03/06 03:44:47, hirono wrote: > On 2014/03/05 18:30:41, yoshiki wrote: > > On 2014/03/05 17:24:01, hirono wrote: > > > Just curious, what goes wrong without the line? > > > I'm guessing it is needed to ensure to assign -1 to currentTrackIndex, but > not > > > sure. > > > > Yes, that's correct. > > So how about removing this.trackListItems_ member and reassigning the new track > items to this.player_.tracks here? > It shows the idea of replacing whole the track list more explicitly. We can > remove the hack. And it saves the number of update from ArrayObserver. Done
Thanks! https://codereview.chromium.org/185653014/diff/40001/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/track_list.js (right): https://codereview.chromium.org/185653014/diff/40001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:111: this.tracks[this.currentTrackIndex].active = true; How about the case where this.currentTrackIndex >= this.tracks.length? https://codereview.chromium.org/185653014/diff/40001/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/js/audio_player.js (right): https://codereview.chromium.org/185653014/diff/40001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:172: if (!this.selectedEntry_) This is not related with this CL and just an edge case. How about the following case? 1. this.select_ is called and fetchMetadata_ starts. 2. onExternallyUnmounted_ is called before fetchMetadata_ completes. Maybe this.selectedEntry_ is still old. If it should be fixed, feel free to handle it in another CL.
hirono, PTAL https://codereview.chromium.org/185653014/diff/40001/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/elements/track_list.js (right): https://codereview.chromium.org/185653014/diff/40001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/elements/track_list.js:111: this.tracks[this.currentTrackIndex].active = true; On 2014/03/06 08:48:44, hirono wrote: > How about the case where this.currentTrackIndex >= this.tracks.length? Done. https://codereview.chromium.org/185653014/diff/40001/chrome/browser/resources... File chrome/browser/resources/file_manager/audio_player/js/audio_player.js (right): https://codereview.chromium.org/185653014/diff/40001/chrome/browser/resources... chrome/browser/resources/file_manager/audio_player/js/audio_player.js:172: if (!this.selectedEntry_) On 2014/03/06 08:48:44, hirono wrote: > This is not related with this CL and just an edge case. > How about the following case? > > 1. this.select_ is called and fetchMetadata_ starts. > 2. onExternallyUnmounted_ is called before fetchMetadata_ completes. > > Maybe this.selectedEntry_ is still old. > If it should be fixed, feel free to handle it in another CL. Thanks for reporting. This should be a bug but minor. I'll fix it separately.
Thanks! lgtm!
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/185653014/80001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
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/185653014/80001
Message was sent while issue was closed.
Change committed as 255394 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
