Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7715023:
Debouncing of function rescanning directory in File Manager (Closed)
|
Created:
9 years, 4 months ago by sidor Modified:
9 years, 4 months ago CC:
chromium-reviews, rginda+watch_chromium.org, arv (Not doing code reviews), achuith+watch_chromium.org Visibility:
Public. |
DescriptionDebouncing of function rescanning directory in File Manager
BUG=chromium-os:19500, chromium-os:19568
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98498
Patch Set 1 #
Total comments: 12
Patch Set 2 : iteration 2 #Patch Set 3 : iteration 3 #
Total comments: 2
Patch Set 4 : iteration 4 #Messages
Total messages: 9 (0 generated)
Hi, Could you please review my code? It's short! :) Thanks, Szymon
http://codereview.chromium.org/7715023/diff/1/chrome/browser/resources/file_m... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/7715023/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:1797: self.rescanDirectory_(undefined, 300); In js, "undefined" just happens to be a variable that hasn't been defined yet, like "foo" or "querty". Use the keyword null any time you mean "no value". http://codereview.chromium.org/7715023/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:2539: var callback = this.rescanDirectory_.callbacks.shift(); s/shift/pop/ push/pop work from the end of the array. shift/unshift work from the front. If you push to the end, and shift from the front, then you'll break in the case where 'count' matters. http://codereview.chromium.org/7715023/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:2545: this.rescanDirectory_ = []; this.rescanDirectory_ is the function we're in. This code will never run. http://codereview.chromium.org/7715023/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:2554: var delayMS = 100; No need for var here. delayMS is already declared as a parameter. 'delayMS = delayMS || 100' is a common js idiom for what you're doing here. http://codereview.chromium.org/7715023/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:2557: // a possible overlap between two calls. I don't think this comment (or the minimum 100ms policy) applies here. This is debouncing the case when callers invoke the function too many times in a row. rescanDirectoryNow won't even *start* until the timeout expires. http://codereview.chromium.org/7715023/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:2565: self.rescanDirectoryNow_(done.bind(self, done already has access to self, there is no need to bind it here. You should move the 'var self = this' line above the declaration of done, however, to make it clear.
Please take a look at the comments. http://codereview.chromium.org/7715023/diff/1/chrome/browser/resources/file_m... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/7715023/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:1797: self.rescanDirectory_(undefined, 300); On 2011/08/25 00:29:09, rginda wrote: > In js, "undefined" just happens to be a variable that hasn't been defined yet, > like "foo" or "querty". Use the keyword null any time you mean "no value". Done. http://codereview.chromium.org/7715023/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:2539: var callback = this.rescanDirectory_.callbacks.shift(); On 2011/08/25 00:29:09, rginda wrote: > s/shift/pop/ > > push/pop work from the end of the array. shift/unshift work from the front. If > you push to the end, and shift from the front, then you'll break in the case > where 'count' matters. Hmm.. Nope. I did it on purpose and I really think it works. But yeah it might not be obvious looking at the code. In my case I use this array as a queue. Please ping me when you are in your office, I will come by and explain you how it works. http://codereview.chromium.org/7715023/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:2545: this.rescanDirectory_ = []; On 2011/08/25 00:29:09, rginda wrote: > this.rescanDirectory_ is the function we're in. This code will never run. Done. http://codereview.chromium.org/7715023/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:2554: var delayMS = 100; On 2011/08/25 00:29:09, rginda wrote: > No need for var here. delayMS is already declared as a parameter. 'delayMS = > delayMS || 100' is a common js idiom for what you're doing here. That's kind of cool idiom :) http://codereview.chromium.org/7715023/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:2557: // a possible overlap between two calls. On 2011/08/25 00:29:09, rginda wrote: > I don't think this comment (or the minimum 100ms policy) applies here. This is > debouncing the case when callers invoke the function too many times in a row. > rescanDirectoryNow won't even *start* until the timeout expires. No, no this is not what I mean there. Here's what this is: 1. [0ms] We have one call 1 to rescanDirectory_ . 2. [100ms] We actually call rescanDirectoryNow_ for call 1. 3. [101ms] We have a call 2 to rescanDirectory_. 4. [201ms] We actually call rescanDirectoryNow_ for call 2. Now if the rescanDirectoryNow_ for call 1 is still running (it took more than 100 ms to resolve), we have two overlapping calls of rescanDirectoryNow_. http://codereview.chromium.org/7715023/diff/1/chrome/browser/resources/file_m... chrome/browser/resources/file_manager/js/file_manager.js:2565: self.rescanDirectoryNow_(done.bind(self, On 2011/08/25 00:29:09, rginda wrote: > done already has access to self, there is no need to bind it here. You should > move the 'var self = this' line above the declaration of done, however, to make > it clear. OK, but I still need to pass and argument to the function at this point. Is bind(null, arg) the right solution?
Added comment!
http://codereview.chromium.org/7715023/diff/5001/chrome/browser/resources/fil... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/7715023/diff/5001/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2568: self.rescanDirectoryNow_(done.bind(null, This is usually done with another nested function, rather than a bind call.
Done. http://codereview.chromium.org/7715023/diff/5001/chrome/browser/resources/fil... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/7715023/diff/5001/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2568: self.rescanDirectoryNow_(done.bind(null, On 2011/08/25 19:14:56, rginda wrote: > This is usually done with another nested function, rather than a bind call. Done.
On 2011/08/25 23:54:16, sidor wrote: > Done. > > http://codereview.chromium.org/7715023/diff/5001/chrome/browser/resources/fil... > File chrome/browser/resources/file_manager/js/file_manager.js (right): > > http://codereview.chromium.org/7715023/diff/5001/chrome/browser/resources/fil... > chrome/browser/resources/file_manager/js/file_manager.js:2568: > self.rescanDirectoryNow_(done.bind(null, > On 2011/08/25 19:14:56, rginda wrote: > > This is usually done with another nested function, rather than a bind call. > > Done. LGTM
On 2011/08/26 17:45:40, rginda wrote: > On 2011/08/25 23:54:16, sidor wrote: > > Done. > > > > > http://codereview.chromium.org/7715023/diff/5001/chrome/browser/resources/fil... > > File chrome/browser/resources/file_manager/js/file_manager.js (right): > > > > > http://codereview.chromium.org/7715023/diff/5001/chrome/browser/resources/fil... > > chrome/browser/resources/file_manager/js/file_manager.js:2568: > > self.rescanDirectoryNow_(done.bind(null, > > On 2011/08/25 19:14:56, rginda wrote: > > > This is usually done with another nested function, rather than a bind call. > > > > Done. > > > LGTM Thanks for review!
Change committed as 98498 |
