Add a TaskQueue.

Add a TaskQueue.

The TaskQueue handles the basics of running a set of queued up tasks, and providing updates and various other callbacks for clients to be notified about queue and task state.

BUG=

Committed: https://crrev.com/d53fb76a05cf242abbf7c6736f099dfffbd4317f
Cr-Commit-Position: refs/heads/master@{#308444}

[Ben Kwa, 2014-12-12 04:34:18]

kenobi@chromium.org changed reviewers:
+ mtomasz@chromium.org, smckay@chromium.org

[mtomasz, 2014-12-12 08:30:24]

mtomasz@chromium.org changed reviewers:
+ hirono@chromium.org

[mtomasz, 2014-12-12 08:30:51]

Mostly nits. I added @hirono, who knows more background about the TaskQueue
design.

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/task_queue.js (right):

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:9: * @constructor
nit: Please add a description to the class.

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:38: // TODO(kenobi):
Use an AsyncGroup
nit: Simply AsyncUtil.Queue would be easier. AsyncGroup is usually to run in
parallel.

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:39:
Promise.resolve().then(function() {
nit: Why Promise.resolve().then() is needed?

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:53:
importer.TaskQueue.prototype.onUpdate = function(callback) {
nit: We use onXXX naming for event handlers. But here it looks like a simple
setter. How about addUpdateListener?

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:63:
importer.TaskQueue.prototype.onActive = function(callback) {
Similarly here, but since we allow only one callback to be registered, then
maybe setResumeCallback or sth like that?

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:79: * @param {string}
reason
nit: Shall the type be UpdateType?

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:111: // all done - go
back to idle
nit: all -> All
nit: idle -> idle.

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:113:
this.idleCallback_.call(null);
nit: Why not simply this.idleCallback_()?

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:117: if
(this.activeCallback_)
I think this is called not only when going from idle, but for each request which
is not the last one. Is it correct?

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:130: this.taskId_ =
taskId;
nit: jsdoc for taskId_ missing.

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:141: this.owner_ =
owner;
nit: jsdoc missing?

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/task_queue_unittest.js (right):

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue_unittest.js:34:
this.wasRun = false;
nit: jsdoc missing?

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue_unittest.js:37:
this.runResolver_ = resolve;
nit: Hm, all members of classes should be declared in constructors. This is
still in the constructor body, but it will be executed after the constructor is
completed. How about declaring this.runResolver_ = null in #35?

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue_unittest.js:46: //
this.notifySuccess();
nit: Can be removed?

[Ben Kwa, 2014-12-12 15:53:48]

Thanks!

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/task_queue.js (right):

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:9: * @constructor
On 2014/12/12 08:30:50, mtomasz wrote:
> nit: Please add a description to the class.

Done.

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:38: // TODO(kenobi):
Use an AsyncGroup
On 2014/12/12 08:30:50, mtomasz wrote:
> nit: Simply AsyncUtil.Queue would be easier. AsyncGroup is usually to run in
> parallel.

Hm, the FileOperationManager uses an AsyncGroup.  I'm unclear on the
distinction, but the main point was that the queuing happens asynchronously.

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:39:
Promise.resolve().then(function() {
On 2014/12/12 08:30:50, mtomasz wrote:
> nit: Why Promise.resolve().then() is needed?

I could have (maybe should have?) done setTimeout instead.  The main thing here
is for the anonymous function you see (enqueuing the task and then starting up
the queue) to be executed in a separate async context.

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:53:
importer.TaskQueue.prototype.onUpdate = function(callback) {
On 2014/12/12 08:30:50, mtomasz wrote:
> nit: We use onXXX naming for event handlers. But here it looks like a simple
> setter. How about addUpdateListener?

Done.

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:63:
importer.TaskQueue.prototype.onActive = function(callback) {
On 2014/12/12 08:30:50, mtomasz wrote:
> Similarly here, but since we allow only one callback to be registered, then
> maybe setResumeCallback or sth like that?

Done.

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:79: * @param {string}
reason
On 2014/12/12 08:30:50, mtomasz wrote:
> nit: Shall the type be UpdateType?

Good catch, thanks!

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:111: // all done - go
back to idle
On 2014/12/12 08:30:50, mtomasz wrote:
> nit: all -> All
> nit: idle -> idle.

Done.

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:113:
this.idleCallback_.call(null);
On 2014/12/12 08:30:50, mtomasz wrote:
> nit: Why not simply this.idleCallback_()?

Done.

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:117: if
(this.activeCallback_)
On 2014/12/12 08:30:50, mtomasz wrote:
> I think this is called not only when going from idle, but for each request
which
> is not the last one. Is it correct?

Fixed - thanks.

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:130: this.taskId_ =
taskId;
On 2014/12/12 08:30:50, mtomasz wrote:
> nit: jsdoc for taskId_ missing.

Done.

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:141: this.owner_ =
owner;
On 2014/12/12 08:30:50, mtomasz wrote:
> nit: jsdoc missing?

Do you mean for this.owner_?  That's up above, on line 132.

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/task_queue_unittest.js (right):

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue_unittest.js:34:
this.wasRun = false;
On 2014/12/12 08:30:51, mtomasz wrote:
> nit: jsdoc missing?

Done.

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue_unittest.js:37:
this.runResolver_ = resolve;
On 2014/12/12 08:30:50, mtomasz wrote:
> nit: Hm, all members of classes should be declared in constructors. This is
> still in the constructor body, but it will be executed after the constructor
is
> completed. How about declaring this.runResolver_ = null in #35?

Actually the promise executor is executed right away, in the current js context.
 So line 37 runs before the constructor returns.  But I think you're right that
I should put a declaration in the ctor.

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue_unittest.js:46: //
this.notifySuccess();
On 2014/12/12 08:30:51, mtomasz wrote:
> nit: Can be removed?

Done.

[Steve McKay, 2014-12-12 17:34:50]

https://codereview.chromium.org/789603004/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/task_queue.js (right):

https://codereview.chromium.org/789603004/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/task_queue.js:39:
importer.TaskQueue.UpdateType = {
This enum should go in a common location if you expect foreground apps to be
able to reference UpdateType directly.

https://codereview.chromium.org/789603004/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/task_queue.js:120:
importer.TaskQueue.prototype.runPending_ = function() {
Is the thinking that we'll have a variety of these task queues for specific
purposes, or that we'll have one central task queue for all tasks?

https://codereview.chromium.org/789603004/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/task_queue.js:145:
importer.TaskQueue.Task = function(taskId) {
Make a separate interface for Task, then an AbstractTask that lazy people (like
me) can inherit from.

https://codereview.chromium.org/789603004/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/task_queue.js:157:
importer.TaskQueue.Task.prototype.setOwner = function(owner) {
Seems like there are valid cases where we might want to listen to individual
tasks...directly. If you generalize this to be just another addObserver method,
then that would be easy. An observer would only need to be {function(UpdateType,
Task)}.

[Ben Kwa, 2014-12-12 19:53:00]

https://codereview.chromium.org/789603004/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/task_queue.js (right):

https://codereview.chromium.org/789603004/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/task_queue.js:39:
importer.TaskQueue.UpdateType = {
On 2014/12/12 17:34:50, Steve McKay wrote:
> This enum should go in a common location if you expect foreground apps to be
> able to reference UpdateType directly.

Discussed offline.  I'll move this when it becomes necessary.

https://codereview.chromium.org/789603004/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/task_queue.js:120:
importer.TaskQueue.prototype.runPending_ = function() {
On 2014/12/12 17:34:50, Steve McKay wrote:
> Is the thinking that we'll have a variety of these task queues for specific
> purposes, or that we'll have one central task queue for all tasks?

My intent was for media import to have one queue (so, scanning and importing
would live on one queue).  If mtomasz@ et al decide to adopt this more widely,
FileOperationManager could potentially use it (so, copy/move/delete/etc would
live on another queue).

https://codereview.chromium.org/789603004/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/task_queue.js:145:
importer.TaskQueue.Task = function(taskId) {
On 2014/12/12 17:34:50, Steve McKay wrote:
> Make a separate interface for Task, then an AbstractTask that lazy people
(like
> me) can inherit from.

Done.

https://codereview.chromium.org/789603004/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/task_queue.js:157:
importer.TaskQueue.Task.prototype.setOwner = function(owner) {
On 2014/12/12 17:34:50, Steve McKay wrote:
> Seems like there are valid cases where we might want to listen to individual
> tasks...directly. If you generalize this to be just another addObserver
method,
> then that would be easy. An observer would only need to be
{function(UpdateType,
> Task)}.

As discussed offline, leaving this as-is for now.  I'll update this CL to
include usage by the MediaImportHandler, which should give us something a bit
more concrete to discuss.

[hirono, 2014-12-15 00:20:07]

lgtm with two nits. Thanks!

https://codereview.chromium.org/789603004/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/task_queue.js (right):

https://codereview.chromium.org/789603004/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/task_queue.js:17: * @constructor
Please add @struct also.

https://codereview.chromium.org/789603004/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/task_queue.js:108:
this.tasks_.shift();
Do you assume this.tasks_[0] === task ? If so, how about adding assertion?

[mtomasz, 2014-12-15 00:58:44]

lgtm

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/task_queue.js (right):

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:39:
Promise.resolve().then(function() {
On 2014/12/12 15:53:48, Ben Kwa wrote:
> On 2014/12/12 08:30:50, mtomasz wrote:
> > nit: Why Promise.resolve().then() is needed?
> 
> I could have (maybe should have?) done setTimeout instead.  The main thing
here
> is for the anonymous function you see (enqueuing the task and then starting up
> the queue) to be executed in a separate async context.  

Why it needs to run in a separate async context? Either setTimer or Promise is
fine if we want to force it.

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:141: this.owner_ =
owner;
On 2014/12/12 15:53:48, Ben Kwa wrote:
> On 2014/12/12 08:30:50, mtomasz wrote:
> > nit: jsdoc missing?
> 
> Do you mean for this.owner_?  That's up above, on line 132.

Ah, you're right.

[Ben Kwa, 2014-12-15 16:27:47]

Patchset #4 (id:60001) has been deleted

[Ben Kwa, 2014-12-15 16:53:01]

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/task_queue.js (right):

https://codereview.chromium.org/789603004/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/task_queue.js:39:
Promise.resolve().then(function() {
On 2014/12/15 00:58:44, mtomasz wrote:
> On 2014/12/12 15:53:48, Ben Kwa wrote:
> > On 2014/12/12 08:30:50, mtomasz wrote:
> > > nit: Why Promise.resolve().then() is needed?
> > 
> > I could have (maybe should have?) done setTimeout instead.  The main thing
> here
> > is for the anonymous function you see (enqueuing the task and then starting
up
> > the queue) to be executed in a separate async context.  
> 
> Why it needs to run in a separate async context? Either setTimer or Promise is
> fine if we want to force it.

Because the Tasks that get pushed on the queue aren't required to actually be
inherently asynchronous - this code forces any Task that's pushed onto the queue
to be run asynchronously.  I added a comment explaining what's going on here.

https://codereview.chromium.org/789603004/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/task_queue.js (right):

https://codereview.chromium.org/789603004/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/task_queue.js:17: * @constructor
On 2014/12/15 00:20:07, hirono wrote:
> Please add @struct also.

Done.

https://codereview.chromium.org/789603004/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/task_queue.js:108:
this.tasks_.shift();
On 2014/12/15 00:20:07, hirono wrote:
> Do you assume this.tasks_[0] === task ? If so, how about adding assertion?
> 

Nice observation.  Thanks!  Done.

[Steve McKay, 2014-12-15 17:57:16]

LGTM. We can iterate on the design.

[Ben Kwa, 2014-12-15 17:57:43]

The CQ bit was checked by kenobi@chromium.org

[commit-bot: I haz the power, 2014-12-15 17:58:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/789603004/80001

[commit-bot: I haz the power, 2014-12-15 19:22:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-15 19:22:25]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ben Kwa, 2014-12-15 21:29:00]

The CQ bit was checked by kenobi@chromium.org

[commit-bot: I haz the power, 2014-12-15 21:31:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/789603004/100001

[commit-bot: I haz the power, 2014-12-15 22:31:33]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2014-12-15 22:32:19]

Patchset 5 (id:??) landed as
https://crrev.com/d53fb76a05cf242abbf7c6736f099dfffbd4317f
Cr-Commit-Position: refs/heads/master@{#308444}