Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7745051:
Added refresh on filesystem change (Closed)
|
Created:
9 years, 4 months ago by Dmitry Zvorygin Modified:
9 years ago CC:
chromium-reviews, Erik does not do reviews, achuith+watch_chromium.org, mihaip+watch_chromium.org, Aaron Boodman, rginda+watch_chromium.org, arv (Not doing code reviews), davemoore+watch_chromium.org, sidor Visibility:
Public. |
DescriptionAdded refresh on filesystem change
BUG=chromium-os:15733
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=101485
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109877
Patch Set 1 #Patch Set 2 : Rollback one file #
Total comments: 23
Patch Set 3 : Fixed comments added filtering #Patch Set 4 : Fixed rginda's comments #Patch Set 5 : Fixed js style #Patch Set 6 : Fixed comments. #Patch Set 7 : Flacky test #
Messages
Total messages: 15 (0 generated)
On 2011/08/26 16:20:48, Dmitry Zvorygin wrote: FYI: Szymon is adding rescanDirectory_ debouncing as part of http://codereview.chromium.org/7715023/.
On 2011/08/26 17:46:06, rginda wrote: > On 2011/08/26 16:20:48, Dmitry Zvorygin wrote: > > FYI: Szymon is adding rescanDirectory_ debouncing as part of > http://codereview.chromium.org/7715023/. I've dealed with Szymon. We agreed that mine debouncing is slightly better Waiting for review...
On 2011/08/29 17:33:23, Dmitry Zvorygin wrote: > On 2011/08/26 17:46:06, rginda wrote: > > On 2011/08/26 16:20:48, Dmitry Zvorygin wrote: > > > > FYI: Szymon is adding rescanDirectory_ debouncing as part of > > http://codereview.chromium.org/7715023/. > > I've dealed with Szymon. We agreed that mine debouncing is slightly better I disagree. Your debounce doesn't deal with the situation where two refreshes happen quickly because they had to. In this code, both callers always get the result of the first rescan, which may have missed changes that the second rescan was intended to see. The rescan in http://codereview.chromium.org/7715023/ works by delaying the start of the rescan each time a new one is requested, guaranteeing that all callers see the latest results. You're also manipulating the rescan queue directly from onFileChanged_, which is kind of dirty. > > Waiting for review...
Ok, I might agree, that queue manipulating in onFileChanged is kind of dirty, but in which situation my debouce miss any change? If there's two refreshes made simultaneously, I'll make first scan for first request, and second scan for second request. If there's ten scans are requested simultaneously, then I'll make first scan for first request, and second scan for the rest nine requests. Each request will receive as result state of filesystem for timestamp not earlier than actual request timestamp. So I think that my code can't miss any changes. I've checked it a lot on chromebook, and it looks to work fine. Is it ok, if I create a separate queue in onFileChanged? Because if user navigates to some directory with log files for example, where files are continuously updated, filebrowser will continuously flicker. Or if some download is in progress, and system periodically flushes it's buffer on disk, thus increasing file size and modification time, Filebrowser will also continuously flicker. On Tue, Aug 30, 2011 at 3:30 AM, <rginda@chromium.org> wrote: > On 2011/08/29 17:33:23, Dmitry Zvorygin wrote: >> >> On 2011/08/26 17:46:06, rginda wrote: >> > On 2011/08/26 16:20:48, Dmitry Zvorygin wrote: >> > >> > FYI: Szymon is adding rescanDirectory_ debouncing as part of >> > http://codereview.chromium.org/7715023/. > >> I've dealed with Szymon. We agreed that mine debouncing is slightly better > > I disagree. Your debounce doesn't deal with the situation where two > refreshes > happen quickly because they had to. In this code, both callers always get > the > result of the first rescan, which may have missed changes that the second > rescan > was intended to see. > > The rescan in http://codereview.chromium.org/7715023/ works by delaying the > start of the rescan each time a new one is requested, guaranteeing that all > callers see the latest results. > > You're also manipulating the rescan queue directly from onFileChanged_, > which is > kind of dirty. > > >> Waiting for review... > > > http://codereview.chromium.org/7745051/ >
Ooooh, I see now that you're copying-and-clearing the callbacks list before starting the rescan. I completely missed that on my first read of the code (I needed Szymon to point it out to me). Sorry about that. Yes, I agree that it is better that the first rescan happens immediately, while subsequent scans get batched and delayed. The code is a bit hard to follow though, and could benefit from some refactoring and additional comments. http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... File chrome/browser/resources/file_manager/js/file_manager.js (left): http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2376: * Please add some description of the new queuing nature of rescanDirectory_. http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:15: // per specified interva; typo: s/;/l./ http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:60: this.rescanPending_ = []; rescanPending_ and rescanRunning_ are *very* similar names, but one is a list and one is a boolean. I suggest a single 'pendingRescanQueue_' array, and test its length to see if there is a rescan in progress? Also a comment to the effect of "// Queue of pending rescanDirectory_() calls." http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2426: // Add current request to pending result list rescanDirectory_ is getting pretty large, can this condition be moved into its own function? http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2429: if (this.rescanRunning_) { No braces necessary here. http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2435: // Save list of items to notify locally in case we'll receive new requests I don't think this comment is enough to describe what's going on here. How about something like... // The current list of callbacks is saved and reset. Subsequent // calls to rescanDirectory_ while we're still pending will be // saved and will cause an additional rescan to happen after a delay. http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2447: if (callbacks[i].onError) { No braces. http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2448: callbacks[i].onError(); If any of these callbacks throw, rescanRunning will be left true forever. http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2465: callbacks[i].onSuccess(); Same issue with failed callbacks here. Also, unnecessary braces. http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2573: // It might be massive change, so let's note somehow, that we need How about just moving this code into a 'rescanDirectoryLater_' function, located close to 'rescanDirectory_'?
http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... File chrome/browser/resources/file_manager/js/file_manager.js (left): http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2376: * On 2011/08/31 00:32:58, rginda wrote: > Please add some description of the new queuing nature of rescanDirectory_. Done. http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... File chrome/browser/resources/file_manager/js/file_manager.js (right): http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:15: // per specified interva; On 2011/08/31 00:32:58, rginda wrote: > typo: s/;/l./ Done. http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:60: this.rescanPending_ = []; We need both array and flag because we empty array on rescan start. On 2011/08/31 00:32:58, rginda wrote: > rescanPending_ and rescanRunning_ are *very* similar names, but one is a list > and one is a boolean. I suggest a single 'pendingRescanQueue_' array, and test > its length to see if there is a rescan in progress? Also a comment to the > effect of "// Queue of pending rescanDirectory_() calls." http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2426: // Add current request to pending result list On 2011/08/31 00:32:58, rginda wrote: > rescanDirectory_ is getting pretty large, can this condition be moved into its > own function? I think it's little useless because that function would be used only once from rescanDirectory_ only. Later maybe I'll rewrite this a little. http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2429: if (this.rescanRunning_) { On 2011/08/31 00:32:58, rginda wrote: > No braces necessary here. Done. http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2435: // Save list of items to notify locally in case we'll receive new requests On 2011/08/31 00:32:58, rginda wrote: > I don't think this comment is enough to describe what's going on here. How > about something like... > > // The current list of callbacks is saved and reset. Subsequent > // calls to rescanDirectory_ while we're still pending will be > // saved and will cause an additional rescan to happen after a delay. Done. http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2447: if (callbacks[i].onError) { On 2011/08/31 00:32:58, rginda wrote: > No braces. Done. http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2448: callbacks[i].onError(); On 2011/08/31 00:32:58, rginda wrote: > If any of these callbacks throw, rescanRunning will be left true forever. Done. http://codereview.chromium.org/7745051/diff/1005/chrome/browser/resources/fil... chrome/browser/resources/file_manager/js/file_manager.js:2465: callbacks[i].onSuccess(); On 2011/08/31 00:32:58, rginda wrote: > Same issue with failed callbacks here. Also, unnecessary braces. Done.
http://codereview.chromium.org/7745051/diff/1005/chrome/browser/chromeos/exte... File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right): http://codereview.chromium.org/7745051/diff/1005/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_browser_event_router.cc:132: base::AutoLock lock(lock_); one space only? http://codereview.chromium.org/7745051/diff/1005/chrome/browser/chromeos/exte... File chrome/browser/chromeos/extensions/file_browser_event_router.h (right): http://codereview.chromium.org/7745051/diff/1005/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_browser_event_router.h:65: typedef struct FileWatcherExtensions { let's make this a class now since it's not just collection of data members anymore
Fixed comments. http://codereview.chromium.org/7745051/diff/1005/chrome/browser/chromeos/exte... File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right): http://codereview.chromium.org/7745051/diff/1005/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_browser_event_router.cc:132: base::AutoLock lock(lock_); On 2011/09/12 15:58:42, zel wrote: > one space only? Done. http://codereview.chromium.org/7745051/diff/1005/chrome/browser/chromeos/exte... File chrome/browser/chromeos/extensions/file_browser_event_router.h (right): http://codereview.chromium.org/7745051/diff/1005/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_browser_event_router.h:65: typedef struct FileWatcherExtensions { On 2011/09/12 15:58:42, zel wrote: > let's make this a class now since it's not just collection of data members > anymore Done.
LGTM
Try job failure for 7745051-18001 on mac for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number... Step "update" is always a major failure. Look at the try server FAQ for more details.
Change committed as 101485
Please review again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zvorygin@chromium.org/7745051/25001
Change committed as 109877 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
