Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9652024:
Only show progress bar after 500ms (Closed)
|
Created:
8 years, 9 months ago by bshe Modified:
8 years, 9 months ago CC:
chromium-reviews, rginda+watch_chromium.org, arv (Not doing code reviews), SeRya Visibility:
Public. |
DescriptionOnly show progress bar after 500ms
BUG=chromium-os:26924
TEST=Open file manager, verify if progress bar behaves as described in 26924
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126694
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address Rick's review. #
Total comments: 9
Patch Set 3 : Address review #Patch Set 4 : Merge to trunk. #
Messages
Total messages: 14 (0 generated)
Hi Rob. Could you please take a look at this small CL? As requested by ken, the progress bar is visible only if the pasting process is longer than 500ms. Thanks!
lgtm
+rbyers this one as well. Thanks!
A couple small suggestions. https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... File chrome/browser/resources/file_manager/js/file_copy_manager.js (right): https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... chrome/browser/resources/file_manager/js/file_copy_manager.js:59: FileCopyManager.Task.prototype.takeNextEntry = function() { This function name is misleading now (you're not 'taking' anything). Maybe it should be 'get nextEntry'? https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... chrome/browser/resources/file_manager/js/file_copy_manager.js:75: this.pendingDirectories.shift(); You're implicitly assuming that 'entry' is still the next entry on the pending list. Are you sure that's always the case (I haven't looked at how this is used)? Regardless I'd suggest removing the risk of mismatch by either: 1) checking that 'entry' still matches the next pending entry and throwing an exception or something if it doesn't, or 2) removing entry wherever it occurs in the pending list (and possibly throwing an exception if it occurs nowhere, but a no-op in that case probably isn't terrible either). https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... chrome/browser/resources/file_manager/js/file_manager.js:1260: var progress = (status.completedItems + 1) / status.totalItems; Since this formula is non-trivial (i.e. the +1 requires explanation in the comment below) it would be nice to avoid duplicating the code. Eg., perhaps you should just pass in progress, or even all of 'options' as a parameter which you supply in the bind below. https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... chrome/browser/resources/file_manager/js/file_manager.js:1289: // TODO(bshe): Need to find a way to update the current progress bar if At a quick glance it looks to me like this should never happen - FileCopyManager maintains a queue of tasks and so I think you should never see a BEGIN until after the END for the previous task. Can you verify? Does anything else use a butter bar today? I suppose we should still be prepared for something different now or in the future to show a butter bar, in which case just hiding it here is probably appropriate. https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... chrome/browser/resources/file_manager/js/file_manager.js:1293: this.hideButter(); I think you should call initButter right away in this case to swap out the progress bar. We don't want to display a butter bar, then hide it for 500ms before displaying another. https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... chrome/browser/resources/file_manager/js/file_manager.js:1297: this.butterTimeout = setTimeout(this.initButter_.bind(this), 500); replace butterTimeout with butterTimeout_ ?
Done and inlined. Thanks https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... File chrome/browser/resources/file_manager/js/file_copy_manager.js (right): https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... chrome/browser/resources/file_manager/js/file_copy_manager.js:59: FileCopyManager.Task.prototype.takeNextEntry = function() { On 2012/03/09 21:45:45, Rick Byers wrote: > This function name is misleading now (you're not 'taking' anything). Maybe it > should be 'get nextEntry'? Done. https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... chrome/browser/resources/file_manager/js/file_copy_manager.js:75: this.pendingDirectories.shift(); On 2012/03/09 21:45:45, Rick Byers wrote: > You're implicitly assuming that 'entry' is still the next entry on the pending > list. Are you sure that's always the case (I haven't looked at how this is > used)? Regardless I'd suggest removing the risk of mismatch by either: > 1) checking that 'entry' still matches the next pending > entry and throwing an exception or something if it doesn't, or > 2) removing entry wherever it occurs in the pending list (and possibly throwing > an exception if it occurs nowhere, but a no-op in that case probably isn't > terrible either). It seems entry is targetEntry and shift function removes the corresponding srcEntry. They will never be the same (src and target file path cant be the same). From the code, it seems only after the targetEntry successfully wrote the file system, the file copy manager then start to fetch next srcEntry in pendingDirectories/Files list. So it should be safe here. We can try to test if the entry is a targetEntry of srcEntry(entry returned from shift function) to improve the safty here. But it probably wont be trivial and we should probably create a new bug to track it. Given the risk if pretty low, I am not sure if it's worth it though... https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... chrome/browser/resources/file_manager/js/file_manager.js:1260: var progress = (status.completedItems + 1) / status.totalItems; On 2012/03/09 21:45:45, Rick Byers wrote: > Since this formula is non-trivial (i.e. the +1 requires explanation in the > comment below) it would be nice to avoid duplicating the code. Eg., perhaps you > should just pass in progress, or even all of 'options' as a parameter which you > supply in the bind below. I moved the progress calculation into a file_copy_manager function called getProgress to avoid duplicated code. The reason I dont pass in progress as parameter is I want to get the latest progress data. https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... chrome/browser/resources/file_manager/js/file_manager.js:1289: // TODO(bshe): Need to find a way to update the current progress bar if On 2012/03/09 21:45:45, Rick Byers wrote: > At a quick glance it looks to me like this should never happen - FileCopyManager > maintains a queue of tasks and so I think you should never see a BEGIN until > after the END for the previous task. Can you verify? Does anything else use a > butter bar today? I suppose we should still be prepared for something different > now or in the future to show a butter bar, in which case just hiding it here is > probably appropriate. Correct. I verified that for copy/paste task, the BEGIN event only dispatch after the END event of previous task. I assumed all the queued tasks were parallel. But they seems to be serial. In this case, showing the butter bar immediately when the second task continues makes more sense. I changed the code in copy_manager to dispatch a CONTINUE event instead of BEGIN event when the second task begins to service. And the butter bar in file manager will immediately show on a CONTINUE event. https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... chrome/browser/resources/file_manager/js/file_manager.js:1293: this.hideButter(); On 2012/03/09 21:45:45, Rick Byers wrote: > I think you should call initButter right away in this case to swap out the > progress bar. We don't want to display a butter bar, then hide it for 500ms > before displaying another. Now the progress bar auto updates itself when there are new tasks coming. https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... chrome/browser/resources/file_manager/js/file_manager.js:1297: this.butterTimeout = setTimeout(this.initButter_.bind(this), 500); On 2012/03/09 21:45:45, Rick Byers wrote: > replace butterTimeout with butterTimeout_ ? Done. https://chromiumcodereview.appspot.com/9652024/diff/5001/chrome/browser/resou... File chrome/browser/resources/file_manager/js/file_manager.js (left): https://chromiumcodereview.appspot.com/9652024/diff/5001/chrome/browser/resou... chrome/browser/resources/file_manager/js/file_manager.js:3009: this.showButter('', {timeout: 0}); Side note: It seems the flicker it caused by a call to this.showButter('PASTE_START',{timeout:0}) which I previously add. I dont think it is necessary to make this call. Maybe I forgot to remove it in the first place. And directly remove it should also prevent the flickr. Plus, now we want to update the current butter bar when there is any new incoming paste task while servicing. And we dont want to remove the current butter and create a new one by this statement either.
Looks good! Just a couple minor style suggestions left. On 2012/03/12 00:13:34, bshe wrote: > Done and inlined. > > Thanks > > https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... > File chrome/browser/resources/file_manager/js/file_copy_manager.js (right): > > https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... > chrome/browser/resources/file_manager/js/file_copy_manager.js:59: > FileCopyManager.Task.prototype.takeNextEntry = function() { > On 2012/03/09 21:45:45, Rick Byers wrote: > > This function name is misleading now (you're not 'taking' anything). Maybe it > > should be 'get nextEntry'? > > Done. > > https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... > chrome/browser/resources/file_manager/js/file_copy_manager.js:75: > this.pendingDirectories.shift(); > On 2012/03/09 21:45:45, Rick Byers wrote: > > You're implicitly assuming that 'entry' is still the next entry on the pending > > list. Are you sure that's always the case (I haven't looked at how this is > > used)? Regardless I'd suggest removing the risk of mismatch by either: > > 1) checking that 'entry' still matches the next pending > > entry and throwing an exception or something if it doesn't, or > > 2) removing entry wherever it occurs in the pending list (and possibly > throwing > > an exception if it occurs nowhere, but a no-op in that case probably isn't > > terrible either). > > It seems entry is targetEntry and shift function removes the corresponding > srcEntry. They will never be the same (src and target file path cant be the > same). > > From the code, it seems only after the targetEntry successfully wrote the file > system, the file copy manager then start to fetch next srcEntry in > pendingDirectories/Files list. So it should be safe here. > > We can try to test if the entry is a targetEntry of srcEntry(entry returned from > shift function) to improve the safty here. But it probably wont be trivial and > we should probably create a new bug to track it. Given the risk if pretty low, I > am not sure if it's worth it though... Ah, yes I see it's not completely trivial. I'm OK with leaving a comment for now (make it clear that we SHOULD be removing the source entry corresponding to the provided target entry). I guess there's no easy way to just set an 'in progress' bit on the entry - making sure we only ever tried to set that once, and only removed entries that had it set would be really nice. But again that looks non-trivial, so just being clear about the intention in comments is good enough for now I think. https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... > File chrome/browser/resources/file_manager/js/file_manager.js (right): > > https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... > chrome/browser/resources/file_manager/js/file_manager.js:1260: var progress = > (status.completedItems + 1) / status.totalItems; > On 2012/03/09 21:45:45, Rick Byers wrote: > > Since this formula is non-trivial (i.e. the +1 requires explanation in the > > comment below) it would be nice to avoid duplicating the code. Eg., perhaps > you > > should just pass in progress, or even all of 'options' as a parameter which > you > > supply in the bind below. > > I moved the progress calculation into a file_copy_manager function called > getProgress to avoid duplicated code. The reason I dont pass in progress as > parameter is I want to get the latest progress data. Looks good. https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... > chrome/browser/resources/file_manager/js/file_manager.js:1289: // TODO(bshe): > Need to find a way to update the current progress bar if > On 2012/03/09 21:45:45, Rick Byers wrote: > > At a quick glance it looks to me like this should never happen - > FileCopyManager > > maintains a queue of tasks and so I think you should never see a BEGIN until > > after the END for the previous task. Can you verify? Does anything else use > a > > butter bar today? I suppose we should still be prepared for something > different > > now or in the future to show a butter bar, in which case just hiding it here > is > > probably appropriate. > > Correct. I verified that for copy/paste task, the BEGIN event only dispatch > after the END event of previous task. I assumed all the queued tasks were > parallel. But they seems to be serial. > In this case, showing the butter bar immediately when the second task continues > makes more sense. I changed the code in copy_manager to dispatch a CONTINUE > event instead of BEGIN event when the second task begins to service. And the > butter bar in file manager will immediately show on a CONTINUE event. Looks good. https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... > chrome/browser/resources/file_manager/js/file_manager.js:1293: > this.hideButter(); > On 2012/03/09 21:45:45, Rick Byers wrote: > > I think you should call initButter right away in this case to swap out the > > progress bar. We don't want to display a butter bar, then hide it for 500ms > > before displaying another. > > Now the progress bar auto updates itself when there are new tasks coming. > > https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... > chrome/browser/resources/file_manager/js/file_manager.js:1297: > this.butterTimeout = setTimeout(this.initButter_.bind(this), 500); > On 2012/03/09 21:45:45, Rick Byers wrote: > > replace butterTimeout with butterTimeout_ ? > > Done. > > https://chromiumcodereview.appspot.com/9652024/diff/5001/chrome/browser/resou... > File chrome/browser/resources/file_manager/js/file_manager.js (left): > > https://chromiumcodereview.appspot.com/9652024/diff/5001/chrome/browser/resou... > chrome/browser/resources/file_manager/js/file_manager.js:3009: > this.showButter('', {timeout: 0}); > Side note: > It seems the flicker it caused by a call to > this.showButter('PASTE_START',{timeout:0}) which I previously add. I dont think > it is necessary to make this call. Maybe I forgot to remove it in the first > place. And directly remove it should also prevent the flickr. > > Plus, now we want to update the current butter bar when there is any new > incoming paste task while servicing. And we dont want to remove the current > butter and create a new one by this statement either.
https://chromiumcodereview.appspot.com/9652024/diff/5001/chrome/browser/resou... File chrome/browser/resources/file_manager/js/file_copy_manager.js (right): https://chromiumcodereview.appspot.com/9652024/diff/5001/chrome/browser/resou... chrome/browser/resources/file_manager/js/file_copy_manager.js:153: FileCopyManager.prototype.getProgress = function() { Since this is a public function you should have a full jsdoc header that desribes the function and it's return value. https://chromiumcodereview.appspot.com/9652024/diff/5001/chrome/browser/resou... chrome/browser/resources/file_manager/js/file_copy_manager.js:155: var result = { No need to create a variable and initializing to 0 first, Just do: return { // TODO(bshe): big comment... percentage: (status.completed ...), pendingItems = status.pendingItems; } https://chromiumcodereview.appspot.com/9652024/diff/5001/chrome/browser/resou... chrome/browser/resources/file_manager/js/file_copy_manager.js:343: self.dispatchEvent(event); Probably worth encapsulating these 3 lines in a simple 'sendProgressEvent_(reason)' helper function - I see it's repeated several times now. https://chromiumcodereview.appspot.com/9652024/diff/5001/chrome/browser/resou... chrome/browser/resources/file_manager/js/file_copy_manager.js:523: onFilesystemError)); Remove added extra space
On 2012/03/12 14:19:55, Rick Byers wrote: > Looks good! Just a couple minor style suggestions left. > > On 2012/03/12 00:13:34, bshe wrote: > > Done and inlined. > > > > Thanks > > > > > https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... > > File chrome/browser/resources/file_manager/js/file_copy_manager.js (right): > > > > > https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... > > chrome/browser/resources/file_manager/js/file_copy_manager.js:59: > > FileCopyManager.Task.prototype.takeNextEntry = function() { > > On 2012/03/09 21:45:45, Rick Byers wrote: > > > This function name is misleading now (you're not 'taking' anything). Maybe > it > > > should be 'get nextEntry'? > > > > Done. > > > > > https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... > > chrome/browser/resources/file_manager/js/file_copy_manager.js:75: > > this.pendingDirectories.shift(); > > On 2012/03/09 21:45:45, Rick Byers wrote: > > > You're implicitly assuming that 'entry' is still the next entry on the > pending > > > list. Are you sure that's always the case (I haven't looked at how this is > > > used)? Regardless I'd suggest removing the risk of mismatch by either: > > > 1) checking that 'entry' still matches the next pending > > > entry and throwing an exception or something if it doesn't, or > > > 2) removing entry wherever it occurs in the pending list (and possibly > > throwing > > > an exception if it occurs nowhere, but a no-op in that case probably isn't > > > terrible either). > > > > It seems entry is targetEntry and shift function removes the corresponding > > srcEntry. They will never be the same (src and target file path cant be the > > same). > > > > From the code, it seems only after the targetEntry successfully wrote the file > > system, the file copy manager then start to fetch next srcEntry in > > pendingDirectories/Files list. So it should be safe here. > > > > We can try to test if the entry is a targetEntry of srcEntry(entry returned > from > > shift function) to improve the safty here. But it probably wont be trivial and > > we should probably create a new bug to track it. Given the risk if pretty low, > I > > am not sure if it's worth it though... > > Ah, yes I see it's not completely trivial. I'm OK with leaving a comment for > now (make it clear that we SHOULD be removing the source entry corresponding to > the provided target entry). I guess there's no easy way to just set an 'in > progress' bit on the entry - making sure we only ever tried to set that once, > and only removed entries that had it set would be really nice. But again that > looks non-trivial, so just being clear about the intention in comments is good > enough for now I think. > Got it. I guess you are most concern about the first entry in pending list is not the in progress entry. Using the inprogress bit is a good idea in this case and I have added it to the code. I thought that you worry about there might be more than one entries servicing parallel and they compete with each other which might result the second entry finished first or some other racing conditions. I thought we need to check if the entry corresponding to srcEntry by checking their path and also taking care of some edge cases. > https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... > > File chrome/browser/resources/file_manager/js/file_manager.js (right): > > > > > https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... > > chrome/browser/resources/file_manager/js/file_manager.js:1260: var progress = > > (status.completedItems + 1) / status.totalItems; > > On 2012/03/09 21:45:45, Rick Byers wrote: > > > Since this formula is non-trivial (i.e. the +1 requires explanation in the > > > comment below) it would be nice to avoid duplicating the code. Eg., perhaps > > you > > > should just pass in progress, or even all of 'options' as a parameter which > > you > > > supply in the bind below. > > > > I moved the progress calculation into a file_copy_manager function called > > getProgress to avoid duplicated code. The reason I dont pass in progress as > > parameter is I want to get the latest progress data. > > Looks good. > > https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... > > chrome/browser/resources/file_manager/js/file_manager.js:1289: // TODO(bshe): > > Need to find a way to update the current progress bar if > > On 2012/03/09 21:45:45, Rick Byers wrote: > > > At a quick glance it looks to me like this should never happen - > > FileCopyManager > > > maintains a queue of tasks and so I think you should never see a BEGIN until > > > after the END for the previous task. Can you verify? Does anything else > use > > a > > > butter bar today? I suppose we should still be prepared for something > > different > > > now or in the future to show a butter bar, in which case just hiding it here > > is > > > probably appropriate. > > > > Correct. I verified that for copy/paste task, the BEGIN event only dispatch > > after the END event of previous task. I assumed all the queued tasks were > > parallel. But they seems to be serial. > > In this case, showing the butter bar immediately when the second task > continues > > makes more sense. I changed the code in copy_manager to dispatch a CONTINUE > > event instead of BEGIN event when the second task begins to service. And the > > butter bar in file manager will immediately show on a CONTINUE event. > > Looks good. > > https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... > > chrome/browser/resources/file_manager/js/file_manager.js:1293: > > this.hideButter(); > > On 2012/03/09 21:45:45, Rick Byers wrote: > > > I think you should call initButter right away in this case to swap out the > > > progress bar. We don't want to display a butter bar, then hide it for 500ms > > > before displaying another. > > > > Now the progress bar auto updates itself when there are new tasks coming. > > > > > https://chromiumcodereview.appspot.com/9652024/diff/1/chrome/browser/resource... > > chrome/browser/resources/file_manager/js/file_manager.js:1297: > > this.butterTimeout = setTimeout(this.initButter_.bind(this), 500); > > On 2012/03/09 21:45:45, Rick Byers wrote: > > > replace butterTimeout with butterTimeout_ ? > > > > Done. > > > > > https://chromiumcodereview.appspot.com/9652024/diff/5001/chrome/browser/resou... > > File chrome/browser/resources/file_manager/js/file_manager.js (left): > > > > > https://chromiumcodereview.appspot.com/9652024/diff/5001/chrome/browser/resou... > > chrome/browser/resources/file_manager/js/file_manager.js:3009: > > this.showButter('', {timeout: 0}); > > Side note: > > It seems the flicker it caused by a call to > > this.showButter('PASTE_START',{timeout:0}) which I previously add. I dont > think > > it is necessary to make this call. Maybe I forgot to remove it in the first > > place. And directly remove it should also prevent the flickr. > > > > Plus, now we want to update the current butter bar when there is any new > > incoming paste task while servicing. And we dont want to remove the current > > butter and create a new one by this statement either.
Done and inlined. Thanks! https://chromiumcodereview.appspot.com/9652024/diff/5001/chrome/browser/resou... File chrome/browser/resources/file_manager/js/file_copy_manager.js (right): https://chromiumcodereview.appspot.com/9652024/diff/5001/chrome/browser/resou... chrome/browser/resources/file_manager/js/file_copy_manager.js:153: FileCopyManager.prototype.getProgress = function() { On 2012/03/12 14:20:08, Rick Byers wrote: > Since this is a public function you should have a full jsdoc header that > desribes the function and it's return value. Done. https://chromiumcodereview.appspot.com/9652024/diff/5001/chrome/browser/resou... chrome/browser/resources/file_manager/js/file_copy_manager.js:155: var result = { On 2012/03/12 14:20:08, Rick Byers wrote: > No need to create a variable and initializing to 0 first, Just do: > return { > // TODO(bshe): big comment... > percentage: (status.completed ...), > > pendingItems = status.pendingItems; > } Done. https://chromiumcodereview.appspot.com/9652024/diff/5001/chrome/browser/resou... chrome/browser/resources/file_manager/js/file_copy_manager.js:343: self.dispatchEvent(event); On 2012/03/12 14:20:08, Rick Byers wrote: > Probably worth encapsulating these 3 lines in a simple > 'sendProgressEvent_(reason)' helper function - I see it's repeated several times > now. Done. https://chromiumcodereview.appspot.com/9652024/diff/5001/chrome/browser/resou... chrome/browser/resources/file_manager/js/file_copy_manager.js:523: onFilesystemError)); On 2012/03/12 14:20:08, Rick Byers wrote: > Remove added extra space Done.
lgtm - sorry for the delay, totally missed this
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bshe@chromium.org/9652024/7001
Can't apply patch for file chrome/browser/resources/file_manager/js/file_manager.js. While running patch -p1 --forward --force; patching file chrome/browser/resources/file_manager/js/file_manager.js Hunk #1 FAILED at 1254. Hunk #2 succeeded at 1343 with fuzz 1 (offset 32 lines). Hunk #3 succeeded at 2998 with fuzz 1 (offset -7 lines). 1 out of 3 hunks FAILED -- saving rejects to file chrome/browser/resources/file_manager/js/file_manager.js.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bshe@chromium.org/9652024/12001
Change committed as 126694 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
