Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
312493002:
Files.app: Fix flakiness in chaining directory (Closed)
|
Created:
6 years, 6 months ago by yoshiki Modified:
6 years, 6 months ago Reviewers:
hirono CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org, yoshiki+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFiles.app: Fix flakiness in chaining directory
I'm about to commit a patch to improve of filelist draw. But it makes some tests flaky because the currently directory changes faster than before. This patch makes the logic of directory changing more robust.
BUG=367123
TEST=browser_test passes
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275418
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed the comments #
Total comments: 4
Patch Set 3 : Addressed the comments #Patch Set 4 : rebase #Patch Set 5 : Fix bug and test failure #
Total comments: 2
Patch Set 6 : Addressed the comments #Messages
Total messages: 14 (0 generated)
@hirono, PTAL
https://codereview.chromium.org/312493002/diff/80001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_model.js (right): https://codereview.chromium.org/312493002/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:283: var tracker = this.createDirectoryChangeTracker(); It looks tracker does not detect starting search, but this.changeDirectorySequence_ do. Is it intended? https://codereview.chromium.org/312493002/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:347: * @param {function()} callback Callback. Boolean is needed as an argument? https://codereview.chromium.org/312493002/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:710: this.directoryChangeQueue_.run(function(sequence, callback) { Maybe more specific name is needed for callback, since having both opt_callback and callback is confusing.
@hirono: PTAL. Thanks, https://codereview.chromium.org/312493002/diff/80001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_model.js (right): https://codereview.chromium.org/312493002/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:283: var tracker = this.createDirectoryChangeTracker(); On 2014/06/02 09:18:03, hirono wrote: > It looks tracker does not detect starting search, but > this.changeDirectorySequence_ do. Is it intended? Good catch. search should be detected, since it changes DirectoryContents. https://codereview.chromium.org/312493002/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:347: * @param {function()} callback Callback. On 2014/06/02 09:18:03, hirono wrote: > Boolean is needed as an argument? Done. https://codereview.chromium.org/312493002/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:710: this.directoryChangeQueue_.run(function(sequence, callback) { On 2014/06/02 09:18:03, hirono wrote: > Maybe more specific name is needed for callback, since having both opt_callback > and callback is confusing. Done.
https://codereview.chromium.org/312493002/diff/100001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/directory_model.js (right): https://codereview.chromium.org/312493002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/directory_model.js:288: tracker.stop(); Remaining line. https://codereview.chromium.org/312493002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/directory_model.js:679: tracker.stop(); ditto.
@hirono, PTAL. Thanks https://codereview.chromium.org/312493002/diff/100001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/directory_model.js (right): https://codereview.chromium.org/312493002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/directory_model.js:288: tracker.stop(); On 2014/06/02 11:52:07, hirono wrote: > Remaining line. Done. https://codereview.chromium.org/312493002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/directory_model.js:679: tracker.stop(); On 2014/06/02 11:52:07, hirono wrote: > ditto. Done.
It looks some tests are failed. Let me investigate.
The test failure seems to go away. PTAL again? Thanks.
https://codereview.chromium.org/312493002/diff/180001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/directory_model.js (right): https://codereview.chromium.org/312493002/diff/180001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/directory_model.js:328: cr.dispatchSimpleEvent(this, 'rescan-completed'); Is it OK to dispatch the event? It looks rescan-completed is dispatched for another directory.
@hirono, PTAL. https://codereview.chromium.org/312493002/diff/180001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/directory_model.js (right): https://codereview.chromium.org/312493002/diff/180001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/directory_model.js:328: cr.dispatchSimpleEvent(this, 'rescan-completed'); On 2014/06/06 04:12:02, hirono wrote: > Is it OK to dispatch the event? > It looks rescan-completed is dispatched for another directory. Nice catch. Done.
On 2014/06/06 04:55:35, yoshiki wrote: > @hirono, PTAL. > > https://codereview.chromium.org/312493002/diff/180001/ui/file_manager/file_ma... > File ui/file_manager/file_manager/foreground/js/directory_model.js (right): > > https://codereview.chromium.org/312493002/diff/180001/ui/file_manager/file_ma... > ui/file_manager/file_manager/foreground/js/directory_model.js:328: > cr.dispatchSimpleEvent(this, 'rescan-completed'); > On 2014/06/06 04:12:02, hirono wrote: > > Is it OK to dispatch the event? > > It looks rescan-completed is dispatched for another directory. > > Nice catch. Done. lgtm!
thanks!
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/312493002/200001
Message was sent while issue was closed.
Change committed as 275418 |
