Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
293053006:
Files.app: Introduce AsyncUtil.ConcurrentQueue (Closed)
|
Created:
6 years, 7 months ago by yoshiki Modified:
6 years, 7 months ago 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: Introduce AsyncUtil.ConcurrentQueue
This patch introduces AsyncUtil.ConcurrentQueue class, which is almost same as AsyncUtil.Queue but it can run multiple jobs concurrently.
BUG=367123
TEST=Files.app works well
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272934
Patch Set 1 #
Total comments: 22
Patch Set 2 : Addressed the comment #
Total comments: 4
Patch Set 3 : Addressed the comment #
Total comments: 5
Patch Set 4 : Addressed the comment #
Total comments: 2
Patch Set 5 : Addressed the comment #
Total comments: 9
Patch Set 6 : Addressed the comment #
Total comments: 8
Patch Set 7 : Addressed the comment #
Total comments: 4
Patch Set 8 : Addressed the comment #Messages
Total messages: 30 (0 generated)
Two concerns: (1) Concurrent AsyncUtil.Queue already exists, and it is called AsyncUtil.Group. (2) Hirono started using Promises extensively, and I think we should follow it. Eg. Promise.race[promise1, promise2, promise3].then() would work like a concurrent queue. +@hirono
On 2014/05/21 12:35:57, mtomasz wrote: > Two concerns: > > (1) Concurrent AsyncUtil.Queue already exists, and it is called AsyncUtil.Group. > > (2) Hirono started using Promises extensively, and I think we should follow it. > Eg. Promise.race[promise1, promise2, promise3].then() would work like a > concurrent queue. > > +@hirono I took a deeper look at the code, and I changed my mind. I'm fine with this solution. Promises can't be easily cancelled, not throttled.
https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/common/js/async_util.js (right): https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:47: * concurrently. In maximum, |maximumJob| jobs will be run simultaneously. nit: In maximum -> At most https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:47: * concurrently. In maximum, |maximumJob| jobs will be run simultaneously. nit: maximumJob -> concurrentTasks / limit https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:48: * @param missing https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:54: this.running_ = 0; AsyncUtil.Group stores tasks in this.pendingTasks_. Can we do it here for consistency? Also, it is easier to debug, since we can see what closures are pending. https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:80: * Cancels all pending tasks. Note that this does NOT cancel the task running nit: the task -> tasks https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:100: AsyncUtil.ConcurrentQueue.prototype.setFinishCallback = function(callback) { Queue should be created for one purpose, so there should be no reason to change finish callback multiple times. How about passing the callback to the constructor, like it is done in AsyncUtil.Aggregation? https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:104: AsyncUtil.ConcurrentQueue.prototype.continue_ = function() { @jsdoc missing https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:124: AsyncUtil.ConcurrentQueue.prototype.onTaskFinished_ = function() { @jsdoc missing https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:140: * @constructor @extends missing
https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/common/js/async_util.js (right): https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:89: * @return {boolean} True when a task is running but requeted to cancel, How about the case that the cancel method is called when the queue is empty? https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:106: this.isCancelled_ = false; Is it OK assigning false here? Some tasks may not be finished yet.
PTAL. https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/common/js/async_util.js (right): https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:47: * concurrently. In maximum, |maximumJob| jobs will be run simultaneously. On 2014/05/21 12:59:13, mtomasz wrote: > nit: maximumJob -> concurrentTasks / limit Done. https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:47: * concurrently. In maximum, |maximumJob| jobs will be run simultaneously. On 2014/05/21 12:59:13, mtomasz wrote: > nit: In maximum -> At most Done. https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:48: * On 2014/05/21 12:59:13, mtomasz wrote: > @param missing Done. https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:54: this.running_ = 0; On 2014/05/21 12:59:13, mtomasz wrote: > AsyncUtil.Group stores tasks in this.pendingTasks_. Can we do it here for > consistency? Also, it is easier to debug, since we can see what closures are > pending. Done. https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:80: * Cancels all pending tasks. Note that this does NOT cancel the task running On 2014/05/21 12:59:13, mtomasz wrote: > nit: the task -> tasks Done. https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:89: * @return {boolean} True when a task is running but requeted to cancel, On 2014/05/22 03:25:12, hirono wrote: > How about the case that the cancel method is called when the queue is empty? Good point. Fixed. https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:100: AsyncUtil.ConcurrentQueue.prototype.setFinishCallback = function(callback) { On 2014/05/21 12:59:13, mtomasz wrote: > Queue should be created for one purpose, so there should be no reason to change > finish callback multiple times. How about passing the callback to the > constructor, like it is done in AsyncUtil.Aggregation? Done. https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:104: AsyncUtil.ConcurrentQueue.prototype.continue_ = function() { On 2014/05/21 12:59:13, mtomasz wrote: > @jsdoc missing Done. https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:106: this.isCancelled_ = false; On 2014/05/22 03:25:12, hirono wrote: > Is it OK assigning false here? > Some tasks may not be finished yet. Good catch. Done. https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:124: AsyncUtil.ConcurrentQueue.prototype.onTaskFinished_ = function() { On 2014/05/21 12:59:13, mtomasz wrote: > @jsdoc missing Done. https://codereview.chromium.org/293053006/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/common/js/async_util.js:140: * @constructor On 2014/05/21 12:59:13, mtomasz wrote: > @extends missing Done.
https://codereview.chromium.org/293053006/diff/20002/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/async_util.js (right): https://codereview.chromium.org/293053006/diff/20002/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/async_util.js:63: }; Object.seal(this)? https://codereview.chromium.org/293053006/diff/20002/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/async_util.js:77: AsyncUtil.ConcurrentQueue.prototype.run = function(closure) { How about preventing the class from adding new tasks after finishing? Because finishCallback can be specified only once, adding new tasks after finishing looks unexpected.
PTAL https://codereview.chromium.org/293053006/diff/20002/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/async_util.js (right): https://codereview.chromium.org/293053006/diff/20002/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/async_util.js:63: }; On 2014/05/23 03:23:28, hirono wrote: > Object.seal(this)? Done. https://codereview.chromium.org/293053006/diff/20002/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/async_util.js:77: AsyncUtil.ConcurrentQueue.prototype.run = function(closure) { On 2014/05/23 03:23:28, hirono wrote: > How about preventing the class from adding new tasks after finishing? > Because finishCallback can be specified only once, adding new tasks after > finishing looks unexpected. That breaks compatibility of AsyncUtil.Quque. Let me remove them and adding the getters to retrieve the numbers of tasks.
https://codereview.chromium.org/293053006/diff/70001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/async_util.js (right): https://codereview.chromium.org/293053006/diff/70001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/async_util.js:60: Objest.seal(this); typo: Object https://codereview.chromium.org/293053006/diff/70001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/async_util.js:100: if (this.addedTasks_.length > 0) var q = ConcurrentQueue(); q.cancel(); q.isCancelled(); > false Is it expected? Why not always setting isCancelled to true? https://codereview.chromium.org/293053006/diff/70001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/async_util.js:106: * @return {boolean} True when a task is running but requeted to cancel, typo: requested https://codereview.chromium.org/293053006/diff/70001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/async_util.js:146: this.isCancelled_ = false; Why are we setting cancelled to false?
https://codereview.chromium.org/293053006/diff/70001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/async_util.js (right): https://codereview.chromium.org/293053006/diff/70001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/async_util.js:100: if (this.addedTasks_.length > 0) On 2014/05/23 07:16:40, mtomasz wrote: > var q = ConcurrentQueue(); > q.cancel(); > q.isCancelled(); > > > false > > Is it expected? Why not always setting isCancelled to true? Now I think isCancelling is better name. WDYT?
https://codereview.chromium.org/293053006/diff/90001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/async_util.js (right): https://codereview.chromium.org/293053006/diff/90001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/async_util.js:58: this.isCancelling_ = false; It looks the class does not use the flag now. I wonder if we can remove it.
https://codereview.chromium.org/293053006/diff/90001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/async_util.js (right): https://codereview.chromium.org/293053006/diff/90001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/async_util.js:58: this.isCancelling_ = false; On 2014/05/23 08:28:38, hirono wrote: > It looks the class does not use the flag now. I wonder if we can remove it. I'd like to use it in the succeeding patch. https://codereview.chromium.org/298823005/
On 2014/05/23 08:30:48, yoshiki wrote: > https://codereview.chromium.org/293053006/diff/90001/ui/file_manager/file_man... > File ui/file_manager/file_manager/common/js/async_util.js (right): > > https://codereview.chromium.org/293053006/diff/90001/ui/file_manager/file_man... > ui/file_manager/file_manager/common/js/async_util.js:58: this.isCancelling_ = > false; > On 2014/05/23 08:28:38, hirono wrote: > > It looks the class does not use the flag now. I wonder if we can remove it. > > I'd like to use it in the succeeding patch. > https://codereview.chromium.org/298823005/ How about simplifying? Once the queue is cancelled, should not be used anymore, and new one should be created. We would get rid of all the edge cases. WDYT?
On 2014/05/23 08:33:41, mtomasz wrote: > On 2014/05/23 08:30:48, yoshiki wrote: > > > https://codereview.chromium.org/293053006/diff/90001/ui/file_manager/file_man... > > File ui/file_manager/file_manager/common/js/async_util.js (right): > > > > > https://codereview.chromium.org/293053006/diff/90001/ui/file_manager/file_man... > > ui/file_manager/file_manager/common/js/async_util.js:58: this.isCancelling_ = > > false; > > On 2014/05/23 08:28:38, hirono wrote: > > > It looks the class does not use the flag now. I wonder if we can remove it. > > > > I'd like to use it in the succeeding patch. > > https://codereview.chromium.org/298823005/ > > How about simplifying? Once the queue is cancelled, should not be used anymore, > and new one should be created. We would get rid of all the edge cases. WDYT? AsyncUtil.Queue doesn't behave so. Can we change the behavior? WDYT?
On 2014/05/23 08:43:03, yoshiki wrote: > On 2014/05/23 08:33:41, mtomasz wrote: > > On 2014/05/23 08:30:48, yoshiki wrote: > > > > > > https://codereview.chromium.org/293053006/diff/90001/ui/file_manager/file_man... > > > File ui/file_manager/file_manager/common/js/async_util.js (right): > > > > > > > > > https://codereview.chromium.org/293053006/diff/90001/ui/file_manager/file_man... > > > ui/file_manager/file_manager/common/js/async_util.js:58: this.isCancelling_ > = > > > false; > > > On 2014/05/23 08:28:38, hirono wrote: > > > > It looks the class does not use the flag now. I wonder if we can remove > it. > > > > > > I'd like to use it in the succeeding patch. > > > https://codereview.chromium.org/298823005/ > > > > How about simplifying? Once the queue is cancelled, should not be used > anymore, > > and new one should be created. We would get rid of all the edge cases. WDYT? > > AsyncUtil.Queue doesn't behave so. Can we change the behavior? WDYT? prefetchMetadataQueue in directory_contents.js is only the user of 'cancel'. I suppose we can change.
On 2014/05/23 08:43:03, yoshiki wrote: > On 2014/05/23 08:33:41, mtomasz wrote: > > On 2014/05/23 08:30:48, yoshiki wrote: > > > > > > https://codereview.chromium.org/293053006/diff/90001/ui/file_manager/file_man... > > > File ui/file_manager/file_manager/common/js/async_util.js (right): > > > > > > > > > https://codereview.chromium.org/293053006/diff/90001/ui/file_manager/file_man... > > > ui/file_manager/file_manager/common/js/async_util.js:58: this.isCancelling_ > = > > > false; > > > On 2014/05/23 08:28:38, hirono wrote: > > > > It looks the class does not use the flag now. I wonder if we can remove > it. > > > > > > I'd like to use it in the succeeding patch. > > > https://codereview.chromium.org/298823005/ > > > > How about simplifying? Once the queue is cancelled, should not be used > anymore, > > and new one should be created. We would get rid of all the edge cases. WDYT? > > AsyncUtil.Queue doesn't behave so. Can we change the behavior? WDYT? Yes, we should change it. Simplicity is good. Cancelled queue should not be reused.
On 2014/05/23 08:56:22, mtomasz wrote: > On 2014/05/23 08:43:03, yoshiki wrote: > > On 2014/05/23 08:33:41, mtomasz wrote: > > > On 2014/05/23 08:30:48, yoshiki wrote: > > > > > > > > > > https://codereview.chromium.org/293053006/diff/90001/ui/file_manager/file_man... > > > > File ui/file_manager/file_manager/common/js/async_util.js (right): > > > > > > > > > > > > > > https://codereview.chromium.org/293053006/diff/90001/ui/file_manager/file_man... > > > > ui/file_manager/file_manager/common/js/async_util.js:58: > this.isCancelling_ > > = > > > > false; > > > > On 2014/05/23 08:28:38, hirono wrote: > > > > > It looks the class does not use the flag now. I wonder if we can remove > > it. > > > > > > > > I'd like to use it in the succeeding patch. > > > > https://codereview.chromium.org/298823005/ > > > > > > How about simplifying? Once the queue is cancelled, should not be used > > anymore, > > > and new one should be created. We would get rid of all the edge cases. WDYT? > > > > AsyncUtil.Queue doesn't behave so. Can we change the behavior? WDYT? > > Yes, we should change it. Simplicity is good. Cancelled queue should not be > reused. SGTM. I'll change it. Please review after I change it.
Uploaded the new patch set. PTAL?
https://codereview.chromium.org/293053006/diff/260001/ui/file_manager/file_ma... File ui/file_manager/file_manager/common/js/async_util.js (right): https://codereview.chromium.org/293053006/diff/260001/ui/file_manager/file_ma... ui/file_manager/file_manager/common/js/async_util.js:97: return; nit: Is it needed? https://codereview.chromium.org/293053006/diff/260001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/directory_contents.js (right): https://codereview.chromium.org/293053006/diff/260001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/directory_contents.js:571: this.prefetchMetadataQueue_ = new AsyncUtil.Queue(); Does it really need to reassign a new queue? It looks this.scanCancelled_ does not turn back to false. https://codereview.chromium.org/293053006/diff/260001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/directory_contents.js:612: this.prefetchMetadataQueue_.run(function(callback, cancelled) { What is difference between this.scanCancelled_ and the cancelled argument passed via Queue? https://codereview.chromium.org/293053006/diff/260001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/directory_contents.js:669: return callback(); Maybe "callback(); return;" is more free from misreadings.
https://codereview.chromium.org/293053006/diff/260001/ui/file_manager/file_ma... File ui/file_manager/file_manager/common/js/async_util.js (right): https://codereview.chromium.org/293053006/diff/260001/ui/file_manager/file_ma... ui/file_manager/file_manager/common/js/async_util.js:97: return; On 2014/05/26 09:19:42, hirono wrote: > nit: Is it needed? Done. https://codereview.chromium.org/293053006/diff/260001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/directory_contents.js (right): https://codereview.chromium.org/293053006/diff/260001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/directory_contents.js:571: this.prefetchMetadataQueue_ = new AsyncUtil.Queue(); On 2014/05/26 09:19:42, hirono wrote: > Does it really need to reassign a new queue? > It looks this.scanCancelled_ does not turn back to false. That's right. Removed. https://codereview.chromium.org/293053006/diff/260001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/directory_contents.js:612: this.prefetchMetadataQueue_.run(function(callback, cancelled) { On 2014/05/26 09:19:42, hirono wrote: > What is difference between this.scanCancelled_ and the cancelled argument passed > via Queue? In this case, they are same. But in general, the callback function should need to know if the task is cancelled to handle the cancel, and passing the status to the callback is better than passing queue's instance using closure. https://codereview.chromium.org/293053006/diff/260001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/directory_contents.js:669: return callback(); On 2014/05/26 09:19:42, hirono wrote: > Maybe "callback(); return;" is more free from misreadings. Done.
https://codereview.chromium.org/293053006/diff/260001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/directory_contents.js (right): https://codereview.chromium.org/293053006/diff/260001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/directory_contents.js:612: this.prefetchMetadataQueue_.run(function(callback, cancelled) { On 2014/05/26 09:36:33, yoshiki wrote: > On 2014/05/26 09:19:42, hirono wrote: > > What is difference between this.scanCancelled_ and the cancelled argument > passed > > via Queue? > > In this case, they are same. But in general, the callback function should need > to know if the task is cancelled to handle the cancel, and passing the status to > the callback is better than passing queue's instance using closure. How about removing this.scanCancelled_ and using this.prefetchMetadataQueue_.isCancelled() instead? We can avoid having two names for the same thing.
On 2014/05/26 09:41:04, hirono wrote: > https://codereview.chromium.org/293053006/diff/260001/ui/file_manager/file_ma... > File ui/file_manager/file_manager/foreground/js/directory_contents.js (right): > > https://codereview.chromium.org/293053006/diff/260001/ui/file_manager/file_ma... > ui/file_manager/file_manager/foreground/js/directory_contents.js:612: > this.prefetchMetadataQueue_.run(function(callback, cancelled) { > On 2014/05/26 09:36:33, yoshiki wrote: > > On 2014/05/26 09:19:42, hirono wrote: > > > What is difference between this.scanCancelled_ and the cancelled argument > > passed > > > via Queue? > > > > In this case, they are same. But in general, the callback function should need > > to know if the task is cancelled to handle the cancel, and passing the status > to > > the callback is better than passing queue's instance using closure. > > How about removing this.scanCancelled_ and using > this.prefetchMetadataQueue_.isCancelled() instead? > We can avoid having two names for the same thing. The prefetchMetadataQueue_ is a queue for prefetchMetadata and this.scanCancelled_ is a flag of the entire operation of directory content. These values are synced, but we shouldn't merge them since their meaning is different. WDYT?
https://codereview.chromium.org/293053006/diff/290002/ui/file_manager/file_ma... File ui/file_manager/file_manager/common/js/async_util.js (right): https://codereview.chromium.org/293053006/diff/290002/ui/file_manager/file_ma... ui/file_manager/file_manager/common/js/async_util.js:53: console.assert(limit > 0, '|limit| must be larger than 2'); nit: larter than 2 -> larger than 0? https://codereview.chromium.org/293053006/diff/290002/ui/file_manager/file_ma... ui/file_manager/file_manager/common/js/async_util.js:86: * @param {function(function(callback, isCancelled=))} closure Closure with a nit: callback -> function() nit: isCancelled= -> boolean= https://codereview.chromium.org/293053006/diff/290002/ui/file_manager/file_ma... ui/file_manager/file_manager/common/js/async_util.js:134: closure(this.onTaskFinished_.bind(this, closure), this.isCancelled_); this.isCancelled_ is always false here. It is impossible to reach #134 line with isCancelled_ set to true. So, we can remove this argument. https://codereview.chromium.org/293053006/diff/290002/ui/file_manager/file_ma... ui/file_manager/file_manager/common/js/async_util.js:141: * @private nit: @param is missing.
PTAL https://codereview.chromium.org/293053006/diff/290002/ui/file_manager/file_ma... File ui/file_manager/file_manager/common/js/async_util.js (right): https://codereview.chromium.org/293053006/diff/290002/ui/file_manager/file_ma... ui/file_manager/file_manager/common/js/async_util.js:53: console.assert(limit > 0, '|limit| must be larger than 2'); On 2014/05/27 00:23:32, mtomasz wrote: > nit: larter than 2 -> larger than 0? Done. https://codereview.chromium.org/293053006/diff/290002/ui/file_manager/file_ma... ui/file_manager/file_manager/common/js/async_util.js:86: * @param {function(function(callback, isCancelled=))} closure Closure with a On 2014/05/27 00:23:32, mtomasz wrote: > nit: callback -> function() > nit: isCancelled= -> boolean= Done. https://codereview.chromium.org/293053006/diff/290002/ui/file_manager/file_ma... ui/file_manager/file_manager/common/js/async_util.js:134: closure(this.onTaskFinished_.bind(this, closure), this.isCancelled_); On 2014/05/27 00:23:32, mtomasz wrote: > this.isCancelled_ is always false here. It is impossible to reach #134 line with > isCancelled_ set to true. So, we can remove this argument. Nice catch. You're correct. https://codereview.chromium.org/293053006/diff/290002/ui/file_manager/file_ma... ui/file_manager/file_manager/common/js/async_util.js:141: * @private On 2014/05/27 00:23:32, mtomasz wrote: > nit: @param is missing. Done.
lgtm with tiny nits! https://codereview.chromium.org/293053006/diff/370001/ui/file_manager/file_ma... File ui/file_manager/file_manager/common/js/async_util.js (right): https://codereview.chromium.org/293053006/diff/370001/ui/file_manager/file_ma... ui/file_manager/file_manager/common/js/async_util.js:86: * @param {function(function(function()))} closure Closure with a completion nit: Hm, maybe it should be: function(function())? Please double check. https://codereview.chromium.org/293053006/diff/370001/ui/file_manager/file_ma... ui/file_manager/file_manager/common/js/async_util.js:87: * call back to be executed. nit: call back -> callback
https://codereview.chromium.org/293053006/diff/370001/ui/file_manager/file_ma... File ui/file_manager/file_manager/common/js/async_util.js (right): https://codereview.chromium.org/293053006/diff/370001/ui/file_manager/file_ma... ui/file_manager/file_manager/common/js/async_util.js:86: * @param {function(function(function()))} closure Closure with a completion On 2014/05/27 02:19:51, mtomasz wrote: > nit: Hm, maybe it should be: function(function())? Please double check. That's correct. Thanks. https://codereview.chromium.org/293053006/diff/370001/ui/file_manager/file_ma... ui/file_manager/file_manager/common/js/async_util.js:87: * call back to be executed. On 2014/05/27 02:19:51, mtomasz wrote: > nit: call back -> callback Done.
On 2014/05/27 02:49:54, yoshiki wrote: > https://codereview.chromium.org/293053006/diff/370001/ui/file_manager/file_ma... > File ui/file_manager/file_manager/common/js/async_util.js (right): > > https://codereview.chromium.org/293053006/diff/370001/ui/file_manager/file_ma... > ui/file_manager/file_manager/common/js/async_util.js:86: * @param > {function(function(function()))} closure Closure with a completion > On 2014/05/27 02:19:51, mtomasz wrote: > > nit: Hm, maybe it should be: function(function())? Please double check. > > That's correct. Thanks. > > https://codereview.chromium.org/293053006/diff/370001/ui/file_manager/file_ma... > ui/file_manager/file_manager/common/js/async_util.js:87: * call back to be > executed. > On 2014/05/27 02:19:51, mtomasz wrote: > > nit: call back -> callback > > Done. lgtm!
On 2014/05/27 02:55:33, hirono wrote: > On 2014/05/27 02:49:54, yoshiki wrote: > > > https://codereview.chromium.org/293053006/diff/370001/ui/file_manager/file_ma... > > File ui/file_manager/file_manager/common/js/async_util.js (right): > > > > > https://codereview.chromium.org/293053006/diff/370001/ui/file_manager/file_ma... > > ui/file_manager/file_manager/common/js/async_util.js:86: * @param > > {function(function(function()))} closure Closure with a completion > > On 2014/05/27 02:19:51, mtomasz wrote: > > > nit: Hm, maybe it should be: function(function())? Please double check. > > > > That's correct. Thanks. > > > > > https://codereview.chromium.org/293053006/diff/370001/ui/file_manager/file_ma... > > ui/file_manager/file_manager/common/js/async_util.js:87: * call back to be > > executed. > > On 2014/05/27 02:19:51, mtomasz wrote: > > > nit: call back -> callback > > > > Done. > > lgtm! Thansk!
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/293053006/210003
Message was sent while issue was closed.
Change committed as 272934 |
